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