New submission from Yonatan Goldschmidt <yon.goldschmidt@gmail.com>:
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.
----------
components: Interpreter Core
messages: 379546
nosy: Yonatan Goldschmidt
priority: normal
severity: normal
status: open
title: Corruptions in func_dealloc() with partially-created function object
type: crash
versions: Python 3.10
_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue42143>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com
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.
----------
components: Interpreter Core
messages: 379546
nosy: Yonatan Goldschmidt
priority: normal
severity: normal
status: open
title: Corruptions in func_dealloc() with partially-created function object
type: crash
versions: Python 3.10
_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue42143>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com