Skip to content

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

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.

Sign up for GitHub

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
brandtbucher merged 4 commits into from
Jul 9, 2022
Merged

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0b41d1
Add failing regression test
brandtbucher Jul 8, 2022
e21751f
Always pop the new frame after a failed call
brandtbucher Jul 8, 2022
eb6f8c4
blurb add
brandtbucher Jul 8, 2022
5b46418
Fix compiler warning
brandtbucher Jul 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Always pop the new frame after a failed call
  • Loading branch information
@brandtbucher
brandtbucher committed Jul 8, 2022
commit e21751fb852c37d118aab1a3cdc2d6863c0e0b9b
6 changes: 5 additions & 1 deletion Python/ceval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6410,7 +6410,7 @@ _PyEvalFramePushAndInit(PyThreadState *tstate, PyFunctionObject *func,
}
if (initialize_locals(tstate, func, localsarray, args, argcount, kwnames)) {
assert(frame->owner != FRAME_OWNED_BY_GENERATOR);
_PyFrame_Clear(frame);
_PyEvalFrameClearAndPop(tstate, frame);
return NULL;
}
return frame;
Expand All @@ -6432,6 +6432,10 @@ _PyEvalFramePushAndInit(PyThreadState *tstate, PyFunctionObject *func,
static void
_PyEvalFrameClearAndPop(PyThreadState *tstate, _PyInterpreterFrame * frame)
{
PyObject **base = (PyObject **)frame;
Comment thread
brandtbucher marked this conversation as resolved.
Outdated
// 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);
Copy link
Copy Markdown
Member Author

@brandtbucher brandtbucher Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Member

@pablogsal pablogsal Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Copy Markdown
Member Author

@brandtbucher brandtbucher Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@brandtbucher brandtbucher Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member

@pablogsal pablogsal Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤣

tstate->recursion_remaining--;
assert(frame->frame_obj == NULL || frame->frame_obj->f_frame == frame);
assert(frame->owner == FRAME_OWNED_BY_THREAD);
Expand Down