From: eregontp@... Date: 2017-09-12T12:54:07+00:00 Subject: [ruby-core:82759] [Ruby trunk Feature#13245] [PATCH] reject inter-thread TLS modification Issue #13245 has been updated by Eregon (Benoit Daloze). I tried and it doesn't seem so easy. ruby/spec seems to mostly pass, they 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. ---------------------------------------- Feature #13245: [PATCH] reject inter-thread TLS modification https://bugs.ruby-lang.org/issues/13245#change-66616 * Author: shyouhei (Shyouhei Urabe) * Status: Open * Priority: Normal * Assignee: * Target version: ---------------------------------------- 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 ```diff --- 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 ``` -- https://bugs.ruby-lang.org/ Unsubscribe: