{"thread":{"id":"57027","subject":"coverity problems in reftable code","startedAt":"2021-12-04T02:13:53Z","lastAt":"2021-12-08T19:12:33Z","messageCount":7,"participants":["Jeff King","Han-Wen Nienhuys","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"443052","messageId":"YarO3nkrutmWF7nb@coredump.intra.peff.net","threadId":"57027","inReplyTo":null,"subject":"coverity problems in reftable code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-04T02:13:50Z","receivedAt":"2021-12-04T02:13:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Hi Han-Wen,\n\nWe're not doing project-wide analysis with Coverity right now, but I've\nbeen doing builds of my personal repo, which I usually build off of\nnext. And since hn/reftable just hit next, it got included in my latest\nbuild.\n\nIt came up with several complaints. Some of them are dumb and can be\nignored (e.g., using rand() in a test harness, oh no!) but I poked at a\nfew and they look like real issues:\n\n  - the stack overflow in reftable_log_record_print() is real; I think you\n    want HEXSZ instead of RAWSZ for your buffer (also, should it be\n    GIT_MAX_HEXSZ?)\n\n  - various fd/memory leaks on error returns\n\n  - A lot of your structs have vtables. Initializing them to NULL, as in\n    reftable_reader_refs_for_indexed(), leaves the risk that we'll try\n    to call a NULL function pointer, even if it's for something simple\n    like cleaning up. That's the case here (if loading want_rec fails,\n    we'll jump to \"done\" and try to clean up got_rec, even though it's\n    still got a NULL vtable). One solution is for reftable_record_release()\n    to check for NULL (and other destructors; there's a similar case in\n    the last snippet below for reftable_reader_free()).\n\n    But I also wasn't sure how extensive the use of polymorphism was,\n    and if it might be possible for us to do more at initialization\n    time. Something like:\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 2db0019111..8ac248b070 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -653,21 +653,19 @@ static int reftable_reader_refs_for_indexed(struct reftable_reader *r,\n \t\t.hash_prefix = oid,\n \t\t.hash_prefix_len = r->object_id_len,\n \t};\n-\tstruct reftable_record want_rec = { NULL };\n+\tstruct reftable_record want_rec = REFTABLE_RECORD_OBJ(&want);\n \tstruct reftable_iterator oit = { NULL };\n \tstruct reftable_obj_record got = { NULL };\n-\tstruct reftable_record got_rec = { NULL };\n+\tstruct reftable_record got_rec = REFTABLE_RECORD_OBJ(&got);\n \tint err = 0;\n \tstruct indexed_table_ref_iter *itr = NULL;\n \n \t/* Look through the reverse index. */\n-\treftable_record_from_obj(&want_rec, &want);\n \terr = reader_seek(r, &oit, &want_rec);\n \tif (err != 0)\n \t\tgoto done;\n \n \t/* read out the reftable_obj_record */\n-\treftable_record_from_obj(&got_rec, &got);\n \terr = iterator_next(&oit, &got_rec);\n \tif (err < 0)\n \t\tgoto done;\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 5a4bbed6c8..4c34752d18 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -572,7 +572,7 @@ static int not_a_deletion(const void *UNUSED(p))\n \treturn 0;\n }\n \n-static struct reftable_record_vtable reftable_obj_record_vtable = {\n+struct reftable_record_vtable reftable_obj_record_vtable = {\n \t.key = &reftable_obj_record_key,\n \t.type = BLOCK_TYPE_OBJ,\n \t.copy_from = &reftable_obj_record_copy_from,\n@@ -1106,14 +1106,6 @@ void reftable_record_from_ref(struct reftable_record *rec,\n \trec->ops = &reftable_ref_record_vtable;\n }\n \n-void reftable_record_from_obj(struct reftable_record *rec,\n-\t\t\t      struct reftable_obj_record *obj_rec)\n-{\n-\tassert(!rec->ops);\n-\trec->data = obj_rec;\n-\trec->ops = &reftable_obj_record_vtable;\n-}\n-\n void reftable_record_from_index(struct reftable_record *rec,\n \t\t\t\tstruct reftable_index_record *index_rec)\n {\ndiff --git a/reftable/record.h b/reftable/record.h\nindex 498e8c50bf..316cde9182 100644\n--- a/reftable/record.h\n+++ b/reftable/record.h\n@@ -66,6 +66,12 @@ struct reftable_record {\n \tstruct reftable_record_vtable *ops;\n };\n \n+extern struct reftable_record_vtable reftable_obj_record_vtable;\n+#define REFTABLE_RECORD_OBJ(obj_record) { \\\n+\t.data = obj_record, \\\n+\t.ops = &reftable_obj_record_vtable \\\n+}\n+\n /* returns true for recognized block types. Block start with the block type. */\n int reftable_is_block_type(uint8_t typ);\n \n@@ -119,8 +125,6 @@ void reftable_record_destroy(struct reftable_record *rec);\n \n /* initialize generic records from concrete records. The generic record should\n  * be zeroed out. */\n-void reftable_record_from_obj(struct reftable_record *rec,\n-\t\t\t      struct reftable_obj_record *objrec);\n void reftable_record_from_index(struct reftable_record *rec,\n \t\t\t\tstruct reftable_index_record *idxrec);\n void reftable_record_from_ref(struct reftable_record *rec,\n\nbut there may be other cases which really want the more dynamic\ninitialization. And also, the virtual release function would need to\nhandle the NULL data as well.\n\nI similarly wondered if these polymorphic types could be using a union\nwithin reftable_record, rather than pointing to a separate stack\nvariable. Then you could initialize the whole thing without worrying\nabout intermediate NULLs (and also there's less pointer chasing and it's\na little bit more type safe than a void pointer). But again, I don't\nknow the code well enough to know if that would cover all of your cases.\n\nThe summary of issues is below. You can get more details on their site.\nI _think_ I've configured it so that anybody can look at:\n\n  https://scan.coverity.com/projects/peff-git/view_defects\n\nbut you'll probably need to make a log in (or connect with a GitHub\naccount). I usually find the summary enough to find issues, but\nsometimes it's useful to look at the site because it outlines the full\npath of assumptions (not just \"here we dereference NULL\", but the\nsequence of code that goes from knowing that a variable is NULL to the\npoint of dereferencing it).\n\nI'm happy to help dig into any of them more, or even work on patches.\nBut per the above diff, I wasn't sure how far to go. :)\n\n-Peff\n\n-- >8 --\n** CID 1467043:  Resource leaks  (RESOURCE_LEAK)\n/reftable/block.c: 220 in block_reader_init()\n\n\n________________________________________________________________________________________________________\n*** CID 1467043:  Resource leaks  (RESOURCE_LEAK)\n/reftable/block.c: 220 in block_reader_init()\n214     \t\t\t\tblock->data + block_header_skip, &src_len)) {\n215     \t\t\treftable_free(uncompressed);\n216     \t\t\treturn REFTABLE_ZLIB_ERROR;\n217     \t\t}\n218     \n219     \t\tif (dst_len + block_header_skip != sz)\n>>>     CID 1467043:  Resource leaks  (RESOURCE_LEAK)\n>>>     Variable \"uncompressed\" going out of scope leaks the storage it points to.\n220     \t\t\treturn REFTABLE_FORMAT_ERROR;\n221     \n222     \t\t/* We're done with the input data. */\n223     \t\treftable_block_done(block);\n224     \t\tblock->data = uncompressed;\n225     \t\tblock->len = sz;\n\n** CID 1467042:  Null pointer dereferences  (FORWARD_NULL)\n\n\n________________________________________________________________________________________________________\n*** CID 1467042:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/reader.c: 692 in reftable_reader_refs_for_indexed()\n686     \t\tgoto done;\n687     \tgot.offsets = NULL;\n688     \titerator_from_indexed_table_ref_iter(it, itr);\n689     \n690     done:\n691     \treftable_iterator_destroy(&oit);\n>>>     CID 1467042:  Null pointer dereferences  (FORWARD_NULL)\n>>>     Passing \"&got_rec\" to \"reftable_record_release\", which dereferences null \"got_rec.ops\".\n692     \treftable_record_release(&got_rec);\n693     \treturn err;\n694     }\n695     \n696     static int reftable_reader_refs_for_unindexed(struct reftable_reader *r,\n697     \t\t\t\t\t      struct reftable_iterator *it,\n\n** CID 1467041:  Resource leaks  (RESOURCE_LEAK)\n/reftable/blocksource.c: 138 in reftable_block_source_from_file()\n\n\n________________________________________________________________________________________________________\n*** CID 1467041:  Resource leaks  (RESOURCE_LEAK)\n/reftable/blocksource.c: 138 in reftable_block_source_from_file()\n132     \t\t}\n133     \t\treturn -1;\n134     \t}\n135     \n136     \terr = fstat(fd, &st);\n137     \tif (err < 0)\n>>>     CID 1467041:  Resource leaks  (RESOURCE_LEAK)\n>>>     Handle variable \"fd\" going out of scope leaks the handle.\n138     \t\treturn -1;\n139     \n140     \tp = reftable_calloc(sizeof(struct file_block_source));\n141     \tp->size = st.st_size;\n142     \tp->fd = fd;\n143     \n\n** CID 1467040:  Null pointer dereferences  (FORWARD_NULL)\n\n\n________________________________________________________________________________________________________\n*** CID 1467040:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/record_test.c: 159 in test_reftable_ref_record_roundtrip()\n153     \n154     \t\treftable_record_from_ref(&rec_out, &out);\n155     \t\tm = reftable_record_decode(&rec_out, key, i, dest,\n156     \t\t\t\t\t   GIT_SHA1_RAWSZ);\n157     \t\tEXPECT(n == m);\n158     \n>>>     CID 1467040:  Null pointer dereferences  (FORWARD_NULL)\n>>>     Passing \"&out\" to \"reftable_ref_record_equal\", which dereferences null \"out.refname\".\n159     \t\tEXPECT(reftable_ref_record_equal(&in, &out, GIT_SHA1_RAWSZ));\n160     \t\treftable_record_release(&rec_out);\n161     \n162     \t\tstrbuf_release(&key);\n163     \t\treftable_ref_record_release(&in);\n164     \t}\n\n** CID 1467039:    (TAINTED_SCALAR)\n/reftable/block.c: 212 in block_reader_init()\n\n\n________________________________________________________________________________________________________\n*** CID 1467039:    (TAINTED_SCALAR)\n/reftable/block.c: 212 in block_reader_init()\n206     \t\tuint8_t *uncompressed = reftable_malloc(sz);\n207     \n208     \t\t/* Copy over the block header verbatim. It's not compressed. */\n209     \t\tmemcpy(uncompressed, block->data, block_header_skip);\n210     \n211     \t\t/* Uncompress */\n>>>     CID 1467039:    (TAINTED_SCALAR)\n>>>     Passing tainted expression \"dst_len\" to \"uncompress2\", which uses it as an offset.\n212     \t\tif (Z_OK !=\n213     \t\t    uncompress2(uncompressed + block_header_skip, &dst_len,\n214     \t\t\t\tblock->data + block_header_skip, &src_len)) {\n215     \t\t\treftable_free(uncompressed);\n216     \t\t\treturn REFTABLE_ZLIB_ERROR;\n217     \t\t}\n/reftable/block.c: 206 in block_reader_init()\n200     \t\tint block_header_skip = 4 + header_off;\n201     \t\tuLongf dst_len = sz - block_header_skip; /* total size of dest\n202     \t\t\t\t\t\t\t    buffer. */\n203     \t\tuLongf src_len = block->len - block_header_skip;\n204     \t\t/* Log blocks specify the *uncompressed* size in their header.\n205     \t\t */\n>>>     CID 1467039:    (TAINTED_SCALAR)\n>>>     Passing tainted expression \"sz\" to \"reftable_malloc\", which uses it as an allocation size.\n206     \t\tuint8_t *uncompressed = reftable_malloc(sz);\n207     \n208     \t\t/* Copy over the block header verbatim. It's not compressed. */\n209     \t\tmemcpy(uncompressed, block->data, block_header_skip);\n210     \n211     \t\t/* Uncompress */\n\n** CID 1467038:    (DC.WEAK_CRYPTO)\n/reftable/readwrite_test.c: 145 in test_log_buffer_size()\n/reftable/readwrite_test.c: 144 in test_log_buffer_size()\n\n\n________________________________________________________________________________________________________\n*** CID 1467038:    (DC.WEAK_CRYPTO)\n/reftable/readwrite_test.c: 145 in test_log_buffer_size()\n139     \t/* This tests buffer extension for log compression. Must use a random\n140     \t   hash, to ensure that the compressed part is larger than the original.\n141     \t*/\n142     \tuint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];\n143     \tfor (i = 0; i < GIT_SHA1_RAWSZ; i++) {\n144     \t\thash1[i] = (uint8_t)(rand() % 256);\n>>>     CID 1467038:    (DC.WEAK_CRYPTO)\n>>>     \"rand\" should not be used for security-related applications, because linear congruential algorithms are too easy to break.\n145     \t\thash2[i] = (uint8_t)(rand() % 256);\n146     \t}\n147     \tlog.value.update.old_hash = hash1;\n148     \tlog.value.update.new_hash = hash2;\n149     \treftable_writer_set_limits(w, update_index, update_index);\n150     \terr = reftable_writer_add_log(w, &log);\n/reftable/readwrite_test.c: 144 in test_log_buffer_size()\n138     \n139     \t/* This tests buffer extension for log compression. Must use a random\n140     \t   hash, to ensure that the compressed part is larger than the original.\n141     \t*/\n142     \tuint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];\n143     \tfor (i = 0; i < GIT_SHA1_RAWSZ; i++) {\n>>>     CID 1467038:    (DC.WEAK_CRYPTO)\n>>>     \"rand\" should not be used for security-related applications, because linear congruential algorithms are too easy to break.\n144     \t\thash1[i] = (uint8_t)(rand() % 256);\n145     \t\thash2[i] = (uint8_t)(rand() % 256);\n146     \t}\n147     \tlog.value.update.old_hash = hash1;\n148     \tlog.value.update.new_hash = hash2;\n149     \treftable_writer_set_limits(w, update_index, update_index);\n\n** CID 1467037:  Security best practices violations  (DC.WEAK_CRYPTO)\n/reftable/stack.c: 439 in format_name()\n\n\n________________________________________________________________________________________________________\n*** CID 1467037:  Security best practices violations  (DC.WEAK_CRYPTO)\n/reftable/stack.c: 439 in format_name()\n433     \treturn 0;\n434     }\n435     \n436     static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)\n437     {\n438     \tchar buf[100];\n>>>     CID 1467037:  Security best practices violations  (DC.WEAK_CRYPTO)\n>>>     \"rand\" should not be used for security-related applications, because linear congruential algorithms are too easy to break.\n439     \tuint32_t rnd = (uint32_t)rand();\n440     \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n441     \t\t min, max, rnd);\n442     \tstrbuf_reset(dest);\n443     \tstrbuf_addstr(dest, buf);\n444     }\n\n** CID 1467036:  Code maintainability issues  (UNUSED_VALUE)\n/reftable/stack_test.c: 816 in test_reftable_stack_auto_compaction()\n\n\n________________________________________________________________________________________________________\n*** CID 1467036:  Code maintainability issues  (UNUSED_VALUE)\n/reftable/stack_test.c: 816 in test_reftable_stack_auto_compaction()\n810     \t\t};\n811     \t\tsnprintf(name, sizeof(name), \"branch%04d\", i);\n812     \n813     \t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n814     \t\tEXPECT_ERR(err);\n815     \n>>>     CID 1467036:  Code maintainability issues  (UNUSED_VALUE)\n>>>     Assigning value from \"reftable_stack_auto_compact(st)\" to \"err\" here, but that stored value is overwritten before it can be used.\n816     \t\terr = reftable_stack_auto_compact(st);\n817     \t\tEXPECT(i < 3 || st->merged->stack_len < 2 * fastlog2(i));\n818     \t}\n819     \n820     \tEXPECT(reftable_stack_compaction_stats(st)->entries_written <\n821     \t       (uint64_t)(N * fastlog2(N)));\n\n** CID 1467035:    (OVERRUN)\n\n\n________________________________________________________________________________________________________\n*** CID 1467035:    (OVERRUN)\n/reftable/record.c: 603 in reftable_log_record_print()\n597     \t\tprintf(\"log{%s(%\" PRIu64 \") %s <%s> %\" PRIu64 \" %04d\\n\",\n598     \t\t       log->refname, log->update_index, log->value.update.name,\n599     \t\t       log->value.update.email, log->value.update.time,\n600     \t\t       log->value.update.tz_offset);\n601     \t\thex_format(hex, log->value.update.old_hash, hash_size(hash_id));\n602     \t\tprintf(\"%s => \", hex);\n>>>     CID 1467035:    (OVERRUN)\n>>>     Overrunning array \"hex\" of 33 bytes by passing it to a function which accesses it at byte offset 63 using argument \"hash_size(hash_id)\" (which evaluates to 32).\n603     \t\thex_format(hex, log->value.update.new_hash, hash_size(hash_id));\n604     \t\tprintf(\"%s\\n\\n%s\\n}\\n\", hex, log->value.update.message);\n605     \t\tbreak;\n606     \t}\n607     }\n608     \n/reftable/record.c: 601 in reftable_log_record_print()\n595     \t\tbreak;\n596     \tcase REFTABLE_LOG_UPDATE:\n597     \t\tprintf(\"log{%s(%\" PRIu64 \") %s <%s> %\" PRIu64 \" %04d\\n\",\n598     \t\t       log->refname, log->update_index, log->value.update.name,\n599     \t\t       log->value.update.email, log->value.update.time,\n600     \t\t       log->value.update.tz_offset);\n>>>     CID 1467035:    (OVERRUN)\n>>>     Overrunning array \"hex\" of 33 bytes by passing it to a function which accesses it at byte offset 63 using argument \"hash_size(hash_id)\" (which evaluates to 32).\n601     \t\thex_format(hex, log->value.update.old_hash, hash_size(hash_id));\n602     \t\tprintf(\"%s => \", hex);\n603     \t\thex_format(hex, log->value.update.new_hash, hash_size(hash_id));\n604     \t\tprintf(\"%s\\n\\n%s\\n}\\n\", hex, log->value.update.message);\n605     \t\tbreak;\n606     \t}\n\n** CID 1467034:  Error handling issues  (CHECKED_RETURN)\n/reftable/stack_test.c: 92 in test_read_file()\n\n\n________________________________________________________________________________________________________\n*** CID 1467034:  Error handling issues  (CHECKED_RETURN)\n/reftable/stack_test.c: 92 in test_read_file()\n86     \tEXPECT_ERR(err);\n87     \n88     \tfor (i = 0; names[i]; i++) {\n89     \t\tEXPECT(0 == strcmp(want[i], names[i]));\n90     \t}\n91     \tfree_names(names);\n>>>     CID 1467034:  Error handling issues  (CHECKED_RETURN)\n>>>     Calling \"remove(fn)\" without checking return value. This library function may fail and return an error code.\n92     \tremove(fn);\n93     }\n94     \n95     static void test_parse_names(void)\n96     {\n97     \tchar buf[] = \"line\\n\";\n\n** CID 1467033:  Resource leaks  (RESOURCE_LEAK)\n/reftable/stack.c: 951 in stack_compact_range()\n\n\n________________________________________________________________________________________________________\n*** CID 1467033:  Resource leaks  (RESOURCE_LEAK)\n/reftable/stack.c: 951 in stack_compact_range()\n945     \t\tsubtable_locks[j] = subtab_lock.buf;\n946     \t\tdelete_on_success[j] = subtab_file_name.buf;\n947     \t\tj++;\n948     \n949     \t\tif (err != 0)\n950     \t\t\tgoto done;\n>>>     CID 1467033:  Resource leaks  (RESOURCE_LEAK)\n>>>     Handle variable \"sublock_file_fd\" going out of scope leaks the handle.\n951     \t}\n952     \n953     \terr = unlink(lock_file_name.buf);\n954     \tif (err < 0)\n955     \t\tgoto done;\n956     \thave_lock = 0;\n\n** CID 1467032:  Null pointer dereferences  (FORWARD_NULL)\n\n\n________________________________________________________________________________________________________\n*** CID 1467032:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/reader.c: 504 in reader_seek_indexed()\n498     \treftable_record_from_index(&index_result_rec, &index_result);\n499     \n500     \terr = reader_start(r, &index_iter, reftable_record_type(rec), 1);\n501     \tif (err < 0)\n502     \t\tgoto done;\n503     \n>>>     CID 1467032:  Null pointer dereferences  (FORWARD_NULL)\n>>>     Passing \"&index_iter\" to \"reader_seek_linear\", which dereferences null \"index_iter.r\".\n504     \terr = reader_seek_linear(&index_iter, &want_index_rec);\n505     \twhile (1) {\n506     \t\terr = table_iter_next(&index_iter, &index_result_rec);\n507     \t\ttable_iter_block_done(&index_iter);\n508     \t\tif (err != 0)\n509     \t\t\tgoto done;\n\n** CID 1467031:  Null pointer dereferences  (FORWARD_NULL)\n\n\n________________________________________________________________________________________________________\n*** CID 1467031:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/stack.c: 710 in reftable_addition_add()\n704     \t\tunlink(temp_tab_file_name.buf);\n705     \t}\n706     \n707     \tstrbuf_release(&temp_tab_file_name);\n708     \tstrbuf_release(&tab_file_name);\n709     \tstrbuf_release(&next_name);\n>>>     CID 1467031:  Null pointer dereferences  (FORWARD_NULL)\n>>>     Passing null pointer \"wr\" to \"reftable_writer_free\", which dereferences it.\n710     \treftable_writer_free(wr);\n711     \treturn err;\n712     }\n713     \n714     uint64_t reftable_stack_next_update_index(struct reftable_stack *st)\n715     {\n\n** CID 1467030:  Security best practices violations  (DC.WEAK_CRYPTO)\n/reftable/stack.c: 367 in reftable_stack_reload_maybe_reuse()\n\n\n________________________________________________________________________________________________________\n*** CID 1467030:  Security best practices violations  (DC.WEAK_CRYPTO)\n/reftable/stack.c: 367 in reftable_stack_reload_maybe_reuse()\n361     \t\t\tfree_names(names_after);\n362     \t\t\treturn err;\n363     \t\t}\n364     \t\tfree_names(names);\n365     \t\tfree_names(names_after);\n366     \n>>>     CID 1467030:  Security best practices violations  (DC.WEAK_CRYPTO)\n>>>     \"rand\" should not be used for security-related applications, because linear congruential algorithms are too easy to break.\n367     \t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n368     \t\tsleep_millisec(delay);\n369     \t}\n370     \n371     \treturn 0;\n372     }\n\n** CID 1467029:  Resource leaks  (RESOURCE_LEAK)\n/reftable/stack.c: 1065 in stack_compact_range()\n\n\n________________________________________________________________________________________________________\n*** CID 1467029:  Resource leaks  (RESOURCE_LEAK)\n/reftable/stack.c: 1065 in stack_compact_range()\n1059     \t}\n1060     \tstrbuf_release(&new_table_name);\n1061     \tstrbuf_release(&new_table_path);\n1062     \tstrbuf_release(&ref_list_contents);\n1063     \tstrbuf_release(&temp_tab_file_name);\n1064     \tstrbuf_release(&lock_file_name);\n>>>     CID 1467029:  Resource leaks  (RESOURCE_LEAK)\n>>>     Handle variable \"lock_file_fd\" going out of scope leaks the handle.\n1065     \treturn err;\n1066     }\n1067     \n1068     int reftable_stack_compact_all(struct reftable_stack *st,\n1069     \t\t\t       struct reftable_log_expiry_config *config)\n1070     {\n\n** CID 1467028:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/reader.c: 675 in reftable_reader_refs_for_indexed()\n\n\n________________________________________________________________________________________________________\n*** CID 1467028:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/reader.c: 675 in reftable_reader_refs_for_indexed()\n669     \t/* read out the reftable_obj_record */\n670     \treftable_record_from_obj(&got_rec, &got);\n671     \terr = iterator_next(&oit, &got_rec);\n672     \tif (err < 0)\n673     \t\tgoto done;\n674     \n>>>     CID 1467028:  Null pointer dereferences  (FORWARD_NULL)\n>>>     Passing null pointer \"got.hash_prefix\" to \"memcmp\", which dereferences it.\n675     \tif (err > 0 ||\n676     \t    memcmp(want.hash_prefix, got.hash_prefix, r->object_id_len)) {\n677     \t\t/* didn't find it; return empty iterator */\n678     \t\titerator_set_empty(it);\n679     \t\terr = 0;\n680     \t\tgoto done;\n\n** CID 1467027:  Security best practices violations  (SECURE_TEMP)\n/reftable/stack_test.c: 72 in test_read_file()\n\n\n________________________________________________________________________________________________________\n*** CID 1467027:  Security best practices violations  (SECURE_TEMP)\n/reftable/stack_test.c: 72 in test_read_file()\n66     \treturn dir;\n67     }\n68     \n69     static void test_read_file(void)\n70     {\n71     \tchar *fn = get_tmp_template(__LINE__);\n>>>     CID 1467027:  Security best practices violations  (SECURE_TEMP)\n>>>     Calling \"mkstemp\" without securely setting umask first.\n72     \tint fd = mkstemp(fn);\n73     \tchar out[1024] = \"line1\\n\\nline2\\nline3\";\n74     \tint n, err;\n75     \tchar **names = NULL;\n76     \tchar *want[] = { \"line1\", \"line2\", \"line3\" };\n77     \tint i = 0;\n\n** CID 1467026:  Null pointer dereferences  (FORWARD_NULL)\n\n\n________________________________________________________________________________________________________\n*** CID 1467026:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/reader.c: 683 in reftable_reader_refs_for_indexed()\n677     \t\t/* didn't find it; return empty iterator */\n678     \t\titerator_set_empty(it);\n679     \t\terr = 0;\n680     \t\tgoto done;\n681     \t}\n682     \n>>>     CID 1467026:  Null pointer dereferences  (FORWARD_NULL)\n>>>     Passing null pointer \"got.offsets\" to \"new_indexed_table_ref_iter\", which dereferences it.\n683     \terr = new_indexed_table_ref_iter(&itr, r, oid, hash_size(r->hash_id),\n684     \t\t\t\t\t got.offsets, got.offset_len);\n685     \tif (err < 0)\n686     \t\tgoto done;\n687     \tgot.offsets = NULL;\n688     \titerator_from_indexed_table_ref_iter(it, itr);\n\n** CID 1467025:  Security best practices violations  (SECURE_TEMP)\n/reftable/stack.c: 642 in reftable_addition_add()\n\n\n________________________________________________________________________________________________________\n*** CID 1467025:  Security best practices violations  (SECURE_TEMP)\n/reftable/stack.c: 642 in reftable_addition_add()\n636     \tstrbuf_reset(&next_name);\n637     \tformat_name(&next_name, add->next_update_index, add->next_update_index);\n638     \n639     \tstack_filename(&temp_tab_file_name, add->stack, next_name.buf);\n640     \tstrbuf_addstr(&temp_tab_file_name, \".temp.XXXXXX\");\n641     \n>>>     CID 1467025:  Security best practices violations  (SECURE_TEMP)\n>>>     Calling \"mkstemp\" without securely setting umask first.\n642     \ttab_fd = mkstemp(temp_tab_file_name.buf);\n643     \tif (tab_fd < 0) {\n644     \t\terr = REFTABLE_IO_ERROR;\n645     \t\tgoto done;\n646     \t}\n647     \n\n** CID 1467024:  Null pointer dereferences  (FORWARD_NULL)\n\n\n________________________________________________________________________________________________________\n*** CID 1467024:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/reader.c: 799 in reftable_reader_print_file()\n793     \tif (err < 0)\n794     \t\tgoto done;\n795     \n796     \treftable_table_from_reader(&tab, r);\n797     \terr = reftable_table_print(&tab);\n798     done:\n>>>     CID 1467024:  Null pointer dereferences  (FORWARD_NULL)\n>>>     Passing null pointer \"r\" to \"reftable_reader_free\", which dereferences it.\n799     \treftable_reader_free(r);\n800     \treturn err;\n\n"},{"id":"443267","messageId":"CAFQ2z_OrN+RkwnMyrJHdh5xN6ueOP8KKBVQ7-U4kEkA3ApcuNg@mail.gmail.com","threadId":"57027","inReplyTo":"CAFQ2z_OK5949p1WfovJ00Katk5hTv_oeLo-ZRCi1XqrtzQqL2g@mail.gmail.com","subject":"Fwd: coverity problems in reftable code","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-12-07T11:34:15Z","receivedAt":"2021-12-07T11:34:28Z","isPatch":false,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Sat, Dec 4, 2021 at 3:13 AM Jeff King <peff@peff.net> wrote:\n> We're not doing project-wide analysis with Coverity right now, but I've\n> been doing builds of my personal repo, which I usually build off of\n> next. And since hn/reftable just hit next, it got included in my latest\n> build.\n>\n> It came up with several complaints. Some of them are dumb and can be\n> ignored (e.g., using rand() in a test harness, oh no!) but I poked at a\n> few and they look like real issues:\n\nI fixed most of the obvious ones.\n\n>   - A lot of your structs have vtables. Initializing them to NULL, as in\n>     reftable_reader_refs_for_indexed(), leaves the risk that we'll try\n>     to call a NULL function pointer, even if it's for something simple\n\nI have the impression that coverity doesn't understand enough of the\ncontrol flow.  Some of the things it complains of are code paths that\nonly get executed if err==0, in which case, the struct members at hand\nshould not be null.\n\n> I similarly wondered if these polymorphic types could be using a union\n> within reftable_record, rather than pointing to a separate stack\n> variable. Then you could initialize the whole thing without worrying\n> about intermediate NULLs (and also there's less pointer chasing and it's\n> a little bit more type safe than a void pointer). But again, I don't\n> know the code well enough to know if that would cover all of your cases.\n\nThis is a great idea. I've made a change that does this, which I will\npost shortly.\n\n> The summary of issues is below. You can get more details on their site.\n> I _think_ I've configured it so that anybody can look at:\n>\n>   https://scan.coverity.com/projects/peff-git/view_defects\n\nAlas, it says I have no access, even after I logged in.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"443314","messageId":"211207.86o85snvvv.gmgdl@evledraar.gmail.com","threadId":"57027","inReplyTo":"CAFQ2z_OrN+RkwnMyrJHdh5xN6ueOP8KKBVQ7-U4kEkA3ApcuNg@mail.gmail.com","subject":"Re: Fwd: coverity problems in reftable code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-07T17:46:02Z","receivedAt":"2021-12-07T17:54:33Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 07 2021, Han-Wen Nienhuys wrote:\n\n> On Sat, Dec 4, 2021 at 3:13 AM Jeff King <peff@peff.net> wrote:\n>> We're not doing project-wide analysis with Coverity right now, but I've\n>> been doing builds of my personal repo, which I usually build off of\n>> next. And since hn/reftable just hit next, it got included in my latest\n>> build.\n>>\n>> It came up with several complaints. Some of them are dumb and can be\n>> ignored (e.g., using rand() in a test harness, oh no!) but I poked at a\n>> few and they look like real issues:\n>\n> I fixed most of the obvious ones.\n>\n>>   - A lot of your structs have vtables. Initializing them to NULL, as in\n>>     reftable_reader_refs_for_indexed(), leaves the risk that we'll try\n>>     to call a NULL function pointer, even if it's for something simple\n>\n> I have the impression that coverity doesn't understand enough of the\n> control flow.  Some of the things it complains of are code paths that\n> only get executed if err==0, in which case, the struct members at hand\n> should not be null.\n\nI think coverity is right and the code has a logic error as it\nsuggests.\n\nIn the reftable_reader_refs_for_indexed() example Jeff cites we'll \"goto\ndone\" on error, and the reftable_record_release(&got_rec) will proceed\nto segfault since the next thing we do is to try to dereference a NULL\npointer in reftable_record_release().\n\nYou can reproduce that as a segfault in your tests with e.g. this patch\nbelow, which just emulates what would happen if \"err != 0\":\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 006709a645a..4c87b75a982 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -663,6 +663,7 @@ static int reftable_reader_refs_for_indexed(struct reftable_reader *r,\n        /* Look through the reverse index. */\n        reftable_record_from_obj(&want_rec, &want);\n        err = reader_seek(r, &oit, &want_rec);\n+       goto done;\n        if (err != 0)\n                goto done;\n\nIn that particular case this appears to be the quick fix that's needed:\n\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 6a5dac32dc6..e6594d555e5 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -1090,6 +1090,8 @@ int reftable_record_decode(struct reftable_record *rec, struct strbuf key,\n \n void reftable_record_release(struct reftable_record *rec)\n {\n+       if (!rec || !rec->ops)\n+               return;\n        rec->ops->release(rec->data);\n }\n\nBut in general this looks like an excellent candidate for some test\nfuzzing, i.e. to intrstrument the various \"err\" returning functions to\nchaos-monkey return non-zero some of the time and check for segfaults.\n"},{"id":"443394","messageId":"YbAOZMxGDELhgfut@coredump.intra.peff.net","threadId":"57027","inReplyTo":"CAFQ2z_OrN+RkwnMyrJHdh5xN6ueOP8KKBVQ7-U4kEkA3ApcuNg@mail.gmail.com","subject":"Re: Fwd: coverity problems in reftable code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-08T01:46:12Z","receivedAt":"2021-12-08T01:46:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 07, 2021 at 12:34:15PM +0100, Han-Wen Nienhuys wrote:\n\n> >   - A lot of your structs have vtables. Initializing them to NULL, as in\n> >     reftable_reader_refs_for_indexed(), leaves the risk that we'll try\n> >     to call a NULL function pointer, even if it's for something simple\n> \n> I have the impression that coverity doesn't understand enough of the\n> control flow.  Some of the things it complains of are code paths that\n> only get executed if err==0, in which case, the struct members at hand\n> should not be null.\n\nI've definitely run into cases where it doesn't understand some\ninvariant (e.g., \"foo\" is only nonzero if \"bar\" is not NULL). But the\nones I looked at here were triggerable. It's a lot easier to see via\ntheir site which details there view of the control flow...\n\n> > The summary of issues is below. You can get more details on their site.\n> > I _think_ I've configured it so that anybody can look at:\n> >\n> >   https://scan.coverity.com/projects/peff-git/view_defects\n> \n> Alas, it says I have no access, even after I logged in.\n\n...hrmph. I have it \"open to all users\", but maybe you have to be\nassociated with the project. I'll send you an \"invite\" through the\nCoverity site and see if that works (of course don't feel obligated if\nyou don't want to deal further with Coverity; it _does_ produce a ton of\nfalse positives, but it sometimes says useful things, too).\n\n> > I similarly wondered if these polymorphic types could be using a union\n> > within reftable_record, rather than pointing to a separate stack\n> > variable. Then you could initialize the whole thing without worrying\n> > about intermediate NULLs (and also there's less pointer chasing and it's\n> > a little bit more type safe than a void pointer). But again, I don't\n> > know the code well enough to know if that would cover all of your cases.\n> \n> This is a great idea. I've made a change that does this, which I will\n> post shortly.\n\nOh good. I was worried I was going off on a tangent. I'll give your\npatches a look.\n\n-Peff\n"},{"id":"443400","messageId":"YbAmANn6t9S5qKWA@coredump.intra.peff.net","threadId":"57027","inReplyTo":"YbAOZMxGDELhgfut@coredump.intra.peff.net","subject":"Re: Fwd: coverity problems in reftable code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-08T03:26:56Z","receivedAt":"2021-12-08T03:26:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 07, 2021 at 08:46:12PM -0500, Jeff King wrote:\n\n> > > The summary of issues is below. You can get more details on their site.\n> > > I _think_ I've configured it so that anybody can look at:\n> > >\n> > >   https://scan.coverity.com/projects/peff-git/view_defects\n> > \n> > Alas, it says I have no access, even after I logged in.\n> \n> ...hrmph. I have it \"open to all users\", but maybe you have to be\n> associated with the project. I'll send you an \"invite\" through the\n> Coverity site and see if that works (of course don't feel obligated if\n> you don't want to deal further with Coverity; it _does_ produce a ton of\n> false positives, but it sometimes says useful things, too).\n\nI also applied your coverity fixups to my tree, and pushed up the\nresult. As expected, Coverity claims many problems fixed, but also a few\nnew ones found.\n\nSummary is below, but I'm not sure it's that useful without the whole\ncode flow. The unreachable-code one seems just wrong. We can get there\nvia the \"goto done\" in the BLOCK_TYPE_LOG conditional, it looks like.\n\nThe first FORWARD_NULL doesn't look obvious to me from the code. But it\ntriggers a segfault running \"test-tool reftable\". (It didn't immediately\nfor me on Linux, but Windows CI shows it, and compiling with ASan on\nLinux does too).\n\n-- >8 --\n** CID 1467061:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/record_test.c: 356 in test_reftable_obj_record_roundtrip()\n\n________________________________________________________________________________________________________\n*** CID 1467061:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/record_test.c: 356 in test_reftable_obj_record_roundtrip()\n350     \n351     \t\tEXPECT(in.u.obj.hash_prefix_len == out.u.obj.hash_prefix_len);\n352     \t\tEXPECT(in.u.obj.offset_len == out.u.obj.offset_len);\n353     \n354     \t\tEXPECT(!memcmp(in.u.obj.hash_prefix, out.u.obj.hash_prefix,\n355     \t\t\t       in.u.obj.hash_prefix_len));\n>>>     CID 1467061:  Null pointer dereferences  (FORWARD_NULL)\n>>>     Passing null pointer \"out.u.obj.offsets\" to \"memcmp\", which dereferences it.\n356     \t\tEXPECT(0 == memcmp(in.u.obj.offsets, out.u.obj.offsets,\n357     \t\t\t\t   sizeof(uint64_t) * in.u.obj.offset_len));\n358     \t\tstrbuf_release(&key);\n359     \t\treftable_record_release(&out);\n360     \t}\n361     }\n\n** CID 1467060:  Possible Control flow issues  (DEADCODE)\n/reftable/block.c: 263 in block_reader_init()\n\n\n________________________________________________________________________________________________________\n*** CID 1467060:  Possible Control flow issues  (DEADCODE)\n/reftable/block.c: 263 in block_reader_init()\n257     \tbr->header_off = header_off;\n258     \tbr->restart_count = restart_count;\n259     \tbr->restart_bytes = restart_bytes;\n260     \n261     done:\n262     \tif (uncompressed) {\n>>>     CID 1467060:  Possible Control flow issues  (DEADCODE)\n>>>     Execution cannot reach this statement: \"reftable_free(uncompressed);\".\n263     \t\treftable_free(uncompressed);\n264     \t}\n265     \treturn err;\n266     }\n267     \n268     static uint32_t block_reader_restart_offset(struct block_reader *br, int i)\n\n** CID 1467059:  Null pointer dereferences  (FORWARD_NULL)\n\n\n________________________________________________________________________________________________________\n*** CID 1467059:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/record_test.c: 155 in test_reftable_ref_record_roundtrip()\n149     \t\tEXPECT(n > 0);\n150     \n151     \t\t/* decode into a non-zero reftable_record to test for leaks. */\n152     \t\tm = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ);\n153     \t\tEXPECT(n == m);\n154     \n>>>     CID 1467059:  Null pointer dereferences  (FORWARD_NULL)\n>>>     Passing \"&out.u.ref\" to \"reftable_ref_record_equal\", which dereferences null \"out.u.ref.refname\".\n155     \t\tEXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref,\n156     \t\t\t\t\t\t GIT_SHA1_RAWSZ));\n157     \t\treftable_record_release(&in);\n158     \n159     \t\tstrbuf_release(&key);\n160     \t\treftable_record_release(&out);\n\n** CID 1467058:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/record_test.c: 354 in test_reftable_obj_record_roundtrip()\n\n\n________________________________________________________________________________________________________\n*** CID 1467058:  Null pointer dereferences  (FORWARD_NULL)\n/reftable/record_test.c: 354 in test_reftable_obj_record_roundtrip()\n348     \t\t\t\t\t   GIT_SHA1_RAWSZ);\n349     \t\tEXPECT(n == m);\n350     \n351     \t\tEXPECT(in.u.obj.hash_prefix_len == out.u.obj.hash_prefix_len);\n352     \t\tEXPECT(in.u.obj.offset_len == out.u.obj.offset_len);\n353     \n>>>     CID 1467058:  Null pointer dereferences  (FORWARD_NULL)\n>>>     Passing null pointer \"out.u.obj.hash_prefix\" to \"memcmp\", which dereferences it.\n354     \t\tEXPECT(!memcmp(in.u.obj.hash_prefix, out.u.obj.hash_prefix,\n355     \t\t\t       in.u.obj.hash_prefix_len));\n356     \t\tEXPECT(0 == memcmp(in.u.obj.offsets, out.u.obj.offsets,\n357     \t\t\t\t   sizeof(uint64_t) * in.u.obj.offset_len));\n358     \t\tstrbuf_release(&key);\n359     \t\treftable_record_release(&out);\n"},{"id":"443415","messageId":"CAFQ2z_NUCj00981oQiswaYTK6iJ=HuPjNGP46tLL-RGL2xHbEw@mail.gmail.com","threadId":"57027","inReplyTo":"YbAmANn6t9S5qKWA@coredump.intra.peff.net","subject":"Re: Fwd: coverity problems in reftable code","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-12-08T10:50:16Z","receivedAt":"2021-12-08T10:50:30Z","isPatch":false,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Dec 8, 2021 at 4:26 AM Jeff King <peff@peff.net> wrote:\n> I also applied your coverity fixups to my tree, and pushed up the\n> result. As expected, Coverity claims many problems fixed, but also a few\n> new ones found.\n>\n> Summary is below, but I'm not sure it's that useful without the whole\n> code flow. The unreachable-code one seems just wrong. We can get there\n> via the \"goto done\" in the BLOCK_TYPE_LOG conditional, it looks like.\n>\n\nthey're all valid. There is a shadowed variable in the unreachable code one.\n\n\n> The first FORWARD_NULL doesn't look obvious to me from the code. But it\n> triggers a segfault running \"test-tool reftable\". (It didn't immediately\n> for me on Linux, but Windows CI shows it, and compiling with ASan on\n> Linux does too).\n\nyeah, that must be the problem on windows too. The 3rd test case\npasses NULL into memcmp() with a 0 length.\n\nI've folded in your patch and fixed the issues.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"443498","messageId":"YbEDnfZgeQZp6JJj@coredump.intra.peff.net","threadId":"57027","inReplyTo":"CAFQ2z_NUCj00981oQiswaYTK6iJ=HuPjNGP46tLL-RGL2xHbEw@mail.gmail.com","subject":"Re: Fwd: coverity problems in reftable code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-08T19:12:29Z","receivedAt":"2021-12-08T19:12:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 08, 2021 at 11:50:16AM +0100, Han-Wen Nienhuys wrote:\n\n> > Summary is below, but I'm not sure it's that useful without the whole\n> > code flow. The unreachable-code one seems just wrong. We can get there\n> > via the \"goto done\" in the BLOCK_TYPE_LOG conditional, it looks like.\n> \n> they're all valid. There is a shadowed variable in the unreachable code one.\n\nAh, good catch. Yes, that is definitely the problem.\n\n> > The first FORWARD_NULL doesn't look obvious to me from the code. But it\n> > triggers a segfault running \"test-tool reftable\". (It didn't immediately\n> > for me on Linux, but Windows CI shows it, and compiling with ASan on\n> > Linux does too).\n> \n> yeah, that must be the problem on windows too. The 3rd test case\n> passes NULL into memcmp() with a 0 length.\n\nAh, makes sense. I didn't look closely, but that would explain perfectly\nwhy it fails on some platforms but not others.\n\n> I've folded in your patch and fixed the issues.\n\nThanks!\n\n-Peff\n"}]}