Corruptions in func_dealloc() with partially-created function object

[Jongy]

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]

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.

[serhiy-storchaka]

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]

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