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

> 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.