GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-106092: Fix use-after-free crash in frame_dealloc by andersk · Pull Request #106875 · python/cpython

Conversation

@andersk

@andersk andersk commented Jul 18, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

It was possible for the trashcan to delay the deallocation of a PyFrameObject until after its corresponding _PyInterpreterFrame has already been freed. So frame_dealloc needs to avoid dereferencing the f_frame pointer unless it first checks that the pointer still points to valid memory.

Fixes #106092. This should be backported to 3.11 and 3.12.

  • Issue: Segmentation fault in 3.11.4, 3.12.0b3; _PyInterpreterFrame ownership issue #106092

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@andersk
pythongh-106092: Fix use-after-free crash in frame_dealloc
e8a16a6 
It was possible for the trashcan to delay the deallocation of a
PyFrameObject until after its corresponding _PyInterpreterFrame has
already been freed.  So frame_dealloc needs to avoid dereferencing the
f_frame pointer unless it first checks that the pointer still points
to valid memory.

Signed-off-by: Anders Kaseorg <[email protected]>
@andersk
andersk requested a review from markshannon as a code owner July 18, 2023 23:19
@bedevere-bot bedevere-bot added the awaiting review label Jul 18, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jul 18, 2023
Segmentation fault in 3.11.4, 3.12.0b3; _PyInterpreterFrame ownership issue #106092
Closed
@Yhg1s Yhg1s added the release-blocker label Jul 31, 2023
@brandtbucher brandtbucher self-assigned this Jul 31, 2023
@markshannon

markshannon commented Jul 31, 2023

Copy link
Copy Markdown
Member

I think it better to ensure that f->f_frame always points to valid memory, rather than do the additional check.
Something like https://github.com/python/cpython/compare/main...faster-cpython:cpython:safe-clear-frame?expand=1

Do you have a reproducer that doesn't need third-party modules?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brandtbucher brandtbucher assigned markshannon and unassigned brandtbucher Jul 31, 2023
@andersk

andersk commented Jul 31, 2023

Copy link
Copy Markdown
Contributor Author

Something like https://github.com/python/cpython/compare/main...faster-cpython:cpython:safe-clear-frame?expand=1

+static const _PyInterpreterFrame cleared_frame = {
+    .owner = FRAME_CLEARED
+};

Note that FRAME_CLEARED has never been a valid member of enum _frameowner; see #106156.

+        f->f_frame = (_PyInterpreterFrame *)&cleared_frame;
         Py_DECREF(f);

Couldn’t that incorrectly skip the deallocation of f_executable, f_funcobj, and f_locals in frame_dealloc, if the frame was allocated by PyFrame_New and has owner == FRAME_OWNED_BY_FRAME_OBJECT?

Do you have a reproducer that doesn't need third-party modules?

Not at present—further minimization is probably possible but difficult, because the segfault requires a specific coincidence of the position of the stack pointer within the current stack chunk, the GC threshold, and the trashcan recursion depth.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon

markshannon commented Aug 1, 2023

Copy link
Copy Markdown
Member

