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

From: Jeff King <peff@peff.net>
Date: Mar 4, 2025, 06:33 UTC
Message-ID: <20250304063329.GA1283445@coredump.intra.peff.net>
In-Reply-To: <bca5c0ec-0995-421e-9745-330f729357d6@web.de>

On Sat, Mar 01, 2025 at 12:31:33PM +0100, René Scharfe wrote:

Show 24 quoted lines

> --- >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);
>  	}

Coverity complains that "r" might be NULL here. At the top of the function we do:

  REFTABLE_CALLOC_ARRAY(r, 1);
  if (!r) {
	err = REFTABLE_OUT_OF_MEMORY_ERROR;
	goto done;
  }

and then the done label hits your new line (the "done:" is right above the context in your patch). And err of course is non-zero.

So this probably needs an "if (r)", or multiple layered out-labels. Or alternatively we could return directly when the first allocation fails, since there is nothing to clean up at that point.

-Peff