Skip to content

Conversation

@colesbury
Copy link
Contributor

@colesburycolesbury commented Jun 16, 2025

Returns 1 a little less than 1% of the time in
_Py_RecursionLimit_GetMargin() to force the use of the trashcan mechanism.

This isn't intended to be merged. It's just to:

  1. Demonstrate the crash
  2. Test on a variety of platforms and configs from the CI

Returns `1` a little less than 1% of the time in _Py_RecursionLimit_GetMargin() to force the use of the trashcan mechanism.
@colesburycolesbury changed the title gh-135106: Randomly return 1 in _Py_RecursionLimit_GetMargingh-135106: [crash repro] Randomly return 1 in _Py_RecursionLimit_GetMarginJun 16, 2025
@markshannon
Copy link
Member

I don't think this shows anything more than that randomly changing the stack limit causes crashes.

The trashcan mechanism relies on the margin being larger for frames higher up the call stack. This breaks that invariant.

You could try changing margin > 2 and margin >= 4 to much larger numbers to see if that provokes a failure.

@hugovkhugovkforce-pushed the gh-135106-crash-demo branch from f0ef510 to 92e82acCompareJune 17, 2025 07:32
@hugovk

This comment was marked as resolved.

@colesburycolesbury changed the title gh-135106: [crash repro] Randomly return 1 in _Py_RecursionLimit_GetMargingh-135106: [crash repro] Randomly deposit objects in _Py_DeallocJun 17, 2025
@colesburycolesburyforce-pushed the gh-135106-crash-demo branch from da82cbb to 7c86a25CompareJune 17, 2025 17:08
@colesbury
Copy link
ContributorAuthor

I don't think this shows anything more than that randomly changing the stack limit causes crashes.

The only thing that matters here is the condition to deposit objects -- there's no invariant to be broken. I've updated the test to make that more clear. It should always be safe to deposit an object conservatively (i.e., to treat the available stack as smaller).

Here's the equivalent test with the old trashcan behavior, which works without crashing:
https://github.com/colesbury/cpython/tree/gh-135106-test-old-behavior

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@colesbury@markshannon@hugovk@AA-Turner