Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix race condition causing some mutexes to remain locked after forking #415

Closed
wants to merge 1 commit into from

1 participant

@obrie

This patch fixes a race condition in which a mutex gets locked by a background thread and not properly unlocked after forking. This occurs when the background thread is blocked waiting to lock the mutex and the mutex is unlocked right around the time that the process gets forked. When this happens the mutex is not part of keeping_mutexes. Instead, it's still tracked as locking_mutex even though the mutex has actually been locked by the background thread.

This was originally reported by https://bugs.ruby-lang.org/issues/8433. A test has been added verifying the fix.

Completely open to any / all feedback as I'm certainly not a c guru :)

@obrie obrie thread.c: fix some mutexes remaining locked after forking
* thread.c (terminate_atfork_i): fix locking mutexes not unlocked in
  forks when not tracked in thread. [ruby-core:55102] [Bug #8433]
* test/ruby/test_thread.rb: test for above.
d467f42
@obrie

This was closed by 57b8687

@obrie obrie closed this
@obrie obrie deleted the Tapjoy:fix/unlock_thread_mutexes branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 4, 2013
  1. @obrie

    thread.c: fix some mutexes remaining locked after forking

    obrie authored
    * thread.c (terminate_atfork_i): fix locking mutexes not unlocked in
      forks when not tracked in thread. [ruby-core:55102] [Bug #8433]
    * test/ruby/test_thread.rb: test for above.
This page is out of date. Refresh to see the latest.
Showing with 78 additions and 34 deletions.
  1. +6 −0 ChangeLog
  2. +31 −0 test/ruby/test_thread.rb
  3. +41 −34 thread.c
View
6 ChangeLog
@@ -1,3 +1,9 @@
+Fri Oct 4 20:29:06 2013 Aaron Pfeifer <aaron.pfeifer@gmail.com>
+
+ * thread.c (terminate_atfork_i): fix locking mutexes not unlocked in
+ forks when not tracked in thread. [ruby-core:55102] [Bug #8433]
+ * test/ruby/test_thread.rb: test for above.
+
Fri Oct 4 19:54:09 2013 Zachary Scott <e@zzak.io>
* ext/dbm/dbm.c: [DOC] Fix wrong constant name in DBM by @edward
View
31 test/ruby/test_thread.rb
@@ -909,4 +909,35 @@ def test_stack_size
size_large = invoke_rec script, vm_stack_size, 1024 * 1024 * 10
assert_operator(size_default, :<=, size_large, "large size")
end
+
+ def test_blocking_mutex_unlocked_on_fork
+ mutex = Mutex.new
+ mutex.lock
+ flag = false
+
+ Thread.new do
+ mutex.synchronize do
+ begin
+ sleep
+ ensure
+ 1 until flag
+ end
+ end
+ end
+
+ sleep 0.5
+
+ mutex.unlock
+ pid = Process.fork do
+ if mutex.locked?
+ exit(1)
+ else
+ exit(2)
+ end
+ end
+
+ pid, status = Process.waitpid2(pid)
+ flag = true
+ assert_equal(2, status.exitstatus)
+ end
end
View
75 thread.c
@@ -3884,6 +3884,40 @@ rb_thread_atfork_internal(int (*atfork)(st_data_t, st_data_t, st_data_t))
clear_coverage();
}
+#define GetMutexPtr(obj, tobj) \
+ TypedData_Get_Struct((obj), rb_mutex_t, &mutex_data_type, (tobj))
+
+static const char *rb_mutex_unlock_th(rb_mutex_t *mutex, rb_thread_t volatile *th);
+
+#define mutex_mark NULL
+
+static void
+mutex_free(void *ptr)
+{
+ if (ptr) {
+ rb_mutex_t *mutex = ptr;
+ if (mutex->th) {
+ /* rb_warn("free locked mutex"); */
+ const char *err = rb_mutex_unlock_th(mutex, mutex->th);
+ if (err) rb_bug("%s", err);
+ }
+ native_mutex_destroy(&mutex->lock);
+ native_cond_destroy(&mutex->cond);
+ }
+ ruby_xfree(ptr);
+}
+
+static size_t
+mutex_memsize(const void *ptr)
+{
+ return ptr ? sizeof(rb_mutex_t) : 0;
+}
+
+static const rb_data_type_t mutex_data_type = {
+ "mutex",
+ {mutex_mark, mutex_free, mutex_memsize,},
+};
+
static int
terminate_atfork_i(st_data_t key, st_data_t val, st_data_t current_th)
{
@@ -3896,6 +3930,13 @@ terminate_atfork_i(st_data_t key, st_data_t val, st_data_t current_th)
rb_mutex_abandon_all(th->keeping_mutexes);
}
th->keeping_mutexes = NULL;
+ if (th->locking_mutex) {
+ rb_mutex_t *mutex;
+ GetMutexPtr(th->locking_mutex, mutex);
+ if (mutex->th == th) {
+ rb_mutex_abandon_all(mutex);
+ }
+ }
thread_cleanup_func(th, TRUE);
}
return ST_CONTINUE;
@@ -4152,40 +4193,6 @@ thgroup_add(VALUE group, VALUE thread)
*
*/
-#define GetMutexPtr(obj, tobj) \
- TypedData_Get_Struct((obj), rb_mutex_t, &mutex_data_type, (tobj))
-
-static const char *rb_mutex_unlock_th(rb_mutex_t *mutex, rb_thread_t volatile *th);
-
-#define mutex_mark NULL
-
-static void
-mutex_free(void *ptr)
-{
- if (ptr) {
- rb_mutex_t *mutex = ptr;
- if (mutex->th) {
- /* rb_warn("free locked mutex"); */
- const char *err = rb_mutex_unlock_th(mutex, mutex->th);
- if (err) rb_bug("%s", err);
- }
- native_mutex_destroy(&mutex->lock);
- native_cond_destroy(&mutex->cond);
- }
- ruby_xfree(ptr);
-}
-
-static size_t
-mutex_memsize(const void *ptr)
-{
- return ptr ? sizeof(rb_mutex_t) : 0;
-}
-
-static const rb_data_type_t mutex_data_type = {
- "mutex",
- {mutex_mark, mutex_free, mutex_memsize,},
-};
-
VALUE
rb_obj_is_mutex(VALUE obj)
{
Something went wrong with that request. Please try again.