Skip to content
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.

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

faulthandler does not properly restore sigaltstack during teardown #74070

Closed
tich mannequin opened this issue Mar 23, 2017 · 2 comments
Closed

faulthandler does not properly restore sigaltstack during teardown #74070

tich mannequin opened this issue Mar 23, 2017 · 2 comments
Labels
3.7 (EOL) end of life extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@tich
Copy link
Mannequin

tich mannequin commented Mar 23, 2017

BPO 29884
Nosy @vstinner, @Mariatta, @tich
PRs
  • bpo-29884: faulthandler: Restore the old sigaltstack during teardown #777
  • [3.5] bpo-29884: faulthandler: Restore the old sigaltstack during teardown (GH-777) #796
  • [3.6] bpo-29884: faulthandler: Restore the old sigaltstack during teardown (GH-777) #797
  • Files
  • python_failure.txt: AddressSanitizer output
  • python_reproducer.cpp: Reproducer
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-03-24.16:29:34.539>
    created_at = <Date 2017-03-23.05:43:49.472>
    labels = ['extension-modules', 'type-feature', '3.7']
    title = 'faulthandler does not properly restore sigaltstack during teardown'
    updated_at = <Date 2017-03-24.16:29:34.528>
    user = 'https://github.com/tich'

    bugs.python.org fields:

    activity = <Date 2017-03-24.16:29:34.528>
    actor = 'Mariatta'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-03-24.16:29:34.539>
    closer = 'Mariatta'
    components = ['Extension Modules']
    creation = <Date 2017-03-23.05:43:49.472>
    creator = 'tich'
    dependencies = []
    files = ['46754', '46755']
    hgrepos = []
    issue_num = 29884
    keywords = []
    message_count = 2.0
    messages = ['290025', '290095']
    nosy_count = 3.0
    nosy_names = ['vstinner', 'Mariatta', 'tich']
    pr_nums = ['777', '796', '797']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue29884'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

    @tich
    Copy link
    Mannequin Author

    tich mannequin commented Mar 23, 2017

    Looks like faulthandler is not properly tearing down its sigaltstack, causing potential double-free issues in any application that embeds the Python interpreter.
    I stumbled upon this when I enabled AddressSanitizer on my application, which sets up and tears down a Python interpreter instance at runtime. AddressSanitizer complained about a double-free of the stack_t::ss_sp memory region.
    After close inspection, here's what's happening:

    1. When a new thread is created, AddressSanitizer sets up its own alternative stack by calling sigaltstack
    2. Py_Initialize() is called, which initializes faulthandler, which sets up its own alternative stack, therefore overriding the one installed by AddressSanitizer
    3. Py_Finalize() is called, which deinitializes faulthandler, which merely deletes the allocated stack region, but leaves the alternative stack installed. Any signal that occurs after this point will be using a memory region it doesn't own as stack. dangerous stuff.
    4. The thread exits, at which point AddressSanitizer queries sigaltstack for the current alternative stack, blindly assumes that it's the same one that it installed, and attempts to free the allocated stack region. Therefore causing a double free issue

    Regardless of the fact that AddressSanitizer should probably not blindly trust that the currently installed sigaltstack is the same one it installed earlier, the current code in faulthandler leaves the sigaltstack in a very bad state after finalizing.
    This means that the application that embeds the Python interpreter better hope that no signals are raised after it calls Py_Finalize().

    I have a patch that fixes this issue. faulthandler will save the previously installed alternative stack at initialization time. During deinitialization, it will query sigaltstack for the current stack. If it's the same as the one it installed,
    it will restore the saved previous stack.

    'sigaltstack' just sounds like a badly designed API. There is essentially no way to use it 'correctly'.
    Here's how AddressSanitizer uses it (line 164): http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix_libcdep.cc?view=markup
    and here's how the Chrome browser uses it: https://chromium.googlesource.com/breakpad/breakpad/+/chrome_43/src/client/linux/handler/exception_handler.cc#149

    Notice that my approach is closer to what Chrome does, but in the case where the installed stack is no longer ours, I don't disable whatever stack is installed. This is because I don't believe that will make much difference. Whoever switched out the stack could have
    saved our stack somewhere and planned on blindly restoring it upon exit. In which case, whatever we do would be overridden.

    Attached are a tiny reproducer for the issue, along with the complete analysis of what's reported by AddressSanitizer. I'll follow this up by a pull request for my changes.

    Thanks!
    Chris

    @tich tich mannequin added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Mar 23, 2017
    @Mariatta
    Copy link
    Member

    PR was merged and backported into 3.5 and 3.6, so I'm closing this.
    Thanks, Christophe :)

    @Mariatta Mariatta added the 3.7 (EOL) end of life label Mar 24, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant