Lua: Change the TLS callback function type of ThreadLocalState to Upd… by wbpcode · Pull Request #11944 · envoyproxy/envoy

wbpcode · 2020-07-08T02:39:10Z

Signed-off-by: wbpcode comems@msn.com

Change the type of ThreadLocalState's TLS callback to UpdateCb. Through this method, we can avoid capturing this (ThreadLocalState instance) in the callback function, and avoid memory security problems caused by the inconsistency between the lifetime of the ThreadLocalSate instance and the lifetime of the callback function.

This modification is at least harmless.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

…ateCb Signed-off-by: wbpcode <comems@msn.com>

wbpcode · 2020-07-08T02:52:15Z

A related issue is #10241. But I can't fully confirm that this PR will definitely solve the issue. Because I cannot reproduce the issue directly in my environment.

The only way I found to reproduce the issue is to cancel the repeated listener check first, and then add two listeners with the same name in the first xDS response, which will trigger this crash. In this scenario, the crash problem was indeed solved by this PR.

dio · 2020-07-08T16:32:15Z

Looks good. Could you provide a test for this case, please? Thanks!

wbpcode · 2020-07-09T02:38:05Z

Looks good. Could you provide a test for this case, please? Thanks!

I think for this case, it may be difficult to design a good test case, because it is difficult to reproduce this crash. Anyway, I will try it.

Signed-off-by: wbpcode <comems@msn.com>

wbpcode · 2020-07-09T11:54:25Z

Add a new test for ThreadLocalState. @dio

wbpcode · 2020-07-09T11:59:22Z

If the type of callback is not modified, running this test directly will result in a crash (as follows). After changing the type of the callback function to UpdateCb, the test can pass safely.

