From 2c4b2053ca1884f058982fd46f2576bd887920b6 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Tue, 7 Mar 2023 20:23:00 +1300 Subject: 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 . --- thread_sync.c | 57 +++++++++++++++++++++++++++------------------------------ 1 file changed, 27 insertions(+), 30 deletions(-) (limited to 'thread_sync.c') diff --git a/thread_sync.c b/thread_sync.c index 9a3a6c1ba7..cbcb7c2eaf 100644 --- a/thread_sync.c +++ b/thread_sync.c @@ -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; } /* -- cgit v1.2.1