[v3,02/11] reftable: fix resource leak in error path

Commit Message

Han-Wen Nienhuys Dec. 13, 2021, 4:01 p.m. UTC

From: Han-Wen Nienhuys <[e-mail]>

Add test coverage for corrupt zlib data.

This problem was discovered by a Coverity scan.

Signed-off-by: Han-Wen Nienhuys <[e-mail]>
---
 reftable/block.c          | 28 ++++++++++------
 reftable/readwrite_test.c | 67 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 9 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Dec. 13, 2021, 4:19 p.m. UTC | #1

On Mon, Dec 13 2021, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <[e-mail]>
> [...]
> -	return 0;
> +done:
> +	if (uncompressed) {
> +		reftable_free(uncompressed);
> +	}
> +	return err;
>  }

Other things in the codebase don't check for NULL before feeding things
to reftable_free(), and our own free() has a coccicheck rule to catch
this sort of code, we should probably add reftable_free to that list...

>  
>  static uint32_t block_reader_restart_offset(struct block_reader *br, int i)
> diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
> index 5f6bcc2f775..42caf0bde4c 100644
> --- a/reftable/readwrite_test.c
> +++ b/reftable/readwrite_test.c
> @@ -254,6 +254,72 @@ 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 = { NULL };
> +	struct reftable_reader rd = { NULL };
> +	struct reftable_block_source source = { NULL };

Nit: It doesn't matter for semantics, but usually we use "{ 0 }", and
your 1/11 does too. Would be better to do that here for consistency.

> +	for (i = 0; i < sizeof(message)-1; i++) {
> +		message[i] = (uint8_t)(rand() % 64 + ' ');
> +	}

style: braces not needede.

Han-Wen Nienhuys Dec. 13, 2021, 4:44 p.m. UTC | #2

On Mon, Dec 13, 2021 at 5:21 PM Ævar Arnfjörð Bjarmason
<[e-mail]> wrote:
> Other things in the codebase don't check for NULL before feeding things
> to reftable_free(), and our own free() has a coccicheck rule to catch
> this sort of code, we should probably add reftable_free to that list...

Thanks, changed.

> > +     struct reftable_block_source source = { NULL };
>
> Nit: It doesn't matter for semantics, but usually we use "{ 0 }", and
> your 1/11 does too. Would be better to do that here for consistency.

I got a past review where someone complained about this. I don't mind
either way, but would rather not flipflop.

>
> > +     for (i = 0; i < sizeof(message)-1; i++) {
> > +             message[i] = (uint8_t)(rand() % 64 + ' ');
> > +     }
>
> style: braces not needede.

Done.

Junio C Hamano Dec. 13, 2021, 10:10 p.m. UTC | #3

Han-Wen Nienhuys <[e-mail]> writes:

> On Mon, Dec 13, 2021 at 5:21 PM Ævar Arnfjörð Bjarmason
> <[e-mail]> wrote:
>> Other things in the codebase don't check for NULL before feeding things
>> to reftable_free(), and our own free() has a coccicheck rule to catch
>> this sort of code, we should probably add reftable_free to that list...
>
> Thanks, changed.
>
>> > +     struct reftable_block_source source = { NULL };
>>
>> Nit: It doesn't matter for semantics, but usually we use "{ 0 }", and
>> your 1/11 does too. Would be better to do that here for consistency.
>
> I got a past review where someone complained about this. I don't mind
> either way, but would rather not flipflop.

The last part is important.  

For initializers that show the value for the first member in {} to
mean "everything is zero-initialized", only because the language
does not allow us to write

	struct foo var = {};

we historically used { NULL } for pointers and { 0 } for integral
types (primarily because auto checkers like sparse did not like us
to write a NULL pointer as 0), but we started preferring { 0 } as
more recent versions of checkers understand it as an idiom, and we
can freely reorder the struct members if we consistently spell the
"everything is zero-initialized" that way.

So, let's use { 0 } here, too.

Thanks.

Patch

diff --git a/reftable/block.c b/reftable/block.c
index 855e3f5c947..37a85b9576d 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;
 
-	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;
@@ -203,7 +206,7 @@  int block_reader_init(struct block_reader *br, struct reftable_block *block,
 		uLongf src_len = block->len - block_header_skip;
 		/* Log blocks specify the *uncompressed* size in their header.
 		 */
-		uint8_t *uncompressed = reftable_malloc(sz);
+		uncompressed = reftable_malloc(sz);
 
 		/* Copy over the block header verbatim. It's not compressed. */
 		memcpy(uncompressed, block->data, block_header_skip);
@@ -212,16 +215,19 @@  int block_reader_init(struct block_reader *br, struct reftable_block *block,
 		if (Z_OK !=
 		    uncompress2(uncompressed + block_header_skip, &dst_len,
 				block->data + block_header_skip, &src_len)) {
-			reftable_free(uncompressed);
-			return REFTABLE_ZLIB_ERROR;
+			err = REFTABLE_ZLIB_ERROR;
+			goto done;
 		}
 
-		if (dst_len + block_header_skip != sz)
-			return REFTABLE_FORMAT_ERROR;
+		if (dst_len + block_header_skip != sz) {
+			err = REFTABLE_FORMAT_ERROR;
+			goto done;
+		}
 
 		/* We're done with the input data. */
 		reftable_block_done(block);
 		block->data = uncompressed;
+		uncompressed = NULL;
 		block->len = sz;
 		block->source = malloc_block_source();
 		full_block_size = src_len + block_header_skip;
@@ -251,7 +257,11 @@  int block_reader_init(struct block_reader *br, struct reftable_block *block,
 	br->restart_count = restart_count;
 	br->restart_bytes = restart_bytes;
 
-	return 0;
+done:
+	if (uncompressed) {
+		reftable_free(uncompressed);
+	}
+	return err;
 }
 
 static uint32_t block_reader_restart_offset(struct block_reader *br, int i)
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index 5f6bcc2f775..42caf0bde4c 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -254,6 +254,72 @@  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 = { NULL };
+	struct reftable_reader rd = { NULL };
+	struct reftable_block_source source = { NULL };
+	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 };
+	char message[100] = { 0 };
+	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++) {
+		message[i] = (uint8_t)(rand() % 64 + ' ');
+	}
+
+	reftable_writer_set_limits(w, 1, 1);
+
+	err = reftable_writer_add_log(w, &log);
+	EXPECT_ERR(err);
+
+	n = reftable_writer_close(w);
+	EXPECT(n == 0);
+
+	stats = writer_stats(w);
+	EXPECT(stats->log_stats.blocks > 0);
+	reftable_writer_free(w);
+	w = NULL;
+
+	/* corrupt the data. */
+	buf.buf[50] ^= 0x99;
+
+	block_source_from_strbuf(&source, &buf);
+
+	err = init_reader(&rd, &source, "file.log");
+	EXPECT_ERR(err);
+
+	err = reftable_reader_seek_log(&rd, &it, "refname");
+	EXPECT(err == REFTABLE_ZLIB_ERROR);
+
+	reftable_iterator_destroy(&it);
+
+	/* cleanup. */
+	strbuf_release(&buf);
+	reader_close(&rd);
+}
+
 static void test_table_read_write_sequential(void)
 {
 	char **names;
@@ -633,6 +699,7 @@  static void test_corrupt_table(void)
 
 int readwrite_test_main(int argc, const char *argv[])
 {
+	RUN_TEST(test_log_zlib_corruption);
 	RUN_TEST(test_corrupt_table);
 	RUN_TEST(test_corrupt_table_empty);
 	RUN_TEST(test_log_write_read);