From 5354cef1c8c8ec4d973ca3f24c1fca28a7dd7012 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 1 Nov 2026 05:06:00 +0000 Subject: [PATCH 1/3] gh-161060: Fix pyexpat crash when the default handler is cleared Expat calls the default handler once per 1024-byte piece of a token that needs encoding conversion. Install a no-op instead of NULL when the handler is cleared after an error or unset from within a callback, as is already done for the character data handler. --- Lib/test/test_pyexpat.py | 52 +++++++++++++++++++ ...-11-01-05-05-38.gh-issue-161060.YOZsVl.rst | 5 ++ Modules/pyexpat.c | 25 +++++++-- 3 files changed, 77 insertions(+), 5 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2026-11-01-05-05-38.gh-issue-161060.YOZsVl.rst diff --git a/Lib/test/test_pyexpat.py b/Lib/test/test_pyexpat.py index c52c640bd4b26f8..00da8a6e1828297 100644 --- a/Lib/test/test_pyexpat.py +++ b/Lib/test/test_pyexpat.py @@ -834,6 +834,58 @@ def handler(text): self.assertRaises(SpecificException, parser.Parse, xml.encode('iso8859')) + +class DefaultHandlerExceptionTest(unittest.TestCase): + # Expat delivers a token that needs encoding conversion to the default + # handler in pieces of at most 1024 bytes, re-reading the handler in + # between. Removing the handler from within the callback must not crash. + xml = ("" + % ('a' * 2000)).encode('iso8859') + + class SpecificException(Exception): + pass + + def check(self, name): + def handler(text): + if text.startswith('" + % ('a' * 2000)).encode('iso8859') + self.assertRaises(self.SpecificException, parser.Parse, xml, True) + + class ChardataBufferTest(unittest.TestCase): """ test setting of chardata buffer size diff --git a/Misc/NEWS.d/next/Library/2026-11-01-05-05-38.gh-issue-161060.YOZsVl.rst b/Misc/NEWS.d/next/Library/2026-11-01-05-05-38.gh-issue-161060.YOZsVl.rst new file mode 100644 index 000000000000000..487d2552ce0c873 --- /dev/null +++ b/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 the +:meth:`~xml.parsers.expat.xmlparser.DefaultHandler` or +:meth:`~xml.parsers.expat.xmlparser.DefaultHandlerExpand` callback raises an +exception or unsets the handler while a token longer than 1024 bytes, such as +a comment, is being reported from a document that is not encoded in UTF-8. diff --git a/Modules/pyexpat.c b/Modules/pyexpat.c index fb9d333babf70b4..01d80c74805c658 100644 --- a/Modules/pyexpat.c +++ b/Modules/pyexpat.c @@ -295,8 +295,10 @@ error_external_entity_ref_handler(XML_Parser parser, 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. */ + used from the character data handler trampoline, and must be + used right after `flag_error()` is called. It is also installed as + the default handler (same signature) by clear_handlers(), since Expat + can't cope with that one being removed from within a callback either. */ static void noop_character_data_handler(void *userData, const XML_Char *data, int len) { @@ -1838,8 +1840,13 @@ 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 calls repeatedly for a single + token that needs encoding conversion. */ + if (self->in_callback + && (handlernum == CharacterData + || handlernum == Default + || handlernum == DefaultHandlerExpand)) { c_handler = (xmlhandler)noop_character_data_handler; } v = NULL; @@ -2786,7 +2793,15 @@ 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) { + /* Like the character data handler, the default handler + can't be safely removed from within a callback: Expat's + reportDefault() re-reads it between the pieces of a + converted token. Replace it with a no-op instead. */ + c_handler = (xmlhandler)noop_character_data_handler; + } + CALL_XML_HANDLER_SETTER(&handler_info[i], self->itself, c_handler); } } } From 2d0f5c6198bb6787fdd64b64af0efa00ea18f375 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 1 Nov 2026 05:55:37 +0000 Subject: [PATCH 2/3] Reword comments about replacing handlers within a callback --- Lib/test/test_pyexpat.py | 13 ++++++++----- Modules/pyexpat.c | 25 ++++++++++++++----------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/Lib/test/test_pyexpat.py b/Lib/test/test_pyexpat.py index 00da8a6e1828297..ef2334c9c450c13 100644 --- a/Lib/test/test_pyexpat.py +++ b/Lib/test/test_pyexpat.py @@ -836,9 +836,10 @@ def handler(text): class DefaultHandlerExceptionTest(unittest.TestCase): - # Expat delivers a token that needs encoding conversion to the default - # handler in pieces of at most 1024 bytes, re-reading the handler in - # between. Removing the handler from within the callback must not crash. + # 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 = ("" % ('a' * 2000)).encode('iso8859') @@ -872,8 +873,10 @@ def handler(text): self.assertTrue(calls[-1].startswith('" + % ('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; diff --git a/Misc/NEWS.d/next/Library/2026-11-01-05-05-38.gh-issue-161060.YOZsVl.rst b/Misc/NEWS.d/next/Library/2026-11-01-05-05-38.gh-issue-161060.YOZsVl.rst index 487d2552ce0c873..8d55e0162c452ed 100644 --- a/Misc/NEWS.d/next/Library/2026-11-01-05-05-38.gh-issue-161060.YOZsVl.rst +++ b/Misc/NEWS.d/next/Library/2026-11-01-05-05-38.gh-issue-161060.YOZsVl.rst @@ -1,5 +1,5 @@ -Fix a crash in :mod:`xml.parsers.expat` when the -:meth:`~xml.parsers.expat.xmlparser.DefaultHandler` or -:meth:`~xml.parsers.expat.xmlparser.DefaultHandlerExpand` callback raises an -exception or unsets the handler while a token longer than 1024 bytes, such as -a comment, is being reported from a document that is not encoded in UTF-8. +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. diff --git a/Modules/pyexpat.c b/Modules/pyexpat.c index 7c633b3fea5e766..a6063f034463c82 100644 --- a/Modules/pyexpat.c +++ b/Modules/pyexpat.c @@ -385,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; } @@ -582,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); \ } \ @@ -2797,11 +2804,12 @@ clear_handlers(xmlparseobject *self, int initial) Py_CLEAR(self->handlers[i]); xmlhandler c_handler = NULL; if (i == Default || i == DefaultHandlerExpand) { - /* Like the character data handler, the default handler - can't be removed from within a callback: 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). */ + /* 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);