coverity problems in reftable code

coverity problems in reftable code

[Jeff King]

Hi Han-Wen,

We're not doing project-wide analysis with Coverity right now, but I've
been doing builds of my personal repo, which I usually build off of
next. And since hn/reftable just hit next, it got included in my latest
build.

It came up with several complaints. Some of them are dumb and can be
ignored (e.g., using rand() in a test harness, oh no!) but I poked at a
few and they look like real issues:

  - the stack overflow in reftable_log_record_print() is real; I think you
    want HEXSZ instead of RAWSZ for your buffer (also, should it be
    GIT_MAX_HEXSZ?)

  - various fd/memory leaks on error returns

  - A lot of your structs have vtables. Initializing them to NULL, as in
    reftable_reader_refs_for_indexed(), leaves the risk that we'll try
    to call a NULL function pointer, even if it's for something simple
    like cleaning up. That's the case here (if loading want_rec fails,
    we'll jump to "done" and try to clean up got_rec, even though it's
    still got a NULL vtable). One solution is for reftable_record_release()
    to check for NULL (and other destructors; there's a similar case in
    the last snippet below for reftable_reader_free()).

    But I also wasn't sure how extensive the use of polymorphism was,
    and if it might be possible for us to do more at initialization
    time. Something like:

diff --git a/reftable/reader.c b/reftable/reader.c
index 2db0019111..8ac248b070 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -653,21 +653,19 @@ static int reftable_reader_refs_for_indexed(struct reftable_reader *r,
 		.hash_prefix = oid,
 		.hash_prefix_len = r->object_id_len,
 	};
-	struct reftable_record want_rec = { NULL };
+	struct reftable_record want_rec = REFTABLE_RECORD_OBJ(&want);
 	struct reftable_iterator oit = { NULL };
 	struct reftable_obj_record got = { NULL };
-	struct reftable_record got_rec = { NULL };
+	struct reftable_record got_rec = REFTABLE_RECORD_OBJ(&got);
 	int err = 0;
 	struct indexed_table_ref_iter *itr = NULL;
 
 	/* Look through the reverse index. */
-	reftable_record_from_obj(&want_rec, &want);
 	err = reader_seek(r, &oit, &want_rec);
 	if (err != 0)
 		goto done;
 
 	/* read out the reftable_obj_record */
-	reftable_record_from_obj(&got_rec, &got);
 	err = iterator_next(&oit, &got_rec);
 	if (err < 0)
 		goto done;
diff --git a/reftable/record.c b/reftable/record.c
index 5a4bbed6c8..4c34752d18 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -572,7 +572,7 @@ static int not_a_deletion(const void *UNUSED(p))
 	return 0;
 }
 
