This series was targeted to 'next'. This addresses some complaints from Coverity that Peff reported. v5: * add generic record equality * add generic record printing * const correctness for record-reftable.h * fix windows crash. * 0-length memcpy paranoia * drop unused file. Han-Wen Nienhuys (16): reftable: fix OOB stack write in print functions reftable: fix resource leak in block.c 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: all xxx_free() functions accept NULL arguments 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.h function signatures const correct reftable: implement record equality generically reftable: remove outdated file reftable.c reftable: make reftable_record a tagged union reftable: add print functions to the record types reftable: be more paranoid about 0-length memcpy calls reftable/block.c | 28 ++- reftable/block_test.c | 22 +-- reftable/blocksource.c | 6 +- reftable/generic.c | 41 ++-- reftable/iter.c | 4 +- reftable/merged.c | 33 ++-- reftable/pq.c | 3 +- reftable/pq_test.c | 27 ++- reftable/reader.c | 113 ++++++----- reftable/readwrite_test.c | 67 ++++++- reftable/record.c | 376 ++++++++++++++++++++++++------------- reftable/record.h | 49 ++--- reftable/record_test.c | 197 +++++++++---------- reftable/reftable-record.h | 14 +- reftable/reftable.c | 115 ------------ reftable/stack.c | 12 +- reftable/stack_test.c | 3 +- reftable/writer.c | 43 +++-- t/helper/test-reftable.c | 9 +- 19 files changed, 629 insertions(+), 533 deletions(-) delete mode 100644 reftable/reftable.c base-commit: 69a9c10c95e28df457e33b3c7400b16caf2e2962 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1152%2Fhanwen%2Freftable-coverity-v5 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1152/hanwen/reftable-coverity-v5 Pull-Request: https://github.com/git/git/pull/1152 Range-diff vs v4: 1: 1c1a3ff92bd = 1: e7f1be7bbec reftable: fix OOB stack write in print functions 2: 923293b79b4 ! 2: 9ab631a3b29 reftable: fix resource leak in block.c error path @@ Metadata ## Commit message ## reftable: fix resource leak in block.c error path - Add test coverage for corrupt zlib data. + Add test coverage for corrupt zlib data. Fix memory leaks demonstrated by + unittest. This problem was discovered by a Coverity scan. @@ 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/reader.c ## +@@ reftable/reader.c: 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) ++ reftable_block_done(&block); ++ ++ return err; + } + + static int table_iter_next_block(struct table_iter *dest, + ## reftable/readwrite_test.c ## @@ reftable/readwrite_test.c: static void test_log_write_read(void) reader_close(&rd); 3: 7a914f77756 = 3: 8301000e83b reftable: fix resource leak blocksource.c 4: 51b4a17a2e1 = 4: e6e53aabc39 reftable: check reftable_stack_auto_compact() return value 5: 43989afcb5a = 5: 7551bcdd917 reftable: ignore remove() return value in stack_test.c 6: 1c7f15d811c = 6: 700a4e247e7 reftable: fix resource warning 7: 47ba5ddceb8 = 7: 30016fee4c8 reftable: all xxx_free() functions accept NULL arguments 8: aba8b8113ad = 8: f90846cc103 reftable: order unittests by complexity 9: f2af404d7f1 = 9: 425e12d3667 reftable: drop stray printf in readwrite_test 10: d2ce1bfef9f = 10: 67d858ec59e reftable: handle null refnames in reftable_ref_record_equal -: ----------- > 11: 1e50924894f reftable: make reftable-record.h function signatures const correct -: ----------- > 12: c81b17ad57e reftable: implement record equality generically -: ----------- > 13: 4175089ec43 reftable: remove outdated file reftable.c 11: 0ffc1cf0e42 ! 14: ede2e792ab6 reftable: make reftable_record a tagged union @@ reftable/generic.c: void reftable_iterator_destroy(struct reftable_iterator *it) - struct reftable_record rec = { NULL }; - reftable_record_from_ref(&rec, ref); - return iterator_next(it, &rec); -+ struct reftable_record rec = { .type = BLOCK_TYPE_REF }; ++ struct reftable_record rec = { ++ .type = BLOCK_TYPE_REF, ++ .u.ref = *ref, ++ }; + int err = iterator_next(it, &rec); + *ref = rec.u.ref; + return err; @@ reftable/generic.c: void reftable_iterator_destroy(struct reftable_iterator *it) - struct reftable_record rec = { NULL }; - reftable_record_from_log(&rec, log); - return iterator_next(it, &rec); -+ struct reftable_record rec = { .type = BLOCK_TYPE_LOG }; ++ struct reftable_record rec = { ++ .type = BLOCK_TYPE_LOG, ++ .u.log = *log, ++ }; + int err = iterator_next(it, &rec); + *log = rec.u.log; + return err; @@ reftable/merged.c: int reftable_merged_table_seek_log_at(struct reftable_merged_ - struct reftable_log_record log = { - .refname = (char *)name, - .update_index = update_index, -+ struct reftable_record rec = { -+ .type = BLOCK_TYPE_LOG, -+ .u.log = { -+ .refname = (char *)name, -+ .update_index = update_index, -+ } - }; +- }; - struct reftable_record rec = { NULL }; - reftable_record_from_log(&rec, &log); ++ struct reftable_record rec = { .type = BLOCK_TYPE_LOG, ++ .u.log = { ++ .refname = (char *)name, ++ .update_index = update_index, ++ } }; return merged_table_seek_record(mt, it, &rec); } @@ reftable/pq_test.c: static void test_pq(void) - - reftable_record_as_ref(&rec)->refname = names[i]; - e.rec = rec; -+ struct pq_entry e = { -+ .rec = { -+ .type = BLOCK_TYPE_REF, -+ .u.ref = { -+ .refname = names[i], -+ } -+ } -+ }; ++ struct pq_entry e = { .rec = { .type = BLOCK_TYPE_REF, ++ .u.ref = { ++ .refname = names[i], ++ } } }; merged_iter_pqueue_add(&pq, e); merged_iter_pqueue_check(pq); i = (i * 7) % N; @@ reftable/reader.c: static int table_iter_next_in_block(struct table_iter *ti, } return res; -@@ 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_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_linear(struct reftable_reader *r, struct table_iter *ti, done: @@ reftable/reader.c: static int reader_seek_indexed(struct reftable_reader *r, - struct reftable_index_record index_result = { .last_key = STRBUF_INIT }; - struct reftable_record index_result_rec = { NULL }; + struct reftable_record want_index = { -+ .type = BLOCK_TYPE_INDEX, -+ .u.idx = { .last_key = STRBUF_INIT } ++ .type = BLOCK_TYPE_INDEX, .u.idx = { .last_key = STRBUF_INIT } + }; + struct reftable_record index_result = { + .type = BLOCK_TYPE_INDEX, @@ reftable/reader.c: static int reader_seek_indexed(struct reftable_reader *r, return err; } -@@ reftable/reader.c: static int reader_seek_internal(struct reftable_reader *r, - struct reftable_iterator *it, - struct reftable_record *rec) - { -- struct reftable_reader_offsets *offs = -- reader_offsets_for(r, reftable_record_type(rec)); -+ 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; @@ 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/reader.c: int reftable_reader_seek_log_at(struct reftable_reader *r, - struct reftable_log_record log = { - .refname = (char *)name, - .update_index = update_index, -+ struct reftable_record rec = { -+ .type = BLOCK_TYPE_LOG, -+ .u.log = { -+ .refname = (char *)name, -+ .update_index = update_index, -+ } - }; +- }; - struct reftable_record rec = { NULL }; - reftable_record_from_log(&rec, &log); ++ struct reftable_record rec = { .type = BLOCK_TYPE_LOG, ++ .u.log = { ++ .refname = (char *)name, ++ .update_index = update_index, ++ } }; return reader_seek(r, it, &rec); } @@ reftable/reader.c: static int reftable_reader_refs_for_indexed(struct reftable_r err = new_indexed_table_ref_iter(&itr, r, oid, hash_size(r->hash_id), - got.offsets, got.offset_len); -+ got.u.obj.offsets, got.u.obj.offset_len); ++ got.u.obj.offsets, ++ got.u.obj.offset_len); if (err < 0) goto done; - got.offsets = NULL; @@ reftable/record.c: https://developers.google.com/open-source/licenses/bsd int get_var_int(uint64_t *dest, struct string_view *in) { int ptr = 0; +@@ reftable/record.c: static void reftable_obj_record_copy_from(void *rec, const void *src_rec, + (const struct reftable_obj_record *)src_rec; + + reftable_obj_record_release(obj); +- *obj = *src; +- obj->hash_prefix = reftable_malloc(obj->hash_prefix_len); +- memcpy(obj->hash_prefix, src->hash_prefix, obj->hash_prefix_len); ++ obj->hash_prefix = reftable_malloc(src->hash_prefix_len); ++ obj->hash_prefix_len = src->hash_prefix_len; ++ if (src->hash_prefix_len) ++ memcpy(obj->hash_prefix, src->hash_prefix, obj->hash_prefix_len); + +- obj->offsets = reftable_malloc(obj->offset_len * sizeof(uint64_t)); +- COPY_ARRAY(obj->offsets, src->offsets, obj->offset_len); ++ obj->offsets = reftable_malloc(src->offset_len * sizeof(uint64_t)); ++ obj->offset_len = src->offset_len; ++ COPY_ARRAY(obj->offsets, src->offsets, src->offset_len); + } + + static uint8_t reftable_obj_record_val_type(const void *rec) @@ reftable/record.c: static struct reftable_record_vtable reftable_log_record_vtable = { - .is_deletion = &reftable_log_record_is_deletion_void, + .equal = &reftable_log_record_equal_void }; -struct reftable_record reftable_new_record(uint8_t typ) @@ reftable/record.c: static struct reftable_record_vtable reftable_index_record_vt int reftable_record_is_deletion(struct reftable_record *rec) { - return rec->ops->is_deletion(rec->data); ++ return reftable_record_vtable(rec)->is_deletion( ++ reftable_record_data(rec)); + } + + int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size) + { +- if (a->ops != b->ops) ++ if (a->type != b->type) + return 0; +- return a->ops->equal(a->data, b->data, hash_size); -} - -void reftable_record_from_ref(struct reftable_record *rec, @@ reftable/record.c: static struct reftable_record_vtable reftable_index_record_vt -{ - assert(reftable_record_type(rec) == BLOCK_TYPE_LOG); - return rec->data; -+ return reftable_record_vtable(rec)->is_deletion( -+ reftable_record_data(rec)); ++ return reftable_record_vtable(a)->equal( ++ reftable_record_data(a), reftable_record_data(b), hash_size); } static int hash_equal(uint8_t *a, uint8_t *b, int hash_size) @@ reftable/record.c: void string_view_consume(struct string_view *s, int n) + +struct reftable_record reftable_new_record(uint8_t typ) +{ -+ struct reftable_record clean_idx = { -+ .type = BLOCK_TYPE_INDEX, -+ .u.idx = { -+ .last_key = STRBUF_INIT, -+ }, -+ }; + struct reftable_record clean = { + .type = typ, + }; + -+ if (typ == BLOCK_TYPE_INDEX) { -+ return clean_idx; ++ /* the following is involved, but the naive solution (just return ++ * `clean` as is, except for BLOCK_TYPE_INDEX), returns a garbage ++ * clean.u.obj.offsets pointer on Windows VS CI. Go figure. ++ */ ++ switch (typ) { ++ case BLOCK_TYPE_OBJ: ++ { ++ struct reftable_obj_record obj = { 0 }; ++ clean.u.obj = obj; ++ break; ++ } ++ case BLOCK_TYPE_INDEX: ++ { ++ struct reftable_index_record idx = { ++ .last_key = STRBUF_INIT, ++ }; ++ clean.u.idx = idx; ++ break; ++ } ++ case BLOCK_TYPE_REF: ++ { ++ struct reftable_ref_record ref = { 0 }; ++ clean.u.ref = ref; ++ break; ++ } ++ case BLOCK_TYPE_LOG: ++ { ++ struct reftable_log_record log = { 0 }; ++ clean.u.log = log; ++ break; ++ } + } -+ + return clean; +} ## reftable/record.h ## @@ reftable/record.h: struct reftable_record_vtable { - int (*is_deletion)(const void *rec); + int (*equal)(const void *a, const void *b, int hash_size); }; -/* record is a generic wrapper for different types of records. */ @@ reftable/record.h: struct reftable_obj_record { +}; + /* see struct record_vtable */ - + int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size); void reftable_record_key(struct reftable_record *rec, struct strbuf *dest); @@ reftable/record.h: int reftable_record_decode(struct reftable_record *rec, struct strbuf key, int hash_size); @@ reftable/record_test.c { - struct reftable_record copy = - reftable_new_record(reftable_record_type(rec)); -+ struct reftable_record copy = reftable_new_record(reftable_record_type(rec)); ++ struct reftable_record copy = { 0 }; ++ uint8_t typ; + ++ typ = reftable_record_type(rec); ++ copy = reftable_new_record(typ); reftable_record_copy_from(©, rec, GIT_SHA1_RAWSZ); /* do it twice to catch memory leaks */ reftable_record_copy_from(©, rec, GIT_SHA1_RAWSZ); -- switch (reftable_record_type(©)) { -+ switch (copy.type) { - case BLOCK_TYPE_REF: -- EXPECT(reftable_ref_record_equal(reftable_record_as_ref(©), -- reftable_record_as_ref(rec), -+ EXPECT(reftable_ref_record_equal(©.u.ref, &rec->u.ref, - GIT_SHA1_RAWSZ)); - break; - case BLOCK_TYPE_LOG: -- EXPECT(reftable_log_record_equal(reftable_record_as_log(©), -- reftable_record_as_log(rec), -+ EXPECT(reftable_log_record_equal(©.u.log, &rec->u.log, - GIT_SHA1_RAWSZ)); - break; - } + EXPECT(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); - reftable_record_destroy(©); + reftable_record_release(©); } @@ reftable/record_test.c: static void test_reftable_ref_record_roundtrip(void) + struct reftable_record in = { + .type = BLOCK_TYPE_REF, + }; -+ struct reftable_record out = { -+ .type = BLOCK_TYPE_REF -+ }; ++ struct reftable_record out = { .type = BLOCK_TYPE_REF }; struct strbuf key = STRBUF_INIT; - struct reftable_record rec = { NULL }; uint8_t buffer[1024] = { 0 }; @@ reftable/record_test.c: static void test_reftable_ref_record_roundtrip(void) - set_hash(in.value.val2.value, 1); - in.value.val2.target_value = + in.u.ref.value.val2.value = -+ reftable_malloc(GIT_SHA1_RAWSZ); -+ set_hash(in.u.ref.value.val2.value, 1); -+ in.u.ref.value.val2.target_value = reftable_malloc(GIT_SHA1_RAWSZ); - set_hash(in.value.val2.target_value, 2); ++ set_hash(in.u.ref.value.val2.value, 1); ++ in.u.ref.value.val2.target_value = ++ reftable_malloc(GIT_SHA1_RAWSZ); + set_hash(in.u.ref.value.val2.target_value, 2); break; case REFTABLE_REF_SYMREF: @@ reftable/record_test.c: static void test_reftable_log_record_roundtrip(void) set_test_hash(in[0].value.update.old_hash, 2); for (i = 0; i < ARRAY_SIZE(in); i++) { - struct reftable_record rec = { NULL }; -+ struct reftable_record rec = { -+ .type = BLOCK_TYPE_LOG -+ }; ++ struct reftable_record rec = { .type = BLOCK_TYPE_LOG }; struct strbuf key = STRBUF_INIT; uint8_t buffer[1024] = { 0 }; struct string_view dest = { @@ reftable/record_test.c: static void test_reftable_obj_record_roundtrip(void) struct strbuf key = STRBUF_INIT; - struct reftable_obj_record out = { NULL }; - struct reftable_record rec_out = { NULL }; -+ struct reftable_record out = { -+ .type = BLOCK_TYPE_OBJ -+ }; ++ struct reftable_record out = { .type = BLOCK_TYPE_OBJ }; int n, m; uint8_t extra; @@ reftable/record_test.c: static void test_reftable_obj_record_roundtrip(void) GIT_SHA1_RAWSZ); EXPECT(n == m); -- EXPECT(in.hash_prefix_len == out.hash_prefix_len); -- EXPECT(in.offset_len == out.offset_len); -- -- EXPECT(!memcmp(in.hash_prefix, out.hash_prefix, -- in.hash_prefix_len)); -- EXPECT(0 == memcmp(in.offsets, out.offsets, -- sizeof(uint64_t) * in.offset_len)); -+ EXPECT(in.u.obj.hash_prefix_len == out.u.obj.hash_prefix_len); -+ EXPECT(in.u.obj.offset_len == out.u.obj.offset_len); -+ if (in.u.obj.hash_prefix_len) -+ EXPECT(!memcmp(in.u.obj.hash_prefix, out.u.obj.hash_prefix, -+ in.u.obj.hash_prefix_len)); -+ if (in.u.obj.offset_len) -+ EXPECT(!memcmp(in.u.obj.offsets, out.u.obj.offsets, -+ sizeof(uint64_t) * in.u.obj.offset_len)); +- EXPECT(reftable_record_equal(&rec, &rec_out, GIT_SHA1_RAWSZ)); ++ EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); strbuf_release(&key); - reftable_record_release(&rec_out); + reftable_record_release(&out); @@ reftable/record_test.c: static void test_reftable_index_record_roundtrip(void) + m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ); EXPECT(m == n); -- EXPECT(in.offset == out.offset); -+ EXPECT(in.u.idx.offset == out.u.idx.offset); +- EXPECT(reftable_record_equal(&rec, &out_rec, GIT_SHA1_RAWSZ)); ++ EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); - reftable_record_release(&out_rec); + reftable_record_release(&out); @@ reftable/writer.c: static void write_object_record(void *void_arg, void *key) - .hash_prefix_len = arg->w->stats.object_id_len, - .offsets = entry->offsets, - .offset_len = entry->offset_len, -+ struct reftable_record rec = { -+ .type = BLOCK_TYPE_OBJ, -+ .u.obj = { -+ .hash_prefix = (uint8_t *)entry->hash.buf, -+ .hash_prefix_len = arg->w->stats.object_id_len, -+ .offsets = entry->offsets, -+ .offset_len = entry->offset_len, -+ } - }; +- }; - struct reftable_record rec = { NULL }; ++ struct reftable_record ++ rec = { .type = BLOCK_TYPE_OBJ, ++ .u.obj = { ++ .hash_prefix = (uint8_t *)entry->hash.buf, ++ .hash_prefix_len = arg->w->stats.object_id_len, ++ .offsets = entry->offsets, ++ .offset_len = entry->offset_len, ++ } }; if (arg->err < 0) goto done; -: ----------- > 15: 6c06bd91662 reftable: add print functions to the record types -: ----------- > 16: e16bf0c5212 reftable: be more paranoid about 0-length memcpy calls -- gitgitgadget
