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 From 8bc792a602311e426cbd69293627fdc9287a5c7b Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 22 Nov 2015 14:49:58 +0200 Subject: Issue #25624: ZipFile now always writes a ZIP_STORED header for directory entries. Patch by Dingyuan Wang. --- Lib/test/test_shutil.py | 23 +++++++++++++++++++++++ Lib/zipfile.py | 4 +++- Misc/ACKS | 1 + Misc/NEWS | 3 +++ 4 files changed, 30 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index 673d36b..d2d4d03 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -1099,6 +1099,29 @@ class TestShutil(unittest.TestCase): names2 = zf.namelist() self.assertEqual(sorted(names), sorted(names2)) + @requires_zlib + @unittest.skipUnless(ZIP_SUPPORT, 'Need zip support to run') + @unittest.skipUnless(shutil.which('unzip'), + 'Need the unzip command to run') + def test_unzip_zipfile(self): + root_dir, base_dir = self._create_files() + base_name = os.path.join(self.mkdtemp(), 'archive') + archive = make_archive(base_name, 'zip', root_dir, base_dir) + + # check if ZIP file was created + self.assertEqual(archive, base_name + '.zip') + self.assertTrue(os.path.isfile(archive)) + + # now check the ZIP file using `unzip -t` + zip_cmd = ['unzip', '-t', archive] + with support.change_cwd(root_dir): + try: + subprocess.check_output(zip_cmd, stderr=subprocess.STDOUT) + except subprocess.CalledProcessError as exc: + details = exc.output.decode(errors="replace") + msg = "{}\n\n**Unzip Output**\n{}" + self.fail(msg.format(exc, details)) + def test_make_archive(self): tmpdir = self.mkdtemp() base_name = os.path.join(tmpdir, 'archive') diff --git a/Lib/zipfile.py b/Lib/zipfile.py index bda6134..11d7cf9 100644 --- a/Lib/zipfile.py +++ b/Lib/zipfile.py @@ -1337,7 +1337,9 @@ class ZipFile: arcname += '/' zinfo = ZipInfo(arcname, date_time) zinfo.external_attr = (st[0] & 0xFFFF) << 16 # Unix attributes - if compress_type is None: + if isdir: + zinfo.compress_type = ZIP_STORED + elif compress_type is None: zinfo.compress_type = self.compression else: zinfo.compress_type = compress_type diff --git a/Misc/ACKS b/Misc/ACKS index ec572a1..4b41efa 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -1471,6 +1471,7 @@ Richard Walker Larry Wall Kevin Walzer Rodrigo Steinmuller Wanderley +Dingyuan Wang Ke Wang Greg Ward Tom Wardill diff --git a/Misc/NEWS b/Misc/NEWS index 6a5f53e..a13e198 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -106,6 +106,9 @@ Core and Builtins Library ------- +- Issue #25624: ZipFile now always writes a ZIP_STORED header for directory + entries. Patch by Dingyuan Wang. + - Issue #25583: Avoid incorrect errors raised by os.makedirs(exist_ok=True) when the OS gives priority to errors such as EACCES over EEXIST. -- cgit v0.12 From 21cecb904f80a53a23dc972c95cb0c4c997afbe1 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Sun, 22 Nov 2015 18:20:11 -0800 Subject: Fixes upload directories for Windows installer. --- Tools/msi/bundle/bundle.targets | 2 +- Tools/msi/uploadrelease.proj | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Tools/msi/bundle/bundle.targets b/Tools/msi/bundle/bundle.targets index 57ca1dc..aeeff3b 100644 --- a/Tools/msi/bundle/bundle.targets +++ b/Tools/msi/bundle/bundle.targets @@ -18,7 +18,7 @@ $(DownloadUrlBase.TrimEnd(`/`))/{version}/{arch}{releasename}/{msi} - $(DefineConstants);DownloadUrl=$(DownloadUrl.Replace(`{version}`, `$(MajorVersionNumber).$(MinorVersionNumber).$(MicroVersionNumber)`).Replace(`{arch}`, `$(ArchName)`).Replace(`{releasename}`, `$(ReleaseName)`).Replace(`{msi}`, `{2}`)) + $(DefineConstants);DownloadUrl=$(DownloadUrl.Replace(`{version}`, `$(MajorVersionNumber).$(MinorVersionNumber).$(MicroVersionNumber)`).Replace(`{arch}`, `$(ArchName)`).Replace(`{releasename}`, `$(ReleaseLevelName)`).Replace(`{msi}`, `{2}`)) $(DefineConstants);DownloadUrl={2} diff --git a/Tools/msi/uploadrelease.proj b/Tools/msi/uploadrelease.proj index ec4ede5..0d472de 100644 --- a/Tools/msi/uploadrelease.proj +++ b/Tools/msi/uploadrelease.proj @@ -16,7 +16,7 @@ $(DownloadUrlBase.TrimEnd(`/`))/$(MajorVersionNumber).$(MinorVersionNumber).$(MicroVersionNumber) - $(DownloadUrl.TrimEnd(`/`)) + $(DownloadUrl.Replace(`{version}`, `$(MajorVersionNumber).$(MinorVersionNumber).$(MicroVersionNumber)`).Replace(`{arch}`, `$(ArchName)`).Replace(`{releasename}`, `$(ReleaseLevelName)`).Replace(`{msi}`, ``).TrimEnd(`/`)) -- cgit v0.12 From b6aa5375d5a2f81370338357506034befe62aa31 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 23 Nov 2015 08:42:25 +0200 Subject: Issue #25691: Fixed crash on deleting ElementTree.Element attributes. --- Lib/test/test_xml_etree.py | 27 +++++++++++++++++++++++++++ Lib/test/test_xml_etree_c.py | 32 ++++++++++++++++++++++++++++++++ Misc/NEWS | 2 ++ Modules/_elementtree.c | 6 ++++++ 4 files changed, 67 insertions(+) diff --git a/Lib/test/test_xml_etree.py b/Lib/test/test_xml_etree.py index 9261592..8511cfe 100644 --- a/Lib/test/test_xml_etree.py +++ b/Lib/test/test_xml_etree.py @@ -244,6 +244,33 @@ class ElementTreeTest(unittest.TestCase): self.assertEqual(ET.XML, ET.fromstring) self.assertEqual(ET.PI, ET.ProcessingInstruction) + def test_set_attribute(self): + element = ET.Element('tag') + + self.assertEqual(element.tag, 'tag') + element.tag = 'Tag' + self.assertEqual(element.tag, 'Tag') + element.tag = 'TAG' + self.assertEqual(element.tag, 'TAG') + + self.assertIsNone(element.text) + element.text = 'Text' + self.assertEqual(element.text, 'Text') + element.text = 'TEXT' + self.assertEqual(element.text, 'TEXT') + + self.assertIsNone(element.tail) + element.tail = 'Tail' + self.assertEqual(element.tail, 'Tail') + element.tail = 'TAIL' + self.assertEqual(element.tail, 'TAIL') + + self.assertEqual(element.attrib, {}) + element.attrib = {'a': 'b', 'c': 'd'} + self.assertEqual(element.attrib, {'a': 'b', 'c': 'd'}) + element.attrib = {'A': 'B', 'C': 'D'} + self.assertEqual(element.attrib, {'A': 'B', 'C': 'D'}) + def test_simpleops(self): # Basic method sanity checks. diff --git a/Lib/test/test_xml_etree_c.py b/Lib/test/test_xml_etree_c.py index 816aa86..409ec6a 100644 --- a/Lib/test/test_xml_etree_c.py +++ b/Lib/test/test_xml_etree_c.py @@ -22,6 +22,38 @@ class MiscTests(unittest.TestCase): finally: data = None + def test_del_attribute(self): + element = cET.Element('tag') + + element.tag = 'TAG' + with self.assertRaises(AttributeError): + del element.tag + self.assertEqual(element.tag, 'TAG') + + with self.assertRaises(AttributeError): + del element.text + self.assertIsNone(element.text) + element.text = 'TEXT' + with self.assertRaises(AttributeError): + del element.text + self.assertEqual(element.text, 'TEXT') + + with self.assertRaises(AttributeError): + del element.tail + self.assertIsNone(element.tail) + element.tail = 'TAIL' + with self.assertRaises(AttributeError): + del element.tail + self.assertEqual(element.tail, 'TAIL') + + with self.assertRaises(AttributeError): + del element.attrib + self.assertEqual(element.attrib, {}) + element.attrib = {'A': 'B', 'C': 'D'} + with self.assertRaises(AttributeError): + del element.attrib + self.assertEqual(element.attrib, {'A': 'B', 'C': 'D'}) + @unittest.skipUnless(cET, 'requires _elementtree') class TestAliasWorking(unittest.TestCase): diff --git a/Misc/NEWS b/Misc/NEWS index a13e198..635da46 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -106,6 +106,8 @@ Core and Builtins Library ------- +- Issue #25691: Fixed crash on deleting ElementTree.Element attributes. + - Issue #25624: ZipFile now always writes a ZIP_STORED header for directory entries. Patch by Dingyuan Wang. diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index da784a0..3b6ee89 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -1852,6 +1852,12 @@ static int element_setattro(ElementObject* self, PyObject* nameobj, PyObject* value) { char *name = ""; + + if (value == NULL) { + PyErr_SetString(PyExc_AttributeError, + "can't delete attribute"); + return -1; + } if (PyUnicode_Check(nameobj)) name = _PyUnicode_AsString(nameobj); if (name == NULL) -- cgit v0.12