-static struct reftable_record_vtable reftable_obj_record_vtable = {
+struct reftable_record_vtable reftable_obj_record_vtable = {
 	.key = &reftable_obj_record_key,
 	.type = BLOCK_TYPE_OBJ,
 	.copy_from = &reftable_obj_record_copy_from,
@@ -1106,14 +1106,6 @@ void reftable_record_from_ref(struct reftable_record *rec,
 	rec->ops = &reftable_ref_record_vtable;
 }
 
-void reftable_record_from_obj(struct reftable_record *rec,
-			      struct reftable_obj_record *obj_rec)
-{
-	assert(!rec->ops);
-	rec->data = obj_rec;
-	rec->ops = &reftable_obj_record_vtable;
-}
-
 void reftable_record_from_index(struct reftable_record *rec,
 				struct reftable_index_record *index_rec)
 {
diff --git a/reftable/record.h b/reftable/record.h
index 498e8c50bf..316cde9182 100644
--- a/reftable/record.h
+++ b/reftable/record.h
@@ -66,6 +66,12 @@ struct reftable_record {
 	struct reftable_record_vtable *ops;
 };
 
+extern struct reftable_record_vtable reftable_obj_record_vtable;
+#define REFTABLE_RECORD_OBJ(obj_record) { \
+	.data = obj_record, \
+	.ops = &reftable_obj_record_vtable \
+}
+
 /* returns true for recognized block types. Block start with the block type. */
 int reftable_is_block_type(uint8_t typ);
 
@@ -119,8 +125,6 @@ void reftable_record_destroy(struct reftable_record *rec);
 
 /* initialize generic records from concrete records. The generic record should
  * be zeroed out. */
-void reftable_record_from_obj(struct reftable_record *rec,
-			      struct reftable_obj_record *objrec);
 void reftable_record_from_index(struct reftable_record *rec,
 				struct reftable_index_record *idxrec);
 void reftable_record_from_ref(struct reftable_record *rec,

but there may be other cases which really want the more dynamic
initialization. And also, the virtual release function would need to
handle the NULL data as well.

I similarly wondered if these polymorphic types could be using a union
within reftable_record, rather than pointing to a separate stack
variable. Then you could initialize the whole thing without worrying
about intermediate NULLs (and also there's less pointer chasing and it's
a little bit more type safe than a void pointer). But again, I don't
know the code well enough to know if that would cover all of your cases.

The summary of issues is below. You can get more details on their site.
I _think_ I've configured it so that anybody can look at:

  https://scan.coverity.com/projects/peff-git/view_defects

but you'll probably need to make a log in (or connect with a GitHub
account). I usually find the summary enough to find issues, but
sometimes it's useful to look at the site because it outlines the full
path of assumptions (not just "here we dereference NULL", but the
sequence of code that goes from knowing that a variable is NULL to the
point of dereferencing it).

I'm happy to help dig into any of them more, or even work on patches.
But per the above diff, I wasn't sure how far to go. :)

-Peff

-- >8 --
** CID 1467043:  Resource leaks  (RESOURCE_LEAK)
/reftable/block.c: 220 in block_reader_init()


________________________________________________________________________________________________________
*** CID 1467043:  Resource leaks  (RESOURCE_LEAK)
/reftable/block.c: 220 in block_reader_init()
214     				block->data + block_header_skip, &src_len)) {
215     			reftable_free(uncompressed);
216     			return REFTABLE_ZLIB_ERROR;
217     		}
218     
219     		if (dst_len + block_header_skip != sz)
>>>     CID 1467043:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "uncompressed" going out of scope leaks the storage it points to.
220     			return REFTABLE_FORMAT_ERROR;
221     
222     		/* We're done with the input data. */
223     		reftable_block_done(block);
224     		block->data = uncompressed;
225     		block->len = sz;

** CID 1467042:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1467042:  Null pointer dereferences  (FORWARD_NULL)
/reftable/reader.c: 692 in reftable_reader_refs_for_indexed()
686     		goto done;
687     	got.offsets = NULL;
688     	iterator_from_indexed_table_ref_iter(it, itr);
689     
690     done:
691     	reftable_iterator_destroy(&oit);
>>>     CID 1467042:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing "&got_rec" to "reftable_record_release", which dereferences null "got_rec.ops".
692     	reftable_record_release(&got_rec);
693     	return err;
694     }
695     
696     static int reftable_reader_refs_for_unindexed(struct reftable_reader *r,
697     					      struct reftable_iterator *it,

** CID 1467041:  Resource leaks  (RESOURCE_LEAK)
/reftable/blocksource.c: 138 in reftable_block_source_from_file()


________________________________________________________________________________________________________
*** CID 1467041:  Resource leaks  (RESOURCE_LEAK)
/reftable/blocksource.c: 138 in reftable_block_source_from_file()
132     		}
133     		return -1;
134     	}
135     
136     	err = fstat(fd, &st);
137     	if (err < 0)
>>>     CID 1467041:  Resource leaks  (RESOURCE_LEAK)
>>>     Handle variable "fd" going out of scope leaks the handle.
138     		return -1;
139     
140     	p = reftable_calloc(sizeof(struct file_block_source));
141     	p->size = st.st_size;
142     	p->fd = fd;
143     

** CID 1467040:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1467040:  Null pointer dereferences  (FORWARD_NULL)
/reftable/record_test.c: 159 in test_reftable_ref_record_roundtrip()
153     
154     		reftable_record_from_ref(&rec_out, &out);
155     		m = reftable_record_decode(&rec_out, key, i, dest,
156     					   GIT_SHA1_RAWSZ);
157     		EXPECT(n == m);
158     
>>>     CID 1467040:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing "&out" to "reftable_ref_record_equal", which dereferences null "out.refname".
159     		EXPECT(reftable_ref_record_equal(&in, &out, GIT_SHA1_RAWSZ));
160     		reftable_record_release(&rec_out);
161     
162     		strbuf_release(&key);
163     		reftable_ref_record_release(&in);
164     	}

** CID 1467039:    (TAINTED_SCALAR)
/reftable/block.c: 212 in block_reader_init()


