Re: [PATCH v2 03/11] reftable/block: check deflateInit() return value

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> The function already uses REFTABLE_ZLIB_ERROR for deflate()
> failures later in the code path (lines 171, 199), so returning
> the same error code for deflateInit() failure is consistent.
>
> Pointed out by Coverity.
>
> Assisted-by: Claude Opus 4.6
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  reftable/block.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index 920b3f4486..ec81fd0493 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -87,7 +87,8 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
>  		REFTABLE_CALLOC_ARRAY(bw->zstream, 1);
>  		if (!bw->zstream)
>  			return REFTABLE_OUT_OF_MEMORY_ERROR;
> -		deflateInit(bw->zstream, 9);
> +		if (deflateInit(bw->zstream, 9) != Z_OK)
> +			return REFTABLE_ZLIB_ERROR;
>  	}

Presumably bw->zstream occupies some memory allocated on the heap.
Does a failing deflateInit() release it?  If not, do we leak memory
here?    Or do we need

		if (deflateInit(bw->zstream, 9) !+ Z_OK) {
			REFTABLE_FREE_AND_NULL(bw->zstream);
			return REFTABLE_ZLIB_ERROR;
		}

here?

Noticing and returning an error is a good first step.  The only
caller of it is reftable/writer.c:writer_reinit_block_writer(), and
it checks and relays the error code from here to its callers, but
not all callers of it check the error condition.  The most blatant
offender being reftable_writer_new() that happily keeps going.  I do
not know if we end up calling zlib on bw->zstream for such a broken
block_writer(), as I didn't trace the call graph fully myself.

Stepping back a bit, if REFTABLE_CALLOC_ARRAY() fails, bw->zstream
would be NULL, and a caller that does not check the return value of
writer_reinit_block_writer() would be holding a block writer whose
zstream is NULL.  If the block writer is eventually passed to the
block_writer_release() function, we would call deflateEnd() on it.