classification
Title: "RuntimeError: Dictionary changed size during iteration" when copying a WeakValueDictionary
Type: crash Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: djromberg, eamanu, fdrake, ltfish, pitrou, remi.lapeyre, serhiy.storchaka, tpaetz
Priority: normal Keywords: patch, patch, patch, patch

Created on 2018-12-29 20:45 by ltfish, last changed 2021-01-27 15:37 by tpaetz. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11384 merged python-dev, 2018-12-31 02:37
PR 11384 merged python-dev, 2018-12-31 02:37
PR 11384 merged python-dev, 2018-12-31 02:37
PR 11384 merged python-dev, 2018-12-31 02:37
PR 11785 merged miss-islington, 2019-02-07 19:52
PR 11785 merged miss-islington, 2019-02-07 19:52
PR 11785 merged miss-islington, 2019-02-07 19:52
Messages (9)
msg332734 - (view) Author: Fish Wang (ltfish) * Date: 2018-12-29 20:45
I come across this issue recently when developing a multi-threaded PySide2 (Qt) application. When I'm calling .copy() on a WeakValueDictionary, there is a high chance that my application crashes with the following stack backtrace:

------

Traceback (most recent call last):
  File "F:\angr\angr-management\angrmanagement\ui\widgets\qdisasm_graph.py", line 239, in mouseDoubleClickEvent
    block.on_mouse_doubleclicked(event.button(), self._to_graph_pos(event.pos()))
  File "F:\angr\angr-management\angrmanagement\ui\widgets\qblock.py", line 130, in on_mouse_doubleclicked
    obj.on_mouse_doubleclicked(button, pos)
  File "F:\angr\angr-management\angrmanagement\ui\widgets\qinstruction.py", line 128, in on_mouse_doubleclicked
    op.on_mouse_doubleclicked(button, pos)
  File "F:\angr\angr-management\angrmanagement\ui\widgets\qoperand.py", line 162, in on_mouse_doubleclicked
    self.disasm_view.jump_to(self._branch_target, src_ins_addr=self.insn.addr)
  File "F:\angr\angr-management\angrmanagement\ui\views\disassembly_view.py", line 258, in jump_to
    self._jump_to(addr)
  File "F:\angr\angr-management\angrmanagement\ui\views\disassembly_view.py", line 372, in _jump_to
    self._display_function(function)
  File "F:\angr\angr-management\angrmanagement\ui\views\disassembly_view.py", line 343, in _display_function
    vr = self.workspace.instance.project.analyses.VariableRecoveryFast(the_func)
  File "f:\angr\angr\angr\analyses\analysis.py", line 109, in __call__
    oself.__init__(*args, **kwargs)
  File "f:\angr\angr\angr\analyses\variable_recovery\variable_recovery_fast.py", line 618, in __init__
    self._analyze()
  File "f:\angr\angr\angr\analyses\forward_analysis.py", line 557, in _analyze
    self._analysis_core_graph()
  File "f:\angr\angr\angr\analyses\forward_analysis.py", line 580, in _analysis_core_graph
    changed, output_state = self._run_on_node(n, job_state)
  File "f:\angr\angr\angr\analyses\variable_recovery\variable_recovery_fast.py", line 712, in _run_on_node
    input_state = prev_state.merge(input_state, successor=node.addr)
  File "f:\angr\angr\angr\analyses\variable_recovery\variable_recovery_fast.py", line 488, in merge
    merged_register_region = self.register_region.copy().replace(replacements).merge(other.register_region,
  File "f:\angr\angr\angr\keyed_region.py", line 159, in copy
    kr._object_mapping = self._object_mapping.copy()
  File "D:\My Program Files\Python37\lib\weakref.py", line 174, in copy
    for key, wr in self.data.items():
RuntimeError: dictionary changed size during iteration

------

I went ahead and read the related methods in Lib\weakref.py, and it seems to me that the WeakValueDictionary.copy() method is missing the protection of an _IterationGuard: It is iterating through self.data.items(), which, might have entries removed because of GC during the iteration.

It seems that this crash can be fixed by wrapping the iteration with `with _IterationGuard(self):`. It worked for me in my tests.

If my above analysis is correct, the following methods all require protection of _IterationGuard (which are currently missing):

- WeakValueDictionary.copy()
- WeakValueDictionary.__deepcopy__()
- WeakKeyDictionary.copy()
- WeakKeyDictionary.__deepcopy__()

Please let me know if this is a legitimate issue, in which case I will be happy to provide a patch. Thanks.
msg332781 - (view) Author: RĂ©mi Lapeyre (remi.lapeyre) * Date: 2018-12-31 01:17
Hi, I think this is indeed a bug, `copy()` is expected to succeed. I was able to reproduce the issue in an interpreter but did not succeed to write a test case that triggers the race-condition.

I think the fix you suggested is the right solution.
msg332789 - (view) Author: Emmanuel Arias (eamanu) * Date: 2018-12-31 02:16
Hi!

Seems like a bug. Please provide a patch. 

This can be apply to 3.8?
msg332790 - (view) Author: Fish Wang (ltfish) * Date: 2018-12-31 02:19
Thanks for your reply.

I'm preparing a PR. However, I'm not sure how to write a reliable test case to trigger the crash outside my application. I will submit the PR for now, and see if anyone on the mailing list has a better idea of what a reliable test case should look like.
msg332793 - (view) Author: Fish Wang (ltfish) * Date: 2018-12-31 02:47
Just submitted a PR against the master branch on GitHub.

> This can be apply to 3.8?

I think so.
msg332794 - (view) Author: Fish Wang (ltfish) * Date: 2018-12-31 02:57
Just checked weakref.py on different branches, I think this bug exists in Python 2.7, 3.4, 3.5, and 3.6 as well.
msg335038 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-02-07 19:52
New changeset 96d37dbcd23e65a7a57819aeced9034296ef747e by Antoine Pitrou (Fish) in branch 'master':
bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. (GH-11384)
https://github.com/python/cpython/commit/96d37dbcd23e65a7a57819aeced9034296ef747e
msg335039 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-02-07 20:09
New changeset 48769a28ad6ef4183508951fa6a378531ace26a4 by Antoine Pitrou (Miss Islington (bot)) in branch '3.7':
bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. (GH-11384) (GH-11785)
https://github.com/python/cpython/commit/48769a28ad6ef4183508951fa6a378531ace26a4
msg385638 - (view) Author: Daniel Romberg (djromberg) Date: 2021-01-25 15:44
I think this issue needs to be reopened. The problem has not been solved completely. We experience a lot fewer crashes in weakref than before this fix, however, there are recursive situations in which copy() is invoked while iterating the WeakValueDictionary (e.g., in our case it is a signal/slot implementation where the slots are stored in a WeakValueDictionary). _commit_removals(), which is called at the beginning of the copy operation, might change the dictionary if there are items that are to be removed. If there is an ongoing iteration, the corresponding RuntimeError is raised.

I haven't thought that through entirely, but I wonder whether the copy (and also deepcopy) operation could just blindly copy everything without "committing removals". After the copy, both instances would do their _commit_removals on their own upon access.
History
Date User Action Args
2021-01-27 15:37:56tpaetzsetnosy: + tpaetz
2021-01-25 15:44:14djrombergsetnosy: + djromberg
messages: + msg385638
2019-02-07 20:09:29pitrousetkeywords: patch, patch, patch, patch
status: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-02-07 20:09:19pitrousetmessages: + msg335039
2019-02-07 19:52:51miss-islingtonsetpull_requests: + pull_request11775
2019-02-07 19:52:43miss-islingtonsetpull_requests: + pull_request11774
2019-02-07 19:52:36miss-islingtonsetpull_requests: + pull_request11773
2019-02-07 19:52:03pitrousetmessages: + msg335038
2018-12-31 06:05:54serhiy.storchakasetkeywords: patch, patch, patch, patch
nosy: + fdrake, pitrou, serhiy.storchaka
2018-12-31 02:57:40ltfishsetmessages: + msg332794
2018-12-31 02:49:58eamanusetversions: + Python 3.8
2018-12-31 02:47:24ltfishsetmessages: + msg332793
2018-12-31 02:37:45python-devsetkeywords: + patch
stage: patch review
pull_requests: + pull_request10740
2018-12-31 02:37:38python-devsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request10739
2018-12-31 02:37:30python-devsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request10738
2018-12-31 02:37:22python-devsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request10737
2018-12-31 02:19:45ltfishsetmessages: + msg332790
2018-12-31 02:16:53eamanusetnosy: + eamanu
messages: + msg332789
2018-12-31 01:17:43remi.lapeyresetnosy: + remi.lapeyre
messages: + msg332781
2018-12-29 20:45:37ltfishcreate