tcp_proxy: fix a crash when h2 tunnel is set (envoyproxy#11401)

Fix a crash following the pattern in TcpUpstream. The upstream handle should not be created if the newStream returns nullptr, regardless on failure or on immediate success. Additional Description: Without the fix, the below backtrace could be detected by the new test case. Risk: low: bugfix with HTTP/2 CONNECT Testing: new integration test Signed-off-by: Yuchen Dai <[email protected]>
atoztoa1 · Jun 3, 2020 · 981507f · 981507f
1 parent 5d8f7d7
commit 981507f
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 2 deletions.
diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc
@@ -437,8 +437,12 @@ Network::FilterStatus Filter::initializeUpstreamConnection() {
       upstream_ = std::make_unique<HttpUpstream>(*upstream_callbacks_,
                                                  config_->tunnelingConfig()->hostname());
       HttpUpstream* http_upstream = static_cast<HttpUpstream*>(upstream_.get());
-      upstream_handle_ = std::make_shared<HttpConnectionHandle>(
-          conn_pool->newStream(http_upstream->responseDecoder(), *this));
+      Http::ConnectionPool::Cancellable* cancellable =
+          conn_pool->newStream(http_upstream->responseDecoder(), *this);
+      if (cancellable) {
+        ASSERT(upstream_handle_.get() == nullptr);
+        upstream_handle_ = std::make_shared<HttpConnectionHandle>(cancellable);
+      }
       return Network::FilterStatus::StopIteration;
     }
   }

diff --git a/test/integration/tcp_tunneling_integration_test.cc b/test/integration/tcp_tunneling_integration_test.cc
@@ -476,6 +476,49 @@ TEST_P(TcpTunnelingIntegrationTest, TcpProxyUpstreamFlush) {
   tcp_client->waitForHalfClose();
 }
 
+// Test that h2 connection is reused.
+TEST_P(TcpTunnelingIntegrationTest, H2ConnectionReuse) {
+  initialize();
+
+  // Establish a connection.
+  IntegrationTcpClientPtr tcp_client1 = makeTcpConnection(lookupPort("tcp_proxy"));
+  ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_));
+  ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
+  ASSERT_TRUE(upstream_request_->waitForHeadersComplete());
+  upstream_request_->encodeHeaders(default_response_headers_, false);
+
+  // Send data in both directions.
+  tcp_client1->write("hello1", false);
+  ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, "hello1"));
+
+  // Send data from upstream to downstream with an end stream and make sure the data is received
+  // before the connection is half-closed.
+  upstream_request_->encodeData("world1", true);
+  tcp_client1->waitForData("world1");
+  tcp_client1->waitForHalfClose();
+  tcp_client1->close();
+  ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_));
+
+  // Establish a new connection.
+  IntegrationTcpClientPtr tcp_client2 = makeTcpConnection(lookupPort("tcp_proxy"));
+
+  // The new CONNECT stream is established in the existing h2 connection.
+  ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
+  ASSERT_TRUE(upstream_request_->waitForHeadersComplete());
+  upstream_request_->encodeHeaders(default_response_headers_, false);
+
+  tcp_client2->write("hello2", false);
+  ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, "hello2"));
+
+  // Send data from upstream to downstream with an end stream and make sure the data is received
+  // before the connection is half-closed.
+  upstream_request_->encodeData("world2", true);
+  tcp_client2->waitForData("world2");
+  tcp_client2->waitForHalfClose();
+  tcp_client2->close();
+  ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_));
+}
+
 INSTANTIATE_TEST_SUITE_P(IpVersions, TcpTunnelingIntegrationTest,
                          testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
                          TestUtility::ipTestParamsToString);