Re: [PATCH v5 02/16] reftable: fix resource leak in block.c error path
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 22, 2021, 22:51 UTC
- Message-ID
- <xmqqtuf0fe3r.fsf@gitster.g>
- In-Reply-To
- <9ab631a3b29addaa54415139e7f60a79a19a6edb.1640199396.git.gitgitgadget@gmail.com>
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 42 quoted lines
> 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.
Show 22 quoted lines
> 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.
Show 17 quoted lines
> + 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);