Correctly clean up `keeping_mutexes` before resuming any other threads. (#7460)

It's possible (but very rare) to have a race condition between setting
`mutex->fiber = NULL` and `thread_mutex_remove(th, mutex)` which results
in the following bug:

```
[BUG] invalid keeping_mutexes: Attempt to unlock a mutex which is not locked
```

Fixes <https://bugs.ruby-lang.org/issues/19480>.
This commit is contained in:
Samuel Williams 2023-03-07 20:23:00 +13:00 коммит произвёл GitHub
Родитель 011c08b643
Коммит 2c4b2053ca
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: 4AEE18F83AFDEB23
2 изменённых файлов: 28 добавлений и 31 удалений

Просмотреть файл

@ -428,7 +428,7 @@ rb_threadptr_unlock_all_locking_mutexes(rb_thread_t *th)
rb_mutex_t *mutex = th->keeping_mutexes;
th->keeping_mutexes = mutex->next_mutex;
/* rb_warn("mutex #<%p> remains to be locked by terminated thread", (void *)mutexes); */
// rb_warn("mutex #<%p> was not unlocked by thread #<%p>", (void *)mutex, (void*)th);
const char *error_message = rb_mutex_unlock_th(mutex, th, mutex->fiber);
if (error_message) rb_bug("invalid keeping_mutexes: %s", error_message);

Просмотреть файл

@ -435,46 +435,43 @@ rb_mutex_owned_p(VALUE self)
static const char *
rb_mutex_unlock_th(rb_mutex_t *mutex, rb_thread_t *th, rb_fiber_t *fiber)
{
const char *err = NULL;
if (mutex->fiber == 0) {
err = "Attempt to unlock a mutex which is not locked";
return "Attempt to unlock a mutex which is not locked";
}
else if (mutex->fiber != fiber) {
err = "Attempt to unlock a mutex which is locked by another thread/fiber";
return "Attempt to unlock a mutex which is locked by another thread/fiber";
}
else {
struct sync_waiter *cur = 0, *next;
mutex->fiber = 0;
ccan_list_for_each_safe(&mutex->waitq, cur, next, node) {
ccan_list_del_init(&cur->node);
struct sync_waiter *cur = 0, *next;
if (cur->th->scheduler != Qnil && cur->fiber) {
rb_fiber_scheduler_unblock(cur->th->scheduler, cur->self, rb_fiberptr_self(cur->fiber));
goto found;
}
else {
switch (cur->th->status) {
case THREAD_RUNNABLE: /* from someone else calling Thread#run */
case THREAD_STOPPED_FOREVER: /* likely (rb_mutex_lock) */
rb_threadptr_interrupt(cur->th);
goto found;
case THREAD_STOPPED: /* probably impossible */
rb_bug("unexpected THREAD_STOPPED");
case THREAD_KILLED:
/* not sure about this, possible in exit GC? */
rb_bug("unexpected THREAD_KILLED");
continue;
}
mutex->fiber = 0;
thread_mutex_remove(th, mutex);
ccan_list_for_each_safe(&mutex->waitq, cur, next, node) {
ccan_list_del_init(&cur->node);
if (cur->th->scheduler != Qnil && cur->fiber) {
rb_fiber_scheduler_unblock(cur->th->scheduler, cur->self, rb_fiberptr_self(cur->fiber));
return NULL;
}
else {
switch (cur->th->status) {
case THREAD_RUNNABLE: /* from someone else calling Thread#run */
case THREAD_STOPPED_FOREVER: /* likely (rb_mutex_lock) */
rb_threadptr_interrupt(cur->th);
return NULL;
case THREAD_STOPPED: /* probably impossible */
rb_bug("unexpected THREAD_STOPPED");
case THREAD_KILLED:
/* not sure about this, possible in exit GC? */
rb_bug("unexpected THREAD_KILLED");
continue;
}
}
found:
thread_mutex_remove(th, mutex);
}
return err;
// We did not find any threads to wake up, so we can just return with no error:
return NULL;
}
/*