Show 77 quoted lines
>
> 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
>