GH-91079: Implement C stack limits using addresses, not counters. #130007

markshannon · 2025-02-11T15:59:12Z

Hopefully, this will also fix #113655.

Issue: Implement stack overflow protection for supported platforms #91079

… is an imbalance in the AST somewhere.

…m independent constants

encukou

This adds new public API, which needs documentation.
It also removes public API; that needs deprecation or SC exception.

bedevere-app · 2025-02-17T14:23:38Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

zooba · 2025-02-17T14:34:55Z

Love seeing this change! Thanks, Mark.

markshannon · 2025-02-17T15:53:34Z

This adds new public API, which needs documentation.

The public API is Py_TRASHCAN_BEGIN. Py_ReachedRecursionLimit is the necessary ABI addition to support it.
I could add it to the unstable API, if I can't make it private.

It also removes public API; that needs deprecation or SC exception.

What API?

markshannon · 2025-02-17T16:18:32Z

Benchmarking shows a 0.0%-0.4% speedup on 64 bit platforms, and a 2% speedup on 32 bit windows, so it isn't slower.

markshannon · 2025-02-17T16:20:46Z

@encukou I've hidden the new function. I don't see any removals though, which API do you mean?

encukou · 2025-02-17T16:49:17Z

Thanks for hiding it!

There's also the newly added PyThreadState.c_stack_* members; are those meant to be private?

I don't see any removals though

I see these in Include/cpython/pystate.h:

PyThreadState.c_recursion_remaining
Py_C_RECURSION_LIMIT

I don't think getting a SC exception should be a problem.

markshannon · 2025-02-17T17:05:10Z

We can add back c_recursion_remaining and Py_C_RECURSION_LIMIT, if you want.
The semantics have never been defined, so their values are meaningless to third party code and always have been.

markshannon · 2025-02-17T17:08:11Z

There's also the newly added PyThreadState.c_stack_* members; are those meant to be private?

It's C code, there is no such thing as private.

encukou · 2025-02-17T17:08:13Z

The semantics have never been defined

Well, not documented, which just means people read the code to figure out what they mean.
It's probably best to remove them; should I ask the SC for you?

markshannon · 2025-02-17T17:13:29Z

Well, not documented, which just means people read the code to figure out what they mean.

I really don't think we should be wasting our time on supporting incompetent C/C++ programmers who can't use APIs.
Do we really want to start prefixing every field with _DONT_USE_THIS_ just in case?

markshannon · 2025-02-17T17:24:49Z

I've just discovered _PyThreadStateImpl which is a hack to avoid this problem. I'll add stuff to that.

…kage

markshannon · 2025-02-18T09:46:11Z

@encukou struct _ts (the thread state) is unchanged and Py_C_RECURSION_LIMIT restored.

encukou

Thank you!

Do we really want to start prefixing every field with _DONT_USE_THIS_ just in case?

No, an underscore is fine.
It's C, nothing is private at the language level, so we have a convention to mark the private/unstable stuff.

encukou · 2025-02-18T10:20:29Z

Python/ceval.c

-#ifdef USE_STACKCHECK
-    if (PyOS_CheckStack()) {


Could you adjust the Py_EnterRecursiveCall docs?

encukou · 2025-02-18T10:31:24Z

Python/ceval.c

+   // test_descr crashed on sparc64 with >7000 but let's keep a margin of error.
+#  define Py_C_STACK_SIZE 1600000


The 7000 (and 8000 below) doesn't make much sense to me here.

encukou · 2025-02-18T10:36:03Z

Include/pythonrun.h


-#if defined(WIN32) && !defined(MS_WIN64) && !defined(_M_ARM) && defined(_MSC_VER) && _MSC_VER >= 1300
+#if defined(WIN32)


The definition of PyOS_CheckStack still uses defined(_MSC_VER).
Looks like the #if there (and the MSVC-specific includes) can be removed.

Could you remove the MSVC reference from the PyOS_CheckStack docs?

encukou · 2025-02-18T10:51:11Z

Lib/test/list_tests.py

    @skip_emscripten_stack_overflow()
    def test_repr_deep(self):
        a = self.type2test([])
-        for i in range(get_c_recursion_limit() + 1):
+        for i in range(100_000):


Could you comment on where the 100_000, and similar magic constants below, came from?

Perhaps it would be best to reuse the name exceeds_recursion_limit, and set that to 100_000?

encukou · 2025-02-18T11:30:40Z

Lib/test/test_fstring.py

@@ -628,23 +628,34 @@ def test_mismatched_parens(self):
                            r"does not match opening parenthesis '\('",
                            ["f'{a(4}'",
                            ])
