GH-139653: Only raise an exception (or fatal error) when the stack pointer is about to overflow the stack. #141711
Conversation
…out to overflow the stack. Only raises if the stack pointer is both below the limit *and* above the stack base. This prevents false positives for user-space threads, as the stack pointer will be outside those bounds if the stack has been swapped.
markshannon
added
3.14
| # endif | ||
| uintptr_t here_addr = _Py_get_machine_stack_pointer(); | ||
| uintptr_t top_addr = _Py_SIZE_ROUND_UP(here_addr, 4096); | ||
| // Add some space for caller function then round to minimum page size |
There was a problem hiding this comment.
Thanks for the comment.
Both “space for caller function” and “minimum page size” are guesses; what happens if they're wrong?
There was a problem hiding this comment.
Nothing bad. We only use the upper limit (for stack growing down) for reporting stack use in case of an overflow.
If we are wrong the report value will be off by a few kb, but it is already an estimate. We are assuming that there isn't much stack above the call to create the thread state.
There was a problem hiding this comment.
Would it make sense to avoid calculation, and use here_addr as is? With the Py_C_STACK_SIZE guesses, 4k indeed doesn't matter.
There was a problem hiding this comment.
It's not a perfect guess, but I don't see any reason to make it worse.
There was a problem hiding this comment.
What I see is unnecessary complexity: adding 60 bytes of faux precision to a guess that's in the order of megabytes is rather confusing. These look like more numbers to tweak if you get spurious overflow check failures
If this needs to go in, could you add a comment that this guess can be wildly wrong?
encukou
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @encukou for commit 41e9404 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F141711%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
Python/ceval.c
Outdated
|
|
||
| /* The function _Py_EnterRecursiveCallTstate() only calls _Py_CheckRecursiveCall() | ||
| if the recursion_depth reaches recursion_limit. */ | ||
| if the stack poineter is between the stack base and c_stack_hard_limit. */ |
There was a problem hiding this comment.
| if the stack poineter is between the stack base and c_stack_hard_limit. */ | |
| if the stack pointer is between the stack base and c_stack_hard_limit. */ |
| # endif | ||
| uintptr_t here_addr = _Py_get_machine_stack_pointer(); | ||
| uintptr_t top_addr = _Py_SIZE_ROUND_UP(here_addr, 4096); | ||
| // Add some space for caller function then round to minimum page size |
There was a problem hiding this comment.
What I see is unnecessary complexity: adding 60 bytes of faux precision to a guess that's in the order of megabytes is rather confusing. These look like more numbers to tweak if you get spurious overflow check failures
If this needs to go in, could you add a comment that this guess can be wildly wrong?
|
Thanks @markshannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14. |
|
Sorry, @markshannon, I could not cleanly backport this to |
…ack pointer is about to overflow the stack. (pythonGH-141711) Only raises if the stack pointer is both below the limit *and* above the stack base. This prevents false positives for user-space threads, as the stack pointer will be outside those bounds if the stack has been swapped. (cherry picked from commit c25a070)
|
GH-141892 is a backport of this pull request to the 3.14 branch. |
… the stack pointer is about to overflow the stack. (pythonGH-141711) Only raises if the stack pointer is both below the limit *and* above the stack base. This prevents false positives for user-space threads, as the stack pointer will be outside those bounds if the stack has been swapped. Cherry-picked from commit c25a070
… the stack pointer is about to overflow the stack. (pythonGH-141711) Only raises if the stack pointer is both below the limit *and* above the stack base. This prevents false positives for user-space threads, as the stack pointer will be outside those bounds if the stack has been swapped. Cherry-picked from commit c25a070
|
GH-141944 is a backport of this pull request to the 3.14 branch. |
…tack pointer is about to overflow the stack. (GH-141711) (GH-141944) Only raises if the stack pointer is both below the limit *and* above the stack base. This prevents false positives for user-space threads, as the stack pointer will be outside those bounds if the stack has been swapped. Cherry-picked from commit c25a070 Co-authored-by: Mark Shannon <[email protected]>
…ack pointer is about to overflow the stack. (pythonGH-141711) Only raises if the stack pointer is both below the limit *and* above the stack base. This prevents false positives for user-space threads, as the stack pointer will be outside those bounds if the stack has been swapped.
…ack pointer is about to overflow the stack. (pythonGH-141711) Only raises if the stack pointer is both below the limit *and* above the stack base. This prevents false positives for user-space threads, as the stack pointer will be outside those bounds if the stack has been swapped.
5 participants
Only raises if the stack pointer is both below the limit and above the stack base. This prevents false positives for user-space threads, as the stack pointer will be outside those bounds if the stack has been swapped.