test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#…

…4319) When the server thread exits independently of the main test thread (e.g. exception catch), we could race on the access to the server admin port via server_. We latch this in this PR to avoid unsafe behavior. We might still have a stale address, but the request should then fail or timeout. Fixes oss-fuzz issues https://oss-fuzz.com/v2/testcase-detail/5719024990683136 and https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10027. Risk level: Low Testing: TSAN integration tests. Signed-off-by: Harvey Tuch <[email protected]>
rshriram · Sep 4, 2018 · 0a1e92a · 0a1e92a
1 parent cddc732
commit 0a1e92a
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 5 deletions.
diff --git a/test/integration/server.cc b/test/integration/server.cc
@@ -58,11 +58,12 @@ void IntegrationTestServer::start(const Network::Address::IpVersion version,
 IntegrationTestServer::~IntegrationTestServer() {
   ENVOY_LOG(info, "stopping integration test server");
 
-  BufferingStreamDecoderPtr response =
-      IntegrationUtil::makeSingleRequest(server_->admin().socket().localAddress(), "POST",
-                                         "/quitquitquit", "", Http::CodecClient::Type::HTTP1);
-  EXPECT_TRUE(response->complete());
-  EXPECT_STREQ("200", response->headers().Status()->value().c_str());
+  if (admin_address_ != nullptr) {
+    BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(
+        admin_address_, "POST", "/quitquitquit", "", Http::CodecClient::Type::HTTP1);
+    EXPECT_TRUE(response->complete());
+    EXPECT_STREQ("200", response->headers().Status()->value().c_str());
+  }
 
   thread_->join();
 }
@@ -107,6 +108,10 @@ void IntegrationTestServer::threadRoutine(const Network::Address::IpVersion vers
       restarter, stats_store, lock, *this, std::move(random_generator), tls));
   pending_listeners_ = server_->listenerManager().listeners().size();
   ENVOY_LOG(info, "waiting for {} test server listeners", pending_listeners_);
+  // This is technically thread unsafe (assigning to a shared_ptr accessed
+  // across threads), but because we synchronize below on server_set, the only
+  // consumer on the main test thread in ~IntegrationTestServer will not race.
+  admin_address_ = server_->admin().socket().localAddress();
   server_set_.setReady();
   server_->run();
   server_.reset();

diff --git a/test/integration/server.h b/test/integration/server.h
@@ -309,6 +309,7 @@ class IntegrationTestServer : Logger::Loggable<Logger::Id::testing>,
   Stats::Store* stat_store_{};
   std::function<void()> on_worker_listener_added_cb_;
   std::function<void()> on_worker_listener_removed_cb_;
+  Network::Address::InstanceConstSharedPtr admin_address_;
 };
 
 } // namespace Envoy