| Message ID | pull.1152.v7.git.git.1643051624.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Reftable coverity fixes | expand |
On Mon, Jan 24 2022, Han-Wen Nienhuys via GitGitGadget wrote: > This series was targeted to 'master'. > > This addresses some complaints from Coverity that Peff reported. > > v7: > > * address avarab's comments. Thanks. I just had time to skim it now but this overall LGTM. I noted a compilation error on xlc in another reply that I'm sorry I didn't test before, but provided a hopefully easy fix for. > @@ reftable/record.c: static int reftable_ref_record_equal_void(const void *a, > +static void reftable_ref_record_print_void(const void *rec, > + int hash_size) > +{ > -+ reftable_ref_record_print_sz((struct reftable_ref_record *) rec, hash_size); > ++ reftable_ref_record_print_sz( > ++ (struct reftable_ref_record *) rec, hash_size); > +} > + Just a nit, and only noting this since you seemed to be adjusting your clang-format settings (or so I recall), but I think the more usual wrapping in the codebase is: diff --git a/reftable/record.c b/reftable/record.c index 8330b7a2589..ee42897460d 100644 --- a/reftable/record.c +++ b/reftable/record.c @@ -464,8 +464,8 @@ static int reftable_ref_record_equal_void(const void *a, static void reftable_ref_record_print_void(const void *rec, int hash_size) { - reftable_ref_record_print_sz( - (struct reftable_ref_record *) rec, hash_size); + reftable_ref_record_print_sz((struct reftable_ref_record *)rec, + hash_size); } static struct reftable_record_vtable reftable_ref_record_vtable = { I.e. to "keep going" with the initial line until we hit 79 characters, and then wrap it. Then for casts "(x *)y" rather than "(x *) y".
This series was targeted to 'master'. This addresses some complaints from Coverity that Peff reported. v7: * address avarab's comments. 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: rename typ to type reftable/block.c | 35 ++-- reftable/block_test.c | 22 +-- reftable/blocksource.c | 6 +- reftable/generic.c | 39 ++-- reftable/iter.c | 4 +- reftable/merged.c | 35 ++-- reftable/merged.h | 2 +- reftable/pq.c | 2 +- reftable/pq_test.c | 29 ++- reftable/reader.c | 167 +++++++++-------- reftable/readwrite_test.c | 67 ++++++- reftable/record.c | 367 +++++++++++++++++++++++-------------- reftable/record.h | 49 ++--- reftable/record_test.c | 193 +++++++++---------- reftable/reftable-record.h | 14 +- reftable/reftable.c | 115 ------------ reftable/stack.c | 10 +- reftable/stack_test.c | 3 +- reftable/writer.c | 72 ++++---- t/helper/test-reftable.c | 9 +- 20 files changed, 666 insertions(+), 574 deletions(-) delete mode 100644 reftable/reftable.c base-commit: 50b2d72e110cad39ecaf2322bfdf1b60cd13dd96 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1152%2Fhanwen%2Freftable-coverity-v7 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1152/hanwen/reftable-coverity-v7 Pull-Request: https://github.com/git/git/pull/1152 Range-diff vs v6: 1: 9218bd59b25 ! 1: cc47ec7a50b reftable: fix OOB stack write in print functions @@ Metadata ## Commit message ## reftable: fix OOB stack write in print functions + Remove /* BUG */ comment, whose meaning has escaped my memory. + Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> ## reftable/record.c ## @@ reftable/record.c: static void hex_format(char *dest, uint8_t *src, int hash_siz uint32_t hash_id) { - char hex[2 * GIT_SHA256_RAWSZ + 1] = { 0 }; /* BUG */ -+ char hex[GIT_MAX_HEXSZ + 1] = { 0 }; /* BUG */ ++ char hex[GIT_MAX_HEXSZ + 1]; printf("ref{%s(%" PRIu64 ") ", ref->refname, ref->update_index); switch (ref->value_type) { case REFTABLE_REF_SYMREF: 2: 315ce62e710 ! 2: 1c5441d2f7e reftable: fix resource leak in block.c error path @@ Commit message ## reftable/block.c ## @@ reftable/block.c: 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; 3: 7cf65e141cf = 3: f11a9964383 reftable: fix resource leak blocksource.c 4: 8740ba92981 ! 4: b0bffb6e29d reftable: check reftable_stack_auto_compact() return value @@ Metadata ## Commit message ## reftable: check reftable_stack_auto_compact() return value - Fixes a problem detected by Coverity. + If compaction consistently errors out, we would accumulate reftables, + and the follow-on EXPECT would start failing, but catching errors + early helps debugging. + + Spotted by Coverity. Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> 5: 08be6d90a48 ! 5: e3d575ae2b4 reftable: ignore remove() return value in stack_test.c @@ reftable/stack_test.c: static void test_read_file(void) } free_names(names); - remove(fn); -+ (void) remove(fn); ++ EXPECT(unlink(fn) == 0); } static void test_parse_names(void) 6: 630f67bdb51 = 6: a21d934f37d reftable: fix resource warning 7: afee67fb60d = 7: b40d91c50a2 reftable: all xxx_free() functions accept NULL arguments 8: 6f4f24c17ad = 8: 7a1f32a2ff0 reftable: order unittests by complexity 9: 0abaab1d7c4 = 9: a118cd40975 reftable: drop stray printf in readwrite_test 10: 004b7a736bf = 10: 1d5cfbbc47a reftable: handle null refnames in reftable_ref_record_equal 11: 951efcebcd2 ! 11: 90c72a2df0c reftable: make reftable-record.h function signatures const correct @@ reftable/record.c: static void hex_format(char *dest, uint8_t *src, int hash_siz +void reftable_ref_record_print(const struct reftable_ref_record *ref, uint32_t hash_id) { - char hex[GIT_MAX_HEXSZ + 1] = { 0 }; /* BUG */ + char hex[GIT_MAX_HEXSZ + 1]; @@ reftable/record.c: static int zero_hash_eq(uint8_t *a, uint8_t *b, int sz) return !memcmp(a, b, sz); } 12: 6385e449ba7 ! 12: a83a9cf59b7 reftable: implement record equality generically @@ reftable/record.c: static int reftable_ref_record_is_deletion_void(const void *p (const struct reftable_ref_record *)p); } -+ +static int reftable_ref_record_equal_void(const void *a, + const void *b, int hash_size) +{ @@ reftable/record.c: static int not_a_deletion(const void *p) + || ra->offset_len != rb->offset_len) + return 0; + -+ if (ra->hash_prefix_len && -+ memcmp(ra->hash_prefix, rb->hash_prefix, ra->hash_prefix_len)) ++ if (memcmp(ra->hash_prefix, rb->hash_prefix, ra->hash_prefix_len)) + return 0; -+ if (ra->offset_len && -+ memcmp(ra->offsets, rb->offsets, ra->offset_len * sizeof(uint64_t))) ++ if (memcmp(ra->offsets, rb->offsets, ra->offset_len * sizeof(uint64_t))) + return 0; + + return 1; 13: 69c4a554882 = 13: 7e23d14d1af reftable: remove outdated file reftable.c 14: 54bcac325bd ! 14: 047bbb75fcb reftable: make reftable_record a tagged union @@ reftable/generic.c: https://developers.google.com/open-source/licenses/bsd { - struct reftable_ref_record ref = { - .refname = (char *)name, -- }; ++ struct reftable_record rec = { ++ .type = BLOCK_TYPE_REF, ++ .u.ref = { .refname = (char *)name, } + }; - struct reftable_record rec = { NULL }; - reftable_record_from_ref(&rec, &ref); -+ struct reftable_record rec = { .type = BLOCK_TYPE_REF, -+ .u.ref = { -+ .refname = (char *)name, -+ } }; return tab->ops->seek_record(tab->table_arg, it, &rec); } @@ reftable/generic.c: https://developers.google.com/open-source/licenses/bsd - struct reftable_log_record log = { - .refname = (char *)name, - .update_index = ~((uint64_t)0), -- }; ++ struct reftable_record rec = { ++ .type = BLOCK_TYPE_LOG, ++ .u.log = { ++ .refname = (char *)name, ++ .update_index = ~((uint64_t)0), ++ } + }; - 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 = ~((uint64_t)0), -+ } }; return tab->ops->seek_record(tab->table_arg, it, &rec); } @@ reftable/merged.c: int reftable_merged_table_seek_ref(struct reftable_merged_tab - .refname = (char *)name, + struct reftable_record rec = { + .type = BLOCK_TYPE_REF, -+ .u.ref = { -+ .refname = (char *)name, -+ }, ++ .u.ref = { .refname = (char *)name, }, }; - struct reftable_record rec = { NULL }; - reftable_record_from_ref(&rec, &ref); @@ 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.c ## -@@ reftable/pq.c: struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq) - void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, struct pq_entry e) - { - int i = 0; -+ - if (pq->len == pq->cap) { - pq->cap = 2 * pq->cap + 1; - pq->heap = reftable_realloc(pq->heap, @@ reftable/pq.c: void merged_iter_pqueue_release(struct merged_iter_pqueue *pq) { int i = 0; @@ 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/pq_test.c: static void test_pq(void) - last = ref->refname; - ref->refname = NULL; - reftable_free(ref); -+ // this is names[i], so don't dealloc. ++ /* this is names[i], so don't dealloc. */ + last = rec->u.ref.refname; + rec->u.ref.refname = NULL; + reftable_record_release(rec); @@ reftable/reader.c: static int reader_seek_indexed(struct reftable_reader *r, - reftable_record_key(rec, &want_index.last_key); - reftable_record_from_index(&want_index_rec, &want_index); - reftable_record_from_index(&index_result_rec, &index_result); -- + reftable_record_key(rec, &want_index.u.idx.last_key); + err = reader_start(r, &index_iter, reftable_record_type(rec), 1); if (err < 0) goto done; @@ reftable/reader.c: static int reader_seek(struct reftable_reader *r, struct reft - .refname = (char *)name, + struct reftable_record rec = { + .type = BLOCK_TYPE_REF, -+ .u.ref = { -+ .refname = (char *)name, -+ }, ++ .u.ref = { .refname = (char *)name, }, }; - struct reftable_record rec = { NULL }; - reftable_record_from_ref(&rec, &ref); @@ 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/record.c: https://developers.google.com/open-source/licenses/bsd +static struct reftable_record_vtable * +reftable_record_vtable(struct reftable_record *rec); -+static void *reftable_record_data(struct reftable_record *rec); ++ ++static void *reftable_record_data(struct reftable_record *rec) ++{ ++ switch (rec->type) { ++ case BLOCK_TYPE_REF: ++ return &rec->u.ref; ++ case BLOCK_TYPE_LOG: ++ return &rec->u.log; ++ case BLOCK_TYPE_INDEX: ++ return &rec->u.idx; ++ case BLOCK_TYPE_OBJ: ++ return &rec->u.obj; ++ } ++ abort(); ++} + int get_var_int(uint64_t *dest, struct string_view *in) { @@ reftable/record.c: static void reftable_obj_record_copy_from(void *rec, const vo 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); + 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); @@ reftable/record.c: static struct reftable_record_vtable reftable_log_record_vtab static void reftable_index_record_key(const void *r, struct strbuf *dest) { const struct reftable_index_record *rec = r; +@@ reftable/record.c: static void reftable_index_record_release(void *rec) + { + struct reftable_index_record *idx = rec; + strbuf_release(&idx->last_key); ++ idx->offset = 0; + } + + static uint8_t reftable_index_record_val_type(const void *rec) @@ reftable/record.c: static struct reftable_record_vtable reftable_index_record_vtable = { void reftable_record_key(struct reftable_record *rec, struct strbuf *dest) @@ reftable/record.c: void string_view_consume(struct string_view *s, int n) s->len -= n; } + -+static void *reftable_record_data(struct reftable_record *rec) -+{ -+ switch (rec->type) { -+ case BLOCK_TYPE_REF: -+ return &rec->u.ref; -+ case BLOCK_TYPE_LOG: -+ return &rec->u.log; -+ case BLOCK_TYPE_INDEX: -+ return &rec->u.idx; -+ case BLOCK_TYPE_OBJ: -+ return &rec->u.obj; -+ } -+ abort(); -+} -+ +static struct reftable_record_vtable * +reftable_record_vtable(struct reftable_record *rec) +{ @@ reftable/record.c: void string_view_consume(struct string_view *s, int n) + abort(); +} + -+struct reftable_record reftable_new_record(uint8_t typ) ++struct reftable_record reftable_new_record(uint8_t type) +{ + struct reftable_record clean = { -+ .type = typ, ++ .type = type, + }; + + /* 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) { ++ switch (type) { + case BLOCK_TYPE_OBJ: + { + struct reftable_obj_record obj = { 0 }; @@ reftable/record_test.c - 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); ++ uint8_t type = reftable_record_type(rec); ++ copy = reftable_new_record(type); reftable_record_copy_from(©, rec, GIT_SHA1_RAWSZ); /* do it twice to catch memory leaks */ reftable_record_copy_from(©, rec, GIT_SHA1_RAWSZ); @@ reftable/record_test.c: static void test_reftable_ref_record_roundtrip(void) uint8_t buffer[1024] = { 0 }; struct string_view dest = { .buf = buffer, - .len = sizeof(buffer), - }; -- +@@ reftable/record_test.c: static void test_reftable_ref_record_roundtrip(void) + int n, m; - in.value_type = i; @@ 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.value.val2.target_value, 2); ++ 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); + reftable_malloc(GIT_SHA1_RAWSZ); +- set_hash(in.value.val2.target_value, 2); + set_hash(in.u.ref.value.val2.target_value, 2); break; case REFTABLE_REF_SYMREF: @@ reftable/writer.c: int reftable_writer_add_refs(struct reftable_writer *w, block_writer_type(w->block_writer) == BLOCK_TYPE_REF) { int err = writer_finish_public_section(w); @@ reftable/writer.c: static int reftable_writer_add_log_verbatim(struct reftable_writer *w, - w->next -= w->pending_padding; w->pending_padding = 0; -- + - reftable_record_from_log(&rec, log); return writer_add_record(w, &rec); } @@ 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; @@ reftable/writer.c: static void write_object_record(void *void_arg, void *key) if (arg->err == 0) goto done; - obj_rec.offset_len = 0; -+ + rec.u.obj.offset_len = 0; arg->err = block_writer_add(arg->w->block_writer, &rec); 15: 82f140cab5c ! 15: 1e8785bbc26 reftable: add print functions to the record types @@ reftable/record.c: static void hex_format(char *dest, uint8_t *src, int hash_siz +static void reftable_ref_record_print_sz(const struct reftable_ref_record *ref, + int hash_size) { - char hex[GIT_MAX_HEXSZ + 1] = { 0 }; /* BUG */ + char hex[GIT_MAX_HEXSZ + 1]; printf("ref{%s(%" PRIu64 ") ", ref->refname, ref->update_index); @@ reftable/record.c: void reftable_ref_record_print(const struct reftable_ref_record *ref, printf("=> %s", ref->value.symref); @@ reftable/record.c: static int reftable_ref_record_equal_void(const void *a, +static void reftable_ref_record_print_void(const void *rec, + int hash_size) +{ -+ reftable_ref_record_print_sz((struct reftable_ref_record *) rec, hash_size); ++ reftable_ref_record_print_sz( ++ (struct reftable_ref_record *) rec, hash_size); +} + static struct reftable_record_vtable reftable_ref_record_vtable = { @@ reftable/record.c: static void reftable_obj_record_release(void *rec) + int i; + + for (i = 0; i < obj->offset_len; i++) -+ strbuf_addf(&offset_str, "%" PRIu64 " ", obj->offsets[i]); ++ strbuf_addf(&offset_str, "%"PRIu64 " ", obj->offsets[i]); + hex_format(hex, obj->hash_prefix, obj->hash_prefix_len); + printf("prefix %s (len %d), offsets [%s]\n", + hex, obj->hash_prefix_len, offset_str.buf); @@ reftable/record.c: static struct reftable_record_vtable reftable_obj_record_vtab switch (log->value_type) { case REFTABLE_LOG_DELETION: - printf("log{%s(%" PRIu64 ") delete", log->refname, -+ printf("log{%s(%" PRIu64 ") delete\n", log->refname, ++ printf("log{%s(%"PRIu64 ") delete\n", log->refname, log->update_index); break; case REFTABLE_LOG_UPDATE: - printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n", +- printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n", - log->refname, log->update_index, log->value.update.name, - log->value.update.email, log->value.update.time, ++ printf("log{%s(%"PRIu64 ") %s <%s> %" PRIu64 " %04d\n", + log->refname, log->update_index, + log->value.update.name ? log->value.update.name : "", + log->value.update.email ? log->value.update.email : "", @@ reftable/record.c: static struct reftable_record_vtable reftable_index_record_vt }; void reftable_record_key(struct reftable_record *rec, struct strbuf *dest) -@@ reftable/record.c: struct reftable_record reftable_new_record(uint8_t typ) +@@ reftable/record.c: struct reftable_record reftable_new_record(uint8_t type) } return clean; } @@ reftable/record_test.c: static void test_reftable_log_record_equal(void) { int i; - struct reftable_log_record in[2] = { -+ + struct reftable_log_record in[] = { { .refname = xstrdup("refs/heads/master"), @@ reftable/record_test.c: static void test_reftable_log_record_roundtrip(void) .value_type = REFTABLE_LOG_DELETION, + }, + { ++ /* make sure we handle optional fields correctly. */ + .refname = xstrdup("branch"), + .update_index = 33, + .value_type = REFTABLE_LOG_UPDATE, @@ reftable/record_test.c: static void test_reftable_log_record_roundtrip(void) + .update = { + .old_hash = reftable_malloc(GIT_SHA1_RAWSZ), + .new_hash = reftable_malloc(GIT_SHA1_RAWSZ), -+ /* rest of fields left empty. */ + }, + }, } -: ----------- > 16: 556fc11080a reftable: rename typ to type