gh-161060: Fix pyexpat crash when the default handler is cleared

Lib/test/test_pyexpat.py

@@ -834,6 +834,103 @@ def handler(text):
 
         self.assertRaises(SpecificException, parser.Parse, xml.encode('iso8859'))
 
+
+class DefaultHandlerExceptionTest(unittest.TestCase):
+    # In a document that is not UTF-8, Expat passes a long piece of text such
+    # as this comment to the default handler in chunks of at most 1024 bytes,
+    # re-reading the handler between chunks.  Clearing the handler from within
+    # the callback, by raising or by setting it to None, must not crash.
+    xml = ("<?xml version='1.0' encoding='iso8859'?><s><!--%s--></s>"
+           % ('a' * 2000)).encode('iso8859')
+
+    class SpecificException(Exception):
+        pass
+
+    def check(self, name):
+        def handler(text):
+            if text.startswith('<!--'):
+                raise self.SpecificException
+
+        parser = expat.ParserCreate()
+        setattr(parser, name, handler)
+        self.assertRaises(self.SpecificException, parser.Parse, self.xml, True)
+
+    def test_DefaultHandler(self):
+        self.check('DefaultHandler')
+
+    def test_DefaultHandlerExpand(self):
+        self.check('DefaultHandlerExpand')
+
+    def check_unset_in_callback(self, name):
+        parser = expat.ParserCreate()
+        calls = []
+        def handler(text):
+            calls.append(text)
+            if text.startswith('<!--'):
+                setattr(parser, name, None)
+        setattr(parser, name, handler)
+        parser.Parse(self.xml, True)
+        self.assertTrue(calls[-1].startswith('<!--'), calls)
+
+    def test_unset_DefaultHandler_in_callback(self):
+        self.check_unset_in_callback('DefaultHandler')
+
+    def test_unset_DefaultHandlerExpand_in_callback(self):
+        self.check_unset_in_callback('DefaultHandlerExpand')
+
+    def check_unset_in_result_destructor(self, name):
+        # The handler's result is released after the handler has returned,
+        # but while Expat is still passing the comment to the default handler.
+        parser = expat.ParserCreate()
+        class Result:
+            def __del__(self):
+                setattr(parser, name, None)
+        calls = []
+        def handler(text):
+            calls.append(text)
+            if text.startswith('<!--'):
+                return Result()
+        setattr(parser, name, handler)
+        parser.Parse(self.xml, True)
+        self.assertTrue(calls[-1].startswith('<!--'), calls)
+
+    def test_unset_DefaultHandler_in_result_destructor(self):
+        self.check_unset_in_result_destructor('DefaultHandler')
+
+    def test_unset_DefaultHandlerExpand_in_result_destructor(self):
+        self.check_unset_in_result_destructor('DefaultHandlerExpand')
+
+    def test_unset_DefaultHandler_in_CharacterDataHandler_result(self):
+        # With buffer_text, the CharacterDataHandler is called, and its
+        # result released, just before the default handler is called.
+        parser = expat.ParserCreate()
+        class Result:
+            def __del__(self):
+                parser.DefaultHandler = None
+        parser.buffer_text = True
+        parser.CharacterDataHandler = lambda text: Result()
+        parser.DefaultHandler = lambda text: None
+        xml = ("<?xml version='1.0' encoding='iso8859'?><s>a<!--%s--></s>"
+               % ('a' * 2000)).encode('iso8859')
+        parser.Parse(xml, True)
+
+    def test_CharacterDataHandler_with_buffer_text(self):
+        # With buffer_text, buffered character data is passed to the
+        # CharacterDataHandler just before the default handler is called;
+        # an exception there clears the handlers while Expat is still
+        # passing the comment to the default handler.
+        def handler(text):
+            raise self.SpecificException
+
+        parser = expat.ParserCreate()
+        parser.buffer_text = True
+        parser.CharacterDataHandler = handler
+        parser.DefaultHandler = lambda text: None
+        xml = ("<?xml version='1.0' encoding='iso8859'?><s>a<!--%s--></s>"
+               % ('a' * 2000)).encode('iso8859')
+        self.assertRaises(self.SpecificException, parser.Parse, xml, True)
+
+
 class ChardataBufferTest(unittest.TestCase):
     """
     test setting of chardata buffer size

Misc/NEWS.d/next/Library/2026-11-01-05-05-38.gh-issue-161060.YOZsVl.rst

@@ -0,0 +1,5 @@
+Fix a crash in :mod:`xml.parsers.expat` when a handler raises an exception,
+or the :meth:`~xml.parsers.expat.xmlparser.DefaultHandler` or
+:meth:`~xml.parsers.expat.xmlparser.DefaultHandlerExpand` handler is unset,
+while a token longer than 1024 bytes, such as a comment, is being reported to
+the default handler from a document that is not encoded in UTF-8.

Modules/pyexpat.c

@@ -293,10 +293,13 @@ error_external_entity_ref_handler(XML_Parser parser,
 
 /* Dummy character data handler used when an error (exception) has
    been detected, and the actual parsing can be terminated early.
-   This is needed since character data handler can't be safely removed
-   from within the character data handler, but can be replaced.  It is
-   used only from the character data handler trampoline, and must be
-   used right after `flag_error()` is called. */
+   This is needed since the character data and default handlers can't be
+   safely removed from within a callback, only replaced: Expat may call
+   them several times for one piece of text (a long comment in a document
+   that is not UTF-8, for example).  The default handler has the same
+   signature, so this function serves for both.  clear_handlers() installs
+   it after `flag_error()`, and the handler setter installs it when a
+   handler is set to None from within a callback. */
 static void
 noop_character_data_handler(void *userData, const XML_Char *data, int len)
 {
@@ -382,15 +385,18 @@ call_character_handler(xmlparseobject *self, const XML_Char *buffer, int len)
     temp = call_with_frame("CharacterData", __LINE__,
                            self->handlers[CharacterData], args, self);
     /* temp is an owned reference again, or NULL */
-    self->in_callback = 0;
     Py_DECREF(args);
     if (temp == NULL) {
+        self->in_callback = 0;
         flag_error(self);
         XML_SetCharacterDataHandler(self->itself,
                                     noop_character_data_handler);
         return -1;
     }
+    /* Releasing the result can run arbitrary code, which must still be
+       treated as being within the callback by the handler setter. */
     Py_DECREF(temp);
+    self->in_callback = 0;
     return 0;
 }
 
@@ -579,14 +585,18 @@ my_ ## NAME ## Handler PARAMS {                     \
     PyObject *rv = call_with_frame(                 \
         #NAME, __LINE__,                            \
         self->handlers[NAME], args, self);          \
-    self->in_callback = 0;                          \
     Py_DECREF(args);                                \
     if (rv == NULL) {                               \
+        self->in_callback = 0;                      \
         flag_error(self);                           \
         return RETURN_VARIABLE;                     \
     }                                               \
     CONVERSION                                      \
+    /* Releasing the result can run arbitrary code, \
+       which must still be treated as being within  \
+       the callback by the handler setter. */       \
     Py_DECREF(rv);                                  \
+    self->in_callback = 0;                          \
     if (PyErr_Occurred()) {                         \
         invalid_expat_handler_rv(#NAME);            \
     }                                               \
@@ -1838,8 +1848,14 @@ xmlparse_handler_setter(PyObject *op, PyObject *v, void *closure)
            active, so Expat does a little more work than it
            otherwise would, but that's really an odd case.  A more
            elaborate system of handlers and state could remove the
-           C handler more effectively. */
-        if (handlernum == CharacterData && self->in_callback) {
+           C handler more effectively.  The same applies to the
+           default handler, which Expat can call several times for one
+           piece of text, such as a long comment in a document that is
+           not UTF-8. */
+        if (self->in_callback
+            && (handlernum == CharacterData
+                || handlernum == Default
+                || handlernum == DefaultHandlerExpand)) {
             c_handler = (xmlhandler)noop_character_data_handler;
         }
         v = NULL;
@@ -2786,7 +2802,17 @@ clear_handlers(xmlparseobject *self, int initial)
         }
         else {
             Py_CLEAR(self->handlers[i]);
-            CALL_XML_HANDLER_SETTER(&handler_info[i], self->itself, NULL);
+            xmlhandler c_handler = NULL;
+            if (i == Default || i == DefaultHandlerExpand) {
+                /* This may be called from within a callback, where the
+                   default handler can't be removed: for a document that is
+                   not UTF-8, Expat passes a long piece of text to it in
+                   several calls and re-reads the handler before each one
+                   (reportDefault() in xmlparse.c).  The no-op handler is
+                   harmless otherwise. */
+                c_handler = (xmlhandler)noop_character_data_handler;
+            }
+            CALL_XML_HANDLER_SETTER(&handler_info[i], self->itself, c_handler);
         }
     }
 }