bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. by ltfish · Pull Request #11384 · python/cpython · GitHub

Protect dict iterations by wrapping them with _IterationGuard in the
following methods:

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

https://bugs.python.org/issue35615

Protect dict iterations by wrapping them with _IterationGuard in the following methods: - WeakValueDictionary.copy() - WeakValueDictionary.__deepcopy__() - WeakKeyDictionary.copy() - WeakKeyDictionary.__deepcopy__()

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

LGTM. We might want to add a test (see check_weak_destroy_and_mutate_while_iterating in test_weakref.py).

Is it possible to write a test reproducing the bug? (to then make sure that the fix works as expected)

Is it possible to write a test reproducing the bug? (to then make sure that the fix works as expected)

As much as I want, I couldn't write a reliable test case in a standalone Python file to trigger this bug. I could, however, reliably trigger it when debugging my multithreaded PySide2 application. We also happened to trigger it once on our CI after hundreds of runs. I'm open to ideas of how to write a reliable test case to trigger this bug!

I explained above where to look for and how to add a test. Please take a look.

Edit: ok, the challenge here is to execute arbitrary code during the copy method since the iterator doesn't survive the method call. Other methods using IterationGuard are easier to test since they are generators, so you can easily execute code in-between loop iterations.

@pitrou I took a look at check_weak_destroy_and_mutate_while_iterating(). It seems to me that this test case is ensuring keys/values of weakref dictionary can be destroyed during iterations. However, without monkeypatch, I cannot inject code to delete an object during the execution copy() or __deepcopy__() -- unless I'm missing something. Any suggestions?

@ltfish You're right, I was too optimistic. I think we're left to using two threads: one that heavily copies a very large weakdict, another that loses references to the weakdict keys' or values', one at a time, in more or less random order.

@pitrou Thanks. Lemme give it a try!

On master branch:

$ ../python -m unittest -v test.test_weakref.MappingTestCase
test_make_weak_keyed_dict_from_dict (test.test_weakref.MappingTestCase) ... ok
test_make_weak_keyed_dict_from_weak_keyed_dict (test.test_weakref.MappingTestCase) ... ok
test_make_weak_keyed_dict_repr (test.test_weakref.MappingTestCase) ... ok
test_make_weak_valued_dict_from_dict (test.test_weakref.MappingTestCase) ... ok
test_make_weak_valued_dict_from_weak_valued_dict (test.test_weakref.MappingTestCase) ... ok
test_make_weak_valued_dict_misc (test.test_weakref.MappingTestCase) ... ok
test_make_weak_valued_dict_repr (test.test_weakref.MappingTestCase) ... ok
test_threaded_weak_key_dict_copy (test.test_weakref.MappingTestCase) ... FAIL
test_threaded_weak_key_dict_deepcopy (test.test_weakref.MappingTestCase) ... FAIL
test_threaded_weak_value_dict_copy (test.test_weakref.MappingTestCase) ... FAIL
test_threaded_weak_value_dict_deepcopy (test.test_weakref.MappingTestCase) ... FAIL
test_threaded_weak_valued_consistency (test.test_weakref.MappingTestCase) ... ok
test_threaded_weak_valued_pop (test.test_weakref.MappingTestCase) ... ok
test_threaded_weak_valued_setdefault (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_bad_delitem (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_cascading_deletes (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_delitem (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_dict_popitem (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_dict_setdefault (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_dict_update (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_iters (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_len_cycles (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_len_race (test.test_weakref.MappingTestCase) ... ok
test_weak_keys (test.test_weakref.MappingTestCase) ... ok
test_weak_keys_destroy_while_iterating (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_delitem (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_dict_popitem (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_dict_setdefault (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_dict_update (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_iters (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_len_cycles (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_len_race (test.test_weakref.MappingTestCase) ... ok
test_weak_values (test.test_weakref.MappingTestCase) ... ok
test_weak_values_destroy_while_iterating (test.test_weakref.MappingTestCase) ... ok

======================================================================
FAIL: test_threaded_weak_key_dict_copy (test.test_weakref.MappingTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/fish/projects/cpython/Lib/test/test_weakref.py", line 1760, in test_threaded_weak_key_dict_copy
    self.check_threaded_weak_dict_copy(weakref.WeakKeyDictionary, False)
  File "/home/fish/projects/cpython/Lib/test/test_weakref.py", line 1753, in check_threaded_weak_dict_copy
    self.fail("Caught a RuntimeError: \"%s\". WeakRef dictionary copying failed." % exc[0])
AssertionError: Caught a RuntimeError: "dictionary changed size during iteration". WeakRef dictionary copying failed.

======================================================================
FAIL: test_threaded_weak_key_dict_deepcopy (test.test_weakref.MappingTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/fish/projects/cpython/Lib/test/test_weakref.py", line 1765, in test_threaded_weak_key_dict_deepcopy
    self.check_threaded_weak_dict_copy(weakref.WeakKeyDictionary, True)
  File "/home/fish/projects/cpython/Lib/test/test_weakref.py", line 1753, in check_threaded_weak_dict_copy
    self.fail("Caught a RuntimeError: \"%s\". WeakRef dictionary copying failed." % exc[0])
AssertionError: Caught a RuntimeError: "dictionary changed size during iteration". WeakRef dictionary copying failed.

======================================================================
FAIL: test_threaded_weak_value_dict_copy (test.test_weakref.MappingTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/fish/projects/cpython/Lib/test/test_weakref.py", line 1770, in test_threaded_weak_value_dict_copy
    self.check_threaded_weak_dict_copy(weakref.WeakValueDictionary, False)
  File "/home/fish/projects/cpython/Lib/test/test_weakref.py", line 1753, in check_threaded_weak_dict_copy
    self.fail("Caught a RuntimeError: \"%s\". WeakRef dictionary copying failed." % exc[0])
AssertionError: Caught a RuntimeError: "dictionary changed size during iteration". WeakRef dictionary copying failed.

======================================================================
FAIL: test_threaded_weak_value_dict_deepcopy (test.test_weakref.MappingTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/fish/projects/cpython/Lib/test/test_weakref.py", line 1775, in test_threaded_weak_value_dict_deepcopy
    self.check_threaded_weak_dict_copy(weakref.WeakValueDictionary, True)
  File "/home/fish/projects/cpython/Lib/test/test_weakref.py", line 1753, in check_threaded_weak_dict_copy
    self.fail("Caught a RuntimeError: \"%s\". WeakRef dictionary copying failed." % exc[0])
AssertionError: Caught a RuntimeError: "dictionary changed size during iteration". WeakRef dictionary copying failed.

----------------------------------------------------------------------
Ran 34 tests in 4.820s

FAILED (failures=4)

With this PR:

$ ../python -m unittest -v test.test_weakref.MappingTestCase
test_make_weak_keyed_dict_from_dict (test.test_weakref.MappingTestCase) ... ok
test_make_weak_keyed_dict_from_weak_keyed_dict (test.test_weakref.MappingTestCase) ... ok
test_make_weak_keyed_dict_repr (test.test_weakref.MappingTestCase) ... ok
test_make_weak_valued_dict_from_dict (test.test_weakref.MappingTestCase) ... ok
test_make_weak_valued_dict_from_weak_valued_dict (test.test_weakref.MappingTestCase) ... ok
test_make_weak_valued_dict_misc (test.test_weakref.MappingTestCase) ... ok
test_make_weak_valued_dict_repr (test.test_weakref.MappingTestCase) ... ok
test_threaded_weak_key_dict_copy (test.test_weakref.MappingTestCase) ... ok
test_threaded_weak_key_dict_deepcopy (test.test_weakref.MappingTestCase) ... ok
test_threaded_weak_value_dict_copy (test.test_weakref.MappingTestCase) ... ok
test_threaded_weak_value_dict_deepcopy (test.test_weakref.MappingTestCase) ... ok
test_threaded_weak_valued_consistency (test.test_weakref.MappingTestCase) ... ok
test_threaded_weak_valued_pop (test.test_weakref.MappingTestCase) ... ok
test_threaded_weak_valued_setdefault (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_bad_delitem (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_cascading_deletes (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_delitem (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_dict_popitem (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_dict_setdefault (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_dict_update (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_iters (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_len_cycles (test.test_weakref.MappingTestCase) ... ok
test_weak_keyed_len_race (test.test_weakref.MappingTestCase) ... ok
test_weak_keys (test.test_weakref.MappingTestCase) ... ok
test_weak_keys_destroy_while_iterating (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_delitem (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_dict_popitem (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_dict_setdefault (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_dict_update (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_iters (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_len_cycles (test.test_weakref.MappingTestCase) ... ok
test_weak_valued_len_race (test.test_weakref.MappingTestCase) ... ok
test_weak_values (test.test_weakref.MappingTestCase) ... ok
test_weak_values_destroy_while_iterating (test.test_weakref.MappingTestCase) ... ok

----------------------------------------------------------------------
Ran 34 tests in 6.228s

OK

Hi there, is there any update on this PR?

Sorry @ltfish. I'm gonna review soon.

LGTM, but I would prefer to have a second core dev like @pitrou to review this change.

As soon as the change has a test, it looks good to me :-)

Does the tests fail without the fix?

Does the tests fail without the fix?

Please see my previous reply. Without the fix, my newly added test case fails reliably on my machine. I can also open a PR without my fix if needed (to make sure the test case works as expected on the CI).

Thanks @ltfish for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

Thanks @ltfish !

…honGH-11384) Protect dict iterations by wrapping them with _IterationGuard in the following methods: - WeakValueDictionary.copy() - WeakValueDictionary.__deepcopy__() - WeakKeyDictionary.copy() - WeakKeyDictionary.__deepcopy__() (cherry picked from commit 96d37db) Co-authored-by: Fish <ltfish@users.noreply.github.com>

GH-11785 is a backport of this pull request to the 3.7 branch.

…11384) (GH-11785) Protect dict iterations by wrapping them with _IterationGuard in the following methods: - WeakValueDictionary.copy() - WeakValueDictionary.__deepcopy__() - WeakKeyDictionary.copy() - WeakKeyDictionary.__deepcopy__() (cherry picked from commit 96d37db) Co-authored-by: Fish <ltfish@users.noreply.github.com>

@pitrou @vstinner Thanks a lot!

bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary.

4eeabe6

Protect dict iterations by wrapping them with _IterationGuard in the following methods: - WeakValueDictionary.copy() - WeakValueDictionary.__deepcopy__() - WeakKeyDictionary.copy() - WeakKeyDictionary.__deepcopy__()

the-knights-who-say-ni added the CLA not signed label Dec 31, 2018

bedevere-bot added the awaiting review label Dec 31, 2018

Add a news entry.

c2e2c35

serhiy-storchaka requested a review from pitrou December 31, 2018 06:07

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.7 labels Dec 31, 2018

pitrou approved these changes Dec 31, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Dec 31, 2018

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jan 1, 2019

vstinner reviewed Jan 4, 2019

View reviewed changes

Comment thread Misc/NEWS.d/next/Library/2018-12-30-20-00-05.bpo-35615.Uz1SVh.rst Outdated

Mention the module in the news entry.

cd48e0b

ltfish force-pushed the fix/weakref_dicts_copy_crash branch from a8af3a7 to bfaeb71 Compare January 5, 2019 00:35

Add test cases.

e2b9d24

ltfish force-pushed the fix/weakref_dicts_copy_crash branch from bfaeb71 to e2b9d24 Compare January 5, 2019 00:36

ltfish mentioned this pull request Jan 12, 2019

VariableRecoveryFast fails miserably on CGC binaries angr/angr#1392

Closed

q3k mentioned this pull request Jan 24, 2019

Use weakref library with upstream fix (bugs.python.org/issue35615) angr/angr#1410

Closed

vstinner approved these changes Jan 24, 2019

View reviewed changes

ltfish mentioned this pull request Feb 4, 2019

VariableRecovery Dictionary Changed Size During Iteration Exception angr/angr#1429

Closed

rhelmot added a commit to angr/angr that referenced this pull request Feb 5, 2019

monkeypatch python/cpython#11384

228533a

rhelmot added a commit to angr/angr that referenced this pull request Feb 5, 2019

monkeypatch python/cpython#11384

f90e559

pitrou added 2 commits February 7, 2019 20:23

Merge branch 'master' into fix/weakref_dicts_copy_crash

ac0df69

Some nits

192ac40

pitrou merged commit 96d37db into python:master Feb 7, 2019

bedevere-bot removed the awaiting merge label Feb 7, 2019

bedevere-bot removed the needs backport to 3.7 label Feb 7, 2019

ltfish deleted the fix/weakref_dicts_copy_crash branch February 7, 2019 20:48

bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. by ltfish · Pull Request #11384 · python/cpython · GitHub

Repository navigation

bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary. - #11384

bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary.#11384pitrou merged 6 commits intopython:masterpython/cpython:masterfrom ltfish:fix/weakref_dicts_copy_crashltfish/cpython:fix/weakref_dicts_copy_crashCopy head branch name to clipboard

Conversation

ltfish commented Dec 31, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Dec 31, 2018

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ltfish commented Jan 4, 2019

Uh oh!

pitrou commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ltfish commented Jan 4, 2019

Uh oh!

pitrou commented Jan 4, 2019

Uh oh!

ltfish commented Jan 4, 2019

Uh oh!

ltfish commented Jan 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ltfish commented Jan 24, 2019

Uh oh!

pitrou commented Jan 24, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

ltfish commented Jan 25, 2019

Uh oh!

miss-islington commented Feb 7, 2019

Uh oh!

pitrou commented Feb 7, 2019

Uh oh!

bedevere-bot commented Feb 7, 2019

Uh oh!

ltfish commented Feb 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bpo-35615: Fix crashes when copying a Weak{Key,Value}Dictionary.#11384
pitrou merged 6 commits into
python:masterfrom
ltfish:fix/weakref_dicts_copy_crash

ltfish commented Dec 31, 2018 •
edited by bedevere-bot
Loading

pitrou commented Jan 4, 2019 •
edited
Loading

ltfish commented Jan 5, 2019 •
edited
Loading