-        self.assertRaises(SyntaxError, eval, "f'{" + "("*500 + "}'")
+        self.assertRaises(SyntaxError, eval, "f'{" + "("*20 + "}'")


This looks worrying. Did the limit for nested parentheses go down over an order of magnitude?

No. Previously, there was a counter and if it was exceeded a SyntaxError was raised, but it could crash if the stack overflowed before the limit was reached.
Now we raise a SyntaxError for unbalanced parens and a RecursionError for a (potential) stack overflow.
See the new tests added below (line 656)

encukou · 2025-02-18T11:33:48Z

Lib/test/test_fstring.py

+            try:
+                eval(txt)
+            except SyntaxError:
+                pass
+            except MemoryError:
+                pass
+            except Exception as ex:
+                self.fail(f"Should raise SyntaxError or MemoryError, not {type(ex)}")
+            else:
+                self.fail("No exception raised")


FWIW, you could use with self.assertRaises((SyntaxError, MemoryError)):

Oh, I don't know that was a thing. Thanks.

encukou · 2025-02-18T11:45:37Z

Python/pythonrun.c

@@ -1538,24 +1538,21 @@ _Py_SourceAsString(PyObject *cmd, const char *funcname, const char *what, PyComp
 /*
 * Return non-zero when we run out of memory on the stack; zero otherwise.
 */
-int
-PyOS_CheckStack(void)
+int PyOS_CheckStack(void)


Nitpick: please revert the style change

Suggested change

int PyOS_CheckStack(void)

int

PyOS_CheckStack(void)

encukou · 2025-02-18T11:49:33Z

Python/pythonrun.c

    }
-    return 1;
+    else {
+        return -1;


Could this be 1 as before? Nowadays -1 is usually used when an exception is set.

markshannon · 2025-02-18T16:02:23Z

I have made the requested changes; please review again

bedevere-app · 2025-02-18T16:02:29Z

Thanks for making the requested changes!

@iritkatriel, @encukou: please review the changes made to this pull request.

markshannon added 4 commits February 11, 2025 11:09

Hide that C recursion protection is implemented with a counter. There…

fc910a3

… is an imbalance in the AST somewhere.

Implement C recursion protection with limit pointers

afeb866

Use uintptr_t instead of char * to avoid warnings and UB

22ca169

Merge branch 'main' into c-recursion-limit

e8d8c4b

bedevere-app bot mentioned this pull request Feb 11, 2025

Implement stack overflow protection for supported platforms #91079

Open

markshannon closed this Feb 11, 2025

markshannon reopened this Feb 11, 2025

markshannon added 23 commits February 11, 2025 16:18

Fix typo and update stable ABI

b3638a5

Tweak AST test numbers

774efb5

Improve logic handling trial C stack overflow

151c88f

Remove calls to PyOS_CheckStack

350f8ec

Up the limits for recursion tests

428c46a

Use deeper stack for test

a9be141

Remove exceeds_recursion_limit and get_c_recursion_limit. Use platfor…

03fc52e

…m independent constants

Do fewer probes when growing stack limit

afac1e6

Tweak depths

9da904d

Merge branch 'main' into c-recursion-limit

a802ff6

Perform lazy initialization of c recursion check

dbcf6f0

Post merge fixup

2cc3287

Up depth again

e697926

Drop 'failing' depth

f8a9143

Add news

7d6d77f

Increase headroom

31a83dc

Update test

47c50aa

Tweak some more thresholds and tests

495c4ea

Add stack protection to parser

9e0cc67

Make tests more robust to low stacks

857a7bb

Improve error messages for stack overflow

9c9326a

Merge branch 'main' into c-recursion-limit

75d3219

Fix formatting

3e41b46

bedevere-app bot added the awaiting merge label Feb 17, 2025

encukou requested changes Feb 17, 2025

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting merge labels Feb 17, 2025

Make ABI function needed for Py_TRASHCAN_BEGIN private

7761d31

markshannon added 3 commits February 17, 2025 17:38

Move new fields to _PyThreadStateImpl to avoid any potential API brea…

b067e3e

…kage

Fix missing cast

b0e695f

yet another missing _

c4cd68f

encukou reviewed Feb 18, 2025

View reviewed changes

markshannon added 3 commits February 18, 2025 14:38

Tidy up a bit

9654790

Restore use of exceeds_recursion_limit

2cef96f

Address remaining review comments

c5d8a40

bedevere-app bot added awaiting change review and removed awaiting changes labels Feb 18, 2025

bedevere-app bot requested review from encukou and iritkatriel February 18, 2025 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-91079: Implement C stack limits using addresses, not counters. #130007

GH-91079: Implement C stack limits using addresses, not counters. #130007

markshannon commented Feb 11, 2025 •

edited by bedevere-app bot

Loading

encukou left a comment

bedevere-app bot commented Feb 17, 2025

zooba commented Feb 17, 2025

markshannon commented Feb 17, 2025 •

edited

Loading

markshannon commented Feb 17, 2025

markshannon commented Feb 17, 2025

encukou commented Feb 17, 2025

markshannon commented Feb 17, 2025

markshannon commented Feb 17, 2025

encukou commented Feb 17, 2025

markshannon commented Feb 17, 2025

markshannon commented Feb 17, 2025

markshannon commented Feb 18, 2025 •

edited

Loading

encukou left a comment

encukou Feb 18, 2025

encukou Feb 18, 2025

encukou Feb 18, 2025

encukou Feb 18, 2025

encukou Feb 18, 2025

markshannon Feb 18, 2025

encukou Feb 18, 2025

markshannon Feb 18, 2025

encukou Feb 18, 2025

encukou Feb 18, 2025

markshannon commented Feb 18, 2025

bedevere-app bot commented Feb 18, 2025

		// test_descr crashed on sparc64 with >7000 but let's keep a margin of error.
		# define Py_C_STACK_SIZE 1600000


		#if defined(WIN32) && !defined(MS_WIN64) && !defined(_M_ARM) && defined(_MSC_VER) && _MSC_VER >= 1300
		#if defined(WIN32)

GH-91079: Implement C stack limits using addresses, not counters. #130007

Are you sure you want to change the base?

GH-91079: Implement C stack limits using addresses, not counters. #130007

Conversation

markshannon commented Feb 11, 2025 • edited by bedevere-app bot Loading

encukou left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Feb 17, 2025

zooba commented Feb 17, 2025

markshannon commented Feb 17, 2025 • edited Loading

markshannon commented Feb 17, 2025

markshannon commented Feb 17, 2025

encukou commented Feb 17, 2025

markshannon commented Feb 17, 2025

markshannon commented Feb 17, 2025

encukou commented Feb 17, 2025

markshannon commented Feb 17, 2025

markshannon commented Feb 17, 2025

markshannon commented Feb 18, 2025 • edited Loading

encukou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon commented Feb 18, 2025

bedevere-app bot commented Feb 18, 2025

markshannon commented Feb 11, 2025 •

edited by bedevere-app bot

Loading

markshannon commented Feb 17, 2025 •

edited

Loading

markshannon commented Feb 18, 2025 •

edited

Loading