{"thread":{"id":"65882","subject":"[PATCH] reftable: fix unlikely leak on API error","startedAt":"2026-06-28T09:03:15Z","lastAt":"2026-06-29T06:22:03Z","messageCount":3,"participants":["Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"546596","messageId":"20260628090314.GA661068@coredump.intra.peff.net","threadId":"65882","inReplyTo":null,"subject":"[PATCH] reftable: fix unlikely leak on API error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-28T09:03:14Z","receivedAt":"2026-06-28T09:03:15Z","isPatch":true,"body":"If the reftable writer sees a bogus block size, we return with\nREFTABLE_API_ERROR, leaking the reftable_writer struct we previously\nallocated. Originally this case was a BUG(), but it became a regular\nreturn in 445f9f4f35 (reftable: stop using `BUG()` in trivial cases,\n2025-02-18).\n\nWe could obviously fix it by calling \"reftable_free(wp)\". But we can\nobserve that we never use the allocated \"wp\" until after we've validated\nthe input options. So let's just bump the allocation down. That fixes\nthe leak, and I think makes the flow of the function more logical\n(we validate our inputs before doing any work).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNoticed by Coverity as a \"new\" problem, though it has been there for\nover a year.  Presumably the nearby changes from 44f46f2be5 (reftable:\nsplit up write options, 2026-06-25) confused it. There's a backlog of\nhundreds of Coverity problems, most of which are garbage, so I tend to\nonly look at the ones it marks as new.\n\n reftable/writer.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 0133b64975..1bd4aa388b 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -152,16 +152,16 @@ int reftable_writer_new(struct reftable_writer **out,\n \tstruct reftable_write_options opts = {0};\n \tstruct reftable_writer *wp;\n \n-\twp = reftable_calloc(1, sizeof(*wp));\n-\tif (!wp)\n-\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n-\n \tif (_opts)\n \t\topts = *_opts;\n \toptions_set_defaults(&opts);\n \tif (opts.block_size >= (1 << 24))\n \t\treturn REFTABLE_API_ERROR;\n \n+\twp = reftable_calloc(1, sizeof(*wp));\n+\tif (!wp)\n+\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n+\n \treftable_buf_init(&wp->block_writer_data.last_key);\n \treftable_buf_init(&wp->last_key);\n \treftable_buf_init(&wp->scratch);\n-- \n2.55.0.rc2.353.gf769b6597e\n"},{"id":"546597","messageId":"20260628090619.GA699336@coredump.intra.peff.net","threadId":"65882","inReplyTo":"20260628090314.GA661068@coredump.intra.peff.net","subject":"Re: [PATCH] reftable: fix unlikely leak on API error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-28T09:06:19Z","receivedAt":"2026-06-28T09:06:20Z","isPatch":true,"body":"On Sun, Jun 28, 2026 at 05:03:14AM -0400, Jeff King wrote:\n\n> Noticed by Coverity as a \"new\" problem, though it has been there for\n> over a year.  Presumably the nearby changes from 44f46f2be5 (reftable:\n> split up write options, 2026-06-25) confused it. There's a backlog of\n> hundreds of Coverity problems, most of which are garbage, so I tend to\n> only look at the ones it marks as new.\n\nThis does conflict textually with 44f46f2be5, which adds a new line\nnearby. Resolving like:\n\ndiff --cc reftable/writer.c\nindex f850e9d599,1bd4aa388b..d969a6a021\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@@ -161,9 -158,10 +157,13 @@@ int reftable_writer_new(struct reftable\n  \tif (opts.block_size >= (1 << 24))\n  \t\treturn REFTABLE_API_ERROR;\n  \n +\tif (!hash_id)\n +\t\thash_id = REFTABLE_HASH_SHA1;\n +\n+ \twp = reftable_calloc(1, sizeof(*wp));\n+ \tif (!wp)\n+ \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n+ \n  \treftable_buf_init(&wp->block_writer_data.last_key);\n  \treftable_buf_init(&wp->last_key);\n  \treftable_buf_init(&wp->scratch);\n\nmakes sense to me, as it keeps the hash_id setting with the \"opts\"\nsetup.\n\n-Peff\n"},{"id":"546627","messageId":"akIPBJLtPqDjQt-A@pks.im","threadId":"65882","inReplyTo":"20260628090314.GA661068@coredump.intra.peff.net","subject":"Re: [PATCH] reftable: fix unlikely leak on API error","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-29T06:21:56Z","receivedAt":"2026-06-29T06:22:03Z","isPatch":true,"body":"On Sun, Jun 28, 2026 at 05:03:14AM -0400, Jeff King wrote:\n> If the reftable writer sees a bogus block size, we return with\n> REFTABLE_API_ERROR, leaking the reftable_writer struct we previously\n> allocated. Originally this case was a BUG(), but it became a regular\n> return in 445f9f4f35 (reftable: stop using `BUG()` in trivial cases,\n> 2025-02-18).\n> \n> We could obviously fix it by calling \"reftable_free(wp)\". But we can\n> observe that we never use the allocated \"wp\" until after we've validated\n> the input options. So let's just bump the allocation down. That fixes\n> the leak, and I think makes the flow of the function more logical\n> (we validate our inputs before doing any work).\n\nAnother alternative would be to create a common exit path where we free\nthe structure when we're about to return an error. But that might not\neven be worth it.\n\n> diff --git a/reftable/writer.c b/reftable/writer.c\n> index 0133b64975..1bd4aa388b 100644\n> --- a/reftable/writer.c\n> +++ b/reftable/writer.c\n> @@ -152,16 +152,16 @@ int reftable_writer_new(struct reftable_writer **out,\n>  \tstruct reftable_write_options opts = {0};\n>  \tstruct reftable_writer *wp;\n>  \n> -\twp = reftable_calloc(1, sizeof(*wp));\n> -\tif (!wp)\n> -\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n> -\n>  \tif (_opts)\n>  \t\topts = *_opts;\n>  \toptions_set_defaults(&opts);\n>  \tif (opts.block_size >= (1 << 24))\n>  \t\treturn REFTABLE_API_ERROR;\n>  \n> +\twp = reftable_calloc(1, sizeof(*wp));\n> +\tif (!wp)\n> +\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n> +\n>  \treftable_buf_init(&wp->block_writer_data.last_key);\n>  \treftable_buf_init(&wp->last_key);\n>  \treftable_buf_init(&wp->scratch);\n\nMakes sense. There's another early return in this function, but there we\nalready know to free the writer.\n\nThanks!\n\nPatrick\n"}]}