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
