Re: undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records

[Patrick Steinhardt <ps@pks.im>]

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