Skip to content

Commit

Permalink
Correctly clean up keeping_mutexes before resuming any other threads.
Browse files Browse the repository at this point in the history
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>.
  • Loading branch information
ioquatix committed Mar 7, 2023
1 parent 072fc76 commit d452712
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 31 deletions.
2 changes: 1 addition & 1 deletion thread.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
57 changes: 27 additions & 30 deletions thread_sync.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/*
Expand Down

0 comments on commit d452712

Please sign in to comment.