Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 35.2k
GH-139653: Only raise an exception (or fatal error) when the stack pointer is about to overflow the stack.#141711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 4 additions & 0 deletions
4 Misc/NEWS.d/next/Core_and_Builtins/2025-11-17-14-40-45.gh-issue-139653.LzOy1M.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Only raise a ``RecursionError`` or trigger a fatal error if the stack | ||
| pointer is both below the limit pointer *and* above the stack base. If | ||
| outside of these bounds assume that it is OK. This prevents false positives | ||
| when user-space threads swap stacks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -362,9 +362,11 @@ _Py_ReachedRecursionLimitWithMargin(PyThreadState *tstate, int margin_count) | ||
| _Py_InitializeRecursionLimits(tstate); | ||
| } | ||
| #if _Py_STACK_GROWS_DOWN | ||
| return here_addr <= _tstate->c_stack_soft_limit + margin_count * _PyOS_STACK_MARGIN_BYTES; | ||
| return here_addr <= _tstate->c_stack_soft_limit + margin_count * _PyOS_STACK_MARGIN_BYTES && | ||
| here_addr >= _tstate->c_stack_soft_limit - 2 * _PyOS_STACK_MARGIN_BYTES; | ||
| #else | ||
| return here_addr > _tstate->c_stack_soft_limit - margin_count * _PyOS_STACK_MARGIN_BYTES; | ||
| return here_addr > _tstate->c_stack_soft_limit - margin_count * _PyOS_STACK_MARGIN_BYTES && | ||
| here_addr <= _tstate->c_stack_soft_limit + 2 * _PyOS_STACK_MARGIN_BYTES; | ||
| #endif | ||
| } | ||
| @@ -455,7 +457,7 @@ int pthread_attr_destroy(pthread_attr_t *a) | ||
| #endif | ||
| static void | ||
| hardware_stack_limits(uintptr_t *base, uintptr_t *top) | ||
| hardware_stack_limits(uintptr_t *base, uintptr_t *top, uintptr_t sp) | ||
| { | ||
| #ifdef WIN32 | ||
| ULONG_PTR low, high; | ||
| @@ -491,10 +493,19 @@ hardware_stack_limits(uintptr_t *base, uintptr_t *top) | ||
| return; | ||
| } | ||
| # 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 | ||
| // This is a guess at the top of the stack, but should be a reasonably | ||
| // good guess if called from _PyThreadState_Attach when creating a thread. | ||
| // If the thread is attached deep in a call stack, then the guess will be poor. | ||
| #if _Py_STACK_GROWS_DOWN | ||
| uintptr_t top_addr = _Py_SIZE_ROUND_UP(sp + 8*sizeof(void*), SYSTEM_PAGE_SIZE); | ||
| *top = top_addr; | ||
| *base = top_addr - Py_C_STACK_SIZE; | ||
| # else | ||
| uintptr_t base_addr = _Py_SIZE_ROUND_DOWN(sp - 8*sizeof(void*), SYSTEM_PAGE_SIZE); | ||
| *base = base_addr; | ||
| *top = base_addr + Py_C_STACK_SIZE; | ||
| #endif | ||
| #endif | ||
| } | ||
| @@ -543,7 +554,8 @@ void | ||
| _Py_InitializeRecursionLimits(PyThreadState *tstate) | ||
| { | ||
| uintptr_t base, top; | ||
| hardware_stack_limits(&base, &top); | ||
| uintptr_t here_addr = _Py_get_machine_stack_pointer(); | ||
| hardware_stack_limits(&base, &top, here_addr); | ||
| assert(top != 0); | ||
| tstate_set_stack(tstate, base, top); | ||
| @@ -587,7 +599,7 @@ PyUnstable_ThreadState_ResetStackProtection(PyThreadState *tstate) | ||
| /* The function _Py_EnterRecursiveCallTstate() only calls _Py_CheckRecursiveCall() | ||
| if the recursion_depth reaches recursion_limit. */ | ||
| if the stack pointer is between the stack base and c_stack_hard_limit. */ | ||
| int | ||
| _Py_CheckRecursiveCall(PyThreadState *tstate, const char *where) | ||
| { | ||
| @@ -596,10 +608,12 @@ _Py_CheckRecursiveCall(PyThreadState *tstate, const char *where) | ||
| assert(_tstate->c_stack_soft_limit != 0); | ||
encukou marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| assert(_tstate->c_stack_hard_limit != 0); | ||
| #if _Py_STACK_GROWS_DOWN | ||
| assert(here_addr >= _tstate->c_stack_hard_limit - _PyOS_STACK_MARGIN_BYTES); | ||
| if (here_addr < _tstate->c_stack_hard_limit) { | ||
| /* Overflowing while handling an overflow. Give up. */ | ||
| int kbytes_used = (int)(_tstate->c_stack_top - here_addr)/1024; | ||
| #else | ||
| assert(here_addr <= _tstate->c_stack_hard_limit + _PyOS_STACK_MARGIN_BYTES); | ||
| if (here_addr > _tstate->c_stack_hard_limit) { | ||
| /* Overflowing while handling an overflow. Give up. */ | ||
| int kbytes_used = (int)(here_addr - _tstate->c_stack_top)/1024; | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to avoid calculation, and use
here_addras is? With thePy_C_STACK_SIZEguesses, 4k indeed doesn't matter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a perfect guess, but I don't see any reason to make it worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.