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

Corruptions in func_dealloc() with partially-created function object #86309

Closed
Jongy mannequin opened this issue Oct 24, 2020 · 4 comments
Closed

Corruptions in func_dealloc() with partially-created function object #86309

Jongy mannequin opened this issue Oct 24, 2020 · 4 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@Jongy
Copy link
Mannequin

Jongy mannequin commented Oct 24, 2020

BPO 42143
Nosy @serhiy-storchaka, @miss-islington, @Jongy
PRs
  • bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object #22953
  • [3.9] bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object (GH-22953) #23021
  • [3.8] bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object (GH-22953) #23022
  • 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 2020-10-29.11:03:13.423>
    created_at = <Date 2020-10-24.23:49:05.892>
    labels = ['interpreter-core', '3.10', '3.8', '3.9', 'type-crash']
    title = 'Corruptions in func_dealloc() with partially-created function object'
    updated_at = <Date 2020-10-29.11:03:20.782>
    user = 'https://github.com/Jongy'

    bugs.python.org fields:

    activity = <Date 2020-10-29.11:03:20.782>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-10-29.11:03:13.423>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2020-10-24.23:49:05.892>
    creator = 'Yonatan Goldschmidt'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42143
    keywords = ['patch']
    message_count = 4.0
    messages = ['379546', '379863', '379864', '379866']
    nosy_count = 3.0
    nosy_names = ['serhiy.storchaka', 'miss-islington', 'Yonatan Goldschmidt']
    pr_nums = ['22953', '23021', '23022']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue42143'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @Jongy
    Copy link
    Mannequin Author

    Jongy mannequin commented Oct 24, 2020

    While reading funcobject.c, I noticed that PyFunction_NewWithQualName() may exit early if it fails retrieving __name__ from the globals dict.
    It will destroy the partially-created PyFunction object. However, this is done before ever initializing ->func_qualname. Since the object is allocated with PyObject_GC_New() (which does *not* provide zero-initialized memory), we are using Py_CLEAR() on uninitialized memory.

    I wrote a simple snippet to produce this bug, expecting to see a crash due to the Py_DECREF() call on garbage memory:

        from types import FunctionType
        import random
    
        class BadEq:
            def __hash__(self):
                print("hash called")
                return hash("__name__")
            def __eq__(self, other):
                print("eq called")
                raise Exception()
    
        # run until we hit a 0x61616161.. pointer for PyFunctionObject->func_qualname, and crash
        while True:
            s = int(random.random() * 1000) * "a"
            try:
                FunctionType(BadEq.__hash__.__code__, {BadEq(): 1})
            except Exception:
                pass

    However, I got *another* crash immediately: func_dealloc() calls _PyObject_GC_UNTRACK() which crashes if _PyObject_GC_TRACK() was not called beforehand. When running with "--with-pydebug", you get this nice assert:

    [Objects/funcobject.c:595](https://github.com/python/cpython/blob/main/Objects/funcobject.c#L595): _PyObject_GC_UNTRACK: Assertion "(((PyGC_Head *)(op)-1)->_gc_next != 0)" failed: object not tracked by the garbage collector
    

    I replaced "_PyObject_GC_UNTRACK(op);" in func_dealloc() with:

        if (PyObject_GC_IsTracked(op)) {
            _PyObject_GC_UNTRACK(op);
        }

    And ran my snippet again, this time it crashes after some loops with the error I was expecting to receive
    (here I print _py_tmp, the temporary variable inside Py_CLEAR()):

    Program received signal SIGSEGV, Segmentation fault.
    func_clear (op=op@entry=0x7f9c8a25faf0) at [Objects/funcobject.c:587](https://github.com/python/cpython/blob/main/Objects/funcobject.c#L587)
    587     Py_CLEAR(op->func_qualname);
    (gdb) p _py_tmp
    $1 = (PyObject *) 0x6161616161616161
    

    This issue exists since #11112, which fixed tons of call sites to use PyDict_GetItemWithError(), I guess that this specific flow was not tested.

    As a fix, I think we should run the part that can result in errors *before* creating the PyFunction, so we can no longer get an error while we have a partial object. This fixes both of the problems I've described. Thus we can move the dict lookup part to the top of
    PyFunction_NewWithQualName().

    I see no reason not to make such a change in the function's flow. If we'd rather keep the flow as-is, we can make sure to initialize ->func_qualname to NULL, and wrap _PyObject_GC_UNTRACK() with _PyObject_GC_IS_TRACKED() in func_dealloc(). I like this solution less because it adds this unnecessary "if" statement in func_dealloc().

    I'll post a PR in GitHub soon.

    @Jongy Jongy mannequin added 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Oct 24, 2020
    @serhiy-storchaka
    Copy link
    Member

    New changeset 3505261 by Yonatan Goldschmidt in branch 'master':
    bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object (GH-22953)
    3505261

    @miss-islington
    Copy link
    Contributor

    New changeset 9ede1b0 by Miss Skeleton (bot) in branch '3.8':
    bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object (GH-22953)
    9ede1b0

    @serhiy-storchaka serhiy-storchaka added 3.8 only security fixes 3.9 only security fixes labels Oct 29, 2020
    @serhiy-storchaka serhiy-storchaka added 3.8 only security fixes 3.9 only security fixes labels Oct 29, 2020
    @serhiy-storchaka
    Copy link
    Member

    New changeset 60324d2 by Miss Skeleton (bot) in branch '3.9':
    bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object (GH-22953) (GH-23021)
    60324d2

    @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.8 only security fixes 3.9 only security fixes 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants