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

ltfish · 2018-12-31T02:37:21Z

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__()

the-knights-who-say-ni · 2018-12-31T02:37:24Z

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!

pitrou

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

vstinner

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

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

ltfish · 2019-01-04T22:15:44Z

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!

pitrou · 2019-01-04T22:17:36Z

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.

ltfish · 2019-01-04T22:31:39Z

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

pitrou · 2019-01-04T22:34:34Z

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

ltfish · 2019-01-04T22:36:06Z

@pitrou Thanks. Lemme give it a try!

ltfish · 2019-01-05T00:36:54Z

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

ltfish · 2019-01-24T06:38:20Z

Hi there, is there any update on this PR?

pitrou · 2019-01-24T09:25:16Z

Sorry @ltfish. I'm gonna review soon.

vstinner

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?

ltfish · 2019-01-25T07:43:07Z

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

miss-islington · 2019-02-07T19:52:02Z

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

pitrou · 2019-02-07T19:52:09Z

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 <[email protected]>

bedevere-bot · 2019-02-07T19:52:37Z

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 <[email protected]>

ltfish · 2019-02-07T20:49:32Z

@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

Misc/NEWS.d/next/Library/2018-12-30-20-00-05.bpo-35615.Uz1SVh.rst Outdated Show resolved Hide resolved

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

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

ltfish commented Dec 31, 2018 •

edited by bedevere-bot

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

pitrou left a comment

vstinner left a comment

ltfish commented Jan 4, 2019

pitrou commented Jan 4, 2019 •

edited

ltfish commented Jan 4, 2019

pitrou commented Jan 4, 2019

ltfish commented Jan 4, 2019

ltfish commented Jan 5, 2019 •

edited

ltfish commented Jan 24, 2019

pitrou commented Jan 24, 2019

vstinner left a comment

ltfish commented Jan 25, 2019

miss-islington commented Feb 7, 2019

pitrou commented Feb 7, 2019

bedevere-bot commented Feb 7, 2019

ltfish commented Feb 7, 2019

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

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

Conversation

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

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

pitrou left a comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

ltfish commented Jan 4, 2019

pitrou commented Jan 4, 2019 • edited

ltfish commented Jan 4, 2019

pitrou commented Jan 4, 2019

ltfish commented Jan 4, 2019

ltfish commented Jan 5, 2019 • edited

ltfish commented Jan 24, 2019

pitrou commented Jan 24, 2019

vstinner left a comment

Choose a reason for hiding this comment

ltfish commented Jan 25, 2019

miss-islington commented Feb 7, 2019

pitrou commented Feb 7, 2019

bedevere-bot commented Feb 7, 2019

ltfish commented Feb 7, 2019

ltfish commented Dec 31, 2018 •

edited by bedevere-bot

pitrou commented Jan 4, 2019 •

edited

ltfish commented Jan 5, 2019 •

edited