________________________________________________________________________________________________________
*** CID 1467039:    (TAINTED_SCALAR)
/reftable/block.c: 212 in block_reader_init()
206     		uint8_t *uncompressed = reftable_malloc(sz);
207     
208     		/* Copy over the block header verbatim. It's not compressed. */
209     		memcpy(uncompressed, block->data, block_header_skip);
210     
211     		/* Uncompress */
>>>     CID 1467039:    (TAINTED_SCALAR)
>>>     Passing tainted expression "dst_len" to "uncompress2", which uses it as an offset.
212     		if (Z_OK !=
213     		    uncompress2(uncompressed + block_header_skip, &dst_len,
214     				block->data + block_header_skip, &src_len)) {
215     			reftable_free(uncompressed);
216     			return REFTABLE_ZLIB_ERROR;
217     		}
/reftable/block.c: 206 in block_reader_init()
200     		int block_header_skip = 4 + header_off;
201     		uLongf dst_len = sz - block_header_skip; /* total size of dest
202     							    buffer. */
203     		uLongf src_len = block->len - block_header_skip;
204     		/* Log blocks specify the *uncompressed* size in their header.
205     		 */
>>>     CID 1467039:    (TAINTED_SCALAR)
>>>     Passing tainted expression "sz" to "reftable_malloc", which uses it as an allocation size.
206     		uint8_t *uncompressed = reftable_malloc(sz);
207     
208     		/* Copy over the block header verbatim. It's not compressed. */
209     		memcpy(uncompressed, block->data, block_header_skip);
210     
211     		/* Uncompress */

** CID 1467038:    (DC.WEAK_CRYPTO)
/reftable/readwrite_test.c: 145 in test_log_buffer_size()
/reftable/readwrite_test.c: 144 in test_log_buffer_size()


________________________________________________________________________________________________________
*** CID 1467038:    (DC.WEAK_CRYPTO)
/reftable/readwrite_test.c: 145 in test_log_buffer_size()
139     	/* This tests buffer extension for log compression. Must use a random
140     	   hash, to ensure that the compressed part is larger than the original.
141     	*/
142     	uint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];
143     	for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
144     		hash1[i] = (uint8_t)(rand() % 256);
>>>     CID 1467038:    (DC.WEAK_CRYPTO)
>>>     "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.
145     		hash2[i] = (uint8_t)(rand() % 256);
146     	}
147     	log.value.update.old_hash = hash1;
148     	log.value.update.new_hash = hash2;
149     	reftable_writer_set_limits(w, update_index, update_index);
150     	err = reftable_writer_add_log(w, &log);
/reftable/readwrite_test.c: 144 in test_log_buffer_size()
138     
139     	/* This tests buffer extension for log compression. Must use a random
140     	   hash, to ensure that the compressed part is larger than the original.
141     	*/
142     	uint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];
143     	for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
>>>     CID 1467038:    (DC.WEAK_CRYPTO)
>>>     "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.
144     		hash1[i] = (uint8_t)(rand() % 256);
145     		hash2[i] = (uint8_t)(rand() % 256);
146     	}
147     	log.value.update.old_hash = hash1;
148     	log.value.update.new_hash = hash2;
149     	reftable_writer_set_limits(w, update_index, update_index);

** CID 1467037:  Security best practices violations  (DC.WEAK_CRYPTO)
/reftable/stack.c: 439 in format_name()


________________________________________________________________________________________________________
*** CID 1467037:  Security best practices violations  (DC.WEAK_CRYPTO)
/reftable/stack.c: 439 in format_name()
433     	return 0;
434     }
435     
436     static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)
437     {
438     	char buf[100];
>>>     CID 1467037:  Security best practices violations  (DC.WEAK_CRYPTO)
>>>     "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.
439     	uint32_t rnd = (uint32_t)rand();
440     	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
441     		 min, max, rnd);
442     	strbuf_reset(dest);
443     	strbuf_addstr(dest, buf);
444     }

** CID 1467036:  Code maintainability issues  (UNUSED_VALUE)
/reftable/stack_test.c: 816 in test_reftable_stack_auto_compaction()


________________________________________________________________________________________________________
*** CID 1467036:  Code maintainability issues  (UNUSED_VALUE)
/reftable/stack_test.c: 816 in test_reftable_stack_auto_compaction()
810     		};
811     		snprintf(name, sizeof(name), "branch%04d", i);
812     
813     		err = reftable_stack_add(st, &write_test_ref, &ref);
814     		EXPECT_ERR(err);
815     
>>>     CID 1467036:  Code maintainability issues  (UNUSED_VALUE)
>>>     Assigning value from "reftable_stack_auto_compact(st)" to "err" here, but that stored value is overwritten before it can be used.
816     		err = reftable_stack_auto_compact(st);
817     		EXPECT(i < 3 || st->merged->stack_len < 2 * fastlog2(i));
818     	}
819     
820     	EXPECT(reftable_stack_compaction_stats(st)->entries_written <
821     	       (uint64_t)(N * fastlog2(N)));

