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

Activity

1. Jongy commented on Oct 24, 2020

   

   Jongymannequin

   on Oct 24, 2020

   MannequinAuthor

   More actions

   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.
2. Jongy

   added

   3.10 (EOL)end of lifeend of life

   interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)

   type-crashA hard crash of the interpreter, possibly with a core dumpA hard crash of the interpreter, possibly with a core dump

   on Oct 24, 2020
3. serhiy-storchaka commented on Oct 29, 2020

   

   serhiy-storchaka

   on Oct 29, 2020

   Member

   More actions

   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
4. miss-islington commented on Oct 29, 2020

   

   miss-islington

   on Oct 29, 2020

   Contributor

   More actions

   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
5. serhiy-storchaka

   added

   3.8 (EOL)end of lifeend of life

   3.9 (EOL)end of lifeend of life

   on Oct 29, 2020
6. serhiy-storchaka

   closed this as completedon Oct 29, 2020
7. serhiy-storchaka

   added

   3.8 (EOL)end of lifeend of life

   3.9 (EOL)end of lifeend of life

   on Oct 29, 2020
8. serhiy-storchaka

   closed this as completedon Oct 29, 2020
9. serhiy-storchaka commented on Oct 29, 2020

   

   serhiy-storchaka

   on Oct 29, 2020

   Member

   More actions

   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
10. ezio-melotti

    transferred this issue fromon Apr 10, 2022