From 6f2dda44a28fc468f79697c8cae7d2ea06b129d0 Mon Sep 17 00:00:00 2001 From: Dong-hee Na Date: Sat, 19 Oct 2019 23:58:33 +0900 Subject: [PATCH 1/8] bpo-38525: Fix dictreviter_iternext for empty dict --- Lib/test/test_dict.py | 6 ++++++ .../next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst | 1 + Objects/dictobject.c | 2 +- 3 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index 13be857f7ab607f..d22eb109862f311 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1312,6 +1312,12 @@ def test_reversed(self): self.assertEqual(list(r), list('dcba')) self.assertRaises(StopIteration, next, r) + def test_empty_reversed(self): + r = reversed({}) + self.assertEqual(list(r), []) + r = reversed({}.items()) + self.assertEqual(list(r), []) + def test_dict_copy_order(self): # bpo-34320 od = collections.OrderedDict([('a', 1), ('b', 2)]) diff --git a/Misc/NEWS.d/next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst b/Misc/NEWS.d/next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst new file mode 100644 index 000000000000000..125583f23de523f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst @@ -0,0 +1 @@ +Fix dictreviter_iternext for empty :class:`dict` Patch by Dong-hee Na. diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 64876e05191e237..d694d193023fa70 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -3826,7 +3826,7 @@ dictreviter_iternext(dictiterobject *di) PyObject *key, *value, *result; if (d->ma_values) { - if (i < 0) { + if (i < 0 || i >= d->ma_used) { goto fail; } key = DK_ENTRIES(k)[i].me_key; From dc1a73ad8ae158ae2daa8b51eb16b91f0f3030b4 Mon Sep 17 00:00:00 2001 From: Dong-hee Na Date: Sun, 20 Oct 2019 00:12:29 +0900 Subject: [PATCH 2/8] bpo-38525: Apply codereview --- Lib/test/test_dict.py | 17 ++++++++++++----- .../2019-10-19-23-58-09.bpo-38525.CzEl2G.rst | 3 ++- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index d22eb109862f311..497a0a0d653c862 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1312,11 +1312,18 @@ def test_reversed(self): self.assertEqual(list(r), list('dcba')) self.assertRaises(StopIteration, next, r) - def test_empty_reversed(self): - r = reversed({}) - self.assertEqual(list(r), []) - r = reversed({}.items()) - self.assertEqual(list(r), []) + def test_reverse_iterator_for_empty_dict(self): + empty_dict = {} + self.assertEqual(list(reversed(empty_dict)), []) + self.assertEqual(list(reversed(empty_dict.items())), []) + self.assertEqual(list(reversed(empty_dict.values())), []) + self.assertEqual(list(reversed(empty_dict.keys())), []) + + empty_dict = dict() + self.assertEqual(list(reversed(empty_dict)), []) + self.assertEqual(list(reversed(empty_dict.items())), []) + self.assertEqual(list(reversed(empty_dict.values())), []) + self.assertEqual(list(reversed(empty_dict.keys())), []) def test_dict_copy_order(self): # bpo-34320 diff --git a/Misc/NEWS.d/next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst b/Misc/NEWS.d/next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst index 125583f23de523f..6580932c94fc2f7 100644 --- a/Misc/NEWS.d/next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst +++ b/Misc/NEWS.d/next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst @@ -1 +1,2 @@ -Fix dictreviter_iternext for empty :class:`dict` Patch by Dong-hee Na. +Fix a segmentation fault when using reverse iterators of empty ``dict`` +objects that were created as empty ``dict`` literals. Patch by Dong-hee Na. From 2d1c5888b32d0bfef7f231e7fc6938dbd7d36e7a Mon Sep 17 00:00:00 2001 From: Dong-hee Na Date: Sun, 20 Oct 2019 00:36:40 +0900 Subject: [PATCH 3/8] bpo-38525: Apply vstinner's review --- Lib/test/test_dict.py | 18 +++++++----------- .../2019-10-20-00-36-18.bpo-38525.Vty1cA.rst} | 0 2 files changed, 7 insertions(+), 11 deletions(-) rename Misc/NEWS.d/next/{Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst => Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst} (100%) diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index 497a0a0d653c862..dfa0a06f87e34b9 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1313,17 +1313,13 @@ def test_reversed(self): self.assertRaises(StopIteration, next, r) def test_reverse_iterator_for_empty_dict(self): - empty_dict = {} - self.assertEqual(list(reversed(empty_dict)), []) - self.assertEqual(list(reversed(empty_dict.items())), []) - self.assertEqual(list(reversed(empty_dict.values())), []) - self.assertEqual(list(reversed(empty_dict.keys())), []) - - empty_dict = dict() - self.assertEqual(list(reversed(empty_dict)), []) - self.assertEqual(list(reversed(empty_dict.items())), []) - self.assertEqual(list(reversed(empty_dict.values())), []) - self.assertEqual(list(reversed(empty_dict.keys())), []) + # bpo-38525: revered iterator should work properly + + # empty dict is directly used for reference count test + self.assertEqual(list(reversed({})), []) + self.assertEqual(list(reversed({}.items())), []) + self.assertEqual(list(reversed({}.values())), []) + self.assertEqual(list(reversed({}.keys())), []) def test_dict_copy_order(self): # bpo-34320 diff --git a/Misc/NEWS.d/next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst b/Misc/NEWS.d/next/Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst similarity index 100% rename from Misc/NEWS.d/next/Library/2019-10-19-23-58-09.bpo-38525.CzEl2G.rst rename to Misc/NEWS.d/next/Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst From 15183c5b767d438669db8f73bfccc14a98e629cb Mon Sep 17 00:00:00 2001 From: Dong-hee Na Date: Sun, 20 Oct 2019 00:58:06 +0900 Subject: [PATCH 4/8] bpo-38525: Add dict() test case --- Lib/test/test_dict.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index dfa0a06f87e34b9..fb57317da2c829d 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1321,6 +1321,12 @@ def test_reverse_iterator_for_empty_dict(self): self.assertEqual(list(reversed({}.values())), []) self.assertEqual(list(reversed({}.keys())), []) + # dict() and {} don't trigger the same code path + self.assertEqual(list(reversed(dict())), []) + self.assertEqual(list(reversed(dict().items())), []) + self.assertEqual(list(reversed(dict().values())), []) + self.assertEqual(list(reversed(dict().keys())), []) + def test_dict_copy_order(self): # bpo-34320 od = collections.OrderedDict([('a', 1), ('b', 2)]) From d92a1030a32e10f39ce2badc5fae0c225d9643e3 Mon Sep 17 00:00:00 2001 From: Dong-hee Na Date: Sun, 20 Oct 2019 02:05:48 +0900 Subject: [PATCH 5/8] Update news --- .../2019-10-20-00-36-18.bpo-38525.Vty1cA.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst b/Misc/NEWS.d/next/Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst index 6580932c94fc2f7..dbfb943de218ae3 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst @@ -1,2 +1,2 @@ -Fix a segmentation fault when using reverse iterators of empty ``dict`` -objects that were created as empty ``dict`` literals. Patch by Dong-hee Na. +Fix a segmentation fault when using reverse iterators of empty ``dict`` objects. +Patch by Dong-hee Na. From 8618a7a582fdab34618765768cb3f41384b3eeee Mon Sep 17 00:00:00 2001 From: Dong-hee Na Date: Sun, 20 Oct 2019 02:31:52 +0900 Subject: [PATCH 6/8] bpo-38525: di_pos must be initialized from ma_used when dict is key sharing dict --- Lib/test/test_dict.py | 10 ++++++++++ .../2019-10-20-00-36-18.bpo-38525.Vty1cA.rst | 2 +- Objects/dictobject.c | 5 +++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index fb57317da2c829d..5b513765f7b08af 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1327,6 +1327,16 @@ def test_reverse_iterator_for_empty_dict(self): self.assertEqual(list(reversed(dict().values())), []) self.assertEqual(list(reversed(dict().keys())), []) + def test_reverse_iterator_for_shared_shared_dicts(self): + class A: + def __init__(self, x, y): + if x: self.x = x + if y: self.y = y + + self.assertEqual(list(reversed(A(1, 2).__dict__)), ['y', 'x']) + self.assertEqual(list(reversed(A(1, 0).__dict__)), ['x']) + self.assertEqual(list(reversed(A(0, 1).__dict__)), ['y']) + def test_dict_copy_order(self): # bpo-34320 od = collections.OrderedDict([('a', 1), ('b', 2)]) diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst b/Misc/NEWS.d/next/Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst index dbfb943de218ae3..c74d143762253d8 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2019-10-20-00-36-18.bpo-38525.Vty1cA.rst @@ -1,2 +1,2 @@ Fix a segmentation fault when using reverse iterators of empty ``dict`` objects. -Patch by Dong-hee Na. +Patch by Dong-hee Na and Inada Naoki. diff --git a/Objects/dictobject.c b/Objects/dictobject.c index d694d193023fa70..567c288d1f5e308 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -3455,7 +3455,12 @@ dictiter_new(PyDictObject *dict, PyTypeObject *itertype) if ((itertype == &PyDictRevIterKey_Type || itertype == &PyDictRevIterItem_Type || itertype == &PyDictRevIterValue_Type) && dict->ma_used) { + if (dict->ma_values) { + di->di_pos = dict->ma_used - 1; + } + else { di->di_pos = dict->ma_keys->dk_nentries - 1; + } } else { di->di_pos = 0; From b86f723acd1d7bf64b44f7a79d7b55721f5f304f Mon Sep 17 00:00:00 2001 From: Dong-hee Na Date: Sun, 20 Oct 2019 02:50:28 +0900 Subject: [PATCH 7/8] bpo-38525: update --- Objects/dictobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 567c288d1f5e308..caa7d8710047433 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -3452,9 +3452,9 @@ dictiter_new(PyDictObject *dict, PyTypeObject *itertype) di->di_dict = dict; di->di_used = dict->ma_used; di->len = dict->ma_used; - if ((itertype == &PyDictRevIterKey_Type || + if (itertype == &PyDictRevIterKey_Type || itertype == &PyDictRevIterItem_Type || - itertype == &PyDictRevIterValue_Type) && dict->ma_used) { + itertype == &PyDictRevIterValue_Type) { if (dict->ma_values) { di->di_pos = dict->ma_used - 1; } From bf61754fdf6207f6572fe618316f137f94d6a879 Mon Sep 17 00:00:00 2001 From: Dong-hee Na Date: Sun, 20 Oct 2019 02:53:05 +0900 Subject: [PATCH 8/8] bpo-38525: Update --- Objects/dictobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index caa7d8710047433..5ac7bb102b211b8 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -3831,7 +3831,7 @@ dictreviter_iternext(dictiterobject *di) PyObject *key, *value, *result; if (d->ma_values) { - if (i < 0 || i >= d->ma_used) { + if (i < 0) { goto fail; } key = DK_ENTRIES(k)[i].me_key;