| Message ID | YarO3nkrutmWF7nb@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coverity problems in reftable code | expand |
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.
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.
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
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()
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.
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
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,
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: 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()