Re: Fwd: coverity problems in reftable code

From: Jeff King <peff@peff.net>
Date: Dec 8, 2021, 03:26 UTC
Message-ID: <YbAmANn6t9S5qKWA@coredump.intra.peff.net>
In-Reply-To: <YbAOZMxGDELhgfut@coredump.intra.peff.net>

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

Show 12 quoted lines

> > > 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);