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

["René Scharfe" <l.s.r@web.de>]

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