GH-96569: Add two NULL checks to avoid undefined behavior.#96585
Conversation
| @@ -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.
| { | ||
| 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. |
matthiasgoergens
left a comment
There was a problem hiding this comment.
Not that my approval means anything. :)
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 |
Uh oh!
There was an error while loading. Please reload this page.