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@xxxxxxxxx>
Signed-off-by: René Scharfe <l.s.r@xxxxxx>
---
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
