diff --git a/Lib/test/test_pyexpat.py b/Lib/test/test_pyexpat.py index 74a75458289b4d..6d7099750c13cb 100644 --- a/Lib/test/test_pyexpat.py +++ b/Lib/test/test_pyexpat.py @@ -824,6 +824,43 @@ def test_parent_parser_outlives_its_subparsers__chain(self): del subparser +class ExternalEntityParserCreateErrorTest(unittest.TestCase): + """ExternalEntityParserCreate error paths should not crash or leak + refcounts on the parent parser. + + See https://github.com/python/cpython/issues/144984. + """ + + @classmethod + def setUpClass(cls): + cls.testcapi = import_helper.import_module('_testcapi') + + def test_error_path_no_crash(self): + # When an allocation inside ExternalEntityParserCreate fails, + # the partially-initialized subparser is deallocated. This + # must not dereference NULL handlers or double-decrement the + # parent parser's refcount. + parser = expat.ParserCreate() + parser.buffer_text = True + rc_before = sys.getrefcount(parser) + + # We avoid self.assertRaises(MemoryError) here because the + # context manager itself needs memory allocations that fail + # while the nomemory hook is active. + self.testcapi.set_nomemory(1, 10) + raised = False + try: + parser.ExternalEntityParserCreate(None) + except MemoryError: + raised = True + finally: + self.testcapi.remove_mem_hooks() + self.assertTrue(raised, "MemoryError not raised") + + rc_after = sys.getrefcount(parser) + self.assertEqual(rc_after, rc_before) + + class ReparseDeferralTest(unittest.TestCase): def test_getter_setter_round_trip(self): parser = expat.ParserCreate() diff --git a/Misc/NEWS.d/next/Library/2026-02-19-12-00-00.gh-issue-144984.b93995c982.rst b/Misc/NEWS.d/next/Library/2026-02-19-12-00-00.gh-issue-144984.b93995c982.rst new file mode 100644 index 00000000000000..66e07dc3098c5f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-02-19-12-00-00.gh-issue-144984.b93995c982.rst @@ -0,0 +1,3 @@ +Fix crash in :meth:`xml.parsers.expat.xmlparser.ExternalEntityParserCreate` +when an allocation fails. The error paths could dereference NULL ``handlers`` +and double-decrement the parent parser's reference count. diff --git a/Modules/pyexpat.c b/Modules/pyexpat.c index e9255038eee5b5..a1fa51514afc85 100644 --- a/Modules/pyexpat.c +++ b/Modules/pyexpat.c @@ -1076,11 +1076,6 @@ pyexpat_xmlparser_ExternalEntityParserCreate_impl(xmlparseobject *self, return NULL; } - // The new subparser will make use of the parent XML_Parser inside of Expat. - // So we need to take subparsers into account with the reference counting - // of their parent parser. - Py_INCREF(self); - new_parser->buffer_size = self->buffer_size; new_parser->buffer_used = 0; new_parser->buffer = NULL; @@ -1090,7 +1085,10 @@ pyexpat_xmlparser_ExternalEntityParserCreate_impl(xmlparseobject *self, new_parser->ns_prefixes = self->ns_prefixes; new_parser->itself = XML_ExternalEntityParserCreate(self->itself, context, encoding); - new_parser->parent = (PyObject *)self; + // The new subparser will make use of the parent XML_Parser inside of Expat. + // So we need to take subparsers into account with the reference counting + // of their parent parser. + new_parser->parent = Py_NewRef(self); new_parser->handlers = 0; new_parser->intern = Py_XNewRef(self->intern); @@ -1098,13 +1096,11 @@ pyexpat_xmlparser_ExternalEntityParserCreate_impl(xmlparseobject *self, new_parser->buffer = PyMem_Malloc(new_parser->buffer_size); if (new_parser->buffer == NULL) { Py_DECREF(new_parser); - Py_DECREF(self); return PyErr_NoMemory(); } } if (!new_parser->itself) { Py_DECREF(new_parser); - Py_DECREF(self); return PyErr_NoMemory(); } @@ -1118,7 +1114,6 @@ pyexpat_xmlparser_ExternalEntityParserCreate_impl(xmlparseobject *self, new_parser->handlers = PyMem_New(PyObject *, i); if (!new_parser->handlers) { Py_DECREF(new_parser); - Py_DECREF(self); return PyErr_NoMemory(); } clear_handlers(new_parser, 1); @@ -2489,6 +2484,9 @@ PyInit_pyexpat(void) static void clear_handlers(xmlparseobject *self, int initial) { + if (self->handlers == NULL) { + return; + } for (size_t i = 0; handler_info[i].name != NULL; i++) { if (initial) { self->handlers[i] = NULL;