GH-91079: Decouple C stack overflow checks from Python recursion checks. #96510

markshannon · 2022-09-02T16:32:11Z

Issue: Implement stack overflow protection for supported platforms #91079

bedevere-bot · 2022-09-05T13:54:49Z

🤖 New build scheduled with the buildbot fleet by @markshannon for commit e8a1bed 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot · 2022-09-05T14:45:54Z

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 7e21a74 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot · 2022-09-06T10:57:14Z

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 65cca7d 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot · 2022-09-13T08:43:33Z

🤖 New build scheduled with the buildbot fleet by @markshannon for commit d75797c 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

gpshead · 2022-10-03T21:42:08Z

Lib/test/test_dynamic.py

-        # 1,000 on most systems
-        limit = sys.getrecursionlimit()
-        code = "lambda: " + "+".join(f"_number_{i}" for i in range(limit))
+        # Need more than 256 variables to use EXTENDED_ARGS


I assume EXTENDED_ARGS has stack implications? explaining the "why" of this here would be useful.

gpshead · 2022-10-03T21:43:21Z

Lib/test/test_exceptions.py

-                # and is equal to recursion_limit when _gen_throw() calls
-                # PyErr_NormalizeException().
-                recurse(setrecursionlimit(depth + 2) - depth)
+                recurse(5000)


There's a constant of 5000 used in all sorts of tests for the same purpose of being "too high" for recursion with this PR. I suggest making this a named test.support constant and referring to it instead of mystery constants spread throughout the test suite.

(and where other constants are derived from that to be higher or lower scale, use math from the main value?)

gpshead · 2022-10-04T00:04:44Z

Include/cpython/pystate.h

+/* WASI has limited call stack. Python's recursion limit depends on code
+   layout, optimization, and WASI runtime. Wasmtime can handle about 700
+   recursions, sometimes less. 500 is a more conservative limit. */
+#ifndef Py_DEFAULT_RECURSION_LIMIT


ifndef C_RECURSION_LIMIT

gpshead · 2022-10-04T00:06:16Z

Include/internal/pycore_ceval.h

-#    define Py_DEFAULT_RECURSION_LIMIT 1000
-#  endif
-#endif
+#define Py_DEFAULT_RECURSION_LIMIT 1000


keep the ifndef around this, those exist to allow someone setting their own via CFLAGS.

gpshead

this tied in nicely with the 3.11 talk :)

tacaswell · 2022-10-07T13:29:10Z

This broke compilation of https://github.com/python-greenlet/greenlet/

✔ 20:08:04 $ gcc -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/tcaswell/.virtualenvs/bleeding/include -I/home/tcaswell/.pybuild/bleeding/include/python3.12 -c src/greenlet/greenlet.cpp -o build/temp.linux-x86_64-3.12/src/greenlet/greenlet.o
In file included from src/greenlet/greenlet_internal.hpp:19,
                 from src/greenlet/greenlet.cpp:17:
src/greenlet/greenlet_greenlet.hpp: In member function ‘void greenlet::PythonState::operator<<(const PyThreadState*)’:
src/greenlet/greenlet_greenlet.hpp:826:37: error: ‘const PyThreadState’ {aka ‘const struct _ts’} has no member named ‘recursion_limit’; did you mean ‘py_recursion_limit’?
  826 |     this->recursion_depth = tstate->recursion_limit - tstate->recursion_remaining;
      |                                     ^~~~~~~~~~~~~~~
      |                                     py_recursion_limit
src/greenlet/greenlet_greenlet.hpp:826:63: error: ‘const PyThreadState’ {aka ‘const struct _ts’} has no member named ‘recursion_remaining’; did you mean ‘c_recursion_remaining’?
  826 |     this->recursion_depth = tstate->recursion_limit - tstate->recursion_remaining;
      |                                                               ^~~~~~~~~~~~~~~~~~~
      |                                                               c_recursion_remaining
src/greenlet/greenlet_greenlet.hpp: In member function ‘void greenlet::PythonState::operator>>(PyThreadState*)’:
src/greenlet/greenlet_greenlet.hpp:859:13: error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘recursion_remaining’; did you mean ‘c_recursion_remaining’?
  859 |     tstate->recursion_remaining = tstate->recursion_limit - this->recursion_depth;
      |             ^~~~~~~~~~~~~~~~~~~
      |             c_recursion_remaining
src/greenlet/greenlet_greenlet.hpp:859:43: error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘recursion_limit’; did you mean ‘py_recursion_limit’?
  859 |     tstate->recursion_remaining = tstate->recursion_limit - this->recursion_depth;
      |                                           ^~~~~~~~~~~~~~~
      |                                           py_recursion_limit
src/greenlet/greenlet_greenlet.hpp: In member function ‘void greenlet::PythonState::set_initial_state(const PyThreadState*)’:
src/greenlet/greenlet_greenlet.hpp:886:37: error: ‘const PyThreadState’ {aka ‘const struct _ts’} has no member named ‘recursion_limit’; did you mean ‘py_recursion_limit’?
  886 |     this->recursion_depth = tstate->recursion_limit - tstate->recursion_remaining;
      |                                     ^~~~~~~~~~~~~~~
      |                                     py_recursion_limit
src/greenlet/greenlet_greenlet.hpp:886:63: error: ‘const PyThreadState’ {aka ‘const struct _ts’} has no member named ‘recursion_remaining’; did you mean ‘c_recursion_remaining’?
  886 |     this->recursion_depth = tstate->recursion_limit - tstate->recursion_remaining;
      |                                                               ^~~~~~~~~~~~~~~~~~~
      |                                                               c_recursion_remaining

Doing the renames as suggested by the compiler fixes it (although from reading the PR here, maybe py_recursion_remaining should be used instead of c_recursion_remaining).

Is this an unintended side effect of this change and or does greenlets need to adapt?

gpshead · 2022-10-07T17:22:37Z

Not being familiar with greenlet much I suspect it wants py_recursion_remaining. This is a rather internal structure that changes between releases. https://docs.python.org/3.11/whatsnew/3.11.html has a change around these that you've adapted to in https://github.com/python-greenlet/greenlet/blob/master/src/greenlet/greenlet_greenlet.hpp#L825 so this is probably just another one that we need to document in 3.12's "What's New".

gpshead · 2022-10-07T17:23:17Z

BTW @tacaswell major kudos to you for testing against CPython main =)

tacaswell · 2022-10-07T18:45:20Z

Thanks, I'll get a PR to greenlet done in the next few days (I am also barely familiar with greenlet but it is a dependency of a dependency of something I care about).

I have built a whole rube-goldberg machine to test CPython main (https://github.com/tacaswell/build_the_world).

…n checks. (pythonGH-96510)

dimpase · 2023-11-17T23:35:21Z

This, more precisely 7644935, has broken deeply recursive code which uses @cache decorator, see #112215

In such a setting, sys.setrecursionlimit() stops working (sic!)

dimpase · 2023-11-18T08:56:07Z

E.g. the very basic, really CS101, recursion speedup with cache

#  fib.py 
import sys
sys.setrecursionlimit(2000)

from functools import cache

@cache
def fib(n):
    if n<1: return 0
    if n==1: return 1
    return fib(n-1) + fib(n-2)

print(fib(500))

got broken by this PR.

Perhaps needless to say, on the latest main branch,

--- a/Include/cpython/pystate.h
+++ b/Include/cpython/pystate.h
@@ -225,7 +225,7 @@ struct _ts {
 #  define Py_C_RECURSION_LIMIT 500
 #else
    // This value is duplicated in Lib/test/support/__init__.py
-#  define Py_C_RECURSION_LIMIT 1500
+#  define Py_C_RECURSION_LIMIT 3000
 #endif

makes it work (I didn't try to find the minimal needed increase for Py_C_RECURSION_LIMIT)

$ ./python <fib.py
139423224561697880139724382870407283950070256587697307264108962948325571622863290691557658876222521294125

@PeterLuschny

gpshead · 2023-11-18T23:12:42Z

Please stop using this long merged and closed PR as a forum, nobody liatens here. This is not an issue tracker.

If you believe there is a bug, file a new issue.

dimpase · 2023-11-20T12:04:36Z

Please stop using this long merged and closed PR as a forum, nobody liatens here. This is not an issue tracker.

If you believe there is a bug, file a new issue.

These are filed.
This was an attempt at heads-up here, after I learned of and worked on #112215, which explains why this PR introduced a bug (and a big one). I've also opened #112282 to point out at the undocumented behaviour this PR introduced.

I also don't understand why this was merged, while not conforming to python/steering-council#102 (they asked for MemoryError thrown on C stack limit violation, rather than RecursionError)

On Python 3.12, this provokes a stack overflow in the scheduler. It is not quite clear why that's the case; pure-Python recursion even with generators seems to respond well to setrecursionlimit(): ```py def f(n): if n: yield from f(n-1) else: yield 5 import sys sys.setrecursionlimit(3500) print(list(f(3400))) ``` That said, there have been [behavior](python/cpython#96510) [changes](python/cpython#112215) in Py3.12 in this regard, but it is not clear what exactly about Loopy's behavior makes it fall into the 'bad' case.

markshannon added 2 commits September 2, 2022 17:21

Separate Python recursion tracking from C stack tracking.

de4085f

Add tests for super deep recursion.

6b380eb

bedevere-bot added the awaiting core review label Sep 2, 2022

markshannon added 4 commits September 2, 2022 17:44

Fixup Windows code.

b64c83b

Fixup asdl_c.py

e29a4cb

Use C recursion limit to determine AST recursion limits.

4897476

Add news.

e8a1bed

markshannon marked this pull request as ready for review September 5, 2022 09:56

markshannon requested review from isidentical and iritkatriel as code owners September 5, 2022 09:56

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 5, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 5, 2022

Reduce C recursion depth on WASI.

7e21a74

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 5, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 5, 2022

Reduce AST depth to get WASI test to pass.

65cca7d

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 6, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 6, 2022

Merge branch 'main' into stack-overflow-3

d75797c

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 13, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 13, 2022

markshannon requested a review from gpshead September 15, 2022 10:31

gpshead added the sprint label Oct 3, 2022

gpshead self-assigned this Oct 4, 2022

gpshead reviewed Oct 4, 2022

View reviewed changes

markshannon requested a review from rhettinger as a code owner October 4, 2022 21:02

markshannon requested a review from tiran as a code owner October 4, 2022 21:02

Address gps's review comments.

d1abed5

markshannon force-pushed the stack-overflow-3 branch from 63f55aa to d1abed5 Compare October 4, 2022 21:15

gpshead approved these changes Oct 4, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 4, 2022

markshannon merged commit 7644935 into python:main Oct 5, 2022

bedevere-bot removed the awaiting merge label Oct 5, 2022

mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022

pythonGH-91079: Decouple C stack overflow checks from Python recursio…

0ba4aad

…n checks. (pythonGH-96510)

tacaswell mentioned this pull request Oct 29, 2022

support for python3.12 python-greenlet/greenlet#323

Closed

hauntsaninja mentioned this pull request Jul 25, 2023

Regression in 3.12 beta in json.dump deeply nested dict #107263

Closed

markshannon deleted the stack-overflow-3 branch September 26, 2023 12:50

rafsaf mentioned this pull request Oct 5, 2023

add support for python 3.12 unbit/uwsgi#2569

Closed

dimpase mentioned this pull request Nov 20, 2023

Is it OK to implement stack protection? python/steering-council#102

Closed

inducer mentioned this pull request Feb 16, 2024

Add test_long_kernel inducer/loopy#826

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-91079: Decouple C stack overflow checks from Python recursion checks. #96510

GH-91079: Decouple C stack overflow checks from Python recursion checks. #96510

markshannon commented Sep 2, 2022 •

edited by bedevere-bot

Loading

bedevere-bot commented Sep 5, 2022

bedevere-bot commented Sep 5, 2022

bedevere-bot commented Sep 6, 2022

bedevere-bot commented Sep 13, 2022

gpshead Oct 3, 2022

gpshead Oct 3, 2022

gpshead Oct 4, 2022

gpshead Oct 4, 2022

gpshead Oct 4, 2022

gpshead left a comment

tacaswell commented Oct 7, 2022

gpshead commented Oct 7, 2022

gpshead commented Oct 7, 2022

tacaswell commented Oct 7, 2022 •

edited

Loading

dimpase commented Nov 17, 2023

dimpase commented Nov 18, 2023 •

edited

Loading

gpshead commented Nov 18, 2023

dimpase commented Nov 20, 2023

GH-91079: Decouple C stack overflow checks from Python recursion checks. #96510

GH-91079: Decouple C stack overflow checks from Python recursion checks. #96510

Conversation

markshannon commented Sep 2, 2022 • edited by bedevere-bot Loading

bedevere-bot commented Sep 5, 2022

bedevere-bot commented Sep 5, 2022

bedevere-bot commented Sep 6, 2022

bedevere-bot commented Sep 13, 2022

gpshead Oct 3, 2022

Choose a reason for hiding this comment

gpshead Oct 3, 2022

Choose a reason for hiding this comment

gpshead Oct 4, 2022

Choose a reason for hiding this comment

gpshead Oct 4, 2022

Choose a reason for hiding this comment

gpshead Oct 4, 2022

Choose a reason for hiding this comment

gpshead left a comment

Choose a reason for hiding this comment

tacaswell commented Oct 7, 2022

gpshead commented Oct 7, 2022

gpshead commented Oct 7, 2022

tacaswell commented Oct 7, 2022 • edited Loading

dimpase commented Nov 17, 2023

dimpase commented Nov 18, 2023 • edited Loading

gpshead commented Nov 18, 2023

dimpase commented Nov 20, 2023

markshannon commented Sep 2, 2022 •

edited by bedevere-bot

Loading

tacaswell commented Oct 7, 2022 •

edited

Loading

dimpase commented Nov 18, 2023 •

edited

Loading