From 04d759b1e4cb5b2d9ed13555e952565ccd3ce3bd Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 22 Nov 2015 12:18:38 +0200 Subject: Issue #19687: Fixed memory leak on failed Element slice assignment. Added new tests for Element slice assignments. --- Lib/test/test_xml_etree.py | 75 ++++++++++++++++++++++++++++++++++++++++++++++ Modules/_elementtree.c | 36 ++++++++++------------ 2 files changed, 90 insertions(+), 21 deletions(-) diff --git a/Lib/test/test_xml_etree.py b/Lib/test/test_xml_etree.py index d879245..9261592 100644 --- a/Lib/test/test_xml_etree.py +++ b/Lib/test/test_xml_etree.py @@ -2350,6 +2350,7 @@ class ElementSlicingTest(unittest.TestCase): self.assertEqual(e[-2].tag, 'a8') self.assertRaises(IndexError, lambda: e[12]) + self.assertRaises(IndexError, lambda: e[-12]) def test_getslice_range(self): e = self._make_elem_with_children(6) @@ -2368,12 +2369,17 @@ class ElementSlicingTest(unittest.TestCase): self.assertEqual(self._elem_tags(e[::3]), ['a0', 'a3', 'a6', 'a9']) self.assertEqual(self._elem_tags(e[::8]), ['a0', 'a8']) self.assertEqual(self._elem_tags(e[1::8]), ['a1', 'a9']) + self.assertEqual(self._elem_tags(e[3::sys.maxsize]), ['a3']) + self.assertEqual(self._elem_tags(e[3::sys.maxsize<<64]), ['a3']) def test_getslice_negative_steps(self): e = self._make_elem_with_children(4) self.assertEqual(self._elem_tags(e[::-1]), ['a3', 'a2', 'a1', 'a0']) self.assertEqual(self._elem_tags(e[::-2]), ['a3', 'a1']) + self.assertEqual(self._elem_tags(e[3::-sys.maxsize]), ['a3']) + self.assertEqual(self._elem_tags(e[3::-sys.maxsize-1]), ['a3']) + self.assertEqual(self._elem_tags(e[3::-sys.maxsize<<64]), ['a3']) def test_delslice(self): e = self._make_elem_with_children(4) @@ -2400,6 +2406,75 @@ class ElementSlicingTest(unittest.TestCase): del e[::2] self.assertEqual(self._subelem_tags(e), ['a1']) + def test_setslice_single_index(self): + e = self._make_elem_with_children(4) + e[1] = ET.Element('b') + self.assertEqual(self._subelem_tags(e), ['a0', 'b', 'a2', 'a3']) + + e[-2] = ET.Element('c') + self.assertEqual(self._subelem_tags(e), ['a0', 'b', 'c', 'a3']) + + with self.assertRaises(IndexError): + e[5] = ET.Element('d') + with self.assertRaises(IndexError): + e[-5] = ET.Element('d') + self.assertEqual(self._subelem_tags(e), ['a0', 'b', 'c', 'a3']) + + def test_setslice_range(self): + e = self._make_elem_with_children(4) + e[1:3] = [ET.Element('b%s' % i) for i in range(2)] + self.assertEqual(self._subelem_tags(e), ['a0', 'b0', 'b1', 'a3']) + + e = self._make_elem_with_children(4) + e[1:3] = [ET.Element('b')] + self.assertEqual(self._subelem_tags(e), ['a0', 'b', 'a3']) + + e = self._make_elem_with_children(4) + e[1:3] = [ET.Element('b%s' % i) for i in range(3)] + self.assertEqual(self._subelem_tags(e), ['a0', 'b0', 'b1', 'b2', 'a3']) + + def test_setslice_steps(self): + e = self._make_elem_with_children(6) + e[1:5:2] = [ET.Element('b%s' % i) for i in range(2)] + self.assertEqual(self._subelem_tags(e), ['a0', 'b0', 'a2', 'b1', 'a4', 'a5']) + + e = self._make_elem_with_children(6) + with self.assertRaises(ValueError): + e[1:5:2] = [ET.Element('b')] + with self.assertRaises(ValueError): + e[1:5:2] = [ET.Element('b%s' % i) for i in range(3)] + with self.assertRaises(ValueError): + e[1:5:2] = [] + self.assertEqual(self._subelem_tags(e), ['a0', 'a1', 'a2', 'a3', 'a4', 'a5']) + + e = self._make_elem_with_children(4) + e[1::sys.maxsize] = [ET.Element('b')] + self.assertEqual(self._subelem_tags(e), ['a0', 'b', 'a2', 'a3']) + e[1::sys.maxsize<<64] = [ET.Element('c')] + self.assertEqual(self._subelem_tags(e), ['a0', 'c', 'a2', 'a3']) + + def test_setslice_negative_steps(self): + e = self._make_elem_with_children(4) + e[2:0:-1] = [ET.Element('b%s' % i) for i in range(2)] + self.assertEqual(self._subelem_tags(e), ['a0', 'b1', 'b0', 'a3']) + + e = self._make_elem_with_children(4) + with self.assertRaises(ValueError): + e[2:0:-1] = [ET.Element('b')] + with self.assertRaises(ValueError): + e[2:0:-1] = [ET.Element('b%s' % i) for i in range(3)] + with self.assertRaises(ValueError): + e[2:0:-1] = [] + self.assertEqual(self._subelem_tags(e), ['a0', 'a1', 'a2', 'a3']) + + e = self._make_elem_with_children(4) + e[1::-sys.maxsize] = [ET.Element('b')] + self.assertEqual(self._subelem_tags(e), ['a0', 'b', 'a2', 'a3']) + e[1::-sys.maxsize-1] = [ET.Element('c')] + self.assertEqual(self._subelem_tags(e), ['a0', 'c', 'a2', 'a3']) + e[1::-sys.maxsize<<64] = [ET.Element('d')] + self.assertEqual(self._subelem_tags(e), ['a0', 'd', 'a2', 'a3']) + class IOTest(unittest.TestCase): def tearDown(self): diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index 826342a..da784a0 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -1605,7 +1605,7 @@ element_ass_subscr(PyObject* self_, PyObject* item, PyObject* value) Py_ssize_t start, stop, step, slicelen, newlen, cur, i; PyObject* recycle = NULL; - PyObject* seq = NULL; + PyObject* seq; if (!self->extra) { if (create_extra(self, NULL) < 0) @@ -1684,21 +1684,21 @@ element_ass_subscr(PyObject* self_, PyObject* item, PyObject* value) Py_XDECREF(recycle); return 0; } - else { - /* A new slice is actually being assigned */ - seq = PySequence_Fast(value, ""); - if (!seq) { - PyErr_Format( - PyExc_TypeError, - "expected sequence, not \"%.200s\"", Py_TYPE(value)->tp_name - ); - return -1; - } - newlen = PySequence_Size(seq); + + /* A new slice is actually being assigned */ + seq = PySequence_Fast(value, ""); + if (!seq) { + PyErr_Format( + PyExc_TypeError, + "expected sequence, not \"%.200s\"", Py_TYPE(value)->tp_name + ); + return -1; } + newlen = PySequence_Size(seq); if (step != 1 && newlen != slicelen) { + Py_DECREF(seq); PyErr_Format(PyExc_ValueError, "attempt to assign sequence of size %zd " "to extended slice of size %zd", @@ -1710,9 +1710,7 @@ element_ass_subscr(PyObject* self_, PyObject* item, PyObject* value) /* Resize before creating the recycle bin, to prevent refleaks. */ if (newlen > slicelen) { if (element_resize(self, newlen - slicelen) < 0) { - if (seq) { - Py_DECREF(seq); - } + Py_DECREF(seq); return -1; } } @@ -1723,9 +1721,7 @@ element_ass_subscr(PyObject* self_, PyObject* item, PyObject* value) we're done modifying the element */ recycle = PyList_New(slicelen); if (!recycle) { - if (seq) { - Py_DECREF(seq); - } + Py_DECREF(seq); return -1; } for (cur = start, i = 0; i < slicelen; @@ -1753,9 +1749,7 @@ element_ass_subscr(PyObject* self_, PyObject* item, PyObject* value) self->extra->length += newlen - slicelen; - if (seq) { - Py_DECREF(seq); - } + Py_DECREF(seq); /* discard the recycle bin, and everything in it */ Py_XDECREF(recycle); -- cgit v0.12