Skip to content

Commit 4c8d6f4

Browse files
raminfppicnixzencukou
authored
[3.13] gh-144984: Fix crash in Expat's ExternalEntityParserCreate error paths (GH-144992) (#146142)
* gh-144984: Fix crash in Expat's `ExternalEntityParserCreate` error paths (#144992) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> * gh-144984: Skip test under tracerefs (GH-146218) --------- Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> Co-authored-by: Petr Viktorin <encukou@gmail.com>
1 parent 3038ee9 commit 4c8d6f4

File tree

3 files changed

+52
-13
lines changed

3 files changed

+52
-13
lines changed

Lib/test/test_pyexpat.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -827,6 +827,45 @@ def test_parent_parser_outlives_its_subparsers__chain(self):
827827
del subparser
828828

829829

830+
class ExternalEntityParserCreateErrorTest(unittest.TestCase):
831+
"""ExternalEntityParserCreate error paths should not crash or leak
832+
refcounts on the parent parser.
833+
834+
See https://github.com/python/cpython/issues/144984.
835+
"""
836+
837+
@classmethod
838+
def setUpClass(cls):
839+
cls.testcapi = import_helper.import_module('_testcapi')
840+
841+
@unittest.skipIf(support.Py_TRACE_REFS,
842+
'Py_TRACE_REFS conflicts with testcapi.set_nomemory')
843+
def test_error_path_no_crash(self):
844+
# When an allocation inside ExternalEntityParserCreate fails,
845+
# the partially-initialized subparser is deallocated. This
846+
# must not dereference NULL handlers or double-decrement the
847+
# parent parser's refcount.
848+
parser = expat.ParserCreate()
849+
parser.buffer_text = True
850+
rc_before = sys.getrefcount(parser)
851+
852+
# We avoid self.assertRaises(MemoryError) here because the
853+
# context manager itself needs memory allocations that fail
854+
# while the nomemory hook is active.
855+
self.testcapi.set_nomemory(1, 10)
856+
raised = False
857+
try:
858+
parser.ExternalEntityParserCreate(None)
859+
except MemoryError:
860+
raised = True
861+
finally:
862+
self.testcapi.remove_mem_hooks()
863+
self.assertTrue(raised, "MemoryError not raised")
864+
865+
rc_after = sys.getrefcount(parser)
866+
self.assertEqual(rc_after, rc_before)
867+
868+
830869
class ReparseDeferralTest(unittest.TestCase):
831870
def test_getter_setter_round_trip(self):
832871
parser = expat.ParserCreate()
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fix crash in :meth:`xml.parsers.expat.xmlparser.ExternalEntityParserCreate`
2+
when an allocation fails. The error paths could dereference NULL ``handlers``
3+
and double-decrement the parent parser's reference count.

Modules/pyexpat.c

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1045,11 +1045,6 @@ pyexpat_xmlparser_ExternalEntityParserCreate_impl(xmlparseobject *self,
10451045
return NULL;
10461046
}
10471047

1048-
// The new subparser will make use of the parent XML_Parser inside of Expat.
1049-
// So we need to take subparsers into account with the reference counting
1050-
// of their parent parser.
1051-
Py_INCREF(self);
1052-
10531048
new_parser->buffer_size = self->buffer_size;
10541049
new_parser->buffer_used = 0;
10551050
new_parser->buffer = NULL;
@@ -1059,21 +1054,22 @@ pyexpat_xmlparser_ExternalEntityParserCreate_impl(xmlparseobject *self,
10591054
new_parser->ns_prefixes = self->ns_prefixes;
10601055
new_parser->itself = XML_ExternalEntityParserCreate(self->itself, context,
10611056
encoding);
1062-
new_parser->parent = (PyObject *)self;
1057+
// The new subparser will make use of the parent XML_Parser inside of Expat.
1058+
// So we need to take subparsers into account with the reference counting
1059+
// of their parent parser.
1060+
new_parser->parent = Py_NewRef(self);
10631061
new_parser->handlers = 0;
10641062
new_parser->intern = Py_XNewRef(self->intern);
10651063

10661064
if (self->buffer != NULL) {
10671065
new_parser->buffer = PyMem_Malloc(new_parser->buffer_size);
10681066
if (new_parser->buffer == NULL) {
10691067
Py_DECREF(new_parser);
1070-
Py_DECREF(self);
10711068
return PyErr_NoMemory();
10721069
}
10731070
}
10741071
if (!new_parser->itself) {
10751072
Py_DECREF(new_parser);
1076-
Py_DECREF(self);
10771073
return PyErr_NoMemory();
10781074
}
10791075

@@ -1086,7 +1082,6 @@ pyexpat_xmlparser_ExternalEntityParserCreate_impl(xmlparseobject *self,
10861082
new_parser->handlers = PyMem_New(PyObject *, i);
10871083
if (!new_parser->handlers) {
10881084
Py_DECREF(new_parser);
1089-
Py_DECREF(self);
10901085
return PyErr_NoMemory();
10911086
}
10921087
clear_handlers(new_parser, 1);
@@ -2333,11 +2328,13 @@ PyInit_pyexpat(void)
23332328
static void
23342329
clear_handlers(xmlparseobject *self, int initial)
23352330
{
2336-
int i = 0;
2337-
2338-
for (; handler_info[i].name != NULL; i++) {
2339-
if (initial)
2331+
if (self->handlers == NULL) {
2332+
return;
2333+
}
2334+
for (size_t i = 0; handler_info[i].name != NULL; i++) {
2335+
if (initial) {
23402336
self->handlers[i] = NULL;
2337+
}
23412338
else {
23422339
Py_CLEAR(self->handlers[i]);
23432340
handler_info[i].setter(self->itself, NULL);

0 commit comments

Comments
 (0)