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

From: René Scharfe <l.s.r@web.de>
Date: Mar 1, 2025, 11:31 UTC
Message-ID: <bca5c0ec-0995-421e-9745-330f729357d6@web.de>
In-Reply-To: <CAAJd+fZSUiiUm05D_eO3HS7p=WoxAWSZo51dUNjyGjUNJBvyGg@mail.gmail.com>

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.

Show 7 quoted lines

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

Show 5 quoted lines

> 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