GH-96569: Add two NULL checks to avoid undefined behavior. #96585
Conversation
markshannon
commented
Sep 5, 2022
•
markshannon
commented
- Issue: undefined behavior: tstate->datastack_top == NULL #96569
Python/pystate.c
Outdated
| @@ -2197,7 +2197,7 @@ _PyThreadState_PushFrame(PyThreadState *tstate, size_t size) | |||
| assert(size < INT_MAX/sizeof(PyObject *)); | |||
| PyObject **base = tstate->datastack_top; | |||
| PyObject **top = base + size; | |||
There was a problem hiding this comment.
Adding size to NULL base is undefined behavior. top should be calculated after NULL check.
There was a problem hiding this comment.
And similar to the other check, the addition itself might already be undefined behaviour, if it's more than one past the array length.
Include/internal/pycore_frame.h
Outdated
| @@ -194,7 +194,8 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear); | |||
| static inline bool | |||
| _PyThreadState_HasStackSpace(PyThreadState *tstate, int size) | |||
| { | |||
| return tstate->datastack_top + size < tstate->datastack_limit; | |||
| return tstate->datastack_top != NULL && | |||
| tstate->datastack_top + size < tstate->datastack_limit; | |||
There was a problem hiding this comment.
tstate->datastack_top + size may be an undefined behavior if it is past the array end.
size < tstate->datastack_limit - tstate->datastack_top does not have an UB in that case.
I do not know whether this actually happens.
There was a problem hiding this comment.
To be precise, tstate->datastack_limit - tstate->datastack_top needs both pointers to point to entries in the same array to be defined (I think). And from looking at the source, that seems to be the case.
|
Btw, you might want to fix (It's The fix could be to use the difference approach that serhiy-storchaka suggested. And perhaps add a comment to that function with an explanation? |
|
Thanks @matthiasgoergens and @serhiy-storchaka for your suggestions. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Thanks @markshannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
|
Sorry, @markshannon, I could not cleanly backport this to |
