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

Jump to bottom

double free in io.TextIOWrapper #72573

Closed
scufre mannequin opened this issue Oct 7, 2016 · 3 comments
Closed

double free in io.TextIOWrapper #72573

scufre mannequin opened this issue Oct 7, 2016 · 3 comments
Assignees
@serhiy-storchaka
Labels
3.7 expert-IO extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@scufre
Copy link
Mannequin

scufre mannequin commented Oct 7, 2016

BPO 28387
Nosy @benjaminp, @serhiy-storchaka
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
    		•
    Files
  • textiowrapper_crash.py: Proof of concept to demostrate the issue
  • textio.c.patch: Proposed fix
  • textiowrapper_clear-3.5.patch
  • textiowrapper_clear-2.7.patch
    		•

    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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-11-03.13:41:50.317>
created_at = <Date 2016-10-07.19:32:57.587>
labels = ['extension-modules', '3.7', 'expert-IO', 'type-crash']
title = 'double free in io.TextIOWrapper'
updated_at = <Date 2017-03-31.16:36:23.657>
user = 'https://bugs.python.org/scufre'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:23.657>
actor = 'dstufft'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-11-03.13:41:50.317>
closer = 'serhiy.storchaka'
components = ['Extension Modules', 'IO']
creation = <Date 2016-10-07.19:32:57.587>
creator = 'scufre'
dependencies = []
files = ['45004', '45005', '45274', '45275']
hgrepos = []
issue_num = 28387
keywords = ['patch']
message_count = 3.0
messages = ['278263', '279717', '279994']
nosy_count = 5.0
nosy_names = ['benjamin.peterson', 'stutzbach', 'python-dev', 'serhiy.storchaka', 'scufre']
pr_nums = ['552']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue28387'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']
    @scufre
    Copy link
    Mannequin Author

    scufre mannequin commented Oct 7, 2016

    We have found that you can produce a crash when an instance of _io.TextIOWrapper is being deallocated while there's another thread invoking the garbage collector. I've attached a simple script that should reproduce the issue (textiowrapper_crash.py)

    Looking at the code of the _io module, we found that on the dealloc method of the TextIOWrapper class, it first tries to invoke the close method, then releases its internal members, after that removes itself from the garbage collector tracking and finally frees deallocates the remaining stuff. What happens, is that while releasing it's internal members, it might end up calling code that releases the interpreter lock (because its doing an operating system call), letting other threads execute. If for example the thread that comes in, invokes the garbage collector, on debug will raise an assert on gcmodule.c on line 351, where I understand it is complaining that it is tracking an object with refcount 0. In a release build, I suppose this goes on and will end up releasing the object (given it has a refcount of 0), and when the interrupted dealloc thread continues will end up freeing again itself which at some point produces a crash.

    Attached is a proposed fix for the issue in textio.c.patch, where the it the call to _PyObject_GC_UNTRACK is now done right after the call to the close method and before release its internal members.

    As a reference we have been able to reproduce this with Python 2.7.12 on Windows (i386)

    @scufre scufre mannequin added extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump labels Oct 7, 2016
    @serhiy-storchaka serhiy-storchaka self-assigned this Oct 25, 2016
    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Oct 30, 2016

    Thank you for your report and patch Sebastian.

    In Python 3 the solution can be simpler, just move the line "_PyObject_GC_UNTRACK(self);" above the line "textiowrapper_clear(self);". But calling PyObject_ClearWeakRefs() also should be moved up. Otherwise half-destroyed TextIOWrapper instance can be accessed via weak references. Following patches do this (and small refactoring).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 3, 2016

    New changeset 91f024fc9b3a by Serhiy Storchaka in branch '2.7':
    Issue bpo-28387: Fixed possible crash in _io.TextIOWrapper deallocator when
    https://hg.python.org/cpython/rev/91f024fc9b3a

    New changeset 89f7386104e2 by Serhiy Storchaka in branch '3.5':
    Issue bpo-28387: Fixed possible crash in _io.TextIOWrapper deallocator when
    https://hg.python.org/cpython/rev/89f7386104e2

    New changeset c4319c0d0131 by Serhiy Storchaka in branch '3.6':
    Issue bpo-28387: Fixed possible crash in _io.TextIOWrapper deallocator when
    https://hg.python.org/cpython/rev/c4319c0d0131

    New changeset 36af3566b67a by Serhiy Storchaka in branch 'default':
    Issue bpo-28387: Fixed possible crash in _io.TextIOWrapper deallocator when
    https://hg.python.org/cpython/rev/36af3566b67a

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Assignees

    @serhiy-storchaka serhiy-storchaka

    Labels
    3.7 expert-IO extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @serhiy-storchaka