| Message ID | 20250201022409.GA4082344@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records | expand |
Hi Peff On 01/02/2025 02:24, Jeff King wrote: > On Wed, Jan 22, 2025 at 06:35:49AM +0100, Karthik Nayak wrote: > > Coverity complains that this function may have undefined behavior. It's > an issue we have in a lot of other tests that have moved to the > unit-test framework. I've mostly been ignoring it, but this is a pretty > straight-forward example, so I thought I'd write a note. > > The issue is that reftable_new_stack() might fail, leaving "st" as NULL. > And then we feed it to reftable_stack_new_addition(), which dereferences > it. > > In normal production code, we'd expect something like: > > if (err) > return -1; > > to avoid running the rest of the function after the first error. But the > test harness check() function doesn't return. It just complains to > stdout and keeps running! That is to allow the test to add more context with test_msg() or do things like check all the members of a struct before returning. It is a bug in the test if it does not return after finding a NULL pointer, the correct usage is if (!check(ptr)) return; As we're in the process of switching to using clar which does exit the text function if a check fails (that means there may be leaks on failure but if the test is failing then I don't think we should be worrying about leaks) I don't know if it is worth fixing these or not. I guess it depends if there are the list of targets for Seyi's Outreachy project. Best Wishes Phillip So you'll get something like[1]: > > $ t/unit-tests/bin/t-reftable-stack > ok 1 - empty addition to stack > ok 2 - read_lines works > ok 3 - expire reflog entries > # check "!err" failed at t/unit-tests/t-reftable-stack.c:1404 > Segmentation fault > > So...yes, we will probably notice that the test failed from the exit > code. But it's not great when the harness itself barfs so had. Plus a > compiler may be free to reorder things in a confusing way if it can see > that "st" must never be NULL. > > It feels like we probably ought to return as soon as a check() fails. > That does create other headaches, though. E.g., we'd potentially leak > from an early return (which our LSan builds will complain about), > meaning that test code needs to start doing the usual "goto out" type of > cleanup. > > So I dunno. Maybe we just live with it. But it feels pretty ugly. > > -Peff > > [1] This would happen in practice if malloc() failed, but you can > simulate it yourself like this, which is what I used to create the > output above: > > diff --git a/reftable/stack.c b/reftable/stack.c > index 026a9f9742..fe77132102 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -861,6 +861,11 @@ int reftable_stack_new_addition(struct reftable_addition **dest, > int err = 0; > struct reftable_addition empty = REFTABLE_ADDITION_INIT; > > + if (flags & (1 << 16)) { > + *dest = NULL; > + return REFTABLE_OUT_OF_MEMORY_ERROR; > + } > + > REFTABLE_CALLOC_ARRAY(*dest, 1); > if (!*dest) > return REFTABLE_OUT_OF_MEMORY_ERROR; > diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c > index c3f0059c34..73ed9792a5 100644 > --- a/t/unit-tests/t-reftable-stack.c > +++ b/t/unit-tests/t-reftable-stack.c > @@ -1400,7 +1400,7 @@ static void t_reftable_invalid_limit_updates(void) > > reftable_addition_destroy(add); > > - err = reftable_stack_new_addition(&add, st, 0); > + err = reftable_stack_new_addition(&add, st, (1 << 16)); > check(!err); > > /* >
On Fri, Jan 31, 2025 at 09:24:09PM -0500, Jeff King wrote: > On Wed, Jan 22, 2025 at 06:35:49AM +0100, Karthik Nayak wrote: > > > +static void t_reftable_invalid_limit_updates(void) > > +{ > > + struct reftable_ref_record ref = { > > + .refname = (char *) "HEAD", > > + .update_index = 1, > > + .value_type = REFTABLE_REF_SYMREF, > > + .value.symref = (char *) "master", > > + }; > > + struct reftable_write_options opts = { > > + .default_permissions = 0660, > > + }; > > + struct reftable_addition *add = NULL; > > + char *dir = get_tmp_dir(__LINE__); > > + struct reftable_stack *st = NULL; > > + int err; > > + > > + err = reftable_new_stack(&st, dir, &opts); > > + check(!err); > > + > > + reftable_addition_destroy(add); > > + > > + err = reftable_stack_new_addition(&add, st, 0); > > + check(!err); > > Coverity complains that this function may have undefined behavior. It's > an issue we have in a lot of other tests that have moved to the > unit-test framework. I've mostly been ignoring it, but this is a pretty > straight-forward example, so I thought I'd write a note. > > The issue is that reftable_new_stack() might fail, leaving "st" as NULL. > And then we feed it to reftable_stack_new_addition(), which dereferences > it. > > In normal production code, we'd expect something like: > > if (err) > return -1; > > to avoid running the rest of the function after the first error. But the > test harness check() function doesn't return. It just complains to > stdout and keeps running! So you'll get something like[1]: > > $ t/unit-tests/bin/t-reftable-stack > ok 1 - empty addition to stack > ok 2 - read_lines works > ok 3 - expire reflog entries > # check "!err" failed at t/unit-tests/t-reftable-stack.c:1404 > Segmentation fault > > So...yes, we will probably notice that the test failed from the exit > code. But it's not great when the harness itself barfs so had. Plus a > compiler may be free to reorder things in a confusing way if it can see > that "st" must never be NULL. > > It feels like we probably ought to return as soon as a check() fails. > That does create other headaches, though. E.g., we'd potentially leak > from an early return (which our LSan builds will complain about), > meaning that test code needs to start doing the usual "goto out" type of > cleanup. > > So I dunno. Maybe we just live with it. But it feels pretty ugly. It's one of the pitfalls of our own testing framework, from my point of view. It's somewhat unexpected that the test would just continue running as this is the wrong thing to do in almost all cases. When assumptions fail, nothing good will come of it. In any case, issues like this will be fixed once we migrate those tests to the clar unit test framework. The default there is to abort the tests in case an assertion fails, which is much saner from my point of view. So I'm not sure if it is worth fixing this now, or whether refactoring the tests to use clar is the more sensible fix. Patrick
On Sat, Feb 01, 2025 at 10:33:13AM +0000, Phillip Wood wrote: > Hi Peff > > On 01/02/2025 02:24, Jeff King wrote: > > On Wed, Jan 22, 2025 at 06:35:49AM +0100, Karthik Nayak wrote: > > > > Coverity complains that this function may have undefined behavior. It's > > an issue we have in a lot of other tests that have moved to the > > unit-test framework. I've mostly been ignoring it, but this is a pretty > > straight-forward example, so I thought I'd write a note. > > > > The issue is that reftable_new_stack() might fail, leaving "st" as NULL. > > And then we feed it to reftable_stack_new_addition(), which dereferences > > it. > > > > In normal production code, we'd expect something like: > > > > if (err) > > return -1; > > > > to avoid running the rest of the function after the first error. But the > > test harness check() function doesn't return. It just complains to > > stdout and keeps running! > > That is to allow the test to add more context with test_msg() or do things > like check all the members of a struct before returning. It is a bug in the > test if it does not return after finding a NULL pointer, the correct usage > is > > if (!check(ptr)) > return; > > As we're in the process of switching to using clar which does exit the text > function if a check fails (that means there may be leaks on failure but if > the test is failing then I don't think we should be worrying about leaks) I > don't know if it is worth fixing these or not. I guess it depends if there > are the list of targets for Seyi's Outreachy project. Ah, yes, should've read your mail first, as you're saying basically the same as I did :) Patrick
Patrick Steinhardt <ps@pks.im> writes: >> if (!check(ptr)) >> return; >> >> As we're in the process of switching to using clar which does exit the text >> function if a check fails (that means there may be leaks on failure but if >> the test is failing then I don't think we should be worrying about leaks) I >> don't know if it is worth fixing these or not. I guess it depends if there >> are the list of targets for Seyi's Outreachy project. > > Ah, yes, should've read your mail first, as you're saying basically the > same as I did :) Yup, I 100% agree with both of you. Thanks.
Jeff King <peff@peff.net> writes: > On Wed, Jan 22, 2025 at 06:35:49AM +0100, Karthik Nayak wrote: > >> +static void t_reftable_invalid_limit_updates(void) >> +{ >> + struct reftable_ref_record ref = { >> + .refname = (char *) "HEAD", >> + .update_index = 1, >> + .value_type = REFTABLE_REF_SYMREF, >> + .value.symref = (char *) "master", >> + }; >> + struct reftable_write_options opts = { >> + .default_permissions = 0660, >> + }; >> + struct reftable_addition *add = NULL; >> + char *dir = get_tmp_dir(__LINE__); >> + struct reftable_stack *st = NULL; >> + int err; >> + >> + err = reftable_new_stack(&st, dir, &opts); >> + check(!err); >> + >> + reftable_addition_destroy(add); >> + >> + err = reftable_stack_new_addition(&add, st, 0); >> + check(!err); > > Coverity complains that this function may have undefined behavior. It's > an issue we have in a lot of other tests that have moved to the > unit-test framework. I've mostly been ignoring it, but this is a pretty > straight-forward example, so I thought I'd write a note. > > The issue is that reftable_new_stack() might fail, leaving "st" as NULL. > And then we feed it to reftable_stack_new_addition(), which dereferences > it. > > In normal production code, we'd expect something like: > > if (err) > return -1; > > to avoid running the rest of the function after the first error. But the > test harness check() function doesn't return. It just complains to > stdout and keeps running! So you'll get something like[1]: > > $ t/unit-tests/bin/t-reftable-stack > ok 1 - empty addition to stack > ok 2 - read_lines works > ok 3 - expire reflog entries > # check "!err" failed at t/unit-tests/t-reftable-stack.c:1404 > Segmentation fault > > So...yes, we will probably notice that the test failed from the exit > code. But it's not great when the harness itself barfs so had. Plus a > compiler may be free to reorder things in a confusing way if it can see > that "st" must never be NULL. > > It feels like we probably ought to return as soon as a check() fails. > That does create other headaches, though. E.g., we'd potentially leak > from an early return (which our LSan builds will complain about), > meaning that test code needs to start doing the usual "goto out" type of > cleanup. > > So I dunno. Maybe we just live with it. But it feels pretty ugly. > Thanks for pointing it out, I didn't notice this, mostly as I was copying from existing test cases and it does seem like this (wrong) pattern exists in a lot of the tests. Like Phillip and Patrick mentioned, this should go away since we're moving to using the clar test framework. I think it makes sense to keep this as is to stay consistent with the rest of code in this file for now. It is ugly, but seems like that would be simpler while migrating. > -Peff > > [1] This would happen in practice if malloc() failed, but you can > simulate it yourself like this, which is what I used to create the > output above: > > diff --git a/reftable/stack.c b/reftable/stack.c > index 026a9f9742..fe77132102 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -861,6 +861,11 @@ int reftable_stack_new_addition(struct reftable_addition **dest, > int err = 0; > struct reftable_addition empty = REFTABLE_ADDITION_INIT; > > + if (flags & (1 << 16)) { > + *dest = NULL; > + return REFTABLE_OUT_OF_MEMORY_ERROR; > + } > + > REFTABLE_CALLOC_ARRAY(*dest, 1); > if (!*dest) > return REFTABLE_OUT_OF_MEMORY_ERROR; > diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c > index c3f0059c34..73ed9792a5 100644 > --- a/t/unit-tests/t-reftable-stack.c > +++ b/t/unit-tests/t-reftable-stack.c > @@ -1400,7 +1400,7 @@ static void t_reftable_invalid_limit_updates(void) > > reftable_addition_destroy(add); > > - err = reftable_stack_new_addition(&add, st, 0); > + err = reftable_stack_new_addition(&add, st, (1 << 16)); > check(!err); > > /*
On Sat, Feb 01, 2025 at 10:33:13AM +0000, Phillip Wood wrote: > > In normal production code, we'd expect something like: > > > > if (err) > > return -1; > > > > to avoid running the rest of the function after the first error. But the > > test harness check() function doesn't return. It just complains to > > stdout and keeps running! > > That is to allow the test to add more context with test_msg() or do things > like check all the members of a struct before returning. It is a bug in the > test if it does not return after finding a NULL pointer, the correct usage > is > > if (!check(ptr)) > return; > > As we're in the process of switching to using clar which does exit the text > function if a check fails (that means there may be leaks on failure but if > the test is failing then I don't think we should be worrying about leaks) I > don't know if it is worth fixing these or not. I guess it depends if there > are the list of targets for Seyi's Outreachy project. Ah, that's good to hear. I don't think there's any urgency here. These have been popping up since people started adding more unit-tests/ last summer. Waiting a few more months to switch to clar is probably not a big deal. I'm OK with ignoring a leak in a failing test. I do suspect that Coverity might still complain about the leaks, because it is doing static analysis to show that we _can_ leak (rather than the tests, which are seeing if we leaked at runtime). But I'm not sure how much effort we want to spend on making tests do cleanup on failure. Especially in a language like C. Anyway, I'll continue to ignore these Coverity results for now, then. ;) -Peff
On Mon, Feb 03, 2025 at 07:20:16AM -0800, Karthik Nayak wrote: > Like Phillip and Patrick mentioned, this should go away since we're > moving to using the clar test framework. I think it makes sense to keep > this as is to stay consistent with the rest of code in this file for > now. It is ugly, but seems like that would be simpler while migrating. Yeah, definitely not worth addressing this single case. I was more interested in the overall trend, but it sounds like there are plans there already. -Peff
diff --git a/reftable/stack.c b/reftable/stack.c index 026a9f9742..fe77132102 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -861,6 +861,11 @@ int reftable_stack_new_addition(struct reftable_addition **dest, int err = 0; struct reftable_addition empty = REFTABLE_ADDITION_INIT; + if (flags & (1 << 16)) { + *dest = NULL; + return REFTABLE_OUT_OF_MEMORY_ERROR; + } + REFTABLE_CALLOC_ARRAY(*dest, 1); if (!*dest) return REFTABLE_OUT_OF_MEMORY_ERROR; diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c index c3f0059c34..73ed9792a5 100644 --- a/t/unit-tests/t-reftable-stack.c +++ b/t/unit-tests/t-reftable-stack.c @@ -1400,7 +1400,7 @@ static void t_reftable_invalid_limit_updates(void) reftable_addition_destroy(add); - err = reftable_stack_new_addition(&add, st, 0); + err = reftable_stack_new_addition(&add, st, (1 << 16)); check(!err); /*