Repository navigation
Fix free-threaded use-after-free in dict structure comparison - #148
Open
harshitgavita-07 wants to merge 1 commit into
Open
harshitgavita-07 wants to merge 1 commit into
harshitgavita-07 wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Author
|
The Google CLA check is green now. This is ready for maintainer review. The free-threaded crash reproducer passes 5/5 runs after fixing both borrowed-reference paths, and all 70 tests pass on both Python 3.14t and Python 3.10. Happy to adjust the patch based on review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #143.
On free-threaded builds (pybind11 marks the module
Py_MOD_GIL_NOT_USED),tree.assert_same_structurecan dereference freed memory when another thread mutates a compared dict. Two borrowed-reference hazards intree.cc:AssertSameStructureHelperiterated withPyDict_Next, which yields borrowed keys and is documented as unsafe against concurrent mutation. ThePyDict_GetItem(o2, key)lookup can run Python__hash__/__eq__, during which a mutator thread can drop the key's last reference.DictValueIterator::next()usedPyDict_GetItem(borrowed) followed byPy_INCREF- a mutator can free the element in the gap between the lookup and the incref.Fixes:
PyDict_Keys(o1); the list holds strong references for the duration of the comparison. Works identically on all supported Python versions.PyDict_GetItemRefon Python >= 3.13, which returns a strong reference atomically; the legacyPyDict_GetItem+Py_INCREFpath is kept for older versions.Verification
Reproduced the reporter's reproducer (8 comparer threads calling
assert_same_structure, 3 mutator threads replacing keys,strsubclass keys with Python__hash__) on a stockpython3.14.7tbuild (GIL disabled):tree_test.pysuite: 70/70 pass on3.14t(free-threaded) and 70/70 pass on a GIL build (3.10, exercises the legacy#elsepath).