reftable: fix resource leak in block.c error path

Add test coverage for corrupt zlib data. Fix memory leaks demonstrated by
unittest.

This problem was discovered by a Coverity scan.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/reftable/block.c b/reftable/block.c
index 855e3f5c9472d04e1a9ce00da72c2316523f7ff1..6c8e8705205ff72104efc012c4ec7a6044cb326e 100644 (file)
--- 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,9 @@ 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:
+       reftable_free(uncompressed);
+       return err;
 }
 
 static uint32_t block_reader_restart_offset(struct block_reader *br, int i)

diff --git a/reftable/reader.c b/reftable/reader.c
index 006709a645aea2ddbc914673710e66fa2343d569..35781593a29579e8bbb70808f7ea6c72f75a921d 100644 (file)
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -290,28 +290,33 @@ 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:
+       reftable_block_done(&block);
+
+       return err;
 }
 
 static int table_iter_next_block(struct table_iter *dest,

diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index 5f6bcc2f775fdbc2c7b6ea81baf6f5011fd35282..287cbc5999671a8a62d454efbb4bafc40a067c80 100644 (file)
--- 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 };
+       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 +698,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);