On Sat, Mar 01, 2025 at 12:31:33PM +0100, René Scharfe wrote:
> --- >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);
> }
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
