-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
Comments
While reading funcobject.c, I noticed that PyFunction_NewWithQualName() may exit early if it fails retrieving __name__ from the globals dict. 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:
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
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 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. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: