Feature #13245: [PATCH] reject inter-thread TLS modification

• Status: Open
• Assignee: -
• Target version: -

[shyouhei (Shyouhei Urabe)]

Thread local and fiber local storages are by nature expected to be
visible from within the thread / fiber and not from anywhere else. OK?

That's a hoax.

The truth is they are visible from anywhere. Thread local variable
for instance is just a set of key-value pair that is stored inside of
a thread instance variable. No access control is ever attempted since
the beginning. So you can touch any thread's any local storages at
will at any time without any synchronization.

This embarrasing "feature" has been there for a long time. I thinnk
it's too late to remove the TLS accessor methods because they are used
everywhere. BUT, even with that assumption, I strongly believe that
tampering other thread's local storage is something you must not.

Preventing such access should harm no one.

Signed-off-by: Urabe, Shyouhei [e-mail]

---
 test/ruby/test_fiber.rb  | 7 +++++++
 test/ruby/test_thread.rb | 7 +++++++
 thread.c                 | 8 ++++++++
 3 files changed, 22 insertions(+)

diff --git a/test/ruby/test_fiber.rb b/test/ruby/test_fiber.rb
index d1d15828fc..319de9e7b2 100644
--- a/test/ruby/test_fiber.rb
+++ b/test/ruby/test_fiber.rb
@@ -169,6 +169,13 @@ def tvar(var, val)
     assert_equal(nil, Thread.current[:v]);
   end
 
+  def test_inter_thread_tls
+    t = Thread.new { }
+    assert_raise(ThreadError) do
+      t[:foo] = "bar"
+    end
+  end
+
   def test_alive
     fib = Fiber.new{Fiber.yield}
     assert_equal(true, fib.alive?)
diff --git a/test/ruby/test_thread.rb b/test/ruby/test_thread.rb
index c82018dc8e..5ad663becf 100644
--- a/test/ruby/test_thread.rb
+++ b/test/ruby/test_thread.rb
@@ -95,6 +95,13 @@ def test_thread_variable_frozen
     end
   end
 
+  def test_inter_thread_variable
+    t = Thread.new { }
+    assert_raise(ThreadError) do
+      t.thread_variable_set(:foo, "bar")
+    end
+  end
+
   def test_mutex_synchronize
     m = Thread::Mutex.new
     r = 0
diff --git a/thread.c b/thread.c
index 597fd293d6..03f7777aae 100644
--- a/thread.c
+++ b/thread.c
@@ -3154,6 +3154,10 @@ rb_thread_local_aset(VALUE thread, ID id, VALUE val)
 	rb_error_frozen("thread locals");
     }
 
+    if (th != GET_THREAD()) {
+        rb_raise(rb_eThreadError, "inter-thread access of TLS prohibited");
+    }
+
     return threadptr_local_aset(th, id, val);
 }
 
@@ -3231,6 +3235,10 @@ rb_thread_variable_set(VALUE thread, VALUE id, VALUE val)
 	rb_error_frozen("thread locals");
     }
 
+    if (thread != GET_THREAD()->self) {
+        rb_raise(rb_eThreadError, "inter-thread access of TLS prohibited");
+    }
+
     locals = rb_ivar_get(thread, id_locals);
     return rb_hash_aset(locals, rb_to_symbol(id), val);
 }
-- 
2.11.1

[shyouhei (Shyouhei Urabe)]

• Description updated (diff)

Shyouhei Urabe wrote:

Preventing such access should harm no one.

I'm ticketing this as a feature request because there do actually exist people of bad habit who touches other thread's local storage. I believe this is how it should work but at the same time this is a breaking change.

[Eregon (Benoit Daloze)]

I strongly agree.

Since the API is Thread#[key] and not Thread[key] (meaning it should only be possible to get TLS from the current thread), this is a kind of bad API in the first place (same for thread_variable_*).

One of the consequences is that parallel implementations of Ruby must then synchronize when accessing these fake thread-locals,
as they might be modified concurrently by another thread.
Just disallowing writes from another thread does not fully help for this,
reads from another thread should also be prohibited to be truly thread-local and need no synchronization.

There must be many failures though with this change, could you post the output of test-all?

I know a few places in ruby/spec that use TLS wrong,
even though it does look "convenient" as it's a way to store inter-thread information per thread.
I'll be happy to fix those if we go ahead with this.

[Eregon (Benoit Daloze)]

FWIW concurrent-ruby has ThreadLocalVar which does not allow access from another thread:
http://ruby-concurrency.github.io/concurrent-ruby/Concurrent/ThreadLocalVar.html

[normalperson (Eric Wong)]

How about we allow some inter-thread TLS operations, but not others?

Maybe:

Allowed:

• reading values
• updating value of existing keys (maybe)

Disallowed:

• setting value to a new key

In other words, the Allowed stuff is safe using a lock-free hash
table (maybe using rculfhash found in URCU http://liburcu.org/)

At least in non-Ruby (C99) projects, I've found inter-thread
TLS access useful when combined with atomic instructions.

But also, I guess there's no need to use Thread#[] at all if two
threads already agree to share some objects...

[Eregon (Benoit Daloze)]

Another interesting fact related to fiber-local variables is they are inherently racy when accessing fibers locals of another Thread.
By racy, I mean that it's unknown which Fiber of that other Thread will be the current/active one and so we just get a seemingly random Fiber of that Thread.
(unless of course that Thread only has one Fiber or we control when the Thread switches between Fibers)
Example:

$ ruby -e 't=Thread.new { Thread.current[:a] = :root; Fiber.new { Thread.current[:a]=:fiber; sleep 2.5 }.resume }; 5.times { p t[:a]; sleep 1 }'
nil
:fiber
:fiber
:root
:root

In the end, this is more leaky than Fiber.current.
Fiber.current can only be called on the current Thread but this allows to indirectly know the "current" Fiber in the other Thread,
although it might have changed when the call returns.

I think preventing writes from another Thread might be a good first step, if it is compatible enough.

[Eregon (Benoit Daloze)]

I tried and it doesn't seem so easy.

ruby/spec seems to mostly pass, there are just 14 errors:
https://gist.github.com/eregon/36f15069d86fc98fe39ff490ab878e91

but test-all gets stuck in DRb tests due to
https://github.com/ruby/ruby/blob/7f24b0aabd4e1f6d0ff058a944508575a08eaa65/lib/drb/extservm.rb#L90
Removing that line seems to fail many DRb tests.
After DRb tests, the test suite seems broken to what looks like having the wrong current directory.

WEBrick also has a similar usage:

th = start_thread(sock, &block)
th[:WEBrickThread] = true

I did not have time to investigate these usages in further details.

Actually even Process.detach is writing to the fiber-locals of another Thread.
Arguably, that usage is safe in that specific case, assuming there is the GIL or some synchronization to set the variable before the thread is started.

See https://github.com/eregon/ruby/commit/9018e759812fac9af8f481616d86ace17e3a3900 for a rebased patch and a couple tweaks.