** CID 1467035:    (OVERRUN)


________________________________________________________________________________________________________
*** CID 1467035:    (OVERRUN)
/reftable/record.c: 603 in reftable_log_record_print()
597     		printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n",
598     		       log->refname, log->update_index, log->value.update.name,
599     		       log->value.update.email, log->value.update.time,
600     		       log->value.update.tz_offset);
601     		hex_format(hex, log->value.update.old_hash, hash_size(hash_id));
602     		printf("%s => ", hex);
>>>     CID 1467035:    (OVERRUN)
>>>     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).
603     		hex_format(hex, log->value.update.new_hash, hash_size(hash_id));
604     		printf("%s\n\n%s\n}\n", hex, log->value.update.message);
605     		break;
606     	}
607     }
608     
/reftable/record.c: 601 in reftable_log_record_print()
595     		break;
596     	case REFTABLE_LOG_UPDATE:
597     		printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n",
598     		       log->refname, log->update_index, log->value.update.name,
599     		       log->value.update.email, log->value.update.time,
600     		       log->value.update.tz_offset);
>>>     CID 1467035:    (OVERRUN)
>>>     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).
601     		hex_format(hex, log->value.update.old_hash, hash_size(hash_id));
602     		printf("%s => ", hex);
603     		hex_format(hex, log->value.update.new_hash, hash_size(hash_id));
604     		printf("%s\n\n%s\n}\n", hex, log->value.update.message);
605     		break;
606     	}

** CID 1467034:  Error handling issues  (CHECKED_RETURN)
/reftable/stack_test.c: 92 in test_read_file()


________________________________________________________________________________________________________
*** CID 1467034:  Error handling issues  (CHECKED_RETURN)
/reftable/stack_test.c: 92 in test_read_file()
86     	EXPECT_ERR(err);
87     
88     	for (i = 0; names[i]; i++) {
89     		EXPECT(0 == strcmp(want[i], names[i]));
90     	}
91     	free_names(names);
>>>     CID 1467034:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "remove(fn)" without checking return value. This library function may fail and return an error code.
92     	remove(fn);
93     }
94     
95     static void test_parse_names(void)
96     {
97     	char buf[] = "line\n";

** CID 1467033:  Resource leaks  (RESOURCE_LEAK)
/reftable/stack.c: 951 in stack_compact_range()


________________________________________________________________________________________________________
*** CID 1467033:  Resource leaks  (RESOURCE_LEAK)
/reftable/stack.c: 951 in stack_compact_range()
945     		subtable_locks[j] = subtab_lock.buf;
946     		delete_on_success[j] = subtab_file_name.buf;
947     		j++;
948     
949     		if (err != 0)
950     			goto done;
>>>     CID 1467033:  Resource leaks  (RESOURCE_LEAK)
>>>     Handle variable "sublock_file_fd" going out of scope leaks the handle.
951     	}
952     
953     	err = unlink(lock_file_name.buf);
954     	if (err < 0)
955     		goto done;
956     	have_lock = 0;

** CID 1467032:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1467032:  Null pointer dereferences  (FORWARD_NULL)
/reftable/reader.c: 504 in reader_seek_indexed()
498     	reftable_record_from_index(&index_result_rec, &index_result);
499     
500     	err = reader_start(r, &index_iter, reftable_record_type(rec), 1);
501     	if (err < 0)
502     		goto done;
503     
>>>     CID 1467032:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing "&index_iter" to "reader_seek_linear", which dereferences null "index_iter.r".
504     	err = reader_seek_linear(&index_iter, &want_index_rec);
505     	while (1) {
506     		err = table_iter_next(&index_iter, &index_result_rec);
507     		table_iter_block_done(&index_iter);
508     		if (err != 0)
509     			goto done;

** CID 1467031:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1467031:  Null pointer dereferences  (FORWARD_NULL)
/reftable/stack.c: 710 in reftable_addition_add()
704     		unlink(temp_tab_file_name.buf);
705     	}
706     
707     	strbuf_release(&temp_tab_file_name);
708     	strbuf_release(&tab_file_name);
709     	strbuf_release(&next_name);
>>>     CID 1467031:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "wr" to "reftable_writer_free", which dereferences it.
710     	reftable_writer_free(wr);
711     	return err;
712     }
713     
714     uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
715     {

** CID 1467030:  Security best practices violations  (DC.WEAK_CRYPTO)
/reftable/stack.c: 367 in reftable_stack_reload_maybe_reuse()


________________________________________________________________________________________________________
*** CID 1467030:  Security best practices violations  (DC.WEAK_CRYPTO)
/reftable/stack.c: 367 in reftable_stack_reload_maybe_reuse()
361     			free_names(names_after);
362     			return err;
363     		}
364     		free_names(names);
365     		free_names(names_after);
366     
>>>     CID 1467030:  Security best practices violations  (DC.WEAK_CRYPTO)
>>>     "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.
367     		delay = delay + (delay * rand()) / RAND_MAX + 1;
368     		sleep_millisec(delay);
369     	}
370     
371     	return 0;
372     }

