[conn_pool] fix use after free in H/1 connection pool

[asraa]

Signed-off-by: Asra Ali asraa@google.com

Commit Message: Fixes (thanks @yanavlasov @antoniovicente) use after free when dispatcher tries to run conn_pool->onUpstreamReady() after the connection pool was destroyed. Reverts back to a schedulable callback per https://github.com/envoyproxy/envoy/pull/13867/files

Risk Level: Medium
Testing: Added test. This test fails with the use after free at head:

 
+TEST_F(Http1ConnPoolImplTest, RegressionTest) {
+  InSequence s;
+
+  NiceMock<MockResponseDecoder> outer_decoder;
+  ConnPoolCallbacks callbacks;
+  conn_pool_->expectClientCreate();
+  Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks);
+  EXPECT_NE(nullptr, handle);
+
+  NiceMock<MockRequestEncoder> request_encoder;
+  ResponseDecoder* inner_decoder;
+  EXPECT_CALL(*conn_pool_->test_clients_[0].codec_, newStream(_))
+      .WillOnce(DoAll(SaveArgAddress(&inner_decoder), ReturnRef(request_encoder)));
+  EXPECT_CALL(callbacks.pool_ready_, ready());
+  EXPECT_CALL(*conn_pool_->test_clients_[0].connect_timer_, disableTimer());
+  conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected);
+
+  EXPECT_TRUE(
+      callbacks.outer_encoder_
+          ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true)
+          .ok());
+
+  Event::PostCb post_cb;
+  EXPECT_CALL(conn_pool_->mock_dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb));
+
+  dispatcher_.deferredDelete(std::move(conn_pool_));
+
+  inner_decoder->decodeHeaders(
+      ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true);
+
+  // Clear deferred delete list and trigger post callback
+  dispatcher_.clearDeferredDeleteList();
+  post_cb();
+
+  conn_pool_->destructAllConnections();
+}

This test had to be modified now that there is a schedulable callback.What I don't like about the fix/test is that the callback is never scheduled after the fix because it is guarded by hasPendingStreams() which ends up false in this test, so there is never a potential for use after free if I add that if condition. I don't know if in production crashes this is exactly the problem or not. Trying to understand how to make this test better..

[asraa]

Going to close this until I figure out more test failures.

[antoniovicente]

Thanks for taking on the debugging and fixing of this crash.

[asraa]

Working on reproducing the test failure in opt mode (doesn't reproduce locally with bazel)

[antoniovicente]

The new test case is crashing on the gcc build and possibly others. Please take a look.

[asraa]

Seems to be an issue clearing the deferred delete list which contains a connection pool. When conn pool is destroyed, it destroys it's clients, which calls clear deferred delete again, and then recurses.

This wouldn't happen wit ha real dispatcher, so working on modifying the clearDeferredDeleteList to change.

[antoniovicente]

You could remove this empty destructor override.

At the very least change to:
~ConnPoolImplNoDestructForTest() override = default;

[asraa]

I ended up getting rid of it, I dealt with the client destruction in the test instead of in this special connpool

[antoniovicente]

nit: Usual ordering of elements in the private section according to style guide is:

• classes/structs
• functions
• data members