Repository navigation
gh-158803: Fix crash in bytes.join() on a concurrently mutated list - #158910
christianaurichzm wants to merge 2 commits into
Conversation
…list In the free-threaded build, bytes.join() and bytearray.join() read items from the list with borrowed references and without holding its lock, so another thread could replace and free an item before it was increfed. Run the join under Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST, as PyUnicode_Join() already does.
|
The macOS (free-threading) failure is unrelated to this change: the test run was stopped by a SIGINT that escaped |
…-join-exact-fast The fast path holds no reference to the items, so in the free-threaded build it relies on the sequence being locked by the caller. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
eendebakpt
left a comment
There was a problem hiding this comment.
Nice work! The failing CI indeed seems unrelated. Can you merge with main to trigger the CI again?
|
@eendebakpt Thanks! Merged main. The Windows failure is the known |
|
The added lock will add a little bit of overhead in the FT build, but we can easily gain it back in a followup PR (where we can make use of the fact that the iterator argument is locked). For cpython/Objects/unicodeobject.c Line 10439 in 37cc8dd |
|
Thanks! Happy to look into the follow-up once this lands. |
Fixes #158803
In the free-threaded build,
bytes.join(),bytearray.join()andPyBytes_Join()(which shareObjects/stringlib/join.h) read items from a list withPySequence_Fast_GET_ITEM(), which returns a borrowed reference, without holding the list's lock. If another thread replaces an item in the meantime, the old item can be freed beforejoin()takes its own reference to it.str.join()had the same problem and was fixed in gh-119247, which addedPy_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST. This change uses the same pattern: the existing body becomesbytes_join_lock_held(), andbytes_join()calls it inside the critical section, asPyUnicode_Join()does with_PyUnicode_JoinArray().The critical section can be suspended while an item's
__buffer__()runs, or while a large result is copied with the thread state detached. Every item is increfed while the lock is held, and the existing size check still raisesRuntimeErrorif the list changes size, so both cases stay safe.In the default build the macros expand to an empty block.
Verification
test_free_threading.test_bytes_objectcrashes an unpatched free-threaded debug build in most runs (6 to 8 out of 10) and passed 30 out of 30 runs with the fix. The reproducer from the issue segfaults without the fix and runs cleanly for 10 seconds with it.test_free_threading,test_bytesandtest_capi.test_bytespass with no reports.test_bytes,test_capi.test_bytesandtest_free_threading.test_bytes_objectpass with-R 3:3on both builds.