From 2833a881a4d9de36f020297f5e4263b8642b8aad Mon Sep 17 00:00:00 2001 From: WYSIATI Date: Sun, 12 Apr 2026 18:59:18 +0800 Subject: [PATCH 1/5] gh-148427: Fix bare except in expatreader.external_entity_ref() Change bare `except:` to `except Exception:` so that KeyboardInterrupt and SystemExit propagate instead of being silently swallowed during external entity parsing. Move entity stack cleanup into a `finally` clause so the parser state is restored on both success and error paths. --- Lib/test/test_sax.py | 59 +++++++++++++++++++ Lib/xml/sax/expatreader.py | 10 ++-- ...-04-12-09-52-14.gh-issue-148427.xR7v3m.rst | 4 ++ 3 files changed, 68 insertions(+), 5 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2026-04-12-09-52-14.gh-issue-148427.xR7v3m.rst diff --git a/Lib/test/test_sax.py b/Lib/test/test_sax.py index 29babd7bf6996a..befa405b804ace 100644 --- a/Lib/test/test_sax.py +++ b/Lib/test/test_sax.py @@ -1055,6 +1055,65 @@ def test_expat_entityresolver_default(self): self.assertEqual(result.getvalue(), start + b"") + def test_external_entity_ref_keyboard_interrupt(self): + # gh-148427: KeyboardInterrupt must propagate, not be swallowed + class KBHandler(handler.ContentHandler): + def startElement(self, name, attrs): + if name == 'entity': + raise KeyboardInterrupt('test') + + parser = create_parser() + parser.setFeature(feature_external_ges, True) + parser.setEntityResolver(self.TestEntityResolver()) + parser.setContentHandler(KBHandler()) + + with self.assertRaises(KeyboardInterrupt): + parser.feed('\n') + parser.feed(']>\n') + parser.feed('&test;') + parser.close() + + def test_external_entity_ref_system_exit(self): + # gh-148427: SystemExit must propagate, not be swallowed + class ExitHandler(handler.ContentHandler): + def startElement(self, name, attrs): + if name == 'entity': + raise SystemExit(42) + + parser = create_parser() + parser.setFeature(feature_external_ges, True) + parser.setEntityResolver(self.TestEntityResolver()) + parser.setContentHandler(ExitHandler()) + + with self.assertRaises(SystemExit): + parser.feed('\n') + parser.feed(']>\n') + parser.feed('&test;') + parser.close() + + def test_external_entity_ref_stack_cleanup(self): + # gh-148427: _entity_stack must be cleaned up after errors + class ErrorHandler(handler.ContentHandler): + def startElement(self, name, attrs): + if name == 'entity': + raise ValueError('test error') + + parser = create_parser() + parser.setFeature(feature_external_ges, True) + parser.setEntityResolver(self.TestEntityResolver()) + parser.setContentHandler(ErrorHandler()) + + with self.assertRaises(SAXParseException): + parser.feed('\n') + parser.feed(']>\n') + parser.feed('&test;') + parser.close() + + self.assertEqual(len(parser._entity_stack), 0) + # ===== Attributes support class AttrGatherer(ContentHandler): diff --git a/Lib/xml/sax/expatreader.py b/Lib/xml/sax/expatreader.py index 37b1add2848487..8c1f54d2b77c74 100644 --- a/Lib/xml/sax/expatreader.py +++ b/Lib/xml/sax/expatreader.py @@ -424,11 +424,11 @@ def external_entity_ref(self, context, base, sysid, pubid): try: xmlreader.IncrementalParser.parse(self, source) - except: - return 0 # FIXME: save error info here? - - (self._parser, self._source) = self._entity_stack[-1] - del self._entity_stack[-1] + except Exception: + return 0 + finally: + (self._parser, self._source) = self._entity_stack[-1] + del self._entity_stack[-1] return 1 def skipped_entity_handler(self, name, is_pe): diff --git a/Misc/NEWS.d/next/Library/2026-04-12-09-52-14.gh-issue-148427.xR7v3m.rst b/Misc/NEWS.d/next/Library/2026-04-12-09-52-14.gh-issue-148427.xR7v3m.rst new file mode 100644 index 00000000000000..9b2e55a0d338b6 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-04-12-09-52-14.gh-issue-148427.xR7v3m.rst @@ -0,0 +1,4 @@ +Fix bare ``except`` clause in :mod:`xml.sax` expatreader's external entity +handler that silently swallowed :exc:`KeyboardInterrupt` and +:exc:`SystemExit`. Also fix entity parser stack leak on errors by moving +cleanup into a ``finally`` clause. From 26db8edd8608841074c158010d008930f00a11fd Mon Sep 17 00:00:00 2001 From: WYSIATI Date: Sun, 12 Apr 2026 20:04:01 +0800 Subject: [PATCH 2/5] gh-148427: Address review feedback - Keep FIXME comment about error info - Use mock.Mock with startElement.side_effect instead of inner classes - Narrow assertRaises to only the triggering feed() call - Update NEWS wording per reviewer suggestion --- Lib/test/test_sax.py | 45 ++++++++----------- Lib/xml/sax/expatreader.py | 2 +- ...-04-12-09-52-14.gh-issue-148427.xR7v3m.rst | 7 ++- 3 files changed, 22 insertions(+), 32 deletions(-) diff --git a/Lib/test/test_sax.py b/Lib/test/test_sax.py index befa405b804ace..43e858f05ed301 100644 --- a/Lib/test/test_sax.py +++ b/Lib/test/test_sax.py @@ -1057,60 +1057,51 @@ def test_expat_entityresolver_default(self): def test_external_entity_ref_keyboard_interrupt(self): # gh-148427: KeyboardInterrupt must propagate, not be swallowed - class KBHandler(handler.ContentHandler): - def startElement(self, name, attrs): - if name == 'entity': - raise KeyboardInterrupt('test') + eh = mock.Mock() + eh.startElement.side_effect = KeyboardInterrupt('test') parser = create_parser() parser.setFeature(feature_external_ges, True) parser.setEntityResolver(self.TestEntityResolver()) - parser.setContentHandler(KBHandler()) + parser.setContentHandler(eh) + parser.feed('\n') + parser.feed(']>\n') with self.assertRaises(KeyboardInterrupt): - parser.feed('\n') - parser.feed(']>\n') parser.feed('&test;') - parser.close() def test_external_entity_ref_system_exit(self): # gh-148427: SystemExit must propagate, not be swallowed - class ExitHandler(handler.ContentHandler): - def startElement(self, name, attrs): - if name == 'entity': - raise SystemExit(42) + eh = mock.Mock() + eh.startElement.side_effect = SystemExit(42) parser = create_parser() parser.setFeature(feature_external_ges, True) parser.setEntityResolver(self.TestEntityResolver()) - parser.setContentHandler(ExitHandler()) + parser.setContentHandler(eh) + parser.feed('\n') + parser.feed(']>\n') with self.assertRaises(SystemExit): - parser.feed('\n') - parser.feed(']>\n') parser.feed('&test;') - parser.close() def test_external_entity_ref_stack_cleanup(self): # gh-148427: _entity_stack must be cleaned up after errors - class ErrorHandler(handler.ContentHandler): - def startElement(self, name, attrs): - if name == 'entity': - raise ValueError('test error') + eh = mock.Mock() + eh.startElement.side_effect = ValueError('test error') parser = create_parser() parser.setFeature(feature_external_ges, True) parser.setEntityResolver(self.TestEntityResolver()) - parser.setContentHandler(ErrorHandler()) + parser.setContentHandler(eh) + parser.feed('\n') + parser.feed(']>\n') with self.assertRaises(SAXParseException): - parser.feed('\n') - parser.feed(']>\n') parser.feed('&test;') - parser.close() self.assertEqual(len(parser._entity_stack), 0) diff --git a/Lib/xml/sax/expatreader.py b/Lib/xml/sax/expatreader.py index 8c1f54d2b77c74..1a00b362a8581f 100644 --- a/Lib/xml/sax/expatreader.py +++ b/Lib/xml/sax/expatreader.py @@ -425,7 +425,7 @@ def external_entity_ref(self, context, base, sysid, pubid): try: xmlreader.IncrementalParser.parse(self, source) except Exception: - return 0 + return 0 # FIXME: save error info here? finally: (self._parser, self._source) = self._entity_stack[-1] del self._entity_stack[-1] diff --git a/Misc/NEWS.d/next/Library/2026-04-12-09-52-14.gh-issue-148427.xR7v3m.rst b/Misc/NEWS.d/next/Library/2026-04-12-09-52-14.gh-issue-148427.xR7v3m.rst index 9b2e55a0d338b6..4f941c68ec9278 100644 --- a/Misc/NEWS.d/next/Library/2026-04-12-09-52-14.gh-issue-148427.xR7v3m.rst +++ b/Misc/NEWS.d/next/Library/2026-04-12-09-52-14.gh-issue-148427.xR7v3m.rst @@ -1,4 +1,3 @@ -Fix bare ``except`` clause in :mod:`xml.sax` expatreader's external entity -handler that silently swallowed :exc:`KeyboardInterrupt` and -:exc:`SystemExit`. Also fix entity parser stack leak on errors by moving -cleanup into a ``finally`` clause. +Fix bare ``except`` clause in :mod:`xml.sax` Expat's external entity +handler that silently swallowed :exc:`BaseException` instances such +as :exc:`KeyboardInterrupt`. From f7269ce85ee30348c926145ac83acf6e96d2ab0d Mon Sep 17 00:00:00 2001 From: WYSIATI Date: Sun, 12 Apr 2026 20:12:27 +0800 Subject: [PATCH 3/5] gh-148427: Fix test to only raise on entity element The side_effect must only raise for the external entity's element (), not for the main document's element (), since startElement is called for both. --- Lib/test/test_sax.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_sax.py b/Lib/test/test_sax.py index 43e858f05ed301..4137f1f3725492 100644 --- a/Lib/test/test_sax.py +++ b/Lib/test/test_sax.py @@ -1057,8 +1057,11 @@ def test_expat_entityresolver_default(self): def test_external_entity_ref_keyboard_interrupt(self): # gh-148427: KeyboardInterrupt must propagate, not be swallowed + def raise_on_entity(name, attrs): + if name == 'entity': + raise KeyboardInterrupt('test') eh = mock.Mock() - eh.startElement.side_effect = KeyboardInterrupt('test') + eh.startElement.side_effect = raise_on_entity parser = create_parser() parser.setFeature(feature_external_ges, True) @@ -1073,8 +1076,11 @@ def test_external_entity_ref_keyboard_interrupt(self): def test_external_entity_ref_system_exit(self): # gh-148427: SystemExit must propagate, not be swallowed + def raise_on_entity(name, attrs): + if name == 'entity': + raise SystemExit(42) eh = mock.Mock() - eh.startElement.side_effect = SystemExit(42) + eh.startElement.side_effect = raise_on_entity parser = create_parser() parser.setFeature(feature_external_ges, True) @@ -1089,8 +1095,11 @@ def test_external_entity_ref_system_exit(self): def test_external_entity_ref_stack_cleanup(self): # gh-148427: _entity_stack must be cleaned up after errors + def raise_on_entity(name, attrs): + if name == 'entity': + raise ValueError('test error') eh = mock.Mock() - eh.startElement.side_effect = ValueError('test error') + eh.startElement.side_effect = raise_on_entity parser = create_parser() parser.setFeature(feature_external_ges, True) From 55e6890b3983ea3898a8193f11f1f53da94e5ed3 Mon Sep 17 00:00:00 2001 From: WYSIATI Date: Sun, 12 Apr 2026 20:24:55 +0800 Subject: [PATCH 4/5] gh-148427: Consolidate tests into parameterized subTests Use @support.subTests to replace three separate tests with one parameterized test, per reviewer suggestion. --- Lib/test/test_sax.py | 63 +++++++++++--------------------------------- 1 file changed, 16 insertions(+), 47 deletions(-) diff --git a/Lib/test/test_sax.py b/Lib/test/test_sax.py index 4137f1f3725492..b585a7e02bb684 100644 --- a/Lib/test/test_sax.py +++ b/Lib/test/test_sax.py @@ -25,6 +25,7 @@ from urllib.error import URLError import urllib.request from test.support import os_helper +from test import support from test.support import findfile, check__all__ from test.support.os_helper import FakePath, TESTFN @@ -1055,64 +1056,32 @@ def test_expat_entityresolver_default(self): self.assertEqual(result.getvalue(), start + b"") - def test_external_entity_ref_keyboard_interrupt(self): - # gh-148427: KeyboardInterrupt must propagate, not be swallowed + @support.subTests("exc_type", [KeyboardInterrupt, SystemExit, ValueError]) + def test_external_entity_parser_with_exceptions(self, exc_type): + # gh-148427: BaseException subclasses must propagate, not be swallowed def raise_on_entity(name, attrs): if name == 'entity': - raise KeyboardInterrupt('test') - eh = mock.Mock() - eh.startElement.side_effect = raise_on_entity + raise exc_type("test") - parser = create_parser() - parser.setFeature(feature_external_ges, True) - parser.setEntityResolver(self.TestEntityResolver()) - parser.setContentHandler(eh) - - parser.feed('\n') - parser.feed(']>\n') - with self.assertRaises(KeyboardInterrupt): - parser.feed('&test;') - - def test_external_entity_ref_system_exit(self): - # gh-148427: SystemExit must propagate, not be swallowed - def raise_on_entity(name, attrs): - if name == 'entity': - raise SystemExit(42) - eh = mock.Mock() - eh.startElement.side_effect = raise_on_entity - - parser = create_parser() - parser.setFeature(feature_external_ges, True) - parser.setEntityResolver(self.TestEntityResolver()) - parser.setContentHandler(eh) - - parser.feed('\n') - parser.feed(']>\n') - with self.assertRaises(SystemExit): - parser.feed('&test;') - - def test_external_entity_ref_stack_cleanup(self): - # gh-148427: _entity_stack must be cleaned up after errors - def raise_on_entity(name, attrs): - if name == 'entity': - raise ValueError('test error') - eh = mock.Mock() - eh.startElement.side_effect = raise_on_entity + handler = mock.Mock() + handler.startElement = raise_on_entity parser = create_parser() parser.setFeature(feature_external_ges, True) parser.setEntityResolver(self.TestEntityResolver()) - parser.setContentHandler(eh) + parser.setContentHandler(handler) parser.feed('\n') parser.feed(']>\n') - with self.assertRaises(SAXParseException): - parser.feed('&test;') - - self.assertEqual(len(parser._entity_stack), 0) + trigger = '&test;' + + if issubclass(exc_type, Exception): + self.assertRaises(SAXParseException, parser.feed, trigger) + self.assertEqual(len(parser._entity_stack), 0) + else: + with self.assertRaisesRegex(exc_type, "test"): + parser.feed(trigger) # ===== Attributes support From 9c46fbd213d013ae6c1eea24f3828544f1e39b13 Mon Sep 17 00:00:00 2001 From: WYSIATI Date: Sun, 12 Apr 2026 20:51:56 +0800 Subject: [PATCH 5/5] gh-148427: Revert finally clause, remove stack assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the finally clause per reviewer feedback — the stack cleanup behavior change is a separate concern. Remove the entity stack assertion that depended on it. --- Lib/test/test_sax.py | 1 - Lib/xml/sax/expatreader.py | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_sax.py b/Lib/test/test_sax.py index b585a7e02bb684..d851b93af1f563 100644 --- a/Lib/test/test_sax.py +++ b/Lib/test/test_sax.py @@ -1078,7 +1078,6 @@ def raise_on_entity(name, attrs): if issubclass(exc_type, Exception): self.assertRaises(SAXParseException, parser.feed, trigger) - self.assertEqual(len(parser._entity_stack), 0) else: with self.assertRaisesRegex(exc_type, "test"): parser.feed(trigger) diff --git a/Lib/xml/sax/expatreader.py b/Lib/xml/sax/expatreader.py index 1a00b362a8581f..54c768f96bdb1a 100644 --- a/Lib/xml/sax/expatreader.py +++ b/Lib/xml/sax/expatreader.py @@ -426,9 +426,9 @@ def external_entity_ref(self, context, base, sysid, pubid): xmlreader.IncrementalParser.parse(self, source) except Exception: return 0 # FIXME: save error info here? - finally: - (self._parser, self._source) = self._entity_stack[-1] - del self._entity_stack[-1] + + (self._parser, self._source) = self._entity_stack[-1] + del self._entity_stack[-1] return 1 def skipped_entity_handler(self, name, is_pe):