Re: [PATCH v5 02/16] reftable: fix resource leak in block.c error path

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

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/reftable/reader.c b/reftable/reader.c
> index 006709a645a..0d16b098f5e 100644
> --- a/reftable/reader.c
> +++ b/reftable/reader.c
> @@ -290,28 +290,34 @@ int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,
>  
>  	err = reader_get_block(r, &block, next_off, guess_block_size);
>  	if (err < 0)
> -		return err;
> +		goto done;
>  
>  	block_size = extract_block_size(block.data, &block_typ, next_off,
>  					r->version);
> -	if (block_size < 0)
> -		return block_size;
> -
> +	if (block_size < 0) {
> +		err = block_size;
> +		goto done;
> +	}
>  	if (want_typ != BLOCK_TYPE_ANY && block_typ != want_typ) {
> -		reftable_block_done(&block);
> -		return 1;
> +		err = 1;
> +		goto done;
>  	}
>  
>  	if (block_size > guess_block_size) {
>  		reftable_block_done(&block);
>  		err = reader_get_block(r, &block, next_off, block_size);
>  		if (err < 0) {
> -			return err;
> +			goto done;
>  		}
>  	}
>  
> -	return block_reader_init(br, &block, header_off, r->block_size,
> -				 hash_size(r->hash_id));
> +	err = block_reader_init(br, &block, header_off, r->block_size,
> +				hash_size(r->hash_id));
> +done:
> +	if (err)

Is the convention for reader_init() different from all other
functions?  It makes reader wonder why this is not

	if (err < 0)

even though it is not wrong per-se (as long as "zero means success"
is a part of the return value convention).

> +		reftable_block_done(&block);
> +
> +	return err;
>  }

This one is new in this round.  All look good, other than that one
check for error return.

> diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
> index 5f6bcc2f775..6e88182a83a 100644
> --- a/reftable/readwrite_test.c
> +++ b/reftable/readwrite_test.c
> @@ -254,6 +254,71 @@ static void test_log_write_read(void)
>  	reader_close(&rd);
>  }
>  
> +static void test_log_zlib_corruption(void)
> +{
> +	struct reftable_write_options opts = {
> +		.block_size = 256,
> +	};
> +	struct reftable_iterator it = { 0 };
> +	struct reftable_reader rd = { 0 };
> +	struct reftable_block_source source = { 0 };
> +	struct strbuf buf = STRBUF_INIT;
> +	struct reftable_writer *w =
> +		reftable_new_writer(&strbuf_add_void, &buf, &opts);
> +	const struct reftable_stats *stats = NULL;
> +	uint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };
> +	uint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };

Will this code be exercised when compiling with SHA256 support?  If
not, this is perfectly fine, but otherwise, this needs to be MAX,
not SHA1, no?

> +	char message[100] = { 0 };

You're filling this to the sizeof(message)-1, so we can afford to
leave it uninitialized.

> +	int err, i, n;
> +
> +	struct reftable_log_record log = {
> +		.refname = "refname",
> +		.value_type = REFTABLE_LOG_UPDATE,
> +		.value = {
> +			.update = {
> +				.new_hash = hash1,
> +				.old_hash = hash2,
> +				.name = "My Name",
> +				.email = "myname@invalid",
> +				.message = message,
> +			},
> +		},
> +	};
> +
> +	for (i = 0; i < sizeof(message)-1; i++)

Style: SP around "-" on both sides.

> +		message[i] = (uint8_t)(rand() % 64 + ' ');
> +
> +	reftable_writer_set_limits(w, 1, 1);