[thrift][bugfix]try to fix thrift request overflow crash. #12890

pyrl247 · 2020-08-31T14:00:03Z

try to fix thrift request overflow crash.When request overflow ,the OnPoolFailure will set host to null,cause the crash in this case.

Signed-off-by: Guang Yang pyrl247@gmail.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

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

PoolFailure will set host to null,cause the crush in this case. Signed-off-by: Guang Yang <pyrl247@gmail.com>

zuercher · 2020-09-01T03:22:19Z

Thanks. You’ll need to fix the code formatting (if you click through to the details of the failed build you can eventually drill down to the log file that shows what’s formatted incorrectly, or you can install clang-format and use the tools/code_format/check_format.py directly).

Also, let’s add a test to validate the fix.

Signed-off-by: Guang Yang <pyrl247@gmail.com>

pyrl247 · 2020-09-01T07:59:28Z

I didn't found where to add the test case,Could you please tell me where to add it ?

rgs1 · 2020-09-01T13:49:47Z

Thanks for the fix @pyrl247 !

Here's a similar fix with test cases added: #6549.

Let me know if that helps.

rgs1 · 2020-09-01T14:00:12Z

For the test case, I think you can probably use this existing one as an example:

TEST_F(ThriftRouterTest, PoolOverflowFailure) {
  initializeRouter();

  startRequest(MessageType::Call);

  EXPECT_CALL(callbacks_, sendLocalReply(_, _))
      .WillOnce(Invoke([&](const DirectResponse& response, bool end_stream) -> void {
        auto& app_ex = dynamic_cast<const AppException&>(response);
        EXPECT_EQ(AppExceptionType::InternalError, app_ex.type_);
        EXPECT_THAT(app_ex.what(), ContainsRegex(".*too many connections.*"));
        EXPECT_TRUE(end_stream);
      }));
  context_.cluster_manager_.tcp_conn_pool_.poolFailure(ConnectionPool::PoolFailureReason::Overflow);
}

and add something like:

  EXPECT_EQ(FilterStatus::StopIteration, router_->messageBegin(metadata_));

at the end... (which should crash without your fix).

I think something along those lines would work.

zuercher · 2020-09-01T15:37:43Z

source/extensions/filters/network/thrift_proxy/router/router_impl.cc

-            AppExceptionType::InternalError,
-            fmt::format("too many connections to '{}'", upstream_host_->address()->asString())),
+        AppException(AppExceptionType::InternalError,
+                     fmt::format("thrift upstream request: too many connections")),


Two things:

Since you've removed the string interpolation, you don't need to call fmt::format here.

There is another case below that uses upstream_host_ below. That should be modified as well.

Do you mean the Timeout case? When it hits that case ,as far as I can see the upstream_host_ will keep the real one.Could you please check if it's right?

I don't think we should depend on that behavior even if it's true.

I think is better to report more detail about the Exception.How about change like this
`

parent_.callbacks_->sendLocalReply( AppException( AppExceptionType::InternalError, fmt::format("connection failure '{}'", (upstream_host_ != nullptr) ? upstream_host_->address()->asString() : "to upstream")), true);

`

pyrl247 · 2020-09-03T13:20:38Z

@rgs1 What about I change the test/mocks/tcp/mocks.cc the poolFailure function like this?It will trigger the crash.

  Callbacks* cb = callbacks_.front();
  callbacks_.pop_front();
  handles_.pop_front();
  if (reason == ConnectionPool::PoolFailureReason::Overflow) {
    cb->onPoolFailure(reason, nullptr);
  } else {
    cb->onPoolFailure(reason, host_);
  }
}

rgs1 · 2020-09-03T16:35:08Z

@rgs1 What about I change the test/mocks/tcp/mocks.cc the poolFailure function like this?It will trigger the crash.
  Callbacks* cb = callbacks_.front();
  callbacks_.pop_front();
  handles_.pop_front();
  if (reason == ConnectionPool::PoolFailureReason::Overflow) {
    cb->onPoolFailure(reason, nullptr);
  } else {
    cb->onPoolFailure(reason, host_);
  }
}

That probably works too.

Signed-off-by: Guang Yang <pyrl247@gmail.com>

pyrl247 · 2020-09-06T06:52:48Z

@zuercher @rgs1 Please check if it work?

rgs1 · 2020-09-08T16:54:32Z

test/extensions/filters/network/thrift_proxy/router_test.cc

@@ -407,7 +407,8 @@ TEST_F(ThriftRouterTest, PoolOverflowFailure) {
        EXPECT_THAT(app_ex.what(), ContainsRegex(".*too many connections.*"));
        EXPECT_TRUE(end_stream);
      }));
-  context_.cluster_manager_.tcp_conn_pool_.poolFailure(ConnectionPool::PoolFailureReason::Overflow);
+  context_.cluster_manager_.tcp_conn_pool_.poolFailureWithNullHost(
+      ConnectionPool::PoolFailureReason::Overflow);


verified that this crashes without your fix?

yes,if i use the previous code ,it will crash in my ci like this .