** CID 1467029:  Resource leaks  (RESOURCE_LEAK)
/reftable/stack.c: 1065 in stack_compact_range()


________________________________________________________________________________________________________
*** CID 1467029:  Resource leaks  (RESOURCE_LEAK)
/reftable/stack.c: 1065 in stack_compact_range()
1059     	}
1060     	strbuf_release(&new_table_name);
1061     	strbuf_release(&new_table_path);
1062     	strbuf_release(&ref_list_contents);
1063     	strbuf_release(&temp_tab_file_name);
1064     	strbuf_release(&lock_file_name);
>>>     CID 1467029:  Resource leaks  (RESOURCE_LEAK)
>>>     Handle variable "lock_file_fd" going out of scope leaks the handle.
1065     	return err;
1066     }
1067     
1068     int reftable_stack_compact_all(struct reftable_stack *st,
1069     			       struct reftable_log_expiry_config *config)
1070     {

** CID 1467028:  Null pointer dereferences  (FORWARD_NULL)
/reftable/reader.c: 675 in reftable_reader_refs_for_indexed()


________________________________________________________________________________________________________
*** CID 1467028:  Null pointer dereferences  (FORWARD_NULL)
/reftable/reader.c: 675 in reftable_reader_refs_for_indexed()
669     	/* read out the reftable_obj_record */
670     	reftable_record_from_obj(&got_rec, &got);
671     	err = iterator_next(&oit, &got_rec);
672     	if (err < 0)
673     		goto done;
674     
>>>     CID 1467028:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "got.hash_prefix" to "memcmp", which dereferences it.
675     	if (err > 0 ||
676     	    memcmp(want.hash_prefix, got.hash_prefix, r->object_id_len)) {
677     		/* didn't find it; return empty iterator */
678     		iterator_set_empty(it);
679     		err = 0;
680     		goto done;

** CID 1467027:  Security best practices violations  (SECURE_TEMP)
/reftable/stack_test.c: 72 in test_read_file()


________________________________________________________________________________________________________
*** CID 1467027:  Security best practices violations  (SECURE_TEMP)
/reftable/stack_test.c: 72 in test_read_file()
66     	return dir;
67     }
68     
69     static void test_read_file(void)
70     {
71     	char *fn = get_tmp_template(__LINE__);
>>>     CID 1467027:  Security best practices violations  (SECURE_TEMP)
>>>     Calling "mkstemp" without securely setting umask first.
72     	int fd = mkstemp(fn);
73     	char out[1024] = "line1\n\nline2\nline3";
74     	int n, err;
75     	char **names = NULL;
76     	char *want[] = { "line1", "line2", "line3" };
77     	int i = 0;

** CID 1467026:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1467026:  Null pointer dereferences  (FORWARD_NULL)
/reftable/reader.c: 683 in reftable_reader_refs_for_indexed()
677     		/* didn't find it; return empty iterator */
678     		iterator_set_empty(it);
679     		err = 0;
680     		goto done;
681     	}
682     
>>>     CID 1467026:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "got.offsets" to "new_indexed_table_ref_iter", which dereferences it.
683     	err = new_indexed_table_ref_iter(&itr, r, oid, hash_size(r->hash_id),
684     					 got.offsets, got.offset_len);
685     	if (err < 0)
686     		goto done;
687     	got.offsets = NULL;
688     	iterator_from_indexed_table_ref_iter(it, itr);

** CID 1467025:  Security best practices violations  (SECURE_TEMP)
/reftable/stack.c: 642 in reftable_addition_add()


________________________________________________________________________________________________________
*** CID 1467025:  Security best practices violations  (SECURE_TEMP)
/reftable/stack.c: 642 in reftable_addition_add()
636     	strbuf_reset(&next_name);
637     	format_name(&next_name, add->next_update_index, add->next_update_index);
638     
639     	stack_filename(&temp_tab_file_name, add->stack, next_name.buf);
640     	strbuf_addstr(&temp_tab_file_name, ".temp.XXXXXX");
641     
>>>     CID 1467025:  Security best practices violations  (SECURE_TEMP)
>>>     Calling "mkstemp" without securely setting umask first.
642     	tab_fd = mkstemp(temp_tab_file_name.buf);
643     	if (tab_fd < 0) {
644     		err = REFTABLE_IO_ERROR;
645     		goto done;
646     	}
647     

** CID 1467024:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1467024:  Null pointer dereferences  (FORWARD_NULL)
/reftable/reader.c: 799 in reftable_reader_print_file()
793     	if (err < 0)
794     		goto done;
795     
796     	reftable_table_from_reader(&tab, r);
797     	err = reftable_table_print(&tab);
798     done:
>>>     CID 1467024:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "r" to "reftable_reader_free", which dereferences it.
799     	reftable_reader_free(r);
800     	return err;

Fwd: coverity problems in reftable code

[Han-Wen Nienhuys]

On Sat, Dec 4, 2021 at 3:13 AM Jeff King <peff@peff.net> wrote:
> We're not doing project-wide analysis with Coverity right now, but I've
> been doing builds of my personal repo, which I usually build off of
> next. And since hn/reftable just hit next, it got included in my latest
> build.
>
> It came up with several complaints. Some of them are dumb and can be
> ignored (e.g., using rand() in a test harness, oh no!) but I poked at a
> few and they look like real issues:

I fixed most of the obvious ones.

>   - A lot of your structs have vtables. Initializing them to NULL, as in
>     reftable_reader_refs_for_indexed(), leaves the risk that we'll try
>     to call a NULL function pointer, even if it's for something simple

I have the impression that coverity doesn't understand enough of the
control flow.  Some of the things it complains of are code paths that
only get executed if err==0, in which case, the struct members at hand
should not be null.

> I similarly wondered if these polymorphic types could be using a union
> within reftable_record, rather than pointing to a separate stack
> variable. Then you could initialize the whole thing without worrying
> about intermediate NULLs (and also there's less pointer chasing and it's
> a little bit more type safe than a void pointer). But again, I don't
> know the code well enough to know if that would cover all of your cases.

This is a great idea. I've made a change that does this, which I will
post shortly.

> The summary of issues is below. You can get more details on their site.
> I _think_ I've configured it so that anybody can look at:
>
>   https://scan.coverity.com/projects/peff-git/view_defects

Alas, it says I have no access, even after I logged in.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: Fwd: coverity problems in reftable code

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 07 2021, Han-Wen Nienhuys wrote:

> On Sat, Dec 4, 2021 at 3:13 AM Jeff King <peff@peff.net> wrote:
>> We're not doing project-wide analysis with Coverity right now, but I've
>> been doing builds of my personal repo, which I usually build off of
>> next. And since hn/reftable just hit next, it got included in my latest
>> build.
>>
>> It came up with several complaints. Some of them are dumb and can be
>> ignored (e.g., using rand() in a test harness, oh no!) but I poked at a
>> few and they look like real issues:
>
> I fixed most of the obvious ones.
>
>>   - A lot of your structs have vtables. Initializing them to NULL, as in
>>     reftable_reader_refs_for_indexed(), leaves the risk that we'll try
>>     to call a NULL function pointer, even if it's for something simple
>
> I have the impression that coverity doesn't understand enough of the
> control flow.  Some of the things it complains of are code paths that
> only get executed if err==0, in which case, the struct members at hand
> should not be null.

I think coverity is right and the code has a logic error as it
suggests.

In the reftable_reader_refs_for_indexed() example Jeff cites we'll "goto
done" on error, and the reftable_record_release(&got_rec) will proceed
to segfault since the next thing we do is to try to dereference a NULL
pointer in reftable_record_release().

You can reproduce that as a segfault in your tests with e.g. this patch
below, which just emulates what would happen if "err != 0":

diff --git a/reftable/reader.c b/reftable/reader.c
index 006709a645a..4c87b75a982 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -663,6 +663,7 @@ static int reftable_reader_refs_for_indexed(struct reftable_reader *r,
        /* Look through the reverse index. */
        reftable_record_from_obj(&want_rec, &want);
        err = reader_seek(r, &oit, &want_rec);
+       goto done;
        if (err != 0)
                goto done;

In that particular case this appears to be the quick fix that's needed:

diff --git a/reftable/record.c b/reftable/record.c
index 6a5dac32dc6..e6594d555e5 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -1090,6 +1090,8 @@ int reftable_record_decode(struct reftable_record *rec, struct strbuf key,
 
 void reftable_record_release(struct reftable_record *rec)
 {
+       if (!rec || !rec->ops)
+               return;
        rec->ops->release(rec->data);
 }

But in general this looks like an excellent candidate for some test
fuzzing, i.e. to intrstrument the various "err" returning functions to
chaos-monkey return non-zero some of the time and check for segfaults.

Re: Fwd: coverity problems in reftable code

[Jeff King]

On Tue, Dec 07, 2021 at 12:34:15PM +0100, Han-Wen Nienhuys wrote:

> >   - A lot of your structs have vtables. Initializing them to NULL, as in
> >     reftable_reader_refs_for_indexed(), leaves the risk that we'll try
> >     to call a NULL function pointer, even if it's for something simple
> 
> I have the impression that coverity doesn't understand enough of the
> control flow.  Some of the things it complains of are code paths that
> only get executed if err==0, in which case, the struct members at hand
> should not be null.

I've definitely run into cases where it doesn't understand some
invariant (e.g., "foo" is only nonzero if "bar" is not NULL). But the
ones I looked at here were triggerable. It's a lot easier to see via
their site which details there view of the control flow...

> > The summary of issues is below. You can get more details on their site.
> > I _think_ I've configured it so that anybody can look at:
> >
> >   https://scan.coverity.com/projects/peff-git/view_defects
> 
> Alas, it says I have no access, even after I logged in.

...hrmph. I have it "open to all users", but maybe you have to be
associated with the project. I'll send you an "invite" through the
Coverity site and see if that works (of course don't feel obligated if
you don't want to deal further with Coverity; it _does_ produce a ton of
false positives, but it sometimes says useful things, too).

> > I similarly wondered if these polymorphic types could be using a union
> > within reftable_record, rather than pointing to a separate stack
> > variable. Then you could initialize the whole thing without worrying
> > about intermediate NULLs (and also there's less pointer chasing and it's
> > a little bit more type safe than a void pointer). But again, I don't
> > know the code well enough to know if that would cover all of your cases.
> 
> This is a great idea. I've made a change that does this, which I will
> post shortly.

Oh good. I was worried I was going off on a tangent. I'll give your
patches a look.

-Peff

Re: Fwd: coverity problems in reftable code

[Jeff King]

On Tue, Dec 07, 2021 at 08:46:12PM -0500, Jeff King wrote:

> > > The summary of issues is below. You can get more details on their site.
> > > I _think_ I've configured it so that anybody can look at:
> > >
> > >   https://scan.coverity.com/projects/peff-git/view_defects
> > 
> > Alas, it says I have no access, even after I logged in.
> 
> ...hrmph. I have it "open to all users", but maybe you have to be
> associated with the project. I'll send you an "invite" through the
> Coverity site and see if that works (of course don't feel obligated if
> you don't want to deal further with Coverity; it _does_ produce a ton of
> false positives, but it sometimes says useful things, too).

I also applied your coverity fixups to my tree, and pushed up the
result. As expected, Coverity claims many problems fixed, but also a few
new ones found.

Summary is below, but I'm not sure it's that useful without the whole
code flow. The unreachable-code one seems just wrong. We can get there
via the "goto done" in the BLOCK_TYPE_LOG conditional, it looks like.

The first FORWARD_NULL doesn't look obvious to me from the code. But it
triggers a segfault running "test-tool reftable". (It didn't immediately
for me on Linux, but Windows CI shows it, and compiling with ASan on
Linux does too).

-- >8 --
** CID 1467061:  Null pointer dereferences  (FORWARD_NULL)
/reftable/record_test.c: 356 in test_reftable_obj_record_roundtrip()

________________________________________________________________________________________________________
*** CID 1467061:  Null pointer dereferences  (FORWARD_NULL)
/reftable/record_test.c: 356 in test_reftable_obj_record_roundtrip()
350     
351     		EXPECT(in.u.obj.hash_prefix_len == out.u.obj.hash_prefix_len);
352     		EXPECT(in.u.obj.offset_len == out.u.obj.offset_len);
353     
354     		EXPECT(!memcmp(in.u.obj.hash_prefix, out.u.obj.hash_prefix,
355     			       in.u.obj.hash_prefix_len));
>>>     CID 1467061:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "out.u.obj.offsets" to "memcmp", which dereferences it.
356     		EXPECT(0 == memcmp(in.u.obj.offsets, out.u.obj.offsets,
357     				   sizeof(uint64_t) * in.u.obj.offset_len));
358     		strbuf_release(&key);
359     		reftable_record_release(&out);
360     	}
361     }

** CID 1467060:  Possible Control flow issues  (DEADCODE)
/reftable/block.c: 263 in block_reader_init()


________________________________________________________________________________________________________
*** CID 1467060:  Possible Control flow issues  (DEADCODE)
/reftable/block.c: 263 in block_reader_init()
257     	br->header_off = header_off;
258     	br->restart_count = restart_count;
259     	br->restart_bytes = restart_bytes;
260     
261     done:
262     	if (uncompressed) {
>>>     CID 1467060:  Possible Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "reftable_free(uncompressed);".
263     		reftable_free(uncompressed);
264     	}
265     	return err;
266     }
267     
268     static uint32_t block_reader_restart_offset(struct block_reader *br, int i)

