From d68bf0d39a48b81c1f47f29d17237f8e281af74d Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 2 Feb 2024 17:35:09 -0800 Subject: [PATCH 1/4] 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. [^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 --- Python/parking_lot.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Python/parking_lot.c b/Python/parking_lot.c index c83d7443e289c5..def8236c32b251 100644 --- a/Python/parking_lot.c +++ b/Python/parking_lot.c @@ -356,14 +356,18 @@ _PyParkingLot_Unpark(const void *addr, _Py_unpark_fn_t *fn, void *arg) void _PyParkingLot_UnparkAll(const void *addr) { + struct llist_node *node; 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; + } _PyRawMutex_Unlock(&bucket->mutex); - struct llist_node *node; llist_for_each_safe(node, &head) { struct wait_entry *waiter = llist_data(node, struct wait_entry, node); llist_remove(node); From cd85723976d70f9eeb49b38ed1be0af80e0a117a Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sat, 3 Feb 2024 01:48:39 +0000 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2024-02-03-01-48-38.gh-issue-114944.4J5ELD.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-02-03-01-48-38.gh-issue-114944.4J5ELD.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-02-03-01-48-38.gh-issue-114944.4J5ELD.rst b/Misc/NEWS.d/next/Core and Builtins/2024-02-03-01-48-38.gh-issue-114944.4J5ELD.rst new file mode 100644 index 00000000000000..fb41caf7c5f4fa --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-02-03-01-48-38.gh-issue-114944.4J5ELD.rst @@ -0,0 +1 @@ +Fixes a race between ``PyParkingLot_Park`` and ``_PyParkingLot_UnparkAll``. From 33fea6925f43c8625b74bfa266590420010b24b5 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 2 Feb 2024 18:41:06 -0800 Subject: [PATCH 3/4] Set `is_unparking` in dequeue{_all} --- Python/parking_lot.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Python/parking_lot.c b/Python/parking_lot.c index def8236c32b251..6336d1557ad9cf 100644 --- a/Python/parking_lot.c +++ b/Python/parking_lot.c @@ -244,6 +244,7 @@ dequeue(Bucket *bucket, const void *address) if (wait->addr == (uintptr_t)address) { llist_remove(node); --bucket->num_waiters; + wait->is_unparking = true; return wait; } } @@ -262,6 +263,7 @@ dequeue_all(Bucket *bucket, const void *address, struct llist_node *dst) llist_remove(node); llist_insert_tail(dst, node); --bucket->num_waiters; + wait->is_unparking = true; } } } @@ -337,8 +339,6 @@ _PyParkingLot_Unpark(const void *addr, _Py_unpark_fn_t *fn, void *arg) _PyRawMutex_Lock(&bucket->mutex); struct wait_entry *waiter = dequeue(bucket, addr); if (waiter) { - waiter->is_unparking = true; - int has_more_waiters = (bucket->num_waiters > 0); fn(arg, waiter->park_arg, has_more_waiters); } @@ -362,10 +362,6 @@ _PyParkingLot_UnparkAll(const void *addr) _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; - } _PyRawMutex_Unlock(&bucket->mutex); llist_for_each_safe(node, &head) { From b06247de1c065bf173318d852cf86c75d50fcae4 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 2 Feb 2024 18:50:13 -0800 Subject: [PATCH 4/4] Move node declaration back --- Python/parking_lot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/parking_lot.c b/Python/parking_lot.c index 6336d1557ad9cf..8ba50fc1353ebd 100644 --- a/Python/parking_lot.c +++ b/Python/parking_lot.c @@ -356,7 +356,6 @@ _PyParkingLot_Unpark(const void *addr, _Py_unpark_fn_t *fn, void *arg) void _PyParkingLot_UnparkAll(const void *addr) { - struct llist_node *node; struct llist_node head = LLIST_INIT(head); Bucket *bucket = &buckets[((uintptr_t)addr) % NUM_BUCKETS]; @@ -364,6 +363,7 @@ _PyParkingLot_UnparkAll(const void *addr) dequeue_all(bucket, addr, &head); _PyRawMutex_Unlock(&bucket->mutex); + struct llist_node *node; llist_for_each_safe(node, &head) { struct wait_entry *waiter = llist_data(node, struct wait_entry, node); llist_remove(node);