[ RUN ] ThriftRouterTest.PoolOverflowFailure TestRandomGenerator running with seed -443243904 [2020-09-09 13:03:11.969][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:104] Caught Segmentation fault, suspect faulting address 0x0 [2020-09-09 13:03:11.969][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:91] Backtrace (use tools/stack_decode.py to get line numbers): [2020-09-09 13:03:11.969][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:92] Envoy version: 0/1.16.0-dev/test/RELEASE/BoringSSL [2020-09-09 13:03:11.969][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #0: __restore_rt [0x7faf6ee3a8a0] [2020-09-09 13:03:11.975][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #1: Envoy::Tcp::ConnectionPool::MockInstance::poolFailure() [0x9b3dd3] [2020-09-09 13:03:11.981][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #2: Envoy::Extensions::NetworkFilters::ThriftProxy::Router::ThriftRouterTest_PoolOverflowFailure_Test::TestBody() [0x788b44] [2020-09-09 13:03:11.987][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #3: testing::internal::HandleExceptionsInMethodIfSupported<>() [0x1293a78] [2020-09-09 13:03:11.992][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #4: testing::Test::Run() [0x12939a5] [2020-09-09 13:03:11.998][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #5: testing::TestInfo::Run() [0x12947f0] [2020-09-09 13:03:12.003][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #6: testing::TestSuite::Run() [0x1295177] [2020-09-09 13:03:12.009][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #7: testing::internal::UnitTestImpl::RunAllTests() [0x12a2047] [2020-09-09 13:03:12.015][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #8: testing::internal::HandleExceptionsInMethodIfSupported<>() [0x12a1968] [2020-09-09 13:03:12.020][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #9: testing::UnitTest::Run() [0x12a17ef] [2020-09-09 13:03:12.026][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #10: Envoy::TestRunner::RunTests() [0xbf1734] [2020-09-09 13:03:12.031][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #11: main [0xbf092b] [2020-09-09 13:03:12.032][5454][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #12: __libc_start_main [0x7faf6ea58b97] ================================================================================ INFO: Elapsed time: 1017.162s, Critical Path: 329.15s

Great, thanks.

rgs1 · 2020-09-08T16:55:24Z

test/mocks/tcp/mocks.h

@@ -59,6 +59,7 @@ class MockInstance : public Instance {

  Envoy::ConnectionPool::MockCancellable* newConnectionImpl(Callbacks& cb);
  void poolFailure(PoolFailureReason reason);


you can probably just add a bool host_null param and reuse poolFailure()

rgs1

we probably want a changelog fix, e.g.:

https://github.com/envoyproxy/envoy/blame/master/docs/root/version_history/v1.12.0.rst#L104

Signed-off-by: Guang Yang <pyrl247@gmail.com>

docs/root/version_history/current.rst

rgs1

Looks great, thanks. Just one last nit.

Signed-off-by: Guang Yang <pyrl247@gmail.com>

rgs1

Thanks!

zuercher

Thanks. I went ahead and fix the merge conflict on the docs.

pyrl247 requested a review from zuercher as a code owner August 31, 2020 14:00

try to fix thrift request overflow crush.When request overflow ,the On

4207918

PoolFailure will set host to null,cause the crush in this case. Signed-off-by: Guang Yang <pyrl247@gmail.com>

mattklein123 assigned zuercher Aug 31, 2020

zuercher assigned rgs1 Sep 1, 2020

change code format

be4e192

Signed-off-by: Guang Yang <pyrl247@gmail.com>

zuercher reviewed Sep 1, 2020

View reviewed changes

pyrl247 added 2 commits September 5, 2020 11:15

add test case and change exception report in other poolFailure case.

e2eb4aa

Signed-off-by: Guang Yang <pyrl247@gmail.com>

add new poolFailure with no host

d449d78

Signed-off-by: Guang Yang <pyrl247@gmail.com>

rgs1 reviewed Sep 8, 2020

View reviewed changes

rgs1 suggested changes Sep 8, 2020

View reviewed changes

change test case and add changelog

a2102e6

Signed-off-by: Guang Yang <pyrl247@gmail.com>

rgs1 reviewed Sep 9, 2020

View reviewed changes

docs/root/version_history/current.rst Show resolved Hide resolved

rgs1 reviewed Sep 9, 2020

View reviewed changes

change doc format

fcbc093

Signed-off-by: Guang Yang <pyrl247@gmail.com>

rgs1 previously approved these changes Sep 9, 2020

View reviewed changes

Merge branch 'master' into master

9f2a550

zuercher dismissed rgs1’s stale review via 9f2a550 September 10, 2020 02:47

zuercher approved these changes Sep 10, 2020

View reviewed changes

zuercher merged commit 329857f into envoyproxy:master Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[thrift][bugfix]try to fix thrift request overflow crash. #12890

[thrift][bugfix]try to fix thrift request overflow crash. #12890

pyrl247 commented Aug 31, 2020

zuercher commented Sep 1, 2020

pyrl247 commented Sep 1, 2020

rgs1 commented Sep 1, 2020

rgs1 commented Sep 1, 2020

zuercher Sep 1, 2020

pyrl247 Sep 2, 2020

zuercher Sep 2, 2020

pyrl247 Sep 3, 2020

zuercher Sep 8, 2020

pyrl247 commented Sep 3, 2020

rgs1 commented Sep 3, 2020

pyrl247 commented Sep 6, 2020

rgs1 Sep 8, 2020

pyrl247 Sep 9, 2020

rgs1 Sep 9, 2020

rgs1 Sep 8, 2020

rgs1 left a comment

rgs1 left a comment

rgs1 left a comment

zuercher left a comment

		@@ -59,6 +59,7 @@ class MockInstance : public Instance {

		Envoy::ConnectionPool::MockCancellable* newConnectionImpl(Callbacks& cb);
		void poolFailure(PoolFailureReason reason);

[thrift][bugfix]try to fix thrift request overflow crash. #12890

[thrift][bugfix]try to fix thrift request overflow crash. #12890

Conversation

pyrl247 commented Aug 31, 2020

zuercher commented Sep 1, 2020

pyrl247 commented Sep 1, 2020

rgs1 commented Sep 1, 2020

rgs1 commented Sep 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyrl247 commented Sep 3, 2020

rgs1 commented Sep 3, 2020

pyrl247 commented Sep 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgs1 left a comment

Choose a reason for hiding this comment

rgs1 left a comment

Choose a reason for hiding this comment

rgs1 left a comment

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment