Skip to content

Commit 5354cef

Browse files
committed
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.
1 parent e5f1477 commit 5354cef

3 files changed

Lines changed: 77 additions & 5 deletions

File tree

‎Lib/test/test_pyexpat.py‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -834,6 +834,58 @@ def handler(text):
834834

835835
self.assertRaises(SpecificException, parser.Parse, xml.encode('iso8859'))
836836

837+
838+
class DefaultHandlerExceptionTest(unittest.TestCase):
839+
# Expat delivers a token that needs encoding conversion to the default
840+
# handler in pieces of at most 1024 bytes, re-reading the handler in
841+
# between. Removing the handler from within the callback must not crash.
842+
xml = ("<?xml version='1.0' encoding='iso8859'?><s><!--%s--></s>"
843+
% ('a' * 2000)).encode('iso8859')
844+
845+
class SpecificException(Exception):
846+
pass
847+
848+
def check(self, name):
849+
def handler(text):
850+
if text.startswith('<!--'):
851+
raise self.SpecificException
852+
853+
parser = expat.ParserCreate()
854+
setattr(parser, name, handler)
855+
self.assertRaises(self.SpecificException, parser.Parse, self.xml, True)
856+
857+
def test_DefaultHandler(self):
858+
self.check('DefaultHandler')
859+
860+
def test_DefaultHandlerExpand(self):
861+
self.check('DefaultHandlerExpand')
862+
863+
def test_unset_DefaultHandler_in_callback(self):
864+
parser = expat.ParserCreate()
865+
calls = []
866+
def handler(text):
867+
calls.append(text)
868+
if text.startswith('<!--'):
869+
parser.DefaultHandler = None
870+
parser.DefaultHandler = handler
871+
parser.Parse(self.xml, True)
872+
self.assertTrue(calls[-1].startswith('<!--'), calls)
873+
874+
def test_CharacterDataHandler_with_buffer_text(self):
875+
# An exception in the CharacterDataHandler while the default
876+
# handler trampoline flushes the text buffer.
877+
def handler(text):
878+
raise self.SpecificException
879+
880+
parser = expat.ParserCreate()
881+
parser.buffer_text = True
882+
parser.CharacterDataHandler = handler
883+
parser.DefaultHandler = lambda text: None
884+
xml = ("<?xml version='1.0' encoding='iso8859'?><s>a<!--%s--></s>"
885+
% ('a' * 2000)).encode('iso8859')
886+
self.assertRaises(self.SpecificException, parser.Parse, xml, True)
887+
888+
837889
class ChardataBufferTest(unittest.TestCase):
838890
"""
839891
test setting of chardata buffer size
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Fix a crash in :mod:`xml.parsers.expat` when the
2+
:meth:`~xml.parsers.expat.xmlparser.DefaultHandler` or
3+
:meth:`~xml.parsers.expat.xmlparser.DefaultHandlerExpand` callback raises an
4+
exception or unsets the handler while a token longer than 1024 bytes, such as
5+
a comment, is being reported from a document that is not encoded in UTF-8.

‎Modules/pyexpat.c‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -295,8 +295,10 @@ error_external_entity_ref_handler(XML_Parser parser,
295295
been detected, and the actual parsing can be terminated early.
296296
This is needed since character data handler can't be safely removed
297297
from within the character data handler, but can be replaced. It is
298-
used only from the character data handler trampoline, and must be
299-
used right after `flag_error()` is called. */
298+
used from the character data handler trampoline, and must be
299+
used right after `flag_error()` is called. It is also installed as
300+
the default handler (same signature) by clear_handlers(), since Expat
301+
can't cope with that one being removed from within a callback either. */
300302
static void
301303
noop_character_data_handler(void *userData, const XML_Char *data, int len)
302304
{
@@ -1838,8 +1840,13 @@ xmlparse_handler_setter(PyObject *op, PyObject *v, void *closure)
18381840
active, so Expat does a little more work than it
18391841
otherwise would, but that's really an odd case. A more
18401842
elaborate system of handlers and state could remove the
1841-
C handler more effectively. */
1842-
if (handlernum == CharacterData && self->in_callback) {
1843+
C handler more effectively. The same applies to the
1844+
default handler, which Expat calls repeatedly for a single
1845+
token that needs encoding conversion. */
1846+
if (self->in_callback
1847+
&& (handlernum == CharacterData
1848+
|| handlernum == Default
1849+
|| handlernum == DefaultHandlerExpand)) {
18431850
c_handler = (xmlhandler)noop_character_data_handler;
18441851
}
18451852
v = NULL;
@@ -2786,7 +2793,15 @@ clear_handlers(xmlparseobject *self, int initial)
27862793
}
27872794
else {
27882795
Py_CLEAR(self->handlers[i]);
2789-
CALL_XML_HANDLER_SETTER(&handler_info[i], self->itself, NULL);
2796+
xmlhandler c_handler = NULL;
2797+
if (i == Default || i == DefaultHandlerExpand) {
2798+
/* Like the character data handler, the default handler
2799+
can't be safely removed from within a callback: Expat's
2800+
reportDefault() re-reads it between the pieces of a
2801+
converted token. Replace it with a no-op instead. */
2802+
c_handler = (xmlhandler)noop_character_data_handler;
2803+
}
2804+
CALL_XML_HANDLER_SETTER(&handler_info[i], self->itself, c_handler);
27902805
}
27912806
}
27922807
}

0 commit comments

Comments
 (0)