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

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