gh-152682: Fix NULL dereference on OOM in symtable_visit_type_param_bound_or_default#152684
Conversation
|
All commit authors signed the Contributor License Agreement. |
sobolevn
left a comment
sobolevn
left a comment
There was a problem hiding this comment.
Don't forget to add else: raise RuntimeError('Not raised') as well :)
| code = textwrap.dedent("""\ | ||
| import _testcapi | ||
| _testcapi.set_nomemory(0) | ||
| compile("class A[__classdict__]: pass", "<string>", "exec") |
There was a problem hiding this comment.
Can we catch the MemoryError here? And just assert that script works correctly with assert_python_ok?
There was a problem hiding this comment.
Thanks, done, updated the test to catch MemoryError and use assert_python_ok. Imports fixed.
…aram_bound_or_default In symtable_visit_type_param_bound_or_default(), when a reserved name (e.g. __classdict__) is used as a type parameter, PyUnicode_FromFormat() is called to build the SyntaxError message. If the allocation fails and returns NULL, the subsequent PyErr_SetObject() and Py_DECREF() calls would dereference NULL, causing a segfault. Fix by returning 0 immediately when PyUnicode_FromFormat() returns NULL. This propagates the MemoryError set by PyUnicode_FromFormat(). The bug was introduced in pythongh-128632 (commit 891c61c).
sobolevn
added
needs backport to 3.13
|
Thanks @petrvaganoff for the PR, and @sobolevn for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15. |
|
GH-152695 is a backport of this pull request to the 3.15 branch. |
bedevere-app
Bot
removed
the
needs backport to 3.15
|
GH-152696 is a backport of this pull request to the 3.14 branch. |
|
GH-152697 is a backport of this pull request to the 3.13 branch. |
|
Congrats on your first CPython contribution! 🎉 |
| """, "<testcase>", mode="exec") | ||
|
|
||
| @support.nomemtest | ||
| def test_disallowed_type_param_names_oom(self): |
There was a problem hiding this comment.
This test passes for me on main.
There was a problem hiding this comment.
Yes, I also couldn't reproduce it reliably. After investigating, it seems _testcapi.set_nomemory(0) replaces the global CPython allocator with a stub, and there are many allocations happening between the set_nomemory(0) call and PyUnicode_FromFormat inside compile(), so OOM typically hits one of them first. I verified this with a coverage build - the relevant lines are never reached.
I also tried manually injecting an OOM condition in the C code - without the fix it does produce SIGSEGV, confirming the bug is real.
In principle, we could add a fault injection hook in the debug build and expose it via _testinternalcapi. But I think the simpler option is to drop the OOM test entirely - the fix is a straightforward NULL check, and the existing test_disallowed_type_param_names() already covers the code path. What do you think?
3 participants
When a reserved name (e.g.
__classdict__) is used as a type parameter,symtable_visit_type_param_bound_or_default()callsPyUnicode_FromFormat()to build the
SyntaxErrormessage. If the allocation fails and returnsNULL,the subsequent
PyErr_SetObject(PyExc_SyntaxError, NULL)andPy_DECREF(NULL)calls dereference the null pointer, causing a segfault.
Fix by returning
0immediately whenPyUnicode_FromFormat()returnsNULL,which propagates the
MemoryErroralready set byPyUnicode_FromFormat().The bug was introduced in gh-128632 (commit 891c61c).
Found by static analyzer Svace (ISP RAS).
Misc/NEWS.d/test_disallowed_type_param_names_oom)