gh-114944: Fix race between _PyParkingLot_Park and _PyParkingLot_UnparkAll when handling interrupts
#114945
Conversation
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
…hen handling interrupts There is a potential race when `_PyParkingLot_UnparkAll` is executing in one thread and another thread is unblocked because of an interrupt in `_PyParkingLot_Park`. Consider the following scenario: 1. Thread T0 is blocked[^1] in `_PyParkingLot_Park` on address `A`. 2. Thread T1 executes `_PyParkingLot_UnparkAll` on address `A`. It finds the `wait_entry` for `T0` and unlinks[^2] its list node. 3. Immediately after (2), T0 is woken up due to an interrupt. It then segfaults trying to unlink[^3] the node that was previously unlinked in (2). To fix this we mark each waiter as unparking before releasing the bucket lock. `_PyParkingLot_Park` will wait to handle the coming wakeup, and not attempt to unlink the node, when this field is set. `_PyParkingLot_Unpark` does this already, presumably to handle this case. [^1]: https://github.com/python/cpython/blob/f35c7c070ca6b49c5d6a97be34e6c02f828f5873/Python/parking_lot.c#L303 [^2]: https://github.com/python/cpython/blob/f35c7c070ca6b49c5d6a97be34e6c02f828f5873/Python/parking_lot.c#L369 [^3]: https://github.com/python/cpython/blob/f35c7c070ca6b49c5d6a97be34e6c02f828f5873/Python/parking_lot.c#L320
colesbury
reviewed
Feb 3, 2024
Python/parking_lot.c
Outdated
| struct llist_node head = LLIST_INIT(head); | ||
| Bucket *bucket = &buckets[((uintptr_t)addr) % NUM_BUCKETS]; | ||
|
|
||
| _PyRawMutex_Lock(&bucket->mutex); | ||
| dequeue_all(bucket, addr, &head); | ||
| llist_for_each(node, &head) { | ||
| struct wait_entry *waiter = llist_data(node, struct wait_entry, node); | ||
| waiter->is_unparking = true; |
There was a problem hiding this comment.
This looks correct, but let's push the waiter->is_unparking assignment into dequeue_all and dequeue to avoid the extra loop here.
|
@colesbury - Can you take another look? |
colesbury
approved these changes
Feb 3, 2024
DinoV
approved these changes
Feb 5, 2024
fsc-eriker
pushed a commit
to fsc-eriker/cpython
that referenced
this pull request
Feb 14, 2024
…gLot_UnparkAll` when handling interrupts (python#114945) Fix race between `_PyParkingLot_Park` and `_PyParkingLot_UnparkAll` when handling interrupts There is a potential race when `_PyParkingLot_UnparkAll` is executing in one thread and another thread is unblocked because of an interrupt in `_PyParkingLot_Park`. Consider the following scenario: 1. Thread T0 is blocked[^1] in `_PyParkingLot_Park` on address `A`. 2. Thread T1 executes `_PyParkingLot_UnparkAll` on address `A`. It finds the `wait_entry` for `T0` and unlinks[^2] its list node. 3. Immediately after (2), T0 is woken up due to an interrupt. It then segfaults trying to unlink[^3] the node that was previously unlinked in (2). To fix this we mark each waiter as unparking before releasing the bucket lock. `_PyParkingLot_Park` will wait to handle the coming wakeup, and not attempt to unlink the node, when this field is set. `_PyParkingLot_Unpark` does this already, presumably to handle this case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
There is a potential race when
_PyParkingLot_UnparkAllis executing in one thread and another thread is unblocked because of an interrupt in_PyParkingLot_Park. Consider the following scenario:_PyParkingLot_Parkon addressA._PyParkingLot_UnparkAllon addressA. It finds thewait_entryforT0and unlinks2 its list node.To fix this we mark each waiter as unparking before releasing the bucket lock.
_PyParkingLot_Parkwill wait to handle the coming wakeup, and not attempt to unlink the node, when this field is set._PyParkingLot_Unparkdoes this already, presumably to handle this case._PyParkingLot_Parkand_PyParkingLot_UnparkAllwhen handling interrupts #114944Footnotes
https://github.com/python/cpython/blob/f35c7c070ca6b49c5d6a97be34e6c02f828f5873/Python/parking_lot.c#L303 ↩
https://github.com/python/cpython/blob/f35c7c070ca6b49c5d6a97be34e6c02f828f5873/Python/parking_lot.c#L369 ↩
https://github.com/python/cpython/blob/f35c7c070ca6b49c5d6a97be34e6c02f828f5873/Python/parking_lot.c#L320 ↩