** CID 1467059:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1467059:  Null pointer dereferences  (FORWARD_NULL)
/reftable/record_test.c: 155 in test_reftable_ref_record_roundtrip()
149     		EXPECT(n > 0);
150     
151     		/* decode into a non-zero reftable_record to test for leaks. */
152     		m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ);
153     		EXPECT(n == m);
154     
>>>     CID 1467059:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing "&out.u.ref" to "reftable_ref_record_equal", which dereferences null "out.u.ref.refname".
155     		EXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref,
156     						 GIT_SHA1_RAWSZ));
157     		reftable_record_release(&in);
158     
159     		strbuf_release(&key);
160     		reftable_record_release(&out);

** CID 1467058:  Null pointer dereferences  (FORWARD_NULL)
/reftable/record_test.c: 354 in test_reftable_obj_record_roundtrip()


________________________________________________________________________________________________________
*** CID 1467058:  Null pointer dereferences  (FORWARD_NULL)
/reftable/record_test.c: 354 in test_reftable_obj_record_roundtrip()
348     					   GIT_SHA1_RAWSZ);
349     		EXPECT(n == m);
350     
351     		EXPECT(in.u.obj.hash_prefix_len == out.u.obj.hash_prefix_len);
352     		EXPECT(in.u.obj.offset_len == out.u.obj.offset_len);
353     
>>>     CID 1467058:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "out.u.obj.hash_prefix" to "memcmp", which dereferences it.
354     		EXPECT(!memcmp(in.u.obj.hash_prefix, out.u.obj.hash_prefix,
355     			       in.u.obj.hash_prefix_len));
356     		EXPECT(0 == memcmp(in.u.obj.offsets, out.u.obj.offsets,
357     				   sizeof(uint64_t) * in.u.obj.offset_len));
358     		strbuf_release(&key);
359     		reftable_record_release(&out);