[ RUN      ] ThreadSafeTest.StateDestructedBeforeWorkerRun
[2020-07-09 10:23:51.292][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:104] Caught Segmentation fault, suspect faulting address 0x3d055e0
[2020-07-09 10:23:51.292][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:91] Backtrace (use tools/stack_decode.py to get line numbers):
[2020-07-09 10:23:51.292][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:92] Envoy version: 0/1.16.0-dev/redacted/DEBUG/BoringSSL
[2020-07-09 10:23:51.305][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #0: Envoy::SignalAction::sigHandler() [0xad007c]
[2020-07-09 10:23:51.305][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #1: __restore_rt [0x7f676f3418a0]
[2020-07-09 10:23:51.318][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #2: Envoy::Extensions::Filters::Common::Lua::ThreadLocalState::registerType<>()::{lambda()#1}::operator()() [0x441c30]
[2020-07-09 10:23:51.330][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #3: std::_Function_handler<>::_M_invoke() [0x441aed]
[2020-07-09 10:23:51.343][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #4: std::function<>::operator()() [0x47c99e]
[2020-07-09 10:23:51.355][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #5: Envoy::ThreadLocal::InstanceImpl::Bookkeeper::runOnAllThreads()::$_5::operator()() [0x4c4575]
[2020-07-09 10:23:51.368][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #6: std::_Function_handler<>::_M_invoke() [0x4c43bd]
[2020-07-09 10:23:51.368][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #7: std::function<>::operator()() [0x47c99e]
[2020-07-09 10:23:51.380][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #8: Envoy::Event::DispatcherImpl::runPostCallbacks() [0x6b1bed]
[2020-07-09 10:23:51.393][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #9: Envoy::Event::DispatcherImpl::run() [0x6b1b05]
[2020-07-09 10:23:51.406][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #10: Envoy::Extensions::Filters::Common::Lua::(anonymous namespace)::ThreadSafeTest_StateDestructedBeforeWorkerRun_Test::TestBody()::$_0::operator()() [0x445ecb]
[2020-07-09 10:23:51.418][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #11: std::_Function_handler<>::_M_invoke() [0x445d6d]
[2020-07-09 10:23:51.418][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #12: std::function<>::operator()() [0x47c99e]
[2020-07-09 10:23:51.431][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #13: Envoy::Thread::ThreadImplPosix::ThreadImplPosix()::{lambda()#1}::operator()() [0x1670b92]
[2020-07-09 10:23:51.443][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #14: Envoy::Thread::ThreadImplPosix::ThreadImplPosix()::{lambda()#1}::__invoke() [0x1670b65]
[2020-07-09 10:23:51.443][18][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #15: start_thread [0x7f676f3366db]

Signed-off-by: wbpcode <comems@msn.com>

dio

Overall looks good. Flushing some comments and questions.

dio · 2020-07-09T14:23:40Z

source/extensions/filters/common/lua/lua.cc

 uint64_t ThreadLocalState::registerGlobal(const std::string& global) {
-  tls_slot_->runOnAllThreads([this, global]() {
-    LuaThreadLocal& tls = tls_slot_->getTyped<LuaThreadLocal>();
+  tls_slot_->runOnAllThreads([global](ThreadLocal::ThreadLocalObjectSharedPtr ptr) -> auto {


Does spelling out -> auto (-> ThreadLocal::ThreadLocalObjectSharedPtr) here will be a problem for clang-tidy?

And let's rename ptr as previous?

From the existing code, it is acceptable to spell out the return type or not.

Anyway, I will remove it to make the code more concise.

dio · 2020-07-09T14:24:35Z

source/extensions/filters/common/lua/lua.cc

-  tls_slot_->runOnAllThreads([this, global]() {
-    LuaThreadLocal& tls = tls_slot_->getTyped<LuaThreadLocal>();
+  tls_slot_->runOnAllThreads([global](ThreadLocal::ThreadLocalObjectSharedPtr ptr) -> auto {
+    ASSERT(std::dynamic_pointer_cast<LuaThreadLocal>(ptr));


Do we need this assert here?

dio · 2020-07-09T14:25:07Z

test/extensions/filters/common/lua/lua_test.cc

+  // Start a new worker thread to execute the callback functions in the worker dispatcher.
+  Thread::ThreadPtr thread = Thread::threadFactoryForTest().createThread([this]() {
+    worker_dispatcher_->run(Event::Dispatcher::RunType::Block);
+    // Verify we have the expected dispatcher for the new thread thread.


for the new worker thread?

dio · 2020-07-09T14:29:43Z

@snowp, when you have time, could you help to take a look on the test validity? Thanks!

Signed-off-by: wbpcode <comems@msn.com>

dio · 2020-07-10T04:53:57Z

#12018 is merged. Please sync your branch with head to fix the CI (Linux-x64 asan) issue. Thanks!

wbpcode · 2020-07-10T04:58:39Z

#12018 is merged. Please sync your branch with head to fix the CI (Linux-x64 asan) issue. Thanks!
Get it.

wbpcode · 2020-07-14T03:20:56Z

Could you take the time to push this PR to the next step? Thanks. @snowp

snowp

Thanks this makes sense, just one question.

snowp · 2020-07-14T15:57:02Z

test/extensions/filters/common/lua/lua_test.cc

+};
+
+// Test whether ThreadLocalState can be safely released.
+TEST_F(ThreadSafeTest, StateDestructedBeforeWorkerRun) {


Does this test fail without the code change in this PR?

Yes, it will be a crash.

Thanks for verifying!

repokitteh-read-only · 2020-07-15T07:25:27Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #11944 (comment) was created by @wbpcode.

see: more, trace.

snowp

LGTM, thanks!

envoyproxy#11944) Change the type of ThreadLocalState's TLS callback to UpdateCb. Through this method, we can avoid capturing this (ThreadLocalState instance) in the callback function, and avoid memory security problems caused by the inconsistency between the lifetime of the ThreadLocalSate instance and the lifetime of the callback function. Signed-off-by: wbpcode <comems@msn.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

envoyproxy#11944) Change the type of ThreadLocalState's TLS callback to UpdateCb. Through this method, we can avoid capturing this (ThreadLocalState instance) in the callback function, and avoid memory security problems caused by the inconsistency between the lifetime of the ThreadLocalSate instance and the lifetime of the callback function. Signed-off-by: wbpcode <comems@msn.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

Lua: Change the TLS callback function type of ThreadLocalState to Upd… (envoyproxy#11944) Update LuaJIT patch - remove MAP_32BIT (envoyproxy#10867) Signed-off-by: wbpcode <comems@msn.com> Signed-off-by: John Murray <me@johnmurray.io> Signed-off-by: Yuchen Dai <silentdai@gmail.com> Change-Id: I62739dc1c3250fcff755d0a6208ec4f78cda695b

Lua: Change the TLS callback function type of ThreadLocalState to Upd…

fd94452

…ateCb Signed-off-by: wbpcode <comems@msn.com>

ggreenway assigned dio Jul 8, 2020

Lua: add test for thread safety ThreadLocalState

cd3b07c

Signed-off-by: wbpcode <comems@msn.com>

wbpcode added 2 commits July 9, 2020 20:13

Lua: correct format error of 'lua_test.cc'

e679c26

Signed-off-by: wbpcode <comems@msn.com>

Lua: correct spelling error of new test

7335c74

Signed-off-by: wbpcode <comems@msn.com>

dio reviewed Jul 9, 2020

View reviewed changes

wbpcode added 2 commits July 10, 2020 10:42

Lua: update parameter name and some comments

1def9ea

Signed-off-by: wbpcode <comems@msn.com>

Merge branch 'master' of github.com:envoyproxy/envoy into lua-crash

3064066

Merge branch 'master' of github.com:envoyproxy/envoy into lua-crash

0f690af

snowp suggested changes Jul 14, 2020

View reviewed changes

snowp self-assigned this Jul 14, 2020

Merge branch 'master' of github.com:envoyproxy/envoy into lua-crash

97c59b8

wbpcode added 2 commits July 15, 2020 15:27

Merge branch 'master' of github.com:envoyproxy/envoy into lua-crash

a32f9ff

Merge branch 'master' of github.com:envoyproxy/envoy into lua-crash

efd8074

snowp approved these changes Jul 15, 2020

View reviewed changes

snowp merged commit dee8b8d into envoyproxy:master Jul 15, 2020

wbpcode mentioned this pull request Jul 16, 2020

Segmentation faults in Lua filter (LuaThreadLocal) #10241

Closed

lambdai mentioned this pull request May 21, 2021

DONTMERGE: not-a-vulnerability cp a minor fix on lua istio/envoy#325

Closed

Conversation

wbpcode commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dio commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Jul 9, 2020

Uh oh!

wbpcode commented Jul 9, 2020

Uh oh!

wbpcode commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dio commented Jul 9, 2020

Uh oh!

dio commented Jul 10, 2020

Uh oh!

wbpcode commented Jul 10, 2020

Uh oh!

wbpcode commented Jul 14, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jul 15, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wbpcode commented Jul 8, 2020 •

edited

Loading

wbpcode commented Jul 8, 2020 •

edited

Loading

dio commented Jul 8, 2020 •

edited

Loading

wbpcode commented Jul 9, 2020 •

edited

Loading