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(©, 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)Han-Wen Nienhuys via GitGitGadget <gitgitgadget@gmail.com>