Re: Fwd: coverity problems in reftable code

[Han-Wen Nienhuys]

On Wed, Dec 8, 2021 at 4:26 AM Jeff King <peff@peff.net> wrote:
> I also applied your coverity fixups to my tree, and pushed up the
> result. As expected, Coverity claims many problems fixed, but also a few
> new ones found.
>
> Summary is below, but I'm not sure it's that useful without the whole
> code flow. The unreachable-code one seems just wrong. We can get there
> via the "goto done" in the BLOCK_TYPE_LOG conditional, it looks like.
>

they're all valid. There is a shadowed variable in the unreachable code one.


> The first FORWARD_NULL doesn't look obvious to me from the code. But it
> triggers a segfault running "test-tool reftable". (It didn't immediately
> for me on Linux, but Windows CI shows it, and compiling with ASan on
> Linux does too).

yeah, that must be the problem on windows too. The 3rd test case
passes NULL into memcmp() with a 0 length.

I've folded in your patch and fixed the issues.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: Fwd: coverity problems in reftable code

[Jeff King]

On Wed, Dec 08, 2021 at 11:50:16AM +0100, Han-Wen Nienhuys wrote:

> > Summary is below, but I'm not sure it's that useful without the whole
> > code flow. The unreachable-code one seems just wrong. We can get there
> > via the "goto done" in the BLOCK_TYPE_LOG conditional, it looks like.
> 
> they're all valid. There is a shadowed variable in the unreachable code one.

Ah, good catch. Yes, that is definitely the problem.

> > The first FORWARD_NULL doesn't look obvious to me from the code. But it
> > triggers a segfault running "test-tool reftable". (It didn't immediately
> > for me on Linux, but Windows CI shows it, and compiling with ASan on
> > Linux does too).
> 
> yeah, that must be the problem on windows too. The 3rd test case
> passes NULL into memcmp() with a 0 length.

Ah, makes sense. I didn't look closely, but that would explain perfectly
why it fails on some platforms but not others.

> I've folded in your patch and fixed the issues.

Thanks!

-Peff