From e75d9fb94b2c61e88f6fb97d6b1541c818e62728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Thu, 7 Mar 2019 10:29:04 +0100 Subject: [PATCH 1/9] Fix sorting of heterogeneous values in list.sort --- Lib/test/test_sort.py | 3 +++ Objects/listobject.c | 24 ++++++++++++++---------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/Lib/test/test_sort.py b/Lib/test/test_sort.py index f2f53cb1a72f63..f41ca0f0581735 100644 --- a/Lib/test/test_sort.py +++ b/Lib/test/test_sort.py @@ -373,6 +373,9 @@ def test_unsafe_tuple_compare(self): check_against_PyObject_RichCompareBool(self, [float('nan')]*100) check_against_PyObject_RichCompareBool(self, [float('nan') for _ in range(100)]) + + def test_not_all_tuples(self): + self.assertRaises(TypeError, [(1.0, 1.0), (False, "A"), 6].sort) #============================================================================== if __name__ == "__main__": diff --git a/Objects/listobject.c b/Objects/listobject.c index b6524e8bd7fc5d..9f3c2a6affda47 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -2305,6 +2305,7 @@ list_sort_impl(PyListObject *self, PyObject *keyfunc, int reverse) lo.keys[i]); if (key->ob_type != key_type) { + keys_are_in_tuples = 0; keys_are_all_same_type = 0; break; } @@ -2338,21 +2339,24 @@ list_sort_impl(PyListObject *self, PyObject *keyfunc, int reverse) else { ms.key_compare = safe_object_compare; } + + if (keys_are_in_tuples) { + /* Make sure we're not dealing with tuples of tuples + * (remember: here, key_type refers list [key[0] for key in keys]) */ + if (key_type == &PyTuple_Type) { + ms.tuple_elem_compare = safe_object_compare; + } + else { + ms.tuple_elem_compare = ms.key_compare; + } + + ms.key_compare = unsafe_tuple_compare; + } } else { ms.key_compare = safe_object_compare; } - if (keys_are_in_tuples) { - /* Make sure we're not dealing with tuples of tuples - * (remember: here, key_type refers list [key[0] for key in keys]) */ - if (key_type == &PyTuple_Type) - ms.tuple_elem_compare = safe_object_compare; - else - ms.tuple_elem_compare = ms.key_compare; - - ms.key_compare = unsafe_tuple_compare; - } } /* End of pre-sort check: ms is now set properly! */ From 8498a21a71c3724861aa858513a3ddd0809f14f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Thu, 7 Mar 2019 13:06:13 +0100 Subject: [PATCH 2/9] Add blurb --- .../Core and Builtins/2019-03-07-13-05-43.bpo-36218.dZemNt.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2019-03-07-13-05-43.bpo-36218.dZemNt.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-03-07-13-05-43.bpo-36218.dZemNt.rst b/Misc/NEWS.d/next/Core and Builtins/2019-03-07-13-05-43.bpo-36218.dZemNt.rst new file mode 100644 index 00000000000000..462700be06810d --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2019-03-07-13-05-43.bpo-36218.dZemNt.rst @@ -0,0 +1,2 @@ +Fix a segfault occuring when sorting a list of heterogeneous values. Patch +contributed by Rémi Lapeyre. From 60aca1e7cd4970df32bfe3a3ec2fe152f4e0c468 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Tue, 12 Mar 2019 01:54:07 +0100 Subject: [PATCH 3/9] Remove early exit when keys are tuples --- .../2019-03-07-13-05-43.bpo-36218.dZemNt.rst | 2 +- Objects/listobject.c | 31 ++++++++++--------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-03-07-13-05-43.bpo-36218.dZemNt.rst b/Misc/NEWS.d/next/Core and Builtins/2019-03-07-13-05-43.bpo-36218.dZemNt.rst index 462700be06810d..ab6d207f37e201 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2019-03-07-13-05-43.bpo-36218.dZemNt.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2019-03-07-13-05-43.bpo-36218.dZemNt.rst @@ -1,2 +1,2 @@ Fix a segfault occuring when sorting a list of heterogeneous values. Patch -contributed by Rémi Lapeyre. +contributed by Rémi Lapeyre and Elliot Gorokhovsky. \ No newline at end of file diff --git a/Objects/listobject.c b/Objects/listobject.c index 9f3c2a6affda47..dcd3888189bde5 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -2305,9 +2305,12 @@ list_sort_impl(PyListObject *self, PyObject *keyfunc, int reverse) lo.keys[i]); if (key->ob_type != key_type) { - keys_are_in_tuples = 0; keys_are_all_same_type = 0; - break; + /* If keys are in tuple we must loop over the whole list to make + sure all items are tuples */ + if (!keys_are_in_tuples) { + break; + } } if (key_type == &PyLong_Type) { @@ -2340,23 +2343,23 @@ list_sort_impl(PyListObject *self, PyObject *keyfunc, int reverse) ms.key_compare = safe_object_compare; } - if (keys_are_in_tuples) { - /* Make sure we're not dealing with tuples of tuples - * (remember: here, key_type refers list [key[0] for key in keys]) */ - if (key_type == &PyTuple_Type) { - ms.tuple_elem_compare = safe_object_compare; - } - else { - ms.tuple_elem_compare = ms.key_compare; - } - - ms.key_compare = unsafe_tuple_compare; - } } else { ms.key_compare = safe_object_compare; } + if (keys_are_in_tuples) { + /* Make sure we're not dealing with tuples of tuples + * (remember: here, key_type refers list [key[0] for key in keys]) */ + if (key_type == &PyTuple_Type) { + ms.tuple_elem_compare = safe_object_compare; + } + else { + ms.tuple_elem_compare = ms.key_compare; + } + + ms.key_compare = unsafe_tuple_compare; + } } /* End of pre-sort check: ms is now set properly! */ From eaa7c505e425c7c102eb640eb55dfcea3f68feed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Tue, 12 Mar 2019 16:47:38 +0100 Subject: [PATCH 4/9] Fix bug found by test_json --- Lib/test/test_sort.py | 1 + Objects/listobject.c | 1 + 2 files changed, 2 insertions(+) diff --git a/Lib/test/test_sort.py b/Lib/test/test_sort.py index f41ca0f0581735..cedf73e8fa0136 100644 --- a/Lib/test/test_sort.py +++ b/Lib/test/test_sort.py @@ -376,6 +376,7 @@ def test_unsafe_tuple_compare(self): def test_not_all_tuples(self): self.assertRaises(TypeError, [(1.0, 1.0), (False, "A"), 6].sort) + self.assertRaises(TypeError, [('a', 1), (1, 'a')].sort) #============================================================================== if __name__ == "__main__": diff --git a/Objects/listobject.c b/Objects/listobject.c index dcd3888189bde5..ab7d749fb0c4fe 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -2308,6 +2308,7 @@ list_sort_impl(PyListObject *self, PyObject *keyfunc, int reverse) keys_are_all_same_type = 0; /* If keys are in tuple we must loop over the whole list to make sure all items are tuples */ + key_type = key->ob_type; if (!keys_are_in_tuples) { break; } From 3b02a05847194d68456fec3e25d12f763dc87837 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Tue, 12 Mar 2019 16:48:23 +0100 Subject: [PATCH 5/9] Remove empty line --- Objects/listobject.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index ab7d749fb0c4fe..fb128fe1a4d372 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -2343,7 +2343,6 @@ list_sort_impl(PyListObject *self, PyObject *keyfunc, int reverse) else { ms.key_compare = safe_object_compare; } - } else { ms.key_compare = safe_object_compare; From 47a06889a956b3337f807dead43915f41bc5dd69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Thu, 21 Mar 2019 12:06:23 +0100 Subject: [PATCH 6/9] Simplify logic in pre-sort check --- Objects/listobject.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index fb128fe1a4d372..b3c7d6ccda55c5 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -2308,20 +2308,21 @@ list_sort_impl(PyListObject *self, PyObject *keyfunc, int reverse) keys_are_all_same_type = 0; /* If keys are in tuple we must loop over the whole list to make sure all items are tuples */ - key_type = key->ob_type; if (!keys_are_in_tuples) { break; } } - if (key_type == &PyLong_Type) { - if (ints_are_bounded && Py_ABS(Py_SIZE(key)) > 1) - ints_are_bounded = 0; - } - else if (key_type == &PyUnicode_Type){ - if (strings_are_latin && - PyUnicode_KIND(key) != PyUnicode_1BYTE_KIND) - strings_are_latin = 0; + if (keys_are_all_same_type) { + if (key_type == &PyLong_Type) { + if (ints_are_bounded && Py_ABS(Py_SIZE(key)) > 1) + ints_are_bounded = 0; + } + else if (key_type == &PyUnicode_Type){ + if (strings_are_latin && + PyUnicode_KIND(key) != PyUnicode_1BYTE_KIND) + strings_are_latin = 0; + } } } From 467cd16465ff001a42d97d037eacf4aa6f35013a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Thu, 21 Mar 2019 12:06:39 +0100 Subject: [PATCH 7/9] Add test --- Lib/test/test_sort.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_sort.py b/Lib/test/test_sort.py index cedf73e8fa0136..41de4b684f6251 100644 --- a/Lib/test/test_sort.py +++ b/Lib/test/test_sort.py @@ -377,6 +377,7 @@ def test_unsafe_tuple_compare(self): def test_not_all_tuples(self): self.assertRaises(TypeError, [(1.0, 1.0), (False, "A"), 6].sort) self.assertRaises(TypeError, [('a', 1), (1, 'a')].sort) + self.assertRaises(TypeError, [(1, 'a'), ('a', 1)].sort) #============================================================================== if __name__ == "__main__": From 8b2224fad0f6a2d3032e4838774c80d2edda69af Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Thu, 21 Mar 2019 14:03:03 +0100 Subject: [PATCH 8/9] Fix coding style Co-Authored-By: remilapeyre --- Objects/listobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index b3c7d6ccda55c5..7fde90dbca4d3c 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -2318,7 +2318,7 @@ list_sort_impl(PyListObject *self, PyObject *keyfunc, int reverse) if (ints_are_bounded && Py_ABS(Py_SIZE(key)) > 1) ints_are_bounded = 0; } - else if (key_type == &PyUnicode_Type){ + else if (key_type == &PyUnicode_Type) { if (strings_are_latin && PyUnicode_KIND(key) != PyUnicode_1BYTE_KIND) strings_are_latin = 0; From 6286a4bf12bfd4b4d5141582e7a192dbaf606749 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Fri, 22 Mar 2019 10:28:39 +0100 Subject: [PATCH 9/9] Fix indentation --- Objects/listobject.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 7fde90dbca4d3c..c29954283c488e 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -2314,17 +2314,20 @@ list_sort_impl(PyListObject *self, PyObject *keyfunc, int reverse) } if (keys_are_all_same_type) { - if (key_type == &PyLong_Type) { - if (ints_are_bounded && Py_ABS(Py_SIZE(key)) > 1) - ints_are_bounded = 0; + if (key_type == &PyLong_Type && + ints_are_bounded && + Py_ABS(Py_SIZE(key)) > 1) { + + ints_are_bounded = 0; } - else if (key_type == &PyUnicode_Type) { - if (strings_are_latin && - PyUnicode_KIND(key) != PyUnicode_1BYTE_KIND) - strings_are_latin = 0; + else if (key_type == &PyUnicode_Type && + strings_are_latin && + PyUnicode_KIND(key) != PyUnicode_1BYTE_KIND) { + + strings_are_latin = 0; + } } } - } /* Choose the best compare, given what we now know about the keys. */ if (keys_are_all_same_type) {