classification
Title: Strange reversed dict behavior
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Harmon758, corona10, inada.naoki, ivb, miss-islington, ned.deily, pablogsal, remi.lapeyre, serhiy.storchaka, xtreak
Priority: critical Keywords: patch

Created on 2019-10-19 08:00 by ivb, last changed 2019-11-14 10:48 by Harmon758. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 16846 merged corona10, 2019-10-19 15:00
PR 16847 closed pablogsal, 2019-10-19 15:00
PR 16853 merged miss-islington, 2019-10-19 20:01
Messages (15)
msg354931 - (view) Author: Ivan Bykov (ivb) Date: 2019-10-19 08:00
Python 3.8.0 (tags/v3.8.0:fa919fd, Oct 14 2019, 19:37:50) [MSC v.1916 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license()" for more information.
>>> list(reversed({1: 1}))
[1]
>>> list(reversed({}))

================================ RESTART: Shell ================================
>>>
msg354932 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-10-19 08:14
Outside of IDLE, the example causes a segfault.  With debug build of current master HEAD:

Assertion failed: (value != NULL), function dictreviter_iternext, file ../../source/Objects/dictobject.c, line 3834.

    if (d->ma_values) {
        if (i < 0) {
            goto fail;
        }
        key = DK_ENTRIES(k)[i].me_key;
        value = d->ma_values[i];
        assert (value != NULL);
    }
msg354933 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-10-19 08:17
Thanks for the report, by the way!
msg354938 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-10-19 13:16
This issue is related to reversed dict iter.

>>> a = reversed({})
>>> next(a)
Assertion failed: (value != NULL), function dictreviter_iternext, file Objects/dictobject.c, line 3834.
[1]    1366 abort      ./python.exe
msg354939 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-10-19 14:03
>>> reversed({}.items())
<dict_reverseitemiterator object at 0x106a2bc50>
>>> a = reversed({}.items())
>>> next(a)
Assertion failed: (value != NULL), function dictreviter_iternext, file Objects/dictobject.c, line 3834.
[1]    4106 abort      ./python.exe
msg354940 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-10-19 14:08
Can I take look at this issue?
msg354944 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-10-19 15:04
Wops, we made a PR at the same time, Dong-hee Na.

I will close mine :)
msg354945 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-10-19 15:18
> Wops, we made a PR at the same time, Dong-hee Na.

lol

> I will close mine :)

Oh.. thank you for giving me a chance.
msg354946 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-10-19 15:53
The proposed fix fixes a crash, but there is other bug in iterating shared dicts.

class A:
    def __init__(self, x, y):
        if x: self.x = x
        if y: self.y = y

a = A(1, 2)
print(list(iter(a.__dict__)))
print(list(reversed(a.__dict__)))
b = A(1, 0)
print(list(iter(b.__dict__)))
print(list(reversed(b.__dict__)))

With PR 16846 the last print outputs [] instead of expected ['x']. It crashes without PR 16846, so this issue is not only about empty dicts.
msg354947 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-10-19 16:21
> The proposed fix fixes a crash, but there is other bug in iterating shared dicts.

Yes, you are right.
But I can not work on this issue today.
(I have to finalize my paperwork by today :-()
since priority is critical if other core developers finalize this issue today. It will be okay for me.
msg354948 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-10-19 16:24
FYI

c = A(0, 1)
print(type(c.__dict__))
print(list(iter(c.__dict__)))
print(list(reversed(c.__dict__)))

works as we expected.
msg354949 - (view) Author: Inada Naoki (inada.naoki) * (Python committer) Date: 2019-10-19 17:24
When dict is empty, di_pos of reverse iterator must be -1, not 0.
Additionally, di_pos must be initialized from ma_used when dict is key sharing dict.

diff --git a/Objects/dictobject.c b/Objects/dictobject.c
index 64876e0519..6c4b41700b 100644
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -3452,10 +3452,15 @@ 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 ||
-         itertype == &PyDictRevIterItem_Type ||
-         itertype == &PyDictRevIterValue_Type) && dict->ma_used) {
+    if (itertype == &PyDictRevIterKey_Type ||
+            itertype == &PyDictRevIterItem_Type ||
+            itertype == &PyDictRevIterValue_Type) {
+        if (dict->ma_values) {
+            di->di_pos = dict->ma_used - 1;
+        }
+        else {
             di->di_pos = dict->ma_keys->dk_nentries - 1;
+        }
     }
     else {
msg354961 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-10-19 20:01
New changeset 24dc2f8c56697f9ee51a4887cf0814b6600c1815 by Pablo Galindo (Dong-hee Na) in branch 'master':
bpo-38525: Fix a segmentation fault when using reverse iterators of empty dict (GH-16846)
https://github.com/python/cpython/commit/24dc2f8c56697f9ee51a4887cf0814b6600c1815
msg354962 - (view) Author: miss-islington (miss-islington) Date: 2019-10-19 20:20
New changeset d73205d788a32148ba9a2beaa27badbd94ab65ff by Miss Islington (bot) in branch '3.8':
bpo-38525: Fix a segmentation fault when using reverse iterators of empty dict (GH-16846)
https://github.com/python/cpython/commit/d73205d788a32148ba9a2beaa27badbd94ab65ff
msg354963 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-10-19 20:23
Closing this.

Thanks, Dong-hee Na and everyone involved in the issue!
History
Date User Action Args
2019-11-14 10:48:55Harmon758setnosy: + Harmon758
2019-10-19 20:23:30pablogsalsetstatus: open -> closed
resolution: fixed
messages: + msg354963

stage: patch review -> resolved
2019-10-19 20:20:59miss-islingtonsetnosy: + miss-islington
messages: + msg354962
2019-10-19 20:01:33miss-islingtonsetpull_requests: + pull_request16401
2019-10-19 20:01:11pablogsalsetmessages: + msg354961
2019-10-19 17:24:54inada.naokisetmessages: + msg354949
2019-10-19 16:24:19corona10setmessages: + msg354948
2019-10-19 16:21:51corona10setmessages: + msg354947
2019-10-19 15:53:16serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg354946
2019-10-19 15:18:43corona10setmessages: + msg354945
2019-10-19 15:04:54pablogsalsetmessages: + msg354944
2019-10-19 15:04:11pablogsalsetpriority: normal -> critical
nosy: + pablogsal, remi.lapeyre
2019-10-19 15:00:56pablogsalsetpull_requests: + pull_request16397
2019-10-19 15:00:32corona10setkeywords: + patch
stage: patch review
pull_requests: + pull_request16396
2019-10-19 14:08:58corona10setmessages: + msg354940
2019-10-19 14:03:37corona10setmessages: + msg354939
2019-10-19 13:16:26corona10setnosy: + corona10
messages: + msg354938
2019-10-19 13:04:24xtreaksetnosy: + xtreak
2019-10-19 08:17:05ned.deilysetmessages: + msg354933
2019-10-19 08:14:44ned.deilysetversions: + Python 3.9
nosy: + ned.deily, inada.naoki

messages: + msg354932

type: behavior -> crash
2019-10-19 08:00:52ivbcreate