From ab017a8a53fac0afcd17dbd06cb080afc436ab1c Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 18 Sep 2017 10:53:51 +0200 Subject: [PATCH 1/5] bpo-31499, xml.etree: Fix xmlparser_gc_clear() crash xml.etree: xmlparser_gc_clear() now sets self.parser to NULL to prevent a crash in xmlparser_dealloc() if xmlparser_gc_clear() was called previously by the garbage collector, because the parser was part of a reference cycle. --- .../next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst | 3 +++ Modules/_elementtree.c | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst diff --git a/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst b/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst new file mode 100644 index 000000000000000..c731eca7ca31adb --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst @@ -0,0 +1,3 @@ +xml.etree: xmlparser_gc_clear() now sets self.parser to NULL to prevent a +crash in xmlparser_dealloc() if xmlparser_gc_clear() was called previously +by the garbage collector, because the parser was part of a reference cycle. diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index 28d0181594b846e..7cde09ad2672626 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -3411,7 +3411,10 @@ xmlparser_gc_traverse(XMLParserObject *self, visitproc visit, void *arg) static int xmlparser_gc_clear(XMLParserObject *self) { - EXPAT(ParserFree)(self->parser); + if (self->parser != NULL) { + EXPAT(ParserFree)(self->parser); + self->parser = NULL; + } Py_CLEAR(self->handle_close); Py_CLEAR(self->handle_pi); From 3207e77b2f77d03d8ea806491e6a0011aac0dceb Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 18 Sep 2017 11:42:36 +0200 Subject: [PATCH 2/5] Add an unit test for the fixed crash Co-Authored-By: Serhiy Storchaka --- Lib/test/test_xml_etree_c.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/Lib/test/test_xml_etree_c.py b/Lib/test/test_xml_etree_c.py index 171a3f88b9a1f0e..25517a7269c9c01 100644 --- a/Lib/test/test_xml_etree_c.py +++ b/Lib/test/test_xml_etree_c.py @@ -65,6 +65,26 @@ def test_trashcan(self): del root support.gc_collect() + def test_parser_ref_cycle(self): + # bpo-31499: xmlparser_dealloc() crashed with a segmentation fault when + # xmlparser_gc_clear() was called previously by the garbage collector, + # when the parser was part of a reference cycle. + + def parser_ref_cycle(): + parser = cET.XMLParser() + # Create a reference cycle using an exception to keep the frame + # alive, so the parser will be destroyed by the garbage collector + try: + raise ValueError + except ValueError as exc: + err = exc + + # Create a parser part of reference cycle + parser_ref_cycle() + # Trigger an explicit garbage collection to break the reference cycle + # and so destroy the parser + support.gc_collect() + @unittest.skipUnless(cET, 'requires _elementtree') class TestAliasWorking(unittest.TestCase): From 24fd3824f949257e470f90b85b24656048fd0c01 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 18 Sep 2017 12:04:29 +0200 Subject: [PATCH 3/5] Simplify the NEWS entry. --- .../next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst b/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst index c731eca7ca31adb..491efb0a364bde2 100644 --- a/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst +++ b/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst @@ -1,3 +1 @@ -xml.etree: xmlparser_gc_clear() now sets self.parser to NULL to prevent a -crash in xmlparser_dealloc() if xmlparser_gc_clear() was called previously -by the garbage collector, because the parser was part of a reference cycle. +xml.etree: Fix a crash when a parser if part of a reference cycle. From b06208e4cc4961c7a6156c48a0ce7c876c8d803f Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 18 Sep 2017 12:05:10 +0200 Subject: [PATCH 4/5] Make sure that xmlparser_gc_clear() is reentrant. --- Modules/_elementtree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index 7cde09ad2672626..bddac851d9c708e 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -3412,8 +3412,9 @@ static int xmlparser_gc_clear(XMLParserObject *self) { if (self->parser != NULL) { - EXPAT(ParserFree)(self->parser); + XML_Parser parser = self->parser; self->parser = NULL; + EXPAT(ParserFree)(parser); } Py_CLEAR(self->handle_close); From 0a28979eedbb294a90a17ec046df2c1776522d34 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 18 Sep 2017 12:06:25 +0200 Subject: [PATCH 5/5] Fix typo in the NEWS entry. --- .../next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst b/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst index 491efb0a364bde2..22af29fdecbed93 100644 --- a/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst +++ b/Misc/NEWS.d/next/Library/2017-09-18-10-57-04.bpo-31499.BydYhf.rst @@ -1 +1 @@ -xml.etree: Fix a crash when a parser if part of a reference cycle. +xml.etree: Fix a crash when a parser is part of a reference cycle.