patchreftable: fix unlikely leak on API error

3 messages between Jun 28, 2026 and Jun 29, 2026, from Jeff King, Patrick Steinhardt.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Jeff KingJun 28, 2026, 09:03 UTC on lore

If the reftable writer sees a bogus block size, we return with REFTABLE_API_ERROR, leaking the reftable_writer struct we previously allocated. Originally this case was a BUG(), but it became a regular return in 445f9f4f35 (reftable: stop using `BUG()` in trivial cases, 2025-02-18).

We could obviously fix it by calling "reftable_free(wp)". But we can observe that we never use the allocated "wp" until after we've validated the input options. So let's just bump the allocation down. That fixes the leak, and I think makes the flow of the function more logical (we validate our inputs before doing any work).

Signed-off-by: Jeff King <peff@peff.net>
---
Noticed by Coverity as a "new" problem, though it has been there for
over a year.  Presumably the nearby changes from 44f46f2be5 (reftable:
split up write options, 2026-06-25) confused it. There's a backlog of
hundreds of Coverity problems, most of which are garbage, so I tend to
only look at the ones it marks as new.

 reftable/writer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Show changes to reftable/writer.c +4 −4

diff --git a/reftable/writer.c b/reftable/writer.c
index 0133b64975..1bd4aa388b 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -152,16 +152,16 @@ int reftable_writer_new(struct reftable_writer **out,
 	struct reftable_write_options opts = {0};
 	struct reftable_writer *wp;
 
-	wp = reftable_calloc(1, sizeof(*wp));
-	if (!wp)
-		return REFTABLE_OUT_OF_MEMORY_ERROR;
-
 	if (_opts)
 		opts = *_opts;
 	options_set_defaults(&opts);
 	if (opts.block_size >= (1 << 24))
 		return REFTABLE_API_ERROR;
 
+	wp = reftable_calloc(1, sizeof(*wp));
+	if (!wp)
+		return REFTABLE_OUT_OF_MEMORY_ERROR;
+
 	reftable_buf_init(&wp->block_writer_data.last_key);
 	reftable_buf_init(&wp->last_key);
 	reftable_buf_init(&wp->scratch);

-- 
2.55.0.rc2.353.gf769b6597e

Jeff KingJun 28, 2026, 09:06 UTC in reply to Jeff King on lore

Re: [PATCH] reftable: fix unlikely leak on API error

On Sun, Jun 28, 2026 at 05:03:14AM -0400, Jeff King wrote:

Show 5 quoted lines

> Noticed by Coverity as a "new" problem, though it has been there for
> over a year.  Presumably the nearby changes from 44f46f2be5 (reftable:
> split up write options, 2026-06-25) confused it. There's a backlog of
> hundreds of Coverity problems, most of which are garbage, so I tend to
> only look at the ones it marks as new.

This does conflict textually with 44f46f2be5, which adds a new line nearby. Resolving like:

diff --cc reftable/writer.c index f850e9d599,1bd4aa388b..d969a6a021

Show changes to reftable/writer.c +4 −1

--- a/reftable/writer.c
+++ b/reftable/writer.c
@@@ -161,9 -158,10 +157,13 @@@ int reftable_writer_new(struct reftable
  	if (opts.block_size >= (1 << 24))
  		return REFTABLE_API_ERROR;
  
 +	if (!hash_id)
 +		hash_id = REFTABLE_HASH_SHA1;
 +
+ 	wp = reftable_calloc(1, sizeof(*wp));
+ 	if (!wp)
+ 		return REFTABLE_OUT_OF_MEMORY_ERROR;
+ 
  	reftable_buf_init(&wp->block_writer_data.last_key);
  	reftable_buf_init(&wp->last_key);
  	reftable_buf_init(&wp->scratch);

makes sense to me, as it keeps the hash_id setting with the "opts"
setup.

-Peff

Patrick SteinhardtJun 29, 2026, 06:21 UTC in reply to Jeff King on lore

Re: [PATCH] reftable: fix unlikely leak on API error

On Sun, Jun 28, 2026 at 05:03:14AM -0400, Jeff King wrote:

Show 11 quoted lines

> If the reftable writer sees a bogus block size, we return with
> REFTABLE_API_ERROR, leaking the reftable_writer struct we previously
> allocated. Originally this case was a BUG(), but it became a regular
> return in 445f9f4f35 (reftable: stop using `BUG()` in trivial cases,
> 2025-02-18).
> 
> We could obviously fix it by calling "reftable_free(wp)". But we can
> observe that we never use the allocated "wp" until after we've validated
> the input options. So let's just bump the allocation down. That fixes
> the leak, and I think makes the flow of the function more logical
> (we validate our inputs before doing any work).

Another alternative would be to create a common exit path where we free the structure when we're about to return an error. But that might not even be worth it.

Show 25 quoted lines

> diff --git a/reftable/writer.c b/reftable/writer.c
> index 0133b64975..1bd4aa388b 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -152,16 +152,16 @@ int reftable_writer_new(struct reftable_writer **out,
>  	struct reftable_write_options opts = {0};
>  	struct reftable_writer *wp;
>  
> -	wp = reftable_calloc(1, sizeof(*wp));
> -	if (!wp)
> -		return REFTABLE_OUT_OF_MEMORY_ERROR;
> -
>  	if (_opts)
>  		opts = *_opts;
>  	options_set_defaults(&opts);
>  	if (opts.block_size >= (1 << 24))
>  		return REFTABLE_API_ERROR;
>  
> +	wp = reftable_calloc(1, sizeof(*wp));
> +	if (!wp)
> +		return REFTABLE_OUT_OF_MEMORY_ERROR;
> +
>  	reftable_buf_init(&wp->block_writer_data.last_key);
>  	reftable_buf_init(&wp->last_key);
>  	reftable_buf_init(&wp->scratch);

Makes sense. There's another early return in this function, but there we already know to free the writer.

Thanks!

Patrick