Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-139400: Make sure that parent parsers outlive their subparsers in pyexpat#139403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
22 commits Select commit Hold shift + click to select a range
e1c0470 pyexpat.c: Disallow collection of in-use parent parsers
hartwork 3b0afda Add a regression test for issue 139400
hartwork 8f70991 Add news item for issue 139400
hartwork a63bd34 Improve test's docstring
hartwork 22417ec Simplify xmlparseobject_ to xmlparseobject
hartwork 20caacd Leverage Py_CLEAR
hartwork f8c58a3 Simplify call to Py_CLEAR
hartwork f1e3e36 Streamline docsting of ParentParserLifetimeTest
hartwork 1b2937d pyexpat.c: Adjust whitespace around pointer
hartwork 8c5a4e8 Re-write news item
hartwork a6f5520 Turn parent into a PyObject pointer
hartwork 4470cea Add missing parent visit to xmlparse_traverse
hartwork 3759ac7 Re-write comment about mission of "PyObject *parent"
hartwork 30598a9 Add whitespace as requested
hartwork 6d3c424 Add more tests
hartwork 66b77e0 Add comment about Py_CLEAR(self->parent) to xmlparse_clear
hartwork 455787f Add missing full stops to docstrings
hartwork a669454 Drop reference to XML_ExternalEntityParserCreate
hartwork a91ed6b Make test test_cycle waterproof
hartwork c056cfe Full stops and sentences
hartwork 12b8989 Drop test test_cycle
hartwork 0541d6f Drop now-unused import
hartwork File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -771,6 +771,42 @@ def resolve_entity(context, base, system_id, public_id): | ||
| self.assertEqual(handler_call_args, [("bar", "baz")]) | ||
| class ParentParserLifetimeTest(unittest.TestCase): | ||
| """ | ||
| Subparsers make use of their parent XML_Parser inside of Expat. | ||
| As a result, parent parsers need to outlive subparsers. | ||
| See https://github.com/python/cpython/issues/139400. | ||
| """ | ||
| def test_parent_parser_outlives_its_subparsers__single(self): | ||
| parser = expat.ParserCreate() | ||
| subparser = parser.ExternalEntityParserCreate(None) | ||
| # Now try to cause garbage collection of the parent parser | ||
| # while it's still being referenced by a related subparser. | ||
| del parser | ||
| def test_parent_parser_outlives_its_subparsers__multiple(self): | ||
| parser = expat.ParserCreate() | ||
| subparser_one = parser.ExternalEntityParserCreate(None) | ||
| subparser_two = parser.ExternalEntityParserCreate(None) | ||
| # Now try to cause garbage collection of the parent parser | ||
| # while it's still being referenced by a related subparser. | ||
| del parser | ||
| def test_parent_parser_outlives_its_subparsers__chain(self): | ||
| parser = expat.ParserCreate() | ||
| subparser = parser.ExternalEntityParserCreate(None) | ||
| subsubparser = subparser.ExternalEntityParserCreate(None) | ||
| # Now try to cause garbage collection of the parent parsers | ||
| # while they are still being referenced by a related subparser. | ||
| del parser | ||
hartwork marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading. Please reload this page. | ||
| del subparser | ||
| class ReparseDeferralTest(unittest.TestCase): | ||
| def test_getter_setter_round_trip(self): | ||
| parser = expat.ParserCreate() | ||
4 changes: 4 additions & 0 deletions 4 Misc/NEWS.d/next/Core_and_Builtins/2025-09-29-00-01-28.gh-issue-139400.X2T-jO.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| :mod:`xml.parsers.expat`: Make sure that parent Expat parsers are only | ||
| garbage-collected once they are no longer referenced by subparsers created | ||
| by :meth:`~xml.parsers.expat.xmlparser.ExternalEntityParserCreate`. | ||
| Patch by Sebastian Pipping. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -76,6 +76,15 @@ typedef struct{ | ||
| PyObject_HEAD | ||
| XML_Parser itself; | ||
| /* | ||
| * Strong reference to a parent `xmlparseobject` if this parser | ||
| * is a child parser. Set to NULL if this parser is a root parser. | ||
| * This is needed to keep the parent parser alive as long as it has | ||
| * at least one child parser. | ||
| * | ||
| * See https://github.com/python/cpython/issues/139400 for details. | ||
| */ | ||
| PyObject *parent; | ||
| int ordered_attributes; /* Return attributes as a list. */ | ||
| int specified_attributes; /* Report only specified attributes. */ | ||
| int in_callback; /* Is a callback active? */ | ||
| @@ -1065,6 +1074,11 @@ pyexpat_xmlparser_ExternalEntityParserCreate_impl(xmlparseobject *self, | ||
| return NULL; | ||
| } | ||
| // The new subparser will make use of the parent XML_Parser inside of Expat. | ||
| // So we need to take subparsers into account with the reference counting | ||
| // of their parent parser. | ||
| Py_INCREF(self); | ||
| new_parser->buffer_size = self->buffer_size; | ||
| new_parser->buffer_used = 0; | ||
| new_parser->buffer = NULL; | ||
| @@ -1074,18 +1088,21 @@ pyexpat_xmlparser_ExternalEntityParserCreate_impl(xmlparseobject *self, | ||
| new_parser->ns_prefixes = self->ns_prefixes; | ||
| new_parser->itself = XML_ExternalEntityParserCreate(self->itself, context, | ||
| encoding); | ||
| new_parser->parent = (PyObject *)self; | ||
| new_parser->handlers = 0; | ||
| new_parser->intern = Py_XNewRef(self->intern); | ||
| if (self->buffer != NULL){ | ||
| new_parser->buffer = PyMem_Malloc(new_parser->buffer_size); | ||
| if (new_parser->buffer == NULL){ | ||
| Py_DECREF(new_parser); | ||
| Py_DECREF(self); | ||
| return PyErr_NoMemory(); | ||
| } | ||
| } | ||
| if (!new_parser->itself){ | ||
| Py_DECREF(new_parser); | ||
| Py_DECREF(self); | ||
| return PyErr_NoMemory(); | ||
| } | ||
| @@ -1099,6 +1116,7 @@ pyexpat_xmlparser_ExternalEntityParserCreate_impl(xmlparseobject *self, | ||
| new_parser->handlers = PyMem_New(PyObject *, i); | ||
| if (!new_parser->handlers){ | ||
| Py_DECREF(new_parser); | ||
| Py_DECREF(self); | ||
| return PyErr_NoMemory(); | ||
| } | ||
| clear_handlers(new_parser, 1); | ||
| @@ -1479,6 +1497,7 @@ newxmlparseobject(pyexpat_state *state, const char *encoding, | ||
| /* namespace_separator is either NULL or contains one char + \0 */ | ||
| self->itself = XML_ParserCreate_MM(encoding, &ExpatMemoryHandler, | ||
| namespace_separator); | ||
| self->parent = NULL; | ||
| if (self->itself == NULL){ | ||
| PyErr_SetString(PyExc_RuntimeError, | ||
| "XML_ParserCreate failed"); | ||
| @@ -1515,6 +1534,7 @@ xmlparse_traverse(PyObject *op, visitproc visit, void *arg) | ||
| for (size_t i = 0; handler_info[i].name != NULL; i++){ | ||
| Py_VISIT(self->handlers[i]); | ||
| } | ||
| Py_VISIT(self->parent); | ||
| Py_VISIT(Py_TYPE(op)); | ||
| return 0; | ||
| } | ||
| @@ -1525,6 +1545,10 @@ xmlparse_clear(PyObject *op) | ||
| xmlparseobject *self = xmlparseobject_CAST(op); | ||
| clear_handlers(self, 0); | ||
| Py_CLEAR(self->intern); | ||
| // NOTE: We cannot call Py_CLEAR(self->parent) prior to calling | ||
| // XML_ParserFree(self->itself), or a subparser could lose its parent | ||
| // XML_Parser while still making use of it internally. | ||
| // https://github.com/python/cpython/issues/139400 | ||
| return 0; | ||
| } | ||
| @@ -1538,6 +1562,7 @@ xmlparse_dealloc(PyObject *op) | ||
| XML_ParserFree(self->itself); | ||
| } | ||
| self->itself = NULL; | ||
| Py_CLEAR(self->parent); | ||
picnixz marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading. Please reload this page. | ||
| if (self->handlers != NULL){ | ||
| PyMem_Free(self->handlers); | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.