From a9a68759c4721614a1179a8272243d888626ae4e Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Wed, 13 Sep 2017 16:29:14 +0200 Subject: [PATCH 1/5] Avoid calling "PyObject_GetAttrString()" (and potentially executing user code) with a live exception set. --- Modules/_elementtree.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index 857005a2a9b8ad8..f8a986320880c21 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -3313,13 +3313,18 @@ _elementtree_XMLParser___init___impl(XMLParserObject *self, PyObject *html, self->target = target; self->handle_start = PyObject_GetAttrString(target, "start"); + PyErr_Clear(); self->handle_data = PyObject_GetAttrString(target, "data"); + PyErr_Clear(); self->handle_end = PyObject_GetAttrString(target, "end"); + PyErr_Clear(); self->handle_comment = PyObject_GetAttrString(target, "comment"); + PyErr_Clear(); self->handle_pi = PyObject_GetAttrString(target, "pi"); + PyErr_Clear(); self->handle_close = PyObject_GetAttrString(target, "close"); + PyErr_Clear(); self->handle_doctype = PyObject_GetAttrString(target, "doctype"); - PyErr_Clear(); /* configure parser */ From e0b8790f8e123048c0ada330f9a192b10a2a4b88 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Wed, 13 Sep 2017 19:53:53 +0200 Subject: [PATCH 2/5] Ignore only AttributeError on attribute lookups in ElementTree.XMLParser() and propagate all other exceptions. --- Lib/test/test_xml_etree.py | 23 +++++++++++++++++++++++ Modules/_elementtree.c | 32 +++++++++++++++++++++++++------- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_xml_etree.py b/Lib/test/test_xml_etree.py index 233b4da58c3a231..7f1e3f36af5f1ac 100644 --- a/Lib/test/test_xml_etree.py +++ b/Lib/test/test_xml_etree.py @@ -2400,6 +2400,29 @@ def _check_sample1_element(self, e): self.assertEqual(child.tail, 'tail') self.assertEqual(child.attrib, {}) + def test_exceptions(self): + class RaisingBuilder: + def __init__(self, raise_in=None, what=ValueError): + self.raise_in = raise_in + self.what = what + + def __getattr__(self, name): + if name == self.raise_in: + raise self.what(self.raise_in) + def handle(*args): + pass + return handle + + ET.XMLParser(target=RaisingBuilder()) + # cET also checks for 'close' and 'doctype', PyET does it only at need + for event in ('start', 'data', 'end', 'comment', 'pi'): + with self.assertRaisesRegex(ValueError, event): + ET.XMLParser(target=RaisingBuilder(event)) + + ET.XMLParser(target=RaisingBuilder(what=AttributeError)) + for event in ('start', 'data', 'end', 'comment', 'pi'): + ET.XMLParser(target=RaisingBuilder(event, what=AttributeError)) + def test_dummy_builder(self): class BaseDummyBuilder: def close(self): diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index f8a986320880c21..ac7f3c4be7a3053 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -3259,6 +3259,17 @@ xmlparser_new(PyTypeObject *type, PyObject *args, PyObject *kwds) return (PyObject *)self; } +static int +ignore_attribute_error(PyObject *value) { + if (value == NULL) { + if (!PyErr_ExceptionMatches(PyExc_AttributeError)) { + return -1; + } + PyErr_Clear(); + } + return 0; +} + /*[clinic input] _elementtree.XMLParser.__init__ @@ -3313,19 +3324,26 @@ _elementtree_XMLParser___init___impl(XMLParserObject *self, PyObject *html, self->target = target; self->handle_start = PyObject_GetAttrString(target, "start"); - PyErr_Clear(); + if (ignore_attribute_error(self->handle_start)) + return -1; self->handle_data = PyObject_GetAttrString(target, "data"); - PyErr_Clear(); + if (ignore_attribute_error(self->handle_data)) + return -1; self->handle_end = PyObject_GetAttrString(target, "end"); - PyErr_Clear(); + if (ignore_attribute_error(self->handle_end)) + return -1; self->handle_comment = PyObject_GetAttrString(target, "comment"); - PyErr_Clear(); + if (ignore_attribute_error(self->handle_comment)) + return -1; self->handle_pi = PyObject_GetAttrString(target, "pi"); - PyErr_Clear(); + if (ignore_attribute_error(self->handle_pi)) + return -1; self->handle_close = PyObject_GetAttrString(target, "close"); - PyErr_Clear(); + if (ignore_attribute_error(self->handle_close)) + return -1; self->handle_doctype = PyObject_GetAttrString(target, "doctype"); - PyErr_Clear(); + if (ignore_attribute_error(self->handle_doctype)) + return -1; /* configure parser */ EXPAT(SetUserData)(self->parser, self); From 5253d56cc98318c96199c68f43620d3987f120a0 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Wed, 13 Sep 2017 19:58:00 +0200 Subject: [PATCH 3/5] Add News entry for bpo-31544 --- .../next/Library/2017-09-13-19-55-35.bpo-31544.beTh6t.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2017-09-13-19-55-35.bpo-31544.beTh6t.rst diff --git a/Misc/NEWS.d/next/Library/2017-09-13-19-55-35.bpo-31544.beTh6t.rst b/Misc/NEWS.d/next/Library/2017-09-13-19-55-35.bpo-31544.beTh6t.rst new file mode 100644 index 000000000000000..9ea3599ee0b0e90 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-09-13-19-55-35.bpo-31544.beTh6t.rst @@ -0,0 +1,2 @@ +The C accelerator module of ElementTree ignored exceptions raised when +looking up TreeBuilder target methods in XMLParser(). From 3e1c77be7c98afa907daed86de7b8ac3ba18bea8 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Thu, 14 Sep 2017 15:10:41 +0200 Subject: [PATCH 4/5] Give test a more specific name and move it further down as it tests misbehaviour, not basic behaviour. --- Lib/test/test_xml_etree.py | 48 ++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/Lib/test/test_xml_etree.py b/Lib/test/test_xml_etree.py index 7f1e3f36af5f1ac..baa4e1f53427f6d 100644 --- a/Lib/test/test_xml_etree.py +++ b/Lib/test/test_xml_etree.py @@ -2400,29 +2400,6 @@ def _check_sample1_element(self, e): self.assertEqual(child.tail, 'tail') self.assertEqual(child.attrib, {}) - def test_exceptions(self): - class RaisingBuilder: - def __init__(self, raise_in=None, what=ValueError): - self.raise_in = raise_in - self.what = what - - def __getattr__(self, name): - if name == self.raise_in: - raise self.what(self.raise_in) - def handle(*args): - pass - return handle - - ET.XMLParser(target=RaisingBuilder()) - # cET also checks for 'close' and 'doctype', PyET does it only at need - for event in ('start', 'data', 'end', 'comment', 'pi'): - with self.assertRaisesRegex(ValueError, event): - ET.XMLParser(target=RaisingBuilder(event)) - - ET.XMLParser(target=RaisingBuilder(what=AttributeError)) - for event in ('start', 'data', 'end', 'comment', 'pi'): - ET.XMLParser(target=RaisingBuilder(event, what=AttributeError)) - def test_dummy_builder(self): class BaseDummyBuilder: def close(self): @@ -2521,6 +2498,31 @@ def close(self): ('html', '-//W3C//DTD XHTML 1.0 Transitional//EN', 'http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd')) + def test_builder_lookup_errors(self): + class RaisingBuilder: + def __init__(self, raise_in=None, what=ValueError): + self.raise_in = raise_in + self.what = what + + def __getattr__(self, name): + if name == self.raise_in: + raise self.what(self.raise_in) + def handle(*args): + pass + return handle + + ET.XMLParser(target=RaisingBuilder()) + # cET also checks for 'close' and 'doctype', PyET does it only at need + for event in ('start', 'data', 'end', 'comment', 'pi'): + with self.assertRaisesRegex(ValueError, event): + ET.XMLParser(target=RaisingBuilder(event)) + + ET.XMLParser(target=RaisingBuilder(what=AttributeError)) + for event in ('start', 'data', 'end', 'comment', 'pi'): + parser = ET.XMLParser(target=RaisingBuilder(event, what=AttributeError)) + parser.feed(self.sample1) + self.assertIsNone(parser.close()) + class XMLParserTest(unittest.TestCase): sample1 = b'22' From 8cb95c616f59d8e61ab099fcdb68cbce1286538b Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Thu, 14 Sep 2017 15:11:55 +0200 Subject: [PATCH 5/5] Fix C code style. --- Modules/_elementtree.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index ac7f3c4be7a3053..569c78d3b94538b 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -3260,7 +3260,8 @@ xmlparser_new(PyTypeObject *type, PyObject *args, PyObject *kwds) } static int -ignore_attribute_error(PyObject *value) { +ignore_attribute_error(PyObject *value) +{ if (value == NULL) { if (!PyErr_ExceptionMatches(PyExc_AttributeError)) { return -1; @@ -3324,26 +3325,33 @@ _elementtree_XMLParser___init___impl(XMLParserObject *self, PyObject *html, self->target = target; self->handle_start = PyObject_GetAttrString(target, "start"); - if (ignore_attribute_error(self->handle_start)) + if (ignore_attribute_error(self->handle_start)) { return -1; + } self->handle_data = PyObject_GetAttrString(target, "data"); - if (ignore_attribute_error(self->handle_data)) + if (ignore_attribute_error(self->handle_data)) { return -1; + } self->handle_end = PyObject_GetAttrString(target, "end"); - if (ignore_attribute_error(self->handle_end)) + if (ignore_attribute_error(self->handle_end)) { return -1; + } self->handle_comment = PyObject_GetAttrString(target, "comment"); - if (ignore_attribute_error(self->handle_comment)) + if (ignore_attribute_error(self->handle_comment)) { return -1; + } self->handle_pi = PyObject_GetAttrString(target, "pi"); - if (ignore_attribute_error(self->handle_pi)) + if (ignore_attribute_error(self->handle_pi)) { return -1; + } self->handle_close = PyObject_GetAttrString(target, "close"); - if (ignore_attribute_error(self->handle_close)) + if (ignore_attribute_error(self->handle_close)) { return -1; + } self->handle_doctype = PyObject_GetAttrString(target, "doctype"); - if (ignore_attribute_error(self->handle_doctype)) + if (ignore_attribute_error(self->handle_doctype)) { return -1; + } /* configure parser */ EXPAT(SetUserData)(self->parser, self);