Re: [PATCH 02/10] reftable: fix resource leak in error path

From: Derrick Stolee <stolee@gmail.com>
Date: Dec 8, 2021, 14:30 UTC
Message-ID: <a7b4c6aa-c8cd-eaa7-f4b5-dee0028cc4d2@gmail.com>
In-Reply-To: <603bd1d4f6e4ceda02a5165eedd3ae5b92b8929e.1638899124.git.gitgitgadget@gmail.com>

On 12/7/2021 12:45 PM, Han-Wen Nienhuys via GitGitGadget wrote:

Show 24 quoted lines

> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> This would be triggered by corrupt files, so it doesn't have test coverage. This
> was discovered by a Coverity scan.
> 
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  reftable/block.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/reftable/block.c b/reftable/block.c
> index 855e3f5c947..d7347bb3152 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -188,13 +188,16 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
>  	uint32_t full_block_size = table_block_size;
>  	uint8_t typ = block->data[header_off];
>  	uint32_t sz = get_be24(block->data + header_off + 1);
> -
> +	int err = 0;
>  	uint16_t restart_count = 0;
>  	uint32_t restart_start = 0;
>  	uint8_t *restart_bytes = NULL;
> +	uint8_t *uncompressed = NULL;

You define this here...

Show 14 quoted lines

>  
> -	if (!reftable_is_block_type(typ))
> -		return REFTABLE_FORMAT_ERROR;
> +	if (!reftable_is_block_type(typ)) {
> +		err =  REFTABLE_FORMAT_ERROR;
> +		goto done;
> +	}
>  
>  	if (typ == BLOCK_TYPE_LOG) {
>  		int block_header_skip = 4 + header_off;
> @@ -213,15 +216,19 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
>  		    uncompress2(uncompressed + block_header_skip, &dst_len,
>  				block->data + block_header_skip, &src_len)) {
>  			reftable_free(uncompressed);

But it is already used here, because it is defined within the if() block. You need to remove that definition, too, or your change here does nothing.

...

>  		/* We're done with the input data. */
>  		reftable_block_done(block);
>  		block->data = uncompressed;
> +		uncompressed = NULL;

For example, this nullifies the local version of uncompressed.

(I had to double-check that this even compiled, which was surprising to me.)

Show 5 quoted lines

> +done:
> +	if (uncompressed) {
> +		reftable_free(uncompressed);
> +	}
> +	return err;

Remove that local declaration and this will be a good patch.

Thanks, -Stolee