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

[Phillip Wood <phillip.wood123@gmail.com>]

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);
>   
>   	/*
>