gh-140551: Fix dict crash if clear is called at lookup stage

[efimov-mikhail]

• Issue: Crash in dict if hash__eq__ function has side effects #140551

[sergey-miryanov]

For the following repro:

d ={} class X(object): def __hash__(self): return 1 def __eq__(self, other): d.clear() d.setdefault(X(), None) d.setdefault(X(), None) d.setdefault(X(), None) assert len(d) == 1 print(d) 

It crashes inside dict_setdefault_ref_lock_held -> insertion_resize -> dictresize -> unicode_get_hash

[sergey-miryanov]

For the following repro:

d ={} class X(object): def __hash__(self): return 1 def __eq__(self, other): d.clear() d.setdefault(X(), None) d.setdefault(X(), None) assert len(d) == 1 print(d) 

It crashes also inside unicode_get_hash but for line print(d).

Both repro tests on Windows 11 on main branch.

[sergey-miryanov]

Another one I found on main:

d ={} class X(object): def __hash__(self): return 1 def __eq__(self, other): d.clear() d[X()] = None d[X()] = None for _ in range(10): d.pop(X(), None) 

It is aborted with Assertion failed: PyUnicode_CheckExact(ep->me_key), file X:\Work\cpython\main\Objects\dictobject.c, line 1048

[efimov-mikhail]

Another one I found on main:

d[X()] = None d[X()] = None for _ in range(10): d.pop(X(), None) 

Yes, it's expected that d is in incorrect state after two assignments.
There're many ways to trigger assertion at that state.
All of them have been fixed with this PR, I suppose.

[sergey-miryanov]

LGTM

[methane]

I'm not sure if this is the correct solution.

If the keys of a dictionary change during a dict lookup, can we trust the result of the lookup (e.g. ix)?

Shouldn't it raise an error if the keys of a dict change during a lookup?

[vstinner]

I like this error:

PyErr_SetString(PyExc_RuntimeError, "dict mutated during update");

[colesbury]

If the keys of a dictionary change during a dict lookup, can we trust the result of the lookup

CPython behavior has been to retry the lookup if the key changes (since Python 2.2, 453163d). I don't think we should change it.

[efimov-mikhail]

I created #140702 for test_UnixDatagramServer() timeout.

IIUC, problem is fixed and CI is green.

[miss-islington-app]

Thanks @efimov-mikhail for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @efimov-mikhail and @kumaraditya303, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 02202c117b5702f3325e62b07ccdeaa125fc8722 3.14 

[miss-islington-app]

Sorry, @efimov-mikhail and @kumaraditya303, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 02202c117b5702f3325e62b07ccdeaa125fc8722 3.13 

[efimov-mikhail]

Thanks for the merge, @kumaraditya303.
I'll make backports manually.

[bedevere-app]

GH-140743 is a backport of this pull request to the 3.14 branch.

[bedevere-app]

GH-140744 is a backport of this pull request to the 3.13 branch.

[efimov-mikhail]

FYI, @kumaraditya303
The backports are ready and have green CI.