Repository navigation
gh-161407: Allocate audit hook entries with malloc() - #161409
maikschulze wants to merge 2 commits into
Conversation
A hook added with PySys_AddAuditHook() before Py_Initialize() was allocated with PyMem_RawMalloc() before the startup configuration was applied, and freed with PyMem_RawFree() at finalization. When PYTHONMALLOC=mimalloc, -X dev or a debug allocator changed the raw allocator in between, the entry was freed with a different allocator and the process crashed at exit. Allocate and free the entries with plain malloc() and free(), which nothing replaces at startup, and test _testembed test_audit under the affected settings.
|
All commit authors signed the Contributor License Agreement. |
|
|
||
| _Py_AuditHookEntry *e = (_Py_AuditHookEntry*)PyMem_RawMalloc( | ||
| sizeof(_Py_AuditHookEntry)); | ||
| /* Use malloc(), not PyMem_RawMalloc(): a hook can be added before |
There was a problem hiding this comment.
Honestly, I don't think that it's worth it to bother with _PyMem_RawMalloc() wrapper (and pass NULL as context). I would prefer to use malloc() / free() directly.
The "new" PyInitConfig C API also uses directly malloc() / free() to avoid troubles with changing the memory allocator.
There was a problem hiding this comment.
Done in e5d45db, thanks: plain malloc() / free() now, like PyInitConfig.
| def test_audit(self): | ||
| self.run_embedded_interpreter("test_audit") | ||
|
|
||
| def test_audit_hook_survives_allocator_change(self): |
There was a problem hiding this comment.
I don't think that adding a test is useful, since Python/sysmodule.c no longer depends on the Python memory allocator (PYTHONMALLOC) with this change. I suggest removing the test.
As suggested in review: the _PyMem_RawMalloc() wrapper adds nothing here, and with malloc() the audit hook entries no longer depend on PYTHONMALLOC.
|
Thanks for the review! Also confirmed on a Windows machine with a popular endpoint security agent (which adds an audit hook before |
PySys_AddAuditHook()can be called beforePy_Initialize(), but the hook's list entry was allocated withPyMem_RawMalloc()and freed withPyMem_RawFree()at finalization. When the startup configuration changes the raw allocator in between, the entry is freed with a different allocator than it was allocated with, and the process crashes at exit. That happens withPYTHONMALLOC=mimallocon builds with the GIL since 3.15, and whenever the debug hooks are installed (-X dev, the*_debugallocators).This allocates and frees the entries with plain
malloc()/free(), which nothing at startup replaces.I first used
_PyMem_DefaultRawMalloc()/_PyMem_DefaultRawFree(), like other pre-initialization data. In debug builds those go through the debug hooks, whose underlying allocatorPYTHONMALLOC=mimalloc_debugreplaces, so aPy_DEBUGbuild still crashed.Tested on macOS (release with the GIL, free-threaded,
Py_DEBUG) and Windows (release with and without the GIL):_testembed test_audit, which adds a hook beforePy_Initialize(), under eachPYTHONMALLOCvalue and in development mode, andtest_audit,test_embed,test_sysandtest_capi.test_mem. The full test suite passes on macOS (release build with the GIL). On a Windows machine with a popular endpoint security agent, which adds such a hook to everypython.exe,python -c passcrashed at exit under every setting that changes the raw allocator and now exits cleanly.Affected since 3.13 (debug hooks) and 3.15 (
PYTHONMALLOC=mimalloc).I used an AI assistant to investigate this and draft the patch; I reviewed and tested the changes myself.
Py_Initialize()are freed with the wrong allocator, crashing at exit #161407