From 0319494a26f76a06c77a5d1d0aeebcc9bac7b2a9 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 22 Oct 2026 08:11:11 +0000 Subject: [PATCH 1/2] gh-160525: Fix quadratic text concatenation in xml.dom.expatbuilder Collect the pieces of a run of character data in a list and join them once, instead of copying the Text node's data on every Expat callback. ANT-2026-ZRQNVCB1 ANT-2026-QXYQ68DQ --- Lib/test/test_minidom.py | 60 ++++++++++++++++ Lib/xml/dom/expatbuilder.py | 72 +++++++++++++------ ...-10-22-08-10-39.gh-issue-160525.q7TxLd.rst | 4 ++ 3 files changed, 114 insertions(+), 22 deletions(-) create mode 100644 Misc/NEWS.d/next/Security/2026-10-22-08-10-39.gh-issue-160525.q7TxLd.rst diff --git a/Lib/test/test_minidom.py b/Lib/test/test_minidom.py index 446bbe096bd19dc..6b052d8921b7a14 100644 --- a/Lib/test/test_minidom.py +++ b/Lib/test/test_minidom.py @@ -6,6 +6,7 @@ from test import support import unittest +import xml.dom.expatbuilder import xml.dom.minidom from xml.dom.minidom import parse, Attr, Node, Document, Element, parseString @@ -985,6 +986,65 @@ def testParseString(self): self.assertEqual(dom.documentElement.firstChild.data, "\xe9") dom.unlink() + def testParseTextInManyPieces(self): + # Expat delivers a run of character data in several callbacks + # when it is interrupted by markup that adds no node, such as an + # empty CDATA section or a reference to an external entity; + # the pieces must end up in a single Text node. + source = ("" + "g'>]>" + "ab&ext;cd∫∫" + "ij") + for namespaces in True, False: + with self.subTest(namespaces=namespaces): + dom = xml.dom.expatbuilder.parseString(source, namespaces) + children = dom.documentElement.childNodes + self.assertEqual( + [(c.nodeName, getattr(c, 'data', None)) for c in children], + [('#text', 'abc'), ('#comment', 'x'), + ('#text', 'defgefg'), ('#cdata-section', 'h'), + ('#text', 'i'), ('pi', 'x'), ('#text', 'j')]) + # A fragment can end in a run of character data. + fragment = xml.dom.expatbuilder.parseFragmentString( + "ab&ext;c", dom.documentElement, + namespaces) + self.assertEqual( + [(c.nodeName, getattr(c, 'data', None)) + for c in fragment.childNodes], + [('e', None), ('#text', 'abc')]) + dom.unlink() + + def testParseTextInManyPiecesNoQuadraticComplexity(self): + def work(n): + total_chars = 0 + + # Count the characters stored into text nodes as a proxy + # for the work done. + def set_data(self, data): + nonlocal total_chars + total_chars += len(data) + self._data = data + + data = property(xml.dom.minidom.CharacterData._get_data, set_data) + with support.swap_attr(xml.dom.minidom.CharacterData, "data", data): + dom = parseString("" + "a" * n + "") + self.assertEqual(dom.documentElement.firstChild.data, "a" * n) + dom.unlink() + return total_chars + + # Doubling N should not ~quadruple the work. + w1 = work(1024) + w2 = work(2048) + w3 = work(4096) + + self.assertGreater(w1, 0) + r1 = w2 / w1 + r2 = w3 / w2 + self.assertLess( + max(r1, r2), 3.2, + msg=f"Possible quadratic behavior: work={w1,w2,w3} ratios={r1,r2}" + ) + def testComment(self): dom = Document() comment = dom.createComment("comment") diff --git a/Lib/xml/dom/expatbuilder.py b/Lib/xml/dom/expatbuilder.py index e3917c1cc682880..936a51e11ce164b 100644 --- a/Lib/xml/dom/expatbuilder.py +++ b/Lib/xml/dom/expatbuilder.py @@ -20,7 +20,12 @@ # speedup since pyexpat can break up character data into multiple # callbacks even though we set the buffer_text attribute on the # parser. This also gives us the advantage that we don't need a -# separate normalization pass. +# separate normalization pass. The pieces are collected in a list +# and joined once the run of character data ends (see +# ._flush_text()), since a run can arrive in a great many +# callbacks (pyexpat flushes its buffer before every other +# handler) and copying the accumulated text on each of them +# would be quadratic. # # - Determining that a node exists is done using an identity comparison # with None rather than a truth test; this avoids searching for and @@ -170,6 +175,10 @@ def reset(self): self.curNode = self.document self._elem_info = self.document._elem_info self._cdata = False + # Text node currently being extended and the pieces of its data + # which have not been joined yet; see _flush_text(). + self._text_node = None + self._text_parts = None def install(self, parser): """Install the callbacks needed to build the DOM into the parser.""" @@ -262,42 +271,56 @@ def end_doctype_decl_handler(self): if not (self._elem_info or self._filter): self._finish_end_element = id + def _flush_text(self): + # Join the pieces of the current run of character data into + # its Text node. This must be called before anything else is + # added to the tree after the Text node, and before the tree is + # handed to a filter. + node = self._text_node + if node is not None: + node.data = ''.join(self._text_parts) + self._text_node = None + self._text_parts = None + def pi_handler(self, target, data): + self._flush_text() node = self.document.createProcessingInstruction(target, data) _append_child(self.curNode, node) if self._filter and self._filter.acceptNode(node) == FILTER_REJECT: self.curNode.removeChild(node) def character_data_handler_cdata(self, data): + if not self._cdata: + self.character_data_handler(data) + return + # Flushing here rather than in start_cdata_section_handler() + # keeps an empty CDATA section, which adds no node, from ending + # the run of character data around it. + self._flush_text() childNodes = self.curNode.childNodes - if self._cdata: - if ( self._cdata_continue - and childNodes[-1].nodeType == CDATA_SECTION_NODE): - childNodes[-1].appendData(data) - return - node = self.document.createCDATASection(data) - self._cdata_continue = True - elif childNodes and childNodes[-1].nodeType == TEXT_NODE: - node = childNodes[-1] - value = node.data + data - node.data = value + if ( self._cdata_continue + and childNodes[-1].nodeType == CDATA_SECTION_NODE): + childNodes[-1].appendData(data) return - else: - node = minidom.Text() - node.data = data - node.ownerDocument = self.document + node = self.document.createCDATASection(data) + self._cdata_continue = True _append_child(self.curNode, node) def character_data_handler(self, data): + if self._text_node is not None: + self._text_parts.append(data) + return childNodes = self.curNode.childNodes if childNodes and childNodes[-1].nodeType == TEXT_NODE: node = childNodes[-1] - node.data = node.data + data - return - node = minidom.Text() - node.data = node.data + data - node.ownerDocument = self.document - _append_child(self.curNode, node) + self._text_parts = [node.data, data] + else: + node = minidom.Text() + node.data = data + node.ownerDocument = self.document + _append_child(self.curNode, node) + self._text_parts = [data] + self._text_node = node def entity_decl_handler(self, entityName, is_parameter_entity, value, base, systemId, publicId, notationName): @@ -324,6 +347,7 @@ def notation_decl_handler(self, notationName, base, systemId, publicId): del self.document.doctype.notations._seq[-1] def comment_handler(self, data): + self._flush_text() node = self.document.createComment(data) _append_child(self.curNode, node) if self._filter and self._filter.acceptNode(node) == FILTER_REJECT: @@ -347,6 +371,7 @@ def first_element_handler(self, name, attributes): self.start_element_handler(name, attributes) def start_element_handler(self, name, attributes): + self._flush_text() node = self.document.createElement(name) _append_child(self.curNode, node) self.curNode = node @@ -389,6 +414,7 @@ def _finish_start_element(self, node): # be changed to match. # def end_element_handler(self, name): + self._flush_text() curNode = self.curNode self.curNode = curNode.parentNode self._finish_end_element(curNode) @@ -734,6 +760,7 @@ def start_namespace_decl_handler(self, prefix, uri): self._ns_ordered_prefixes.append((prefix, uri)) def start_element_handler(self, name, attributes): + self._flush_text() if ' ' in name: uri, localname, prefix, qname = _parse_ns_name(self, name) else: @@ -789,6 +816,7 @@ def start_element_handler(self, name, attributes): # it needs to be changed as well. # def end_element_handler(self, name): + self._flush_text() curNode = self.curNode if ' ' in name: uri, localname, prefix, qname = _parse_ns_name(self, name) diff --git a/Misc/NEWS.d/next/Security/2026-10-22-08-10-39.gh-issue-160525.q7TxLd.rst b/Misc/NEWS.d/next/Security/2026-10-22-08-10-39.gh-issue-160525.q7TxLd.rst new file mode 100644 index 000000000000000..ff09ffabc743a25 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2026-10-22-08-10-39.gh-issue-160525.q7TxLd.rst @@ -0,0 +1,4 @@ +Fix quadratic complexity in :func:`xml.dom.minidom.parse` and +:func:`xml.dom.minidom.parseString` when the text of an element is +delivered by the parser in many pieces, for example when it is interrupted by +empty CDATA sections or references to external entities. From 99347bef0f9de156b299289bc4feb38b76c67815 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Fri, 23 Oct 2026 23:12:13 +0000 Subject: [PATCH 2/2] gh-160525: Keep expatbuilder text correct when handlers are overridden A subclass of ExpatBuilder can replace the element, comment or processing instruction handlers without calling the base implementations, which are what end the pending run of character data. Check that the Text node being extended is still the last child of the current node before adding to it, and join any pending pieces when parsing ends, so that such subclasses keep building the same tree. --- Lib/test/test_minidom.py | 65 +++++++++++++++++++++++++++++++++++++ Lib/xml/dom/expatbuilder.py | 17 +++++++--- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_minidom.py b/Lib/test/test_minidom.py index 6b052d8921b7a14..7ddd83e336b8e30 100644 --- a/Lib/test/test_minidom.py +++ b/Lib/test/test_minidom.py @@ -1045,6 +1045,71 @@ def set_data(self, data): msg=f"Possible quadratic behavior: work={w1,w2,w3} ratios={r1,r2}" ) + def testParseTextWithOverriddenHandlers(self): + # Character data must end up in the right place in the tree and + # be complete when a subclass of the builder replaces handlers + # without calling the base implementations. + class StartElement: + def start_element_handler(self, name, attributes): + node = self.document.createElement(name) + self.curNode.appendChild(node) + self.curNode = node + + class EndElement: + def end_element_handler(self, name): + self.curNode = self.curNode.parentNode + + class Comment: + def comment_handler(self, data): + self.curNode.appendChild(self.document.createComment(data)) + + class PI: + def pi_handler(self, target, data): + self.curNode.appendChild( + self.document.createProcessingInstruction(target, data)) + + class RemovingEndElement: + def end_element_handler(self, name): + node = self.curNode + self.curNode = node.parentNode + if name == "d": + self.curNode.removeChild(node) + + source = ("bcefgh" + "jknop") + expected = source.replace("", "") + names = ["#text", "d", "#text", "#comment", "#text", "l", "#text", + "d", "#text"] + ExpatBuilder = xml.dom.expatbuilder.ExpatBuilder + FragmentBuilder = xml.dom.expatbuilder.FragmentBuilder + for mixins in [(StartElement,), (EndElement,), (Comment,), (PI,), + (StartElement, EndElement, Comment, PI)]: + with self.subTest(mixins=mixins): + class Builder(*mixins, ExpatBuilder): + pass + dom = Builder().parseString(source) + root = dom.documentElement + self.assertEqual([c.nodeName for c in root.childNodes], names) + self.assertEqual(root.toxml(), expected) + + class Builder(*mixins, FragmentBuilder): + pass + fragment = Builder(dom).parseString("bc") + self.assertEqual( + [(c.nodeName, getattr(c, 'data', None)) + for c in fragment.childNodes], + [('d', None), ('#text', 'bc')]) + dom.unlink() + + class Builder(RemovingEndElement, ExpatBuilder): + pass + dom = Builder().parseString(source) + root = dom.documentElement + self.assertEqual([c.nodeName for c in root.childNodes], + ["#text", "#comment", "#text", "l", "#text"]) + self.assertEqual(root.toxml(), "bcghjknop") + dom.unlink() + def testComment(self): dom = Document() comment = dom.createComment("comment") diff --git a/Lib/xml/dom/expatbuilder.py b/Lib/xml/dom/expatbuilder.py index 936a51e11ce164b..a3ccd2866c1124d 100644 --- a/Lib/xml/dom/expatbuilder.py +++ b/Lib/xml/dom/expatbuilder.py @@ -217,6 +217,7 @@ def parseFile(self, file): parser.Parse(b"", True) except ParseEscape: pass + self._flush_text() doc = self.document self.reset() self._parser = None @@ -230,6 +231,7 @@ def parseString(self, string): self._setup_subset(string) except ParseEscape: pass + self._flush_text() doc = self.document self.reset() self._parser = None @@ -275,7 +277,8 @@ def _flush_text(self): # Join the pieces of the current run of character data into # its Text node. This must be called before anything else is # added to the tree after the Text node, and before the tree is - # handed to a filter. + # handed to a filter. It is also called when parsing ends, as + # the handlers of a subclass might not have called it. node = self._text_node if node is not None: node.data = ''.join(self._text_parts) @@ -307,10 +310,15 @@ def character_data_handler_cdata(self, data): _append_child(self.curNode, node) def character_data_handler(self, data): - if self._text_node is not None: - self._text_parts.append(data) - return childNodes = self.curNode.childNodes + if self._text_node is not None: + if childNodes and childNodes[-1] is self._text_node: + self._text_parts.append(data) + return + # The Text node is no longer the last child of the current + # node, which happens when a handler overridden in a + # subclass changes the tree without calling _flush_text(). + self._flush_text() if childNodes and childNodes[-1].nodeType == TEXT_NODE: node = childNodes[-1] self._text_parts = [node.data, data] @@ -667,6 +675,7 @@ def parseString(self, string): except: self.reset() raise + self._flush_text() fragment = self.fragment self.reset() ## self._parser = None