null array indexing in coverage/ctracer/tracer.c

[Yhg1s]

This issue was found by running UndefinedBehaviourSanitizer in our internal builds at Google: coverage/ctracer/tracer.c's CTracer_handle_return() is sometimes called in situations where self->pdata_stack->stack is NULL after the call to CTracer_set_pdata_stack(), but CTracer_handle_return() still tries to index the stack to set self->pcur_entry. As far as I can tell self->pdata_stack->depth is always -1 in that case, meaning the NULL array gets indexed with a non-0 index (that would also be out of bounds). I can't tell if this is intentional or not (i.e. whether the resulting value of self->pcur_entry actually matters.)

Here's a diff to reproduce the error without ubsan:

--- a/coverage/ctracer/tracer.c
+++ b/coverage/ctracer/tracer.c
@@ -722,6 +722,11 @@ CTracer_handle_return(CTracer *self, PyFrameObject *frame)
     if (CTracer_set_pdata_stack(self) < 0) {
         goto error;
     }
+    if (self->pdata_stack->stack == NULL) {
+        fprintf(stderr, "stack = NULL, depth = %d\n",
+                self->pdata_stack->depth);
+        abort();
+    }
     self->pcur_entry = &self->pdata_stack->stack[self->pdata_stack->depth];

     if (self->pdata_stack->depth >= 0) {

[frigus02]

I reproduced the issue by applying @Yhg1s' patch top of the current head (28d22a3), except for the abort() call. Running tests results in:

$ python3 -m tox -e py311

[...]
================================================================== short test summary info ===================================================================
FAILED tests/test_concurrency.py::ConcurrencyTest::test_greenlet_simple_code - AssertionError: assert 'stack = NULL, depth = -1\nstack = NULL, depth = -1\nstack = NULL, depth = -1\n499500\n' == '499500\n'
FAILED tests/test_concurrency.py::ConcurrencyTest::test_threads_simple_code - AssertionError: assert 'stack = NULL, depth = -1\n499500\n' == '499500\n'
FAILED tests/test_concurrency.py::ConcurrencyTest::test_threads - AssertionError: assert 'stack = NULL, depth = -1\n499500\n' == '499500\n'
FAILED tests/test_concurrency.py::ConcurrencyTest::test_gevent_simple_code - AssertionError: assert 'stack = NULL, depth = -1\nstack = NULL, depth = -1\nstack = NULL, depth = -1\n499500\n' == '499500\n'
FAILED tests/test_concurrency.py::ConcurrencyTest::test_greenlet - AssertionError: assert 'stack = NULL, depth = -1\nstack = NULL, depth = -1\nstack = NULL, depth = -1\nhello world\n42\n' == 'hello world\n42\n'
FAILED tests/test_concurrency.py::ConcurrencyTest::test_gevent - AssertionError: assert 'stack = NULL, depth = -1\nstack = NULL, depth = -1\nstack = NULL, depth = -1\n499500\n' == '499500\n'
FAILED tests/test_concurrency.py::ConcurrencyTest::test_eventlet_simple_code - AssertionError: assert 'stack = NULL, depth = -1\nstack = NULL, depth = -1\nstack = NULL, depth = -1\n499500\n' == '499500\n'
FAILED tests/test_concurrency.py::ConcurrencyTest::test_eventlet - AssertionError: assert 'stack = NULL, depth = -1\nstack = NULL, depth = -1\nstack = NULL, depth = -1\n499500\n' == '499500\n'
[...]
72 failed, 1326 passed, 13 skipped in 45.86s

[nedbat]

This is now released as part of coverage 7.6.2.