https://redmine.ruby-lang.org/https://redmine.ruby-lang.org/favicon.ico?17113305112019-03-09T18:50:27ZRuby Issue Tracking SystemRuby master - Bug #15645: It is possible to escape `Mutex#synchronize` without releasing the mutexhttps://redmine.ruby-lang.org/issues/15645?journal_id=770062019-03-09T18:50:27Zlexi.lambda (Alexis King)
<ul></ul><p>I know very little about Ruby internals, and I haven’t worked with Ruby in years, but I saw a link to this bug and got pretty fascinated trying to understand what was going on. I’m pretty sure I’ve spotted the problem, though I haven’t actually tested if I’m right.</p>
<p>Here’s what I think is happening: <code>Mutex#synchronize</code> is implemented very simply, by calling <code>rb_mutex_lock</code> followed by a call to <code>rb_ensure</code> that sets up <code>rb_mutex_unlock</code> as an ensure handler before invoking the provided block:</p>
<pre><code class="c syntaxhl" data-language="c"><span class="n">VALUE</span>
<span class="nf">rb_mutex_synchronize</span><span class="p">(</span><span class="n">VALUE</span> <span class="n">mutex</span><span class="p">,</span> <span class="n">VALUE</span> <span class="p">(</span><span class="o">*</span><span class="n">func</span><span class="p">)(</span><span class="n">VALUE</span> <span class="n">arg</span><span class="p">),</span> <span class="n">VALUE</span> <span class="n">arg</span><span class="p">)</span>
<span class="p">{</span>
<span class="n">rb_mutex_lock</span><span class="p">(</span><span class="n">mutex</span><span class="p">);</span>
<span class="k">return</span> <span class="n">rb_ensure</span><span class="p">(</span><span class="n">func</span><span class="p">,</span> <span class="n">arg</span><span class="p">,</span> <span class="n">rb_mutex_unlock</span><span class="p">,</span> <span class="n">mutex</span><span class="p">);</span>
<span class="p">}</span>
</code></pre>
<p>Naturally, this can go wrong if the stack is unwound <em>after</em> the mutex has been locked, but <em>before</em> control has left <code>rb_mutex_lock</code>, since the ensure handler has not yet been installed. If we peek inside the implementation of <code>do_mutex_lock</code>, we find that this is, indeed, possible. If a thread fails to acquire the mutex, it goes to sleep, and when it wakes up and acquires the GVL, it locks the mutex for itself (assuming the mutex hasn’t been locked by some other thread in the meantime). However, before control leaves <code>rb_mutex_lock</code>, it catastrophically checks for interrupts! At the time of this writing, this happens on <a href="https://github.com/ruby/ruby/blob/72df0a8e4703c4e14cb2014d59d8ddad09a47859/thread_sync.c#L287-L293" class="external">lines 287–293 of thread_sync.c</a>:</p>
<pre><code class="c syntaxhl" data-language="c"><span class="k">if</span> <span class="p">(</span><span class="n">interruptible_p</span><span class="p">)</span> <span class="p">{</span>
<span class="n">RUBY_VM_CHECK_INTS_BLOCKING</span><span class="p">(</span><span class="n">th</span><span class="o">-></span><span class="n">ec</span><span class="p">);</span> <span class="cm">/* may release mutex */</span>
<span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">mutex</span><span class="o">-></span><span class="n">th</span><span class="p">)</span> <span class="p">{</span>
<span class="n">mutex</span><span class="o">-></span><span class="n">th</span> <span class="o">=</span> <span class="n">th</span><span class="p">;</span>
<span class="n">mutex_locked</span><span class="p">(</span><span class="n">th</span><span class="p">,</span> <span class="n">self</span><span class="p">);</span>
<span class="p">}</span>
<span class="p">}</span>
</code></pre>
<p>The comment in question is right in that interrupt handlers may release the mutex, so it defensively reacquires it if control returns and the mutex is unlocked, but this problem is caused by an inverse situation: control <em>doesn’t</em> return (the stack has been unwound), but the thread still holds the lock.</p>
<p>Therefore, the solution seems simple: <strong>instead of using <code>rb_mutex_lock</code> inside of <code>rb_mutex_synchronize</code>, use <code>mutex_lock_uninterruptible</code>, which sets <code>interruptible_p</code> to <code>FALSE</code> and avoids the issue entirely.</strong></p>
<p>As a final note, it’s possibly worth saying that this issue was introduced in commit <a href="https://git.ruby-lang.org/ruby.git/commit/?id=3586c9e0876e784767a1c1adba9ebc2499fa0ec2" class="external">3586c9e087</a>, which was part of Ruby 2.5.0. Earlier versions of Ruby did not have this bug. However, the program in the original bug report will still crash on versions of Ruby without the bug, and it would be correct to do so, since an exception can be delivered outside the extent of the <code>rescue</code>. Here’s a modified version of the program that I believe avoids all nondeterministic behavior by using <code>Thread.handle_interrupt</code> and waiting until exceptions are masked before starting the raising thread. It’s also slightly simpler, in that only one thread is competing for the mutex and only one is raising exceptions. On correct Ruby implementations, the following program should never terminate:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="vi">@mutex</span> <span class="o">=</span> <span class="no">Mutex</span><span class="p">.</span><span class="nf">new</span>
<span class="k">class</span> <span class="nc">E</span> <span class="o"><</span> <span class="no">StandardError</span><span class="p">;</span> <span class="k">end</span>
<span class="no">Thread</span><span class="p">.</span><span class="nf">new</span> <span class="k">do</span>
<span class="kp">loop</span> <span class="k">do</span>
<span class="vi">@mutex</span><span class="p">.</span><span class="nf">synchronize</span> <span class="p">{}</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="k">def</span> <span class="nf">start_raising_thread</span>
<span class="no">Thread</span><span class="p">.</span><span class="nf">new</span> <span class="k">do</span>
<span class="kp">loop</span> <span class="k">do</span>
<span class="vi">@t</span><span class="p">.</span><span class="nf">raise</span> <span class="no">E</span>
<span class="no">Thread</span><span class="p">.</span><span class="nf">pass</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="vi">@t</span> <span class="o">=</span> <span class="no">Thread</span><span class="p">.</span><span class="nf">new</span> <span class="k">do</span>
<span class="no">Thread</span><span class="p">.</span><span class="nf">handle_interrupt</span><span class="p">(</span><span class="no">E</span> <span class="o">=></span> <span class="ss">:never</span><span class="p">)</span> <span class="k">do</span>
<span class="n">start_raising_thread</span>
<span class="kp">loop</span> <span class="k">do</span>
<span class="k">begin</span>
<span class="no">Thread</span><span class="p">.</span><span class="nf">handle_interrupt</span><span class="p">(</span><span class="no">E</span> <span class="o">=></span> <span class="ss">:immediate</span><span class="p">)</span> <span class="k">do</span>
<span class="vi">@mutex</span><span class="p">.</span><span class="nf">synchronize</span> <span class="p">{}</span>
<span class="k">end</span>
<span class="k">rescue</span> <span class="no">E</span>
<span class="k">end</span>
<span class="k">raise</span> <span class="s2">"UNRELEASED MUTEX"</span> <span class="k">if</span> <span class="vi">@mutex</span><span class="p">.</span><span class="nf">owned?</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="vi">@t</span><span class="p">.</span><span class="nf">join</span>
</code></pre> Ruby master - Bug #15645: It is possible to escape `Mutex#synchronize` without releasing the mutexhttps://redmine.ruby-lang.org/issues/15645?journal_id=784482019-06-11T19:40:50Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>lexi.lambda (Alexis King) wrote:</p>
<blockquote>
<p>Therefore, the solution seems simple: <strong>instead of using <code>rb_mutex_lock</code> inside of <code>rb_mutex_synchronize</code>, use <code>mutex_lock_uninterruptible</code>, which sets <code>interruptible_p</code> to <code>FALSE</code> and avoids the issue entirely.</strong></p>
</blockquote>
<p>With this simple solution:</p>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gd">--- thread_sync.c
</span><span class="gi">+++ thread_sync.c
</span><span class="p">@@ -506,7 +506,7 @@</span> VALUE
rb_mutex_synchronize(VALUE mutex, VALUE (*func)(VALUE arg), VALUE arg)
{
rb_mutex_lock(mutex);
<span class="gd">- return rb_ensure(func, arg, rb_mutex_unlock, mutex);
</span><span class="gi">+ return rb_ensure(func, arg, mutex_lock_uninterruptible, mutex);
</span> }
<span class="err">
</span> /*
<span class="err">
</span></code></pre>
<p>There is a deadlock error when building ruby:</p>
<pre><code>./miniruby -I./lib -I. -I.ext/common ./tool/transform_mjit_header.rb "cc -O0 -g -pipe -fPIC " rb_mjit_header.h .ext/include/x86_64-openbsd6.5/rb_mjit_min_header-2.7.0.h
Traceback (most recent call last):
7: from ./tool/transform_mjit_header.rb:240:in `<main>'
6: from ./tool/transform_mjit_header.rb:177:in `conflicting_types?'
5: from ./tool/transform_mjit_header.rb:188:in `with_code'
4: from ./lib/tempfile.rb:295:in `open'
3: from ./tool/transform_mjit_header.rb:189:in `block in with_code'
2: from ./lib/delegate.rb:349:in `block in delegating_block'
1: from ./lib/delegate.rb:349:in `puts'
./lib/delegate.rb:349:in `write': deadlock; recursive locking (ThreadError)
*** Error 1 in /. (Makefile:890 '.ext/include/x86_64-openbsd6.5/rb_mjit_min_header-2.7.0.h')
</code></pre>
<p>I'm not sure how to fix the issue, assuming it is actually possible to fix. Maybe using the equivalent of <code>Thread.handle_interrupt(Exception => :never)</code> around the call to <code>rb_ensure</code> and <code>Thread.handle_interrupt(Exception => :immediate)</code> around the call to <code>func</code> , as implied by the <code>Thread.handle_interrupt</code> documentation? I think <code>Thread#raise</code> and <code>Thread#kill</code> should be avoided if at all possible, and we should probably update the documentation to describe the problems with them and warn against their use.</p> Ruby master - Bug #15645: It is possible to escape `Mutex#synchronize` without releasing the mutexhttps://redmine.ruby-lang.org/issues/15645?journal_id=784532019-06-11T20:23:15ZEregon (Benoit Daloze)
<ul></ul><p><a class="user active user-mention" href="https://redmine.ruby-lang.org/users/1604">@jeremyevans0 (Jeremy Evans)</a> The patch looks wrong, <code>rb_mutex_lock</code> should be changed, not <code>rb_mutex_unlock</code>.</p> Ruby master - Bug #15645: It is possible to escape `Mutex#synchronize` without releasing the mutexhttps://redmine.ruby-lang.org/issues/15645?journal_id=784542019-06-11T20:27:18Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>Eregon (Benoit Daloze) wrote:</p>
<blockquote>
<p><a class="user active user-mention" href="https://redmine.ruby-lang.org/users/1604">@jeremyevans0 (Jeremy Evans)</a> The patch looks wrong, <code>rb_mutex_lock</code> should be changed, not <code>rb_mutex_unlock</code>.</p>
</blockquote>
<p>Correct, apologies for that. It turns out there is a pull request for this that should may the issue (<a href="https://github.com/ruby/ruby/pull/2131" class="external">https://github.com/ruby/ruby/pull/2131</a>).</p> Ruby master - Bug #15645: It is possible to escape `Mutex#synchronize` without releasing the mutexhttps://redmine.ruby-lang.org/issues/15645?journal_id=784582019-06-11T20:28:19Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/15360">Bug #15360</a>: "ThreadError: deadlock; recursive locking" error when recursive lock shouldn't be possible</i> added</li></ul> Ruby master - Bug #15645: It is possible to escape `Mutex#synchronize` without releasing the mutexhttps://redmine.ruby-lang.org/issues/15645?journal_id=784592019-06-11T22:51:51Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li></ul><p>Fix committed at <a class="changeset" title="do_mutex_lock: release mutex before checking for interrupts (fixes issue 15360)" href="https://redmine.ruby-lang.org/projects/ruby-master/repository/git/revisions/c1d78a7f0ece2004822193a0c1f1fd3dc38c2fdf">c1d78a7f0ece2004822193a0c1f1fd3dc38c2fdf</a></p>