classification
Title: Corruptions in func_dealloc() with partially-created function object
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Yonatan Goldschmidt, miss-islington, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2020-10-24 23:49 by Yonatan Goldschmidt, last changed 2020-10-29 11:03 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 22953 merged Yonatan Goldschmidt, 2020-10-24 23:58
PR 23021 merged miss-islington, 2020-10-29 09:59
PR 23022 merged miss-islington, 2020-10-29 09:59
Messages (4)
msg379546 - (view) Author: Yonatan Goldschmidt (Yonatan Goldschmidt) * Date: 2020-10-24 23:49
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: _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
    587     Py_CLEAR(op->func_qualname);
    (gdb) p _py_tmp
    $1 = (PyObject *) 0x6161616161616161

This issue exists since https://github.com/python/cpython/pull/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.
msg379863 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-10-29 09:59
New changeset 350526105fa9b131d8b941ae753378b741dabb2f by Yonatan Goldschmidt in branch 'master':
bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object (GH-22953)
https://github.com/python/cpython/commit/350526105fa9b131d8b941ae753378b741dabb2f
msg379864 - (view) Author: miss-islington (miss-islington) Date: 2020-10-29 10:24
New changeset 9ede1b071bf5250ba5f2d04d7ba86a24c06d41a1 by Miss Skeleton (bot) in branch '3.8':
bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object (GH-22953)
https://github.com/python/cpython/commit/9ede1b071bf5250ba5f2d04d7ba86a24c06d41a1
msg379866 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-10-29 11:03
New changeset 60324d26b58c89d68abb23fb42f1563d395c3910 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)
https://github.com/python/cpython/commit/60324d26b58c89d68abb23fb42f1563d395c3910
History
Date User Action Args
2020-10-29 11:03:20serhiy.storchakasetmessages: + msg379866
2020-10-29 11:03:13serhiy.storchakasetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.8, Python 3.9
2020-10-29 10:24:16miss-islingtonsetmessages: + msg379864
2020-10-29 09:59:33miss-islingtonsetpull_requests: + pull_request21936
2020-10-29 09:59:29serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg379863
2020-10-29 09:59:24miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request21935
2020-10-24 23:58:15Yonatan Goldschmidtsetkeywords: + patch
stage: patch review
pull_requests: + pull_request21871
2020-10-24 23:49:05Yonatan Goldschmidtcreate