Skip to content

Shim frames can be accessed during frame teardown (via tstate->cframe->current_frame) #100126

Closed
Closed
Shim frames can be accessed during frame teardown (via tstate->cframe->current_frame)#100126
Assignees
brandtbucher
Labels
3.12only security fixesrelease-blockertype-crashA hard crash of the interpreter, possibly with a core dump

Description

@jaraco
jaraco
opened on Dec 9, 2022

Crash report

In jaraco/jaraco.net#5, I've captured a failure that's emerged as Github updated Python 3.12 from a2 to a3 on Linux. The calling code is using ctypes to call libc functions.

I don't yet have a minimal reproducer. I'm unable to replicate the issue locally as I don't yet have Linux with a3.

  • Operating system and architecture: Ubuntu Linux

Linked PRs

Activity

  1. jaraco
    added
    type-crashA hard crash of the interpreter, possibly with a core dump
    on Dec 9, 2022

  2. illia-v commented on Dec 17, 2022

    @illia-v
    illia-v
    on Dec 17, 2022
    Contributor

    Tests of urllib3 detected a segmentation fault on 3.12.0a3 too.
    https://github.com/urllib3/urllib3/actions/runs/3720502510/jobs/6310016874

    Fatal Python error: Segmentation fault on Linux and Windows fatal exception: access violation on Windows happened in one test function.

    The function succeeded on macOS, but Fatal Python error: Bus error happened in a different one.

  3. illia-v commented on Dec 22, 2022

    @illia-v
    illia-v
    on Dec 22, 2022
    Contributor

    b72014c is the first commit where the segfault in urllib3's tests happens.

  4. AlexWaygood commented on Dec 22, 2022

    @AlexWaygood
    AlexWaygood
    on Dec 22, 2022
    Member

    b72014c is the first commit where the segfault in urllib3's tests happens.

    Cc. @brandtbucher

  5. brandtbucher
    self-assigned this
    on Dec 22, 2022

  6. brandtbucher commented on Dec 22, 2022

    @brandtbucher
    brandtbucher
    on Dec 22, 2022
    Member

    I'll look into this too. No promises that I'll have it figured out before the holiday weekend, but it'll be my main priority starting next Tuesday.

    Not sure if #99110 is related... hard to tell without a reproducer.

  7. illia-v commented on Dec 26, 2022

    @illia-v
    illia-v
    on Dec 26, 2022
    Contributor

    I'll look into this too. No promises that I'll have it figured out before the holiday weekend, but it'll be my main priority starting next Tuesday.

    Thanks!

    Not sure if #99110 is related... hard to tell without a reproducer.

    It still happens after PR of #99110 is merged.

  8. brandtbucher commented on Dec 28, 2022

    @brandtbucher
    brandtbucher
    on Dec 28, 2022
    Member

    Here is a reproducer with only pytest:

    # pytest crasher.py

import socket

def test_foo():
    _ = socket.socket()

    Indeed, this looks eerily similar to #99729.

  9. brandtbucher commented on Dec 28, 2022

    @brandtbucher
    brandtbucher
    on Dec 28, 2022
    Member

    Found the issue. PyEval_GetGlobals returns tstate->cframe->current_frame->f_globals. When tearing a frame down, current_frame may be a shim frame. Shim frames have no globals, so the code crashes.

  10. brandtbucher commented on Dec 28, 2022

    @brandtbucher
    brandtbucher
    on Dec 28, 2022
    Member

    Looks like in general, it is not safe for current_frame to be a shim frame. Too many things can break.

  11. brandtbucher commented on Dec 28, 2022

    @brandtbucher
    brandtbucher
    on Dec 28, 2022
    Member

    Minimal reproducer, no pytest:

    # python crasher.py

import operator

class DelRaises:
    def __del__(self):
        assert False

def f():
    local = DelRaises()

# Call from C, so there is a shim frame above f:
operator.call(f)
  12. brandtbucher
    added
    3.12only security fixes
    on Dec 29, 2022
  13. brandtbucher
    changed the title [-]Regression in 3.12a3 - segfault in interpreter trampoline[/-] [+]Shim frames can be accessed during frame teardown (via `tstate->cframe->current_frame`)[/+] on Dec 29, 2022

  14. brandtbucher commented on Dec 29, 2022

    @brandtbucher
    brandtbucher
    on Dec 29, 2022
    Member

    I'm honestly not sure what to do here.

    Perhaps, before tearing down a frame, if our previous frame is a shim frame, we should set tstate->cframe to be cframe.previous. But that's adding code to a perf-sensitive path, and it also makes the interaction with INTERPRETER_EXIT awkward, since it tries to do the same thing.

    The best option is probably just to add more while (frame && _PyFrame_IsIncomplete(frame)) {frame = frame->previous;} loops to all of the affected areas. It probably makes sense to just add a helper function for this (_PyThreadState_GetInterpreterFrame?), rather than continuing to sprinkle them everywhere.

    Thoughts, @markshannon? (I don't think you're out this week... feel free to ignore if so.)

  15. brandtbucher commented on Dec 29, 2022

    @brandtbucher
    brandtbucher
    on Dec 29, 2022
    Member

    Also going to tag as release-blocker (for now).

  16. brandtbucher
    added
    release-blocker
    on Dec 29, 2022
  17. ezio-melotti
    added this to Release and Deferred blockers 🚫on Dec 29, 2022
  18. github-project-automation
    moved this to Todo in Release and Deferred blockers 🚫on Dec 29, 2022

  19. Yhg1s commented on Jan 9, 2023

    @Yhg1s
    Yhg1s
    on Jan 9, 2023
    Member

    @brandtbucher what's the state of this issue (and the attached fix for it)? Should this hold up 3.12.0a4?

  20. brandtbucher commented on Jan 9, 2023

    @brandtbucher
    brandtbucher
    on Jan 9, 2023
    Member

    Sorry, just got back from vacation and thought it was merged while I was gone. I'm in a meeting right now, but I have one small review comment to apply, then I'll merge.

    If it's not too much trouble, I'd like this to make it into the next release. It's not too hard to trigger using frameworks like pytest, so it would be nice to have non-crashy test coverage of the next alpha out in the wild.

  21. brandtbucher
    added a commit that references this issue on Jan 9, 2023

    GH-100126: Skip incomplete frames in more places (GH-100613)

    61762b9

  22. brandtbucher commented on Jan 9, 2023

    @brandtbucher
    brandtbucher
    on Jan 9, 2023
    Member

    @Yhg1s, this is done!

  23. brandtbucher
    closed this as completedon Jan 9, 2023
  24. github-project-automation
    moved this from Todo to Done in Release and Deferred blockers 🚫on Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

3.12only security fixesrelease-blockertype-crashA hard crash of the interpreter, possibly with a core dump

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions