{"thread":{"id":"63029","subject":"Subject: Memory Leak vulnerability in reftable/readwrite_test.c","startedAt":"2025-03-01T06:07:51Z","lastAt":"2025-03-04T16:12:06Z","messageCount":7,"participants":["H Z","René Scharfe","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"513318","messageId":"CAAJd+fZSUiiUm05D_eO3HS7p=WoxAWSZo51dUNjyGjUNJBvyGg@mail.gmail.com","threadId":"63029","inReplyTo":null,"subject":"Subject: Memory Leak vulnerability in reftable/readwrite_test.c","fromName":"H Z","fromEmail":"shiyuyuranzh@gmail.com","sentAt":"2025-03-01T06:07:40Z","receivedAt":"2025-03-01T06:07:51Z","isPatch":false,"sender":{"key":"shiyuyuranzh@gmail.com","avatar":null},"body":"Hi, I have found a potential memory leak bug in\nreftable/readwrite_test.c and would like to report it to the\nmaintainers. Can you please help me to check it? Thank you for your\neffort and patience!\n\nBelow is the execution sequence of the program that may produce the bug.\n\nFirst, in file src/wrapper.c, function xstrdup allocates memory at\nline 40 and returns at line 43.\nSecond, in the file reftable/reader.c, the function init_reader calls\nthe function xstrdup on line 202 to allocate memory for r->name, which\nis the formal parameter of the function init_reader.\nThird, in file reftable/readwrite_test.c, function\ntest_corrupt_table_empty calls function init_reader on line 935 with\n&rd passed as the first argument, causing rd->name to be allocated\nmemory. rd->name is not freed, which would cause the memory leak\nvulnerability.\n\nThank you very much for reading and I look forward to hearing from you!\n"},{"id":"513319","messageId":"CAAJd+fbNhGa5Pe4x-_rbwXVoye9uBD45zoJtu7riiW7GfnU1Cg@mail.gmail.com","threadId":"63029","inReplyTo":"CAAJd+fZSUiiUm05D_eO3HS7p=WoxAWSZo51dUNjyGjUNJBvyGg@mail.gmail.com","subject":"Re: Subject: Memory Leak vulnerability in reftable/readwrite_test.c","fromName":"H Z","fromEmail":"shiyuyuranzh@gmail.com","sentAt":"2025-03-01T06:10:34Z","receivedAt":"2025-03-01T06:10:46Z","isPatch":false,"sender":{"key":"shiyuyuranzh@gmail.com","avatar":null},"body":"Also, here's the code path that leads to that memory leak\nvulnerability, which I've represented with an image.\n\nH Z 于2025年3月1日周六 14:07写道：\n>\n> Hi, I have found a potential memory leak bug in\n> reftable/readwrite_test.c and would like to report it to the\n> maintainers. Can you please help me to check it? Thank you for your\n> effort and patience!\n>\n> Below is the execution sequence of the program that may produce the bug.\n>\n> First, in file src/wrapper.c, function xstrdup allocates memory at\n> line 40 and returns at line 43.\n> Second, in the file reftable/reader.c, the function init_reader calls\n> the function xstrdup on line 202 to allocate memory for r->name, which\n> is the formal parameter of the function init_reader.\n> Third, in file reftable/readwrite_test.c, function\n> test_corrupt_table_empty calls function init_reader on line 935 with\n> &rd passed as the first argument, causing rd->name to be allocated\n> memory. rd->name is not freed, which would cause the memory leak\n> vulnerability.\n>\n> Thank you very much for reading and I look forward to hearing from you!\n"},{"id":"513330","messageId":"CAAJd+fazGWXFJ40dwL7mEMXS6FcK7GLUiN2pvN7F7XrHzsfj5w@mail.gmail.com","threadId":"63029","inReplyTo":"bca5c0ec-0995-421e-9745-330f729357d6@web.de","subject":"Re: Subject: Memory Leak vulnerability in reftable/readwrite_test.c","fromName":"H Z","fromEmail":"shiyuyuranzh@gmail.com","sentAt":"2025-03-01T11:34:07Z","receivedAt":"2025-03-01T11:34:19Z","isPatch":false,"sender":{"key":"shiyuyuranzh@gmail.com","avatar":null},"body":"Thank you very much for your reply.\n\nRené Scharfe 于2025年3月1日周六 19:31写道：\n>\n> Am 01.03.25 um 07:07 schrieb H Z:\n> > Hi, I have found a potential memory leak bug in\n> > reftable/readwrite_test.c and would like to report it to the\n> > maintainers. Can you please help me to check it? Thank you for your\n> > effort and patience!\n>\n> I wouldn't call it a vulnerability if it just affects test code, as it\n> is not executed by git (the executable run by end users). We still want\n> to fix those, however.\n>\n> > Below is the execution sequence of the program that may produce the bug.\n> >\n> > First, in file src/wrapper.c, function xstrdup allocates memory at\n> > line 40 and returns at line 43.\n> > Second, in the file reftable/reader.c, the function init_reader calls\n> > the function xstrdup on line 202 to allocate memory for r->name, which\n> > is the formal parameter of the function init_reader.\n>\n> Not exactly true since 12b9078066 (reftable: handle trivial allocation\n> failures, 2024-10-02); the allocation is done by reftable_strdup() now.\n> And 2de3c0d345 (reftable/reader: inline `init_reader()`, 2024-08-23)\n> got rid of init_reader().\n>\n> > Third, in file reftable/readwrite_test.c, function\n> > test_corrupt_table_empty calls function init_reader on line 935 with\n> > &rd passed as the first argument, causing rd->name to be allocated\n> > memory. rd->name is not freed, which would cause the memory leak\n> > vulnerability.\n>\n> This test was moved to t/unit-tests/t-reftable-readwrite.c by 5b539a5361\n> (t: move reftable/readwrite_test.c to the unit testing framework,\n> 2024-08-13).\n>\n> t_corrupt_table_empty() calls reftable_reader_new() and returns\n> REFTABLE_FORMAT_ERROR before it reaches the reftable_strdup() call, so\n> there is no leak in this test (anymore?).\n>\n> reftable_reader_new() would leak name if its block_source_read_block()\n> or parse_footer() calls failed, though. We could do the name\n> allocation only after those calls to avoid that, but that may\n> complicate matters. Alternative patch below.\n>\n> Also its comment in reftable/reftable-reader.h mentions that\n> reftable_reader_destroy() needs to be called after use, but that\n> function has never existed. Odd.\n>\n> René\n>\n>\n> --- >8 ---\n> Subject: [PATCH] reftable: release name on reftable_reader_new() error\n>\n> If block_source_read_block() or parse_footer() fail, we leak the \"name\"\n> member of struct reftable_reader in reftable_reader_new(). Release it.\n>\n> Reported by: H Z \n> Signed-off-by: René Scharfe \n> ---\n> reftable/reader.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/reftable/reader.c b/reftable/reader.c\n> index 3f2e4b2800..f38c83f140 100644\n> --- a/reftable/reader.c\n> +++ b/reftable/reader.c\n> @@ -666,6 +666,7 @@ int reftable_reader_new(struct reftable_reader **out,\n> reftable_block_done(&footer);\n> reftable_block_done(&header);\n> if (err) {\n> + reftable_free(r->name);\n> reftable_free(r);\n> block_source_close(source);\n> }\n> --\n> 2.48.1\n>\n"},{"id":"513331","messageId":"bca5c0ec-0995-421e-9745-330f729357d6@web.de","threadId":"63029","inReplyTo":"CAAJd+fZSUiiUm05D_eO3HS7p=WoxAWSZo51dUNjyGjUNJBvyGg@mail.gmail.com","subject":"Re: Subject: Memory Leak vulnerability in reftable/readwrite_test.c","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-03-01T11:31:33Z","receivedAt":"2025-03-01T11:36:52Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.03.25 um 07:07 schrieb H Z:\n> Hi, I have found a potential memory leak bug in\n> reftable/readwrite_test.c and would like to report it to the\n> maintainers. Can you please help me to check it? Thank you for your\n> effort and patience!\n\nI wouldn't call it a vulnerability if it just affects test code, as it\nis not executed by git (the executable run by end users). We still want\nto fix those, however.\n\n> Below is the execution sequence of the program that may produce the bug.\n>\n> First, in file src/wrapper.c, function xstrdup allocates memory at\n> line 40 and returns at line 43.\n> Second, in the file reftable/reader.c, the function init_reader calls\n> the function xstrdup on line 202 to allocate memory for r->name, which\n> is the formal parameter of the function init_reader.\n\nNot exactly true since 12b9078066 (reftable: handle trivial allocation\nfailures, 2024-10-02); the allocation is done by reftable_strdup() now.\nAnd 2de3c0d345 (reftable/reader: inline `init_reader()`, 2024-08-23)\ngot rid of init_reader().\n\n> Third, in file reftable/readwrite_test.c, function\n> test_corrupt_table_empty calls function init_reader on line 935 with\n> &rd passed as the first argument, causing rd->name to be allocated\n> memory. rd->name is not freed, which would cause the memory leak\n> vulnerability.\n\nThis test was moved to t/unit-tests/t-reftable-readwrite.c by 5b539a5361\n(t: move reftable/readwrite_test.c to the unit testing framework,\n2024-08-13).\n\nt_corrupt_table_empty() calls reftable_reader_new() and returns\nREFTABLE_FORMAT_ERROR before it reaches the reftable_strdup() call, so\nthere is no leak in this test (anymore?).\n\nreftable_reader_new() would leak name if its block_source_read_block()\nor parse_footer() calls failed, though. We could do the name\nallocation only after those calls to avoid that, but that may\ncomplicate matters. Alternative patch below.\n\nAlso its comment in reftable/reftable-reader.h mentions that\nreftable_reader_destroy() needs to be called after use, but that\nfunction has never existed. Odd.\n\nRené\n\n\n--- >8 ---\nSubject: [PATCH] reftable: release name on reftable_reader_new() error\n\nIf block_source_read_block() or parse_footer() fail, we leak the \"name\"\nmember of struct reftable_reader in reftable_reader_new(). Release it.\n\nReported by: H Z \nSigned-off-by: René Scharfe \n---\n reftable/reader.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 3f2e4b2800..f38c83f140 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -666,6 +666,7 @@ int reftable_reader_new(struct reftable_reader **out,\n \treftable_block_done(&footer);\n \treftable_block_done(&header);\n \tif (err) {\n+\t\treftable_free(r->name);\n \t\treftable_free(r);\n \t\tblock_source_close(source);\n \t}\n--\n2.48.1\n\n"},{"id":"513491","messageId":"20250304063329.GA1283445@coredump.intra.peff.net","threadId":"63029","inReplyTo":"bca5c0ec-0995-421e-9745-330f729357d6@web.de","subject":"Re: Subject: Memory Leak vulnerability in reftable/readwrite_test.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-04T06:33:29Z","receivedAt":"2025-03-04T06:33:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 01, 2025 at 12:31:33PM +0100, René Scharfe wrote:\n\n> --- >8 ---\n> Subject: [PATCH] reftable: release name on reftable_reader_new() error\n> \n> If block_source_read_block() or parse_footer() fail, we leak the \"name\"\n> member of struct reftable_reader in reftable_reader_new(). Release it.\n> \n> Reported by: H Z \n> Signed-off-by: René Scharfe \n> ---\n> reftable/reader.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/reftable/reader.c b/reftable/reader.c\n> index 3f2e4b2800..f38c83f140 100644\n> --- a/reftable/reader.c\n> +++ b/reftable/reader.c\n> @@ -666,6 +666,7 @@ int reftable_reader_new(struct reftable_reader **out,\n> \treftable_block_done(&footer);\n> \treftable_block_done(&header);\n> \tif (err) {\n> +\t\treftable_free(r->name);\n> \t\treftable_free(r);\n> \t\tblock_source_close(source);\n> \t}\n\nCoverity complains that \"r\" might be NULL here. At the top of the\nfunction we do:\n\n REFTABLE_CALLOC_ARRAY(r, 1);\n if (!r) {\n\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n\tgoto done;\n }\n\nand then the done label hits your new line (the \"done:\" is right above\nthe context in your patch). And err of course is non-zero.\n\nSo this probably needs an \"if (r)\", or multiple layered out-labels. Or\nalternatively we could return directly when the first allocation fails,\nsince there is nothing to clean up at that point.\n\n-Peff\n"},{"id":"513501","messageId":"CAAJd+fZ_EK=ZZKptFjuumx2TLsdQ3s7WxTT+n_x3ARwZw23BCw@mail.gmail.com","threadId":"63029","inReplyTo":"20250304063329.GA1283445@coredump.intra.peff.net","subject":"Re: Subject: Memory Leak vulnerability in reftable/readwrite_test.c","fromName":"H Z","fromEmail":"shiyuyuranzh@gmail.com","sentAt":"2025-03-04T07:39:36Z","receivedAt":"2025-03-04T07:39:48Z","isPatch":false,"sender":{"key":"shiyuyuranzh@gmail.com","avatar":null},"body":"Very nice fix, thanks again for doing it!\n\nJeff King 于2025年3月4日周二 14:33写道：\n>\n> On Sat, Mar 01, 2025 at 12:31:33PM +0100, René Scharfe wrote:\n>\n> > --- >8 ---\n> > Subject: [PATCH] reftable: release name on reftable_reader_new() error\n> >\n> > If block_source_read_block() or parse_footer() fail, we leak the \"name\"\n> > member of struct reftable_reader in reftable_reader_new(). Release it.\n> >\n> > Reported by: H Z \n> > Signed-off-by: René Scharfe \n> > ---\n> > reftable/reader.c | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/reftable/reader.c b/reftable/reader.c\n> > index 3f2e4b2800..f38c83f140 100644\n> > --- a/reftable/reader.c\n> > +++ b/reftable/reader.c\n> > @@ -666,6 +666,7 @@ int reftable_reader_new(struct reftable_reader **out,\n> > reftable_block_done(&footer);\n> > reftable_block_done(&header);\n> > if (err) {\n> > + reftable_free(r->name);\n> > reftable_free(r);\n> > block_source_close(source);\n> > }\n>\n> Coverity complains that \"r\" might be NULL here. At the top of the\n> function we do:\n>\n> REFTABLE_CALLOC_ARRAY(r, 1);\n> if (!r) {\n> err = REFTABLE_OUT_OF_MEMORY_ERROR;\n> goto done;\n> }\n>\n> and then the done label hits your new line (the \"done:\" is right above\n> the context in your patch). And err of course is non-zero.\n>\n> So this probably needs an \"if (r)\", or multiple layered out-labels. Or\n> alternatively we could return directly when the first allocation fails,\n> since there is nothing to clean up at that point.\n>\n> -Peff\n"},{"id":"513539","messageId":"77e1fe2e-f652-4aea-bc12-4deae7bcd569@web.de","threadId":"63029","inReplyTo":"20250304063329.GA1283445@coredump.intra.peff.net","subject":"[PATCH v2] reftable: release name on reftable_reader_new() error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-03-04T16:11:54Z","receivedAt":"2025-03-04T16:12:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If block_source_read_block() or parse_footer() fail, we leak the \"name\"\nmember of struct reftable_reader in reftable_reader_new(). Release it.\n\nReported by: H Z \nHelped-by: Jeff King \nSigned-off-by: René Scharfe \n---\nChange since v1: Avoid NULL pointer dereference. Thank you, Peff!\n\n reftable/reader.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 3f2e4b2800..24bae50ac2 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -666,6 +666,8 @@ int reftable_reader_new(struct reftable_reader **out,\n \treftable_block_done(&footer);\n \treftable_block_done(&header);\n \tif (err) {\n+\t\tif (r)\n+\t\t\treftable_free(r->name);\n \t\treftable_free(r);\n \t\tblock_source_close(source);\n \t}\n--\n2.48.1\n"}]}