Re: [PATCH v5 02/16] reftable: fix resource leak in block.c error path
From: Junio C Hamano <hidden>
Date: 2021-12-22 22:51:11
"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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);