[PATCH v3 00/11] Reftable coverity fixes

["Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com>]

This series was targeted to 'next'.

This addresses some complaints from Coverity that Peff reported.

v3:

 * revert some changes suggested by stolee
 * add unittest for zlib corruption.

Han-Wen Nienhuys (11):
  reftable: fix OOB stack write in print functions
  reftable: fix resource leak in error path
  reftable: fix resource leak blocksource.c
  reftable: check reftable_stack_auto_compact() return value
  reftable: ignore remove() return value in stack_test.c
  reftable: fix resource warning
  reftable: fix NULL derefs in error paths
  reftable: order unittests by complexity
  reftable: drop stray printf in readwrite_test
  reftable: handle null refnames in reftable_ref_record_equal
  reftable: make reftable_record a tagged union

 reftable/block.c          |  30 ++++---
 reftable/block_test.c     |  22 ++---
 reftable/blocksource.c    |   6 +-
 reftable/generic.c        |  35 ++++----
 reftable/iter.c           |   4 +-
 reftable/merged.c         |  33 +++----
 reftable/pq.c             |   3 +-
 reftable/pq_test.c        |  31 +++----
 reftable/reader.c         |  96 ++++++++++----------
 reftable/readwrite_test.c |  68 +++++++++++++-
 reftable/record.c         | 185 ++++++++++++++++----------------------
 reftable/record.h         |  42 ++++-----
 reftable/record_test.c    | 181 +++++++++++++++++++------------------
 reftable/stack.c          |  15 ++--
 reftable/stack_test.c     |   3 +-
 reftable/writer.c         |  40 +++++----
 t/helper/test-reftable.c  |   9 +-
 17 files changed, 431 insertions(+), 372 deletions(-)


base-commit: fae76fe5da3df25d752f2251b7ccda3f62813aa9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1152%2Fhanwen%2Freftable-coverity-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1152/hanwen/reftable-coverity-v3
Pull-Request: https://github.com/git/git/pull/1152

Range-diff vs v2:

  1:  7c033815183 =  1:  1c1a3ff92bd reftable: fix OOB stack write in print functions
  2:  1ddcfe61ebc !  2:  975a570d388 reftable: fix resource leak in error path
     @@ Metadata
       ## Commit message ##
          reftable: fix resource leak in error path
      
     -    This would be triggered by corrupt files, so it doesn't have test coverage. This
     -    was discovered by a Coverity scan.
     +    Add test coverage for corrupt zlib data.
     +
     +    This problem was discovered by a Coverity scan.
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
     @@ reftable/block.c: int block_reader_init(struct block_reader *br, struct reftable
       		/* Copy over the block header verbatim. It's not compressed. */
       		memcpy(uncompressed, block->data, block_header_skip);
      @@ reftable/block.c: 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);
     +-			reftable_free(uncompressed);
      -			return REFTABLE_ZLIB_ERROR;
      +			err = REFTABLE_ZLIB_ERROR;
      +			goto done;
     @@ reftable/block.c: int block_reader_init(struct block_reader *br, struct reftable
       }
       
       static uint32_t block_reader_restart_offset(struct block_reader *br, int i)
     +
     + ## reftable/readwrite_test.c ##
     +@@ reftable/readwrite_test.c: 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;
     +@@ reftable/readwrite_test.c: 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);
  3:  e052b2a61d6 =  3:  0b9c7176d71 reftable: fix resource leak blocksource.c
  4:  9063137457b =  4:  1dda4ee717f reftable: check reftable_stack_auto_compact() return value
  5:  5020be156ae =  5:  36858e2070b reftable: ignore remove() return value in stack_test.c
  6:  64c18d01cad =  6:  80b1988b885 reftable: fix resource warning
  7:  700387ac5d3 =  7:  2939286924c reftable: fix NULL derefs in error paths
  8:  713f1d09f68 =  8:  9dce18d7349 reftable: order unittests by complexity
  9:  cb601b51a47 =  9:  6b0af68f0b9 reftable: drop stray printf in readwrite_test
 10:  a0f83eff19f = 10:  bff85cb0809 reftable: handle null refnames in reftable_ref_record_equal
 11:  a2743033cfd ! 11:  b3e592b9c27 reftable: make reftable_record a tagged union
     @@ Commit message
          vtable within the structure.
      
          The only snag is that reftable_index_record contain a strbuf, so it cannot be
     -    zero-initialized. To address this, introduce reftable_record_for() to create a
     -    fresh instance, given a record type.
     +    zero-initialized. To address this, use reftable_new_record() to return fresh
     +    instance, given a record type. Since reftable_new_record() doesn't cause heap
     +    allocation anymore, it should be balanced with reftable_record_release() rather
     +    than reftable_record_destroy().
      
          Thanks to Peff for the suggestion.
      
     @@ Commit message
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
       ## reftable/block.c ##
     -@@ reftable/block.c: int block_reader_seek(struct block_reader *br, struct block_iter *it,
     - 		.key = *want,
     - 		.r = br,
     - 	};
     --	struct reftable_record rec = reftable_new_record(block_reader_type(br));
     -+	struct reftable_record rec = reftable_record_for(block_reader_type(br));
     - 	struct strbuf key = STRBUF_INIT;
     - 	int err = 0;
     - 	struct block_iter next = {
      @@ reftable/block.c: int block_reader_seek(struct block_reader *br, struct block_iter *it,
       done:
       	strbuf_release(&key);
     @@ reftable/iter.c: static int indexed_table_ref_iter_next_block(struct indexed_tab
      
       ## reftable/merged.c ##
      @@ reftable/merged.c: static int merged_iter_init(struct merged_iter *mi)
     - {
     - 	int i = 0;
     - 	for (i = 0; i < mi->stack_len; i++) {
     --		struct reftable_record rec = reftable_new_record(mi->typ);
     -+		struct reftable_record rec = reftable_record_for(mi->typ);
     - 		int err = iterator_next(&mi->stack[i], &rec);
     - 		if (err < 0) {
     - 			return err;
     -@@ reftable/merged.c: static int merged_iter_init(struct merged_iter *mi)
       
       		if (err > 0) {
       			reftable_iterator_destroy(&mi->stack[i]);
     @@ reftable/merged.c: static void merged_iter_close(void *p)
      -	struct reftable_record rec = reftable_new_record(mi->typ);
       	struct pq_entry e = {
      -		.rec = rec,
     -+		.rec = reftable_record_for(mi->typ),
     ++		.rec = reftable_new_record(mi->typ),
       		.index = idx,
       	};
      -	int err = iterator_next(&mi->stack[idx], &rec);
     @@ reftable/merged.c: static int merged_iter_next_entry(struct merged_iter *mi,
       	strbuf_release(&entry_key);
       	return 0;
       }
     -@@ reftable/merged.c: static int merged_table_seek_record(struct reftable_merged_table *mt,
     - 		sizeof(struct reftable_iterator) * mt->stack_len);
     - 	struct merged_iter merged = {
     - 		.stack = iters,
     --		.typ = reftable_record_type(rec),
     -+		.typ = rec->type,
     - 		.hash_id = mt->hash_id,
     - 		.suppress_deletions = mt->suppress_deletions,
     - 	};
      @@ reftable/merged.c: int reftable_merged_table_seek_ref(struct reftable_merged_table *mt,
       				   struct reftable_iterator *it,
       				   const char *name)
     @@ reftable/pq_test.c: static void test_pq(void)
      +		struct reftable_record *rec = &e.rec;
       		merged_iter_pqueue_check(pq);
       
     -+		EXPECT(rec->type == BLOCK_TYPE_REF);
     ++		EXPECT(reftable_record_type(rec) == BLOCK_TYPE_REF);
       		if (last) {
      -			EXPECT(strcmp(last, ref->refname) < 0);
      +			EXPECT(strcmp(last, rec->u.ref.refname) < 0);
     @@ reftable/pq_test.c: static void test_pq(void)
      
       ## reftable/reader.c ##
      @@ reftable/reader.c: static int table_iter_next_in_block(struct table_iter *ti,
     - 				    struct reftable_record *rec)
       {
       	int res = block_iter_next(&ti->bi, rec);
     --	if (res == 0 && reftable_record_type(rec) == BLOCK_TYPE_REF) {
     + 	if (res == 0 && reftable_record_type(rec) == BLOCK_TYPE_REF) {
      -		((struct reftable_ref_record *)rec->data)->update_index +=
      -			ti->r->min_update_index;
     -+	if (res == 0 && rec->type == BLOCK_TYPE_REF) {
      +		rec->u.ref.update_index += ti->r->min_update_index;
       	}
       
       	return res;
     -@@ reftable/reader.c: static int table_iter_next_block(struct table_iter *dest,
     - 
     - static int table_iter_next(struct table_iter *ti, struct reftable_record *rec)
     - {
     --	if (reftable_record_type(rec) != ti->typ)
     -+	if (rec->type != ti->typ)
     - 		return REFTABLE_API_ERROR;
     - 
     - 	while (1) {
      @@ reftable/reader.c: static int reader_start(struct reftable_reader *r, struct table_iter *ti,
       static int reader_seek_linear(struct reftable_reader *r, struct table_iter *ti,
       			      struct reftable_record *want)
       {
      -	struct reftable_record rec =
      -		reftable_new_record(reftable_record_type(want));
     -+	struct reftable_record rec = reftable_record_for(want->type);
     ++	struct reftable_record rec = reftable_new_record(reftable_record_type(want));
       	struct strbuf want_key = STRBUF_INIT;
       	struct strbuf got_key = STRBUF_INIT;
       	struct table_iter next = TABLE_ITER_INIT;
     @@ reftable/reader.c: static int reader_seek_indexed(struct reftable_reader *r,
      -	reftable_record_from_index(&want_index_rec, &want_index);
      -	reftable_record_from_index(&index_result_rec, &index_result);
      -
     --	err = reader_start(r, &index_iter, reftable_record_type(rec), 1);
      +	reftable_record_key(rec, &want_index.u.idx.last_key);
     -+	err = reader_start(r, &index_iter, rec->type, 1);
     + 	err = reader_start(r, &index_iter, reftable_record_type(rec), 1);
       	if (err < 0)
       		goto done;
       
     @@ reftable/reader.c: static int reader_seek_indexed(struct reftable_reader *r,
       		if (err < 0)
       			goto done;
       
     --		if (next.typ == reftable_record_type(rec)) {
     -+		if (next.typ == rec->type) {
     - 			err = 0;
     - 			break;
     - 		}
      @@ reftable/reader.c: static int reader_seek_indexed(struct reftable_reader *r,
       done:
       	block_iter_close(&next.bi);
     @@ reftable/reader.c: static int reader_seek_internal(struct reftable_reader *r,
       {
      -	struct reftable_reader_offsets *offs =
      -		reader_offsets_for(r, reftable_record_type(rec));
     -+	struct reftable_reader_offsets *offs = reader_offsets_for(r, rec->type);
     ++	struct reftable_reader_offsets *offs = reader_offsets_for(r, reftable_record_type(rec));
       	uint64_t idx = offs->index_offset;
       	struct table_iter ti = TABLE_ITER_INIT;
       	int err = 0;
     - 	if (idx > 0)
     - 		return reader_seek_indexed(r, it, rec);
     - 
     --	err = reader_start(r, &ti, reftable_record_type(rec), 0);
     -+	err = reader_start(r, &ti, rec->type, 0);
     - 	if (err < 0)
     - 		return err;
     - 	err = reader_seek_linear(r, &ti, rec);
     -@@ reftable/reader.c: static int reader_seek_internal(struct reftable_reader *r,
     - static int reader_seek(struct reftable_reader *r, struct reftable_iterator *it,
     - 		       struct reftable_record *rec)
     - {
     --	uint8_t typ = reftable_record_type(rec);
     -+	uint8_t typ = rec->type;
     - 
     - 	struct reftable_reader_offsets *offs = reader_offsets_for(r, typ);
     - 	if (!offs->is_present) {
      @@ reftable/reader.c: static int reader_seek(struct reftable_reader *r, struct reftable_iterator *it,
       int reftable_reader_seek_ref(struct reftable_reader *r,
       			     struct reftable_iterator *it, const char *name)
     @@ reftable/record.c: static struct reftable_record_vtable reftable_index_record_vt
       void reftable_record_key(struct reftable_record *rec, struct strbuf *dest)
       {
      -	rec->ops->key(rec->data, dest);
     --}
     --
     --uint8_t reftable_record_type(struct reftable_record *rec)
     --{
     --	return rec->ops->type;
      +	reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);
       }
       
     + uint8_t reftable_record_type(struct reftable_record *rec)
     + {
     +-	return rec->ops->type;
     ++	return rec->type;
     + }
     + 
       int reftable_record_encode(struct reftable_record *rec, struct string_view dest,
       			   int hash_size)
       {
     @@ reftable/record.c: void string_view_consume(struct string_view *s, int n)
      +	abort();
      +}
      +
     -+struct reftable_record reftable_record_for(uint8_t typ)
     ++struct reftable_record reftable_new_record(uint8_t typ)
      +{
      +	struct reftable_record clean_idx = {
      +		.type = BLOCK_TYPE_INDEX,
     @@ reftable/record.h: struct reftable_record_vtable {
       int reftable_is_block_type(uint8_t typ);
       
      -/* creates a malloced record of the given type. Dispose with record_destroy */
     --struct reftable_record reftable_new_record(uint8_t typ);
     --
     ++/* return an initialized record for the given type */
     + struct reftable_record reftable_new_record(uint8_t typ);
     + 
       /* Encode `key` into `dest`. Sets `is_restart` to indicate a restart. Returns
     -  * number of bytes written. */
     - int reftable_encode_key(int *is_restart, struct string_view dest,
      @@ reftable/record.h: struct reftable_obj_record {
       	int offset_len;
       };
     @@ reftable/record.h: struct reftable_obj_record {
      +/* record is a generic wrapper for different types of records. It is normally
      + * created on the stack, or embedded within another struct. If the type is
      + * known, a fresh instance can be initialized explicitly. Otherwise, use
     -+ * reftable_record_for() to initialize generically (as the index_record is not
     ++ * reftable_new_record() to initialize generically (as the index_record is not
      + * valid as 0-initialized structure)
      + */
      +struct reftable_record {
     @@ reftable/record.h: struct reftable_obj_record {
      +
       /* see struct record_vtable */
       
     -+/* return an initialized record for the given type */
     -+struct reftable_record reftable_record_for(uint8_t typ);
       void reftable_record_key(struct reftable_record *rec, struct strbuf *dest);
     - uint8_t reftable_record_type(struct reftable_record *rec);
     - void reftable_record_copy_from(struct reftable_record *rec,
      @@ reftable/record.h: int reftable_record_decode(struct reftable_record *rec, struct strbuf key,
       			   int hash_size);
       int reftable_record_is_deletion(struct reftable_record *rec);
     @@ reftable/record_test.c
       {
      -	struct reftable_record copy =
      -		reftable_new_record(reftable_record_type(rec));
     -+	struct reftable_record copy = reftable_record_for(rec->type);
     ++	struct reftable_record copy = reftable_new_record(reftable_record_type(rec));
      +
       	reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
       	/* do it twice to catch memory leaks */
     @@ reftable/record_test.c: static void test_reftable_index_record_roundtrip(void)
       int record_test_main(int argc, const char *argv[])
      
       ## reftable/writer.c ##
     -@@ reftable/writer.c: static int writer_add_record(struct reftable_writer *w,
     - 	strbuf_reset(&w->last_key);
     - 	strbuf_addbuf(&w->last_key, &key);
     - 	if (w->block_writer == NULL) {
     --		writer_reinit_block_writer(w, reftable_record_type(rec));
     -+		writer_reinit_block_writer(w, rec->type);
     - 	}
     - 
     --	assert(block_writer_type(w->block_writer) == reftable_record_type(rec));
     -+	assert(block_writer_type(w->block_writer) == rec->type);
     - 
     - 	if (block_writer_add(w->block_writer, rec) == 0) {
     - 		err = 0;
     -@@ reftable/writer.c: static int writer_add_record(struct reftable_writer *w,
     - 		goto done;
     - 	}
     - 
     --	writer_reinit_block_writer(w, reftable_record_type(rec));
     -+	writer_reinit_block_writer(w, rec->type);
     - 	err = block_writer_add(w->block_writer, rec);
     - 	if (err < 0) {
     - 		goto done;
      @@ reftable/writer.c: done:
       int reftable_writer_add_ref(struct reftable_writer *w,
       			    struct reftable_ref_record *ref)

-- 
gitgitgadget