A reproducer that causes an assertion failure would be good enough, I think.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markshannon
markshannon approved these changes Aug 1, 2023
View reviewed changes
Comment thread Objects/frameobject.c
if (f->f_frame->owner == FRAME_OWNED_BY_FRAME_OBJECT) {
assert(f->f_frame == (_PyInterpreterFrame *)f->_f_frame_data);
_PyInterpreterFrame *frame = (_PyInterpreterFrame *)f->_f_frame_data;
if (f->f_frame == frame && frame->owner == FRAME_OWNED_BY_FRAME_OBJECT) {

@markshannon markshannon Aug 1, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This looks correct.

Note:
In theory, f->f_frame == frame and frame->owner == FRAME_OWNED_BY_FRAME_OBJECT should be equivalent, but in practice they are not.
We can't just check frame->owner == FRAME_OWNED_BY_FRAME_OBJECT because of the Trashcan mechanism, and we can't just check f->f_frame == frame because of this.
So we need both checks here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@P403n1x87 P403n1x87 Sep 20, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should this check be put in other places too? We are currently investigating the root cause of segfaults in 3.11.5 with call stacks ending with

#0  0x0000000000635d9a in ?? ()
#1  0x00000000004d8e27 in PyFrame_FastToLocalsWithError ()
#2  0x00000000004d8e3e in ?? ()
#3  0x000000000054e5f6 in _PyObject_GenericGetAttrWithDict ()

What we are trying to access here is frame.f_locals. The frame object is obtained from looping over all thread states and calling PyThreadState_GetFrame on them (so we know we have a strong reference to it). Because of the inlining it's not immediately obvious where in PyFrame_FastToLocalsWithError we are segfaulting exactly, but our current hunch is that it is because the referenced _PyInterpreterState might have been deallocated. We're still investigating this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Aug 1, 2023
@markshannon
markshannon merged commit 557b05c into python:main Aug 1, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 1, 2023
@markshannon markshannon added the needs backport to 3.12 only security fixes label Aug 1, 2023
@miss-islington

miss-islington commented Aug 1, 2023

Copy link
Copy Markdown
Contributor

Thanks @andersk for the PR, and @markshannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon markshannon added the needs backport to 3.11 only security fixes label Aug 1, 2023
@bedevere-bot

bedevere-bot commented Aug 1, 2023

Copy link
Copy Markdown

GH-107532 is a backport of this pull request to the 3.12 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Aug 1, 2023

Copy link
Copy Markdown
Contributor

Thanks @andersk for the PR, and @markshannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Aug 1, 2023
@bedevere-bot

bedevere-bot commented Aug 1, 2023

Copy link
Copy Markdown

GH-107533 is a backport of this pull request to the 3.11 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 1, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 1, 2023
@andersk @miss-islington
pythongh-106092: Fix use-after-free crash in frame_dealloc (pythonGH-…
0c4f336 
…106875)

It was possible for the trashcan to delay the deallocation of a
PyFrameObject until after its corresponding _PyInterpreterFrame has
already been freed.  So frame_dealloc needs to avoid dereferencing the
f_frame pointer unless it first checks that the pointer still points
to the interpreter frame within the frame object.

(cherry picked from commit 557b05c)

Co-authored-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 1, 2023
@andersk @miss-islington
pythongh-106092: Fix use-after-free crash in frame_dealloc (pythonGH-…
cac7a94 
…106875)

It was possible for the trashcan to delay the deallocation of a
PyFrameObject until after its corresponding _PyInterpreterFrame has
already been freed.  So frame_dealloc needs to avoid dereferencing the
f_frame pointer unless it first checks that the pointer still points
to the interpreter frame within the frame object.

(cherry picked from commit 557b05c)

Co-authored-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
pablogsal pushed a commit that referenced this pull request Aug 1, 2023
@miss-islington
[3.11] gh-106092: Fix use-after-free crash in frame_dealloc (GH-106875)…
46cae02 
… (#107533)
Yhg1s pushed a commit that referenced this pull request Aug 1, 2023
@miss-islington @andersk
[3.12] gh-106092: Fix use-after-free crash in frame_dealloc (GH-106875)…
b68faa3 
… (#107532)

gh-106092: Fix use-after-free crash in frame_dealloc (GH-106875)

It was possible for the trashcan to delay the deallocation of a
PyFrameObject until after its corresponding _PyInterpreterFrame has
already been freed.  So frame_dealloc needs to avoid dereferencing the
f_frame pointer unless it first checks that the pointer still points
to the interpreter frame within the frame object.

(cherry picked from commit 557b05c)

Signed-off-by: Anders Kaseorg <[email protected]>
Co-authored-by: Anders Kaseorg <[email protected]>
@andersk
andersk deleted the frame_dealloc branch August 1, 2023 16:08
@Zylphrex Zylphrex mentioned this pull request Feb 6, 2024
Segmentation fault in sentry profiler getsentry/sentry-python#2386
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon approved these changes
+1 more reviewer
@P403n1x87 P403n1x87 P403n1x87 left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@markshannon markshannon

Labels

release-blocker

Projects

Release and Deferred blockers 🚫
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Segmentation fault in 3.11.4, 3.12.0b3; _PyInterpreterFrame ownership issue

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@andersk @markshannon @miss-islington @bedevere-bot @P403n1x87 @Yhg1s @brandtbucher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.