gh-148427: Fix bare except in expatreader.external_entity_ref()#148435
gh-148427: Fix bare except in expatreader.external_entity_ref()#148435WYSIATI wants to merge 5 commits intopython:mainfrom
Conversation
2c27245 to
738bda2
Compare
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.
738bda2 to
2833a88
Compare
Misc/NEWS.d/next/Library/2026-04-12-09-52-14.gh-issue-148427.xR7v3m.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2026-04-12-09-52-14.gh-issue-148427.xR7v3m.rst
Outdated
Show resolved
Hide resolved
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
- 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
The side_effect must only raise for the external entity's element (<entity/>), not for the main document's element (<doc>), since startElement is called for both.
Lib/xml/sax/expatreader.py
Outdated
| finally: | ||
| (self._parser, self._source) = self._entity_stack[-1] | ||
| del self._entity_stack[-1] |
There was a problem hiding this comment.
I'm not entirely sure about this. Strictly speaking this is actually a behavior change. After an exception I don't know whether we should leave the stack alone or not.
There was a problem hiding this comment.
The finally is a separate concern from the except: → except Exception: fix. I can remove it to keep this PR focused on the bare except issue. Without finally, the stack still leaks on errors. that's pre-existing behavior and could be addressed separately if needed.
I decided to resolve them together, however I can drop it if you recommend so :-)
There was a problem hiding this comment.
Yes, don't make multiple changes per PR. We need a separate issue and a separate PR. Please read https://devguide.python.org/getting-started/pull-request-lifecycle.
There was a problem hiding this comment.
Thanks for sharing the link, I will be more cautious with future PRs
There was a problem hiding this comment.
BTW, I would like to create a separate issue for this this stack problem, if no objection
There was a problem hiding this comment.
Created an issue #148448 for the stack issue
Use @support.subTests to replace three separate tests with one parameterized test, per reviewer suggestion.
Lib/test/test_sax.py
Outdated
|
|
||
| if issubclass(exc_type, Exception): | ||
| self.assertRaises(SAXParseException, parser.feed, trigger) | ||
| self.assertEqual(len(parser._entity_stack), 0) |
There was a problem hiding this comment.
Don't forget to remove this line once you reverted the other unrelated change.
There was a problem hiding this comment.
So you mean we should revert the finally change?
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.
picnixz
left a comment
There was a problem hiding this comment.
Can you also update the documentation (if it mentions exceptions). If it doesn't mention exceptions, no need to update it.
Checked — the docs don't mention exceptions in the context of external entity handling, so no doc update needed I have made the requested changes; please review again |
|
Thanks for making the requested changes! : please review the changes made to this pull request. |
ExpatParser.external_entity_ref()uses a bareexcept:that silently swallowsKeyboardInterruptandSystemExitduring external entity parsing. It also leaks the_entity_stackon errors since the cleanup code only runs on the success path.This changes
except:toexcept Exception:and moves the stack cleanup into afinallyblock.I verified in pyexpat.c that the C layer handles Python exception propagation correctly —
call_with_frame()callsXML_StopParser()on callback failure, andget_parse_result()checksPyErr_Occurred()before inspecting the return value, so lettingKeyboardInterruptthrough is safe.