Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"RuntimeError: Dictionary changed size during iteration" when copying a WeakValueDictionary #79796

Closed
ltfish mannequin opened this issue Dec 29, 2018 · 9 comments
Closed
Labels
3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@ltfish
Copy link
Mannequin

ltfish mannequin commented Dec 29, 2018

BPO 35615
Nosy @freddrake, @pitrou, @serhiy-storchaka, @eamanu, @remilapeyre, @ltfish, @djromberg
PRs
  • bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. #11384
  • bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. #11384
  • bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. #11384
  • bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. #11384
  • [3.7] bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. (GH-11384) #11785
  • [3.7] bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. (GH-11384) #11785
  • [3.7] bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. (GH-11384) #11785
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2019-02-07.20:09:29.295>
    created_at = <Date 2018-12-29.20:45:37.886>
    labels = ['3.8', '3.7', 'library', 'type-crash']
    title = '"RuntimeError: Dictionary changed size during iteration" when copying a WeakValueDictionary'
    updated_at = <Date 2021-01-27.15:37:56.397>
    user = 'https://github.com/ltfish'

    bugs.python.org fields:

    activity = <Date 2021-01-27.15:37:56.397>
    actor = 'tpaetz'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-02-07.20:09:29.295>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2018-12-29.20:45:37.886>
    creator = 'ltfish'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 35615
    keywords = ['patch', 'patch', 'patch', 'patch']
    message_count = 9.0
    messages = ['332734', '332781', '332789', '332790', '332793', '332794', '335038', '335039', '385638']
    nosy_count = 8.0
    nosy_names = ['fdrake', 'pitrou', 'serhiy.storchaka', 'eamanu', 'remi.lapeyre', 'ltfish', 'djromberg', 'tpaetz']
    pr_nums = ['11384', '11384', '11384', '11384', '11785', '11785', '11785']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue35615'
    versions = ['Python 3.7', 'Python 3.8']

    @ltfish
    Copy link
    Mannequin Author

    ltfish mannequin commented Dec 29, 2018

    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.

    @ltfish ltfish mannequin added 3.7 (EOL) end of life stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump labels Dec 29, 2018
    @remilapeyre
    Copy link
    Mannequin

    remilapeyre mannequin commented Dec 31, 2018

    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.

    @eamanu
    Copy link
    Mannequin

    eamanu mannequin commented Dec 31, 2018

    Hi!

    Seems like a bug. Please provide a patch.

    This can be apply to 3.8?

    @ltfish
    Copy link
    Mannequin Author

    ltfish mannequin commented Dec 31, 2018

    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.

    @ltfish
    Copy link
    Mannequin Author

    ltfish mannequin commented Dec 31, 2018

    Just submitted a PR against the master branch on GitHub.

    This can be apply to 3.8?

    I think so.

    @eamanu eamanu mannequin added the 3.8 only security fixes label Dec 31, 2018
    @ltfish
    Copy link
    Mannequin Author

    ltfish mannequin commented Dec 31, 2018

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 7, 2019

    New changeset 96d37db by Antoine Pitrou (Fish) in branch 'master':
    bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. (GH-11384)
    96d37db

    @pitrou
    Copy link
    Member

    pitrou commented Feb 7, 2019

    New changeset 48769a2 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)
    48769a2

    @pitrou pitrou closed this as completed Feb 7, 2019
    @djromberg
    Copy link
    Mannequin

    djromberg mannequin commented Jan 25, 2021

    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.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant