Subject: Memory Leak vulnerability in reftable/readwrite_test.c

Subject: Memory Leak vulnerability in reftable/readwrite_test.c

[H Z]

Hi, I have found a potential memory leak bug in
reftable/readwrite_test.c and would like to report it to the
maintainers. Can you please help me to check it? Thank you for your
effort and patience!

Below is the execution sequence of the program that may produce the bug.

First, in file src/wrapper.c, function xstrdup allocates memory at
line 40 and returns at line 43.
Second, in the file reftable/reader.c, the function init_reader calls
the function xstrdup on line 202 to allocate memory for r->name, which
is the formal parameter of the function init_reader.
Third, in file reftable/readwrite_test.c, function
test_corrupt_table_empty calls function init_reader on line 935 with
&rd passed as the first argument, causing rd->name to be allocated
memory. rd->name is not freed, which would cause the memory leak
vulnerability.

Thank you very much for reading and I look forward to hearing from you!

Re: Subject: Memory Leak vulnerability in reftable/readwrite_test.c

[H Z]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

Also, here's the code path that leads to that memory leak
vulnerability, which I've represented with an image.

H Z <shiyuyuranzh@gmail.com> 于2025年3月1日周六 14:07写道：
>
> Hi, I have found a potential memory leak bug in
> reftable/readwrite_test.c and would like to report it to the
> maintainers. Can you please help me to check it? Thank you for your
> effort and patience!
>
> Below is the execution sequence of the program that may produce the bug.
>
> First, in file src/wrapper.c, function xstrdup allocates memory at
> line 40 and returns at line 43.
> Second, in the file reftable/reader.c, the function init_reader calls
> the function xstrdup on line 202 to allocate memory for r->name, which
> is the formal parameter of the function init_reader.
> Third, in file reftable/readwrite_test.c, function
> test_corrupt_table_empty calls function init_reader on line 935 with
> &rd passed as the first argument, causing rd->name to be allocated
> memory. rd->name is not freed, which would cause the memory leak
> vulnerability.
>
> Thank you very much for reading and I look forward to hearing from you!

[-- Attachment #2: image.png --]
[-- Type: image/png, Size: 193692 bytes --]

Re: Subject: Memory Leak vulnerability in reftable/readwrite_test.c

[René Scharfe]

Am 01.03.25 um 07:07 schrieb H Z:
> Hi, I have found a potential memory leak bug in
> reftable/readwrite_test.c and would like to report it to the
> maintainers. Can you please help me to check it? Thank you for your
> effort and patience!

I wouldn't call it a vulnerability if it just affects test code, as it
is not executed by git (the executable run by end users).  We still want
to fix those, however.

> Below is the execution sequence of the program that may produce the bug.
>
> First, in file src/wrapper.c, function xstrdup allocates memory at
> line 40 and returns at line 43.
> Second, in the file reftable/reader.c, the function init_reader calls
> the function xstrdup on line 202 to allocate memory for r->name, which
> is the formal parameter of the function init_reader.

Not exactly true since 12b9078066 (reftable: handle trivial allocation
failures, 2024-10-02); the allocation is done by reftable_strdup() now.
And 2de3c0d345 (reftable/reader: inline `init_reader()`, 2024-08-23)
got rid of init_reader().

> Third, in file reftable/readwrite_test.c, function
> test_corrupt_table_empty calls function init_reader on line 935 with
> &rd passed as the first argument, causing rd->name to be allocated
> memory. rd->name is not freed, which would cause the memory leak
> vulnerability.

This test was moved to t/unit-tests/t-reftable-readwrite.c by 5b539a5361
(t: move reftable/readwrite_test.c to the unit testing framework,
2024-08-13).

t_corrupt_table_empty() calls reftable_reader_new() and returns
REFTABLE_FORMAT_ERROR before it reaches the reftable_strdup() call, so
there is no leak in this test (anymore?).

reftable_reader_new() would leak name if its block_source_read_block()
or parse_footer() calls failed, though.  We could do the name
allocation only after those calls to avoid that, but that may
complicate matters.  Alternative patch below.

Also its comment in reftable/reftable-reader.h mentions that
reftable_reader_destroy() needs to be called after use, but that
function has never existed.  Odd.

René


--- >8 ---
Subject: [PATCH] reftable: release name on reftable_reader_new() error

If block_source_read_block() or parse_footer() fail, we leak the "name"
member of struct reftable_reader in reftable_reader_new().  Release it.

Reported by: H Z <shiyuyuranzh@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 reftable/reader.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/reftable/reader.c b/reftable/reader.c
index 3f2e4b2800..f38c83f140 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -666,6 +666,7 @@ int reftable_reader_new(struct reftable_reader **out,
 	reftable_block_done(&footer);
 	reftable_block_done(&header);
 	if (err) {
+		reftable_free(r->name);
 		reftable_free(r);
 		block_source_close(source);
 	}
--
2.48.1

Re: Subject: Memory Leak vulnerability in reftable/readwrite_test.c

[H Z]

Thank you very much for your reply.

René Scharfe <l.s.r@web.de> 于2025年3月1日周六 19:31写道：
>
> Am 01.03.25 um 07:07 schrieb H Z:
> > Hi, I have found a potential memory leak bug in
> > reftable/readwrite_test.c and would like to report it to the
> > maintainers. Can you please help me to check it? Thank you for your
> > effort and patience!
>
> I wouldn't call it a vulnerability if it just affects test code, as it
> is not executed by git (the executable run by end users).  We still want
> to fix those, however.
>
> > Below is the execution sequence of the program that may produce the bug.
> >
> > First, in file src/wrapper.c, function xstrdup allocates memory at
> > line 40 and returns at line 43.
> > Second, in the file reftable/reader.c, the function init_reader calls
> > the function xstrdup on line 202 to allocate memory for r->name, which
> > is the formal parameter of the function init_reader.
>
> Not exactly true since 12b9078066 (reftable: handle trivial allocation
> failures, 2024-10-02); the allocation is done by reftable_strdup() now.
> And 2de3c0d345 (reftable/reader: inline `init_reader()`, 2024-08-23)
> got rid of init_reader().
>
> > Third, in file reftable/readwrite_test.c, function
> > test_corrupt_table_empty calls function init_reader on line 935 with
> > &rd passed as the first argument, causing rd->name to be allocated
> > memory. rd->name is not freed, which would cause the memory leak
> > vulnerability.
>
> This test was moved to t/unit-tests/t-reftable-readwrite.c by 5b539a5361
> (t: move reftable/readwrite_test.c to the unit testing framework,
> 2024-08-13).
>
> t_corrupt_table_empty() calls reftable_reader_new() and returns
> REFTABLE_FORMAT_ERROR before it reaches the reftable_strdup() call, so
> there is no leak in this test (anymore?).
>
> reftable_reader_new() would leak name if its block_source_read_block()
> or parse_footer() calls failed, though.  We could do the name
> allocation only after those calls to avoid that, but that may
> complicate matters.  Alternative patch below.
>
> Also its comment in reftable/reftable-reader.h mentions that
> reftable_reader_destroy() needs to be called after use, but that
> function has never existed.  Odd.
>
> René
>
>
> --- >8 ---
> Subject: [PATCH] reftable: release name on reftable_reader_new() error
>
> If block_source_read_block() or parse_footer() fail, we leak the "name"
> member of struct reftable_reader in reftable_reader_new().  Release it.
>
> Reported by: H Z <shiyuyuranzh@gmail.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  reftable/reader.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/reftable/reader.c b/reftable/reader.c
> index 3f2e4b2800..f38c83f140 100644
> --- a/reftable/reader.c
> +++ b/reftable/reader.c
> @@ -666,6 +666,7 @@ int reftable_reader_new(struct reftable_reader **out,
>         reftable_block_done(&footer);
>         reftable_block_done(&header);
>         if (err) {
> +               reftable_free(r->name);
>                 reftable_free(r);
>                 block_source_close(source);
>         }
> --
> 2.48.1
>

Re: Subject: Memory Leak vulnerability in reftable/readwrite_test.c

[Jeff King]

On Sat, Mar 01, 2025 at 12:31:33PM +0100, René Scharfe wrote:

> --- >8 ---
> Subject: [PATCH] reftable: release name on reftable_reader_new() error
> 
> If block_source_read_block() or parse_footer() fail, we leak the "name"
> member of struct reftable_reader in reftable_reader_new().  Release it.
> 
> Reported by: H Z <shiyuyuranzh@gmail.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  reftable/reader.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/reftable/reader.c b/reftable/reader.c
> index 3f2e4b2800..f38c83f140 100644
> --- a/reftable/reader.c
> +++ b/reftable/reader.c
> @@ -666,6 +666,7 @@ int reftable_reader_new(struct reftable_reader **out,
>  	reftable_block_done(&footer);
>  	reftable_block_done(&header);
>  	if (err) {
> +		reftable_free(r->name);
>  		reftable_free(r);
>  		block_source_close(source);
>  	}

Coverity complains that "r" might be NULL here. At the top of the
function we do:

  REFTABLE_CALLOC_ARRAY(r, 1);
  if (!r) {
	err = REFTABLE_OUT_OF_MEMORY_ERROR;
	goto done;
  }

and then the done label hits your new line (the "done:" is right above
the context in your patch). And err of course is non-zero.

So this probably needs an "if (r)", or multiple layered out-labels.  Or
alternatively we could return directly when the first allocation fails,
since there is nothing to clean up at that point.

-Peff

Re: Subject: Memory Leak vulnerability in reftable/readwrite_test.c

[H Z]

Very nice fix, thanks again for doing it!

Jeff King <peff@peff.net> 于2025年3月4日周二 14:33写道：
>
> On Sat, Mar 01, 2025 at 12:31:33PM +0100, René Scharfe wrote:
>
> > --- >8 ---
> > Subject: [PATCH] reftable: release name on reftable_reader_new() error
> >
> > If block_source_read_block() or parse_footer() fail, we leak the "name"
> > member of struct reftable_reader in reftable_reader_new().  Release it.
> >
> > Reported by: H Z <shiyuyuranzh@gmail.com>
> > Signed-off-by: René Scharfe <l.s.r@web.de>
> > ---
> >  reftable/reader.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/reftable/reader.c b/reftable/reader.c
> > index 3f2e4b2800..f38c83f140 100644
> > --- a/reftable/reader.c
> > +++ b/reftable/reader.c
> > @@ -666,6 +666,7 @@ int reftable_reader_new(struct reftable_reader **out,
> >       reftable_block_done(&footer);
> >       reftable_block_done(&header);
> >       if (err) {
> > +             reftable_free(r->name);
> >               reftable_free(r);
> >               block_source_close(source);
> >       }
>
> Coverity complains that "r" might be NULL here. At the top of the
> function we do:
>
>   REFTABLE_CALLOC_ARRAY(r, 1);
>   if (!r) {
>         err = REFTABLE_OUT_OF_MEMORY_ERROR;
>         goto done;
>   }
>
> and then the done label hits your new line (the "done:" is right above
> the context in your patch). And err of course is non-zero.
>
> So this probably needs an "if (r)", or multiple layered out-labels.  Or
> alternatively we could return directly when the first allocation fails,
> since there is nothing to clean up at that point.
>
> -Peff

[PATCH v2] reftable: release name on reftable_reader_new() error

[René Scharfe]

If block_source_read_block() or parse_footer() fail, we leak the "name"
member of struct reftable_reader in reftable_reader_new().  Release it.

Reported by: H Z <shiyuyuranzh@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Change since v1: Avoid NULL pointer dereference.  Thank you, Peff!

 reftable/reader.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/reftable/reader.c b/reftable/reader.c
index 3f2e4b2800..24bae50ac2 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -666,6 +666,8 @@ int reftable_reader_new(struct reftable_reader **out,
 	reftable_block_done(&footer);
 	reftable_block_done(&header);
 	if (err) {
+		if (r)
+			reftable_free(r->name);
 		reftable_free(r);
 		block_source_close(source);
 	}
--
2.48.1