gh-144984: Fix crash in ExternalEntityParserCreate() error paths#144992
gh-144984: Fix crash in ExternalEntityParserCreate() error paths#144992raminfp wants to merge 2 commits intopython:mainfrom
Conversation
When ExternalEntityParserCreate() hits an error path (allocation failure), Py_DECREF(new_parser) triggers xmlparse_dealloc() on a partially-initialized object: 1. handlers is NULL, so clear_handlers dereferences NULL (SEGV). 2. Py_CLEAR(parent) in dealloc already decrements the parent's refcount, so the explicit Py_DECREF(self) is a double-decrement. Fix by adding a NULL guard in clear_handlers and setting parent to NULL before Py_DECREF(new_parser) in each error path so that dealloc does not over-decrement the parent's refcount.
Lib/test/test_pyexpat.py
Outdated
| _testcapi = import_helper.import_module('_testcapi') | ||
| parser = expat.ParserCreate() | ||
| parser.buffer_text = True | ||
| rc_before = sys.getrefcount(parser) |
There was a problem hiding this comment.
I suspect we'll need _testcapi later for this class (if we find other bugs) so add a setupClass method that binds cls.testcapi so that it can be called inthe methods.
| try: | ||
| parser.ExternalEntityParserCreate(None) | ||
| except MemoryError: | ||
| pass | ||
| finally: |
There was a problem hiding this comment.
Explcitily catch the MemoryError.
There was a problem hiding this comment.
The assertRaises(MemoryError) context manager doesn't work here because set_nomemory is still active and
assertRaises itself needs allocations internally, which causes it to crash. That's why I used an explicit
try/except MemoryError with assertTrue(raised). Is this approach acceptable or do you prefer a different
pattern?
Modules/pyexpat.c
Outdated
| if (self->buffer != NULL) { | ||
| new_parser->buffer = PyMem_Malloc(new_parser->buffer_size); | ||
| if (new_parser->buffer == NULL) { | ||
| new_parser->parent = NULL; |
There was a problem hiding this comment.
Actually the issue isn't here, it's the decref on self. Instead, let's do new_parser->parent = Py_NewRef(self) and let the new parser's deallocator be responsible for decrefing this reference instead of us doing the Py_DECREF(self)
2 participants
Fix crash when
ExternalEntityParserCreate()hits an error path(allocation failure).
Py_DECREF(new_parser)callsxmlparse_dealloc()on a partially-initialized object where
handlersis NULL, causing aNULL pointer dereference in
clear_handlers. Additionally,Py_CLEAR(parent)in dealloc already decrements the parent's refcount,making the subsequent
Py_DECREF(self)a double-decrement.clear_handlersparent = NULLbeforePy_DECREF(new_parser)in each error path