GH-93252: Fix error handling for failed Python calls #94693

brandtbucher · 2022-07-08T18:53:02Z

Also, add an assert so that things like this fail much earlier (and more clearly) on debug builds.

Issue: Python/pystate.c:2218: _PyThreadState_PopFrame: Assertion `tstate->datastack_top >= base' failed. #93252

brandtbucher · 2022-07-08T18:59:02Z

Python/ceval.c

+    PyObject **base = (PyObject **)frame;
+    // Make sure that this is, indeed, the top frame. We can't check this in
+    // _PyThreadState_PopFrame, since f_code is already cleared at that point:
+    assert(base + frame->f_code->co_framesize == tstate->datastack_top);


@pablogsal, just curious: does adding this assert (and not the fix) make tons of tests crash on your build?

If so, my theory is that your compiler is optimizing out the old assert, since the failing case is always undefined behavior according to the C standard.

I will test this night or tomorrow, but I checked and I was indeed compiling in debug mode with asserts, so the old assert should have triggered. I'm curious to see what's going on so I will also investigate that, but that won't affect this issue or the PR, is just that I'm very curious to see what's going on there :)

Well, even if asserts were turned on, the inequality comparison between the pointers would be undefined if they're part of different allocations. Which is exactly the situation it was checking for!

So if the compiler could somehow prove that the comparison it was always true when the result was defined, it could have optimized it into assert(1) or something.

But yeah, it doesn't affect this PR. The thing that's better about this new assert is that it's never undefined, and we don't need to overflow our stack chunk to trigger it. One failed call should do it (which is why so many tests crash if you add this without the fix).

So if the compiler could somehow prove that the comparison it was always true when the result was defined, it could have optimized it into assert(1) or something.

I don't think so because I was compiling with -O0. If the compiler was being smart there I want my money back 🤣

pablogsal

LGTM

Great work 👌

pablogsal · 2022-07-08T19:08:33Z

Lib/test/test_call.py

@@ -26,6 +26,18 @@ def fn(**kw):
        self.assertIsInstance(res, dict)
        self.assertEqual(list(res.items()), expected)

+    def test_frames_are_popped_after_failed_calls(self):


Not that it matters much, but maybe you want to add the CPython specific decorator

I thought about that, but I figure that this should still pass on any CPython implementation. If you somehow crash here, it's a bug, right? 😉

Yeah, we have been approaching these kind of tests in different ways so it doesn't matter :)

Python/ceval.c

bedevere-bot · 2022-07-08T20:37:52Z

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit 5b46418 🤖

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

brandtbucher · 2022-07-08T23:57:44Z

Not sure why some of the wasm32 buildbots are failing, but it looks totally unrelated to these changes:

Warnings:

../../Python/initconfig.c:2284:27: warning: format specifies type 'wint_t' (aka 'int') but the argument has type 'wint_t' (aka 'unsigned int') [-Wformat]
../../Python/initconfig.c:2284:42: warning: format specifies type 'wint_t' (aka 'int') but the argument has type 'wint_t' (aka 'unsigned int') [-Wformat]
../../Python/pytime.c:297:10: warning: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion]
../../Python/pytime.c:352:14: warning: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion]
../../Python/pytime.c:518:10: warning: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion]
../../Modules/expat/xmlparse.c:3107:9: warning: code will never be executed [-Wunreachable-code]
../../Modules/expat/xmlparse.c:4050:9: warning: code will never be executed [-Wunreachable-code]
../../Modules/expat/xmlparse.c:7681:11: warning: format specifies type 'int' but the argument has type 'ptrdiff_t' (aka 'long') [-Wformat]
../../Modules/socketmodule.c:4001:33: warning: comparison of integers of different signs: 'unsigned long' and 'long' [-Wsign-compare]
../../Modules/socketmodule.c:4054:33: warning: comparison of integers of different signs: 'unsigned long' and 'long' [-Wsign-compare]
../../Modules/socketmodule.c:4678:54: warning: comparison of integers of different signs: 'unsigned long' and 'long' [-Wsign-compare]

Error:

error: unable to open output file 'Modules/_testcapi/vectorcall.o': 'No such file or directory'

Maybe related to #93649?

brandtbucher · 2022-07-09T00:02:36Z

Maybe related to #93649?

Ah, yes, it is: #94549 (comment)

miss-islington · 2022-07-09T01:52:29Z

Thanks @brandtbucher for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

…94693) (cherry picked from commit 8a285df) Co-authored-by: Brandt Bucher <[email protected]>

bedevere-bot · 2022-07-09T01:52:44Z

GH-94699 is a backport of this pull request to the 3.11 branch.

kumaraditya303

Post commit LGTM.

tiran · 2022-07-09T11:25:30Z

Not sure why some of the wasm32 buildbots are failing, but it looks totally unrelated to these changes:

FYI, I have fixed the WASM buildbot issues in #94695. Once dependency was wrong and a directory was missing for OOT builds.

…thonGH-94693) (cherry picked from commit 8a285df) Co-authored-by: Brandt Bucher <[email protected]>

…94693)

GH-94708) Automerge-Triggered-By: GH:tiran

brandtbucher added 3 commits July 8, 2022 11:40

Add failing regression test

d0b41d1

Always pop the new frame after a failed call

e21751f

blurb add

eb6f8c4

brandtbucher added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump needs backport to 3.11 only security fixes labels Jul 8, 2022

brandtbucher requested review from pablogsal and kumaraditya303 July 8, 2022 18:53

brandtbucher self-assigned this Jul 8, 2022

brandtbucher requested a review from markshannon as a code owner July 8, 2022 18:53

bedevere-bot added the awaiting core review label Jul 8, 2022

brandtbucher commented Jul 8, 2022

View reviewed changes

pablogsal approved these changes Jul 8, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jul 8, 2022

pablogsal reviewed Jul 8, 2022

View reviewed changes

Python/ceval.c Outdated Show resolved Hide resolved

Fix compiler warning

5b46418

The-Compiler mentioned this pull request Jul 8, 2022

Python/pystate.c:2218: _PyThreadState_PopFrame: Assertion `tstate->datastack_top >= base' failed. #93252

Closed

brandtbucher added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 8, 2022

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

brandtbucher merged commit 8a285df into python:main Jul 9, 2022

bedevere-bot removed the awaiting merge label Jul 9, 2022

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 9, 2022

pythonGH-93252: Fix error handling for failed Python calls (pythonGH-…

23d96a6

…94693) (cherry picked from commit 8a285df) Co-authored-by: Brandt Bucher <[email protected]>

bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 9, 2022

kumaraditya303 reviewed Jul 9, 2022

View reviewed changes

tiran pushed a commit to tiran/cpython that referenced this pull request Jul 9, 2022

[3.11] pythonGH-93252: Fix error handling for failed Python calls (py…

305cfe8

…thonGH-94693) (cherry picked from commit 8a285df) Co-authored-by: Brandt Bucher <[email protected]>

kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Jul 9, 2022

pythonGH-93252: Fix error handling for failed Python calls (pythonGH-…

be41a14

…94693)

miss-islington pushed a commit that referenced this pull request Jul 9, 2022

[3.11] GH-93252: Fix error handling for failed Python calls (GH-94693) (

7a34172

GH-94708) Automerge-Triggered-By: GH:tiran

brandtbucher deleted the failed-call-cleanup branch July 21, 2022 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-93252: Fix error handling for failed Python calls #94693

GH-93252: Fix error handling for failed Python calls #94693

brandtbucher commented Jul 8, 2022 •

edited by bedevere-bot

Loading

brandtbucher Jul 8, 2022

pablogsal Jul 8, 2022

brandtbucher Jul 8, 2022

brandtbucher Jul 8, 2022

pablogsal Jul 8, 2022

pablogsal left a comment

pablogsal Jul 8, 2022

brandtbucher Jul 8, 2022

pablogsal Jul 8, 2022

bedevere-bot commented Jul 8, 2022

brandtbucher commented Jul 8, 2022 •

edited

Loading

brandtbucher commented Jul 9, 2022

miss-islington commented Jul 9, 2022

bedevere-bot commented Jul 9, 2022

kumaraditya303 left a comment

tiran commented Jul 9, 2022

GH-93252: Fix error handling for failed Python calls #94693

GH-93252: Fix error handling for failed Python calls #94693

Conversation

brandtbucher commented Jul 8, 2022 • edited by bedevere-bot Loading

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

pablogsal 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

bedevere-bot commented Jul 8, 2022

brandtbucher commented Jul 8, 2022 • edited Loading

brandtbucher commented Jul 9, 2022

miss-islington commented Jul 9, 2022

bedevere-bot commented Jul 9, 2022

kumaraditya303 left a comment

Choose a reason for hiding this comment

tiran commented Jul 9, 2022

brandtbucher commented Jul 8, 2022 •

edited by bedevere-bot

Loading

brandtbucher commented Jul 8, 2022 •

edited

Loading