Issue 38525: Strange reversed dict behavior

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 ================================
>>>

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);
    }

Thanks for the report, by the way!

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

>>> 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

Can I take look at this issue?

Wops, we made a PR at the same time, Dong-hee Na.

I will close mine :)

> 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.

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.

> 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.

FYI

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

works as we expected.

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 {

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

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

Closing this.

Thanks, Dong-hee Na and everyone involved in the issue!

stage: patch review -> resolved

messages: + msg354932

type: behavior -> crash