Subject: Memory Leak vulnerability in reftable/readwrite_test.c

>
> 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!

> 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.

>
> 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
>

> --- >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);
>  	}

>
> 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