Close HTTP/1.1 sockets when blocked by CORB or CORP.

BUG=1154250

Change-Id: Iff9b03523b6265cb7e5ccc0bbbc43dd911ac9b9b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2575014
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834909}

net/http/http_cache_transaction.cc

@@ -615,6 +615,14 @@ void HttpCache::Transaction::GetConnectionAttempts(
               network_transaction_info_.old_connection_attempts.end());
 }
 
+void HttpCache::Transaction::CloseConnectionOnDestruction() {
+  if (network_trans_) {
+    network_trans_->CloseConnectionOnDestruction();
+  } else if (InWriters()) {
+    entry_->writers->CloseConnectionOnDestruction();
+  }
+}
+
 void HttpCache::Transaction::SetValidatingCannotProceed() {
   DCHECK(!reading_);
   // Ensure this transaction is waiting for a callback.

net/http/http_cache_transaction.h

@@ -160,6 +160,7 @@ class NET_EXPORT_PRIVATE HttpCache::Transaction : public HttpTransaction {
   void SetResponseHeadersCallback(ResponseHeadersCallback callback) override;
   int ResumeNetworkStart() override;
   void GetConnectionAttempts(ConnectionAttempts* out) const override;
+  void CloseConnectionOnDestruction() override;
 
   // Invoked when parallel validation cannot proceed due to response failure
   // and this transaction needs to be restarted.

net/http/http_cache_writers.cc

@@ -225,6 +225,11 @@ void HttpCache::Writers::UpdatePriority() {
   }
 }
 
+void HttpCache::Writers::CloseConnectionOnDestruction() {
+  if (network_transaction_)
+    network_transaction_->CloseConnectionOnDestruction();
+}
+
 bool HttpCache::Writers::ContainsOnlyIdleWriters() const {
   return waiting_for_read_.empty() && !active_transaction_;
 }

net/http/http_cache_writers.h

@@ -125,6 +125,8 @@ class NET_EXPORT_PRIVATE HttpCache::Writers {
     return network_transaction_.get();
   }
 
+  void CloseConnectionOnDestruction();
+
   // Returns the load state of the |network_transaction_| if present else
   // returns LOAD_STATE_IDLE.
   LoadState GetLoadState() const;

net/http/http_network_transaction.cc

@@ -148,7 +148,8 @@ HttpNetworkTransaction::~HttpNetworkTransaction() {
     // TODO(mbelshe): The stream_ should be able to compute whether or not the
     //                stream should be kept alive.  No reason to compute here
     //                and pass it in.
-    if (!stream_->CanReuseConnection() || next_state_ != STATE_NONE) {
+    if (!stream_->CanReuseConnection() || next_state_ != STATE_NONE ||
+        close_connection_on_destruction_) {
       stream_->Close(true /* not reusable */);
     } else if (stream_->IsResponseBodyComplete()) {
       // If the response body is complete, we can just reuse the socket.
@@ -542,6 +543,10 @@ int HttpNetworkTransaction::ResumeNetworkStart() {
   return DoLoop(OK);
 }
 
+void HttpNetworkTransaction::CloseConnectionOnDestruction() {
+  close_connection_on_destruction_ = true;
+}
+
 void HttpNetworkTransaction::OnStreamReady(const SSLConfig& used_ssl_config,
                                            const ProxyInfo& used_proxy_info,
                                            std::unique_ptr<HttpStream> stream) {

net/http/http_network_transaction.h

@@ -88,8 +88,8 @@ class NET_EXPORT_PRIVATE HttpNetworkTransaction
   void SetConnectedCallback(const ConnectedCallback& callback) override;
   void SetRequestHeadersCallback(RequestHeadersCallback callback) override;
   void SetResponseHeadersCallback(ResponseHeadersCallback callback) override;
-
   int ResumeNetworkStart() override;
+  void CloseConnectionOnDestruction() override;
 
   // HttpStreamRequest::Delegate methods:
   void OnStreamReady(const SSLConfig& used_ssl_config,
@@ -435,6 +435,8 @@ class NET_EXPORT_PRIVATE HttpNetworkTransaction
   // Number of times the transaction was restarted via a RestartWith* call.
   size_t num_restarts_;
 
+  bool close_connection_on_destruction_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(HttpNetworkTransaction);
 };
 

net/http/http_network_transaction_unittest.cc

@@ -9390,6 +9390,105 @@ TEST_F(HttpNetworkTransactionTest, RecycleDeadSSLSocket) {
   EXPECT_EQ(1, GetIdleSocketCountInTransportSocketPool(session.get()));
 }
 
+TEST_F(HttpNetworkTransactionTest, CloseConnectionOnDestruction) {
+  enum class TestCase {
+    kReadHeaders,
+    kReadPartOfBodyRead,
+    kReadAllOfBody,
+  };
+
+  for (auto test_case : {TestCase::kReadHeaders, TestCase::kReadPartOfBodyRead,
+                         TestCase::kReadAllOfBody}) {
+    SCOPED_TRACE(testing::Message()
+                 << "Test case: " << static_cast<int>(test_case));
+    for (bool close_connection : {false, true}) {
+      if (test_case != TestCase::kReadAllOfBody || close_connection == false)
+        continue;
+      SCOPED_TRACE(testing::Message()
+                   << "Close connection: " << close_connection);
+
+      HttpRequestInfo request;
+      request.method = "GET";
+      request.url = GURL("http://foo.test/");
+      request.traffic_annotation =
+          net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS);
+
+      std::unique_ptr<HttpNetworkSession> session(
+          CreateSession(&session_deps_));
+
+      std::unique_ptr<HttpNetworkTransaction> trans =
+          std::make_unique<HttpNetworkTransaction>(DEFAULT_PRIORITY,
+                                                   session.get());
+
+      MockRead data_reads[] = {
+          // A part of the response body is received with the response headers.
+          MockRead("HTTP/1.1 200 OK\r\n"
+                   "Content-Length: 11\r\n\r\n"
+                   "hello world"),
+          MockRead(SYNCHRONOUS, OK),
+      };
+
+      StaticSocketDataProvider data(data_reads, base::span<MockWrite>());
+      session_deps_.socket_factory->AddSocketDataProvider(&data);
+
+      TestCompletionCallback callback;
+
+      int rv = trans->Start(&request, callback.callback(), NetLogWithSource());
+      EXPECT_THAT(callback.GetResult(rv), IsOk());
+
+      const HttpResponseInfo* response = trans->GetResponseInfo();
+      ASSERT_TRUE(response);
+
+      EXPECT_TRUE(response->headers);
+      std::string status_line = response->headers->GetStatusLine();
+      EXPECT_EQ("HTTP/1.1 200 OK", status_line);
+
+      EXPECT_EQ(0, GetIdleSocketCountInTransportSocketPool(session.get()));
+
+      std::string response_data;
+      switch (test_case) {
+        case TestCase::kReadHeaders: {
+          // Already read the headers, nothing else to do.
+          break;
+        }
+
+        case TestCase::kReadPartOfBodyRead: {
+          scoped_refptr<IOBuffer> buf = base::MakeRefCounted<IOBuffer>(5);
+          rv = trans->Read(buf.get(), 5, callback.callback());
+          ASSERT_EQ(5, callback.GetResult(rv));
+          response_data.assign(buf->data(), 5);
+          EXPECT_EQ("hello", response_data);
+          break;
+        }
+
+        case TestCase::kReadAllOfBody: {
+          rv = ReadTransaction(trans.get(), &response_data);
+          EXPECT_THAT(rv, IsOk());
+          EXPECT_EQ("hello world", response_data);
+          break;
+        }
+      }
+
+      if (close_connection)
+        trans->CloseConnectionOnDestruction();
+      trans.reset();
+
+      // Wait for the socket to be drained and added to the socket pool or
+      // destroyed.
+      base::RunLoop().RunUntilIdle();
+
+      // In the case all the body was read, the socket will have been released
+      // before the CloseConnectionOnDestruction() call, so will not be
+      // destroyed.
+      if (close_connection && test_case != TestCase::kReadAllOfBody) {
+        EXPECT_EQ(0, GetIdleSocketCountInTransportSocketPool(session.get()));
+      } else {
+        EXPECT_EQ(1, GetIdleSocketCountInTransportSocketPool(session.get()));
+      }
+    }
+  }
+}
+
 // Grab a socket, use it, and put it back into the pool. Then, make
 // low memory notification and ensure the socket pool is flushed.
 TEST_F(HttpNetworkTransactionTest, FlushSocketPoolOnLowMemoryNotifications) {

net/http/http_transaction.h

@@ -206,6 +206,19 @@ class NET_EXPORT_PRIVATE HttpTransaction {
   virtual int ResumeNetworkStart() = 0;
 
   virtual void GetConnectionAttempts(ConnectionAttempts* out) const = 0;
+
+  // Configures the transaction to close the network connection, if any, on
+  // destruction. Intended for cases where keeping the socket alive may leak
+  // data. Does not immediately close the socket. If multiple transactions are
+  // using the same socket, only closes it once all transactions have completed.
+  //
+  // Does not close H2/H3 sessions, but does close H1 tunnels on top of H2/H3
+  // sessions.
+  //
+  // Only applies to currently in-use connections. Does nothing after the last
+  // byte of the response body has been read, as the connection is no longer in
+  // use at that point.
+  virtual void CloseConnectionOnDestruction() = 0;
 };
 
 }  // namespace net

net/http/http_transaction_test_util.cc

@@ -572,6 +572,10 @@ void MockNetworkTransaction::GetConnectionAttempts(
   NOTIMPLEMENTED();
 }
 
+void MockNetworkTransaction::CloseConnectionOnDestruction() {
+  NOTIMPLEMENTED();
+}
+
 void MockNetworkTransaction::CallbackLater(CompletionOnceCallback callback,
                                            int result) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(

net/http/http_transaction_test_util.h

@@ -250,6 +250,8 @@ class MockNetworkTransaction
 
   void GetConnectionAttempts(ConnectionAttempts* out) const override;
 
+  void CloseConnectionOnDestruction() override;
+
   CreateHelper* websocket_handshake_stream_create_helper() {
     return websocket_handshake_stream_create_helper_;
   }

net/url_request/url_request.cc

@@ -866,6 +866,14 @@ void URLRequest::ContinueDespiteLastError() {
   job_->ContinueDespiteLastError();
 }
 
+void URLRequest::AbortAndCloseConnection() {
+  DCHECK_EQ(OK, status_);
+  DCHECK(!has_notified_completion_);
+  DCHECK(job_);
+  job_->CloseConnectionOnDestruction();
+  job_.reset();
+}
+
 void URLRequest::PrepareToRestart() {
   DCHECK(job_.get());
 

net/url_request/url_request.h

@@ -618,6 +618,24 @@ class NET_EXPORT URLRequest : public base::SupportsUserData {
   // cancel the request instead, call Cancel().
   void ContinueDespiteLastError();
 
+  // Aborts the request (without invoking any completion callbacks) and closes
+  // the current connection, rather than returning it to the socket pool. Only
+  // affects HTTP/1.1 connections and tunnels.
+  //
+  // Intended to be used in cases where socket reuse can potentially leak data
+  // across sites.
+  //
+  // May only be called after Delegate::OnResponseStarted() has been invoked
+  // with net::OK, but before the body has been completely read. After the last
+  // body has been read, the socket may have already been handed off to another
+  // consumer.
+  //
+  // Due to transactions potentially being shared by multiple URLRequests in
+  // some cases, it is possible the socket may not be immediately closed, but
+  // will instead be closed when all URLRequests sharing the socket have been
+  // destroyed.
+  void AbortAndCloseConnection();
+
   // Used to specify the context (cookie store, cache) for this request.
   const URLRequestContext* context() const;
 

net/url_request/url_request_context_builder.cc

@@ -541,6 +541,10 @@ std::unique_ptr<URLRequestContext> URLRequestContextBuilder::Build() {
 
   HttpNetworkSession::Context network_session_context;
   SetHttpNetworkSessionComponents(context.get(), &network_session_context);
+  // Unlike the other fields of HttpNetworkSession::Context,
+  // |client_socket_factory| is not mirrored in URLRequestContext.
+  network_session_context.client_socket_factory =
+      client_socket_factory_for_testing_;
 
   storage->set_http_network_session(std::make_unique<HttpNetworkSession>(
       http_network_session_params_, network_session_context));

net/url_request/url_request_context_builder.h

@@ -51,6 +51,7 @@ class ApplicationStatusListener;
 namespace net {
 
 class CertVerifier;
+class ClientSocketFactory;
 class CookieStore;
 class CTPolicyEnforcer;
 class HttpAuthHandlerFactory;
@@ -311,6 +312,13 @@ class NET_EXPORT URLRequestContextBuilder {
       CreateHttpTransactionFactoryCallback
           create_http_network_transaction_factory);
 
+  // Sets a ClientSocketFactory so a test can mock out sockets. The
+  // ClientSocketFactory must be destroyed after the creates URLRequestContext.
+  void set_client_socket_factory_for_testing(
+      ClientSocketFactory* client_socket_factory_for_testing) {
+    client_socket_factory_for_testing_ = client_socket_factory_for_testing;
+  }
+
   // Creates a mostly self-contained URLRequestContext. May only be called once
   // per URLRequestContextBuilder. After this is called, the Builder can be
   // safely destroyed.
@@ -378,6 +386,8 @@ class NET_EXPORT URLRequestContextBuilder {
   std::map<std::string, std::unique_ptr<URLRequestJobFactory::ProtocolHandler>>
       protocol_handlers_;
 
+  ClientSocketFactory* client_socket_factory_for_testing_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(URLRequestContextBuilder);
 };
 

net/url_request/url_request_http_job.cc

@@ -305,6 +305,11 @@ void URLRequestHttpJob::GetConnectionAttempts(ConnectionAttempts* out) const {
     out->clear();
 }
 
+void URLRequestHttpJob::CloseConnectionOnDestruction() {
+  DCHECK(transaction_);
+  transaction_->CloseConnectionOnDestruction();
+}
+
 int URLRequestHttpJob::NotifyConnectedCallback(const TransportInfo& info) {
   return URLRequestJob::NotifyConnected(info);
 }

net/url_request/url_request_http_job.h

@@ -63,6 +63,7 @@ class NET_EXPORT_PRIVATE URLRequestHttpJob : public URLRequestJob {
   void Start() override;
   void Kill() override;
   void GetConnectionAttempts(ConnectionAttempts* out) const override;
+  void CloseConnectionOnDestruction() override;
   std::unique_ptr<SourceStream> SetUpSourceStream() override;
 
   RequestPriority priority() const {

net/url_request/url_request_job.cc

@@ -270,6 +270,8 @@ void URLRequestJob::GetConnectionAttempts(ConnectionAttempts* out) const {
   out->clear();
 }
 
+void URLRequestJob::CloseConnectionOnDestruction() {}
+
 namespace {
 
 // Assuming |url| has already been stripped for use as a referrer, if

net/url_request/url_request_job.h

@@ -240,6 +240,10 @@ class NET_EXPORT URLRequestJob {
   // from the remote party with the actual response headers recieved.
   virtual void SetResponseHeadersCallback(ResponseHeadersCallback callback) {}
 
+  // Causes the current transaction always close its active socket on
+  // destruction. Does not close H2/H3 sessions.
+  virtual void CloseConnectionOnDestruction();
+
   // Given |policy|, |original_referrer|, and |destination|, returns the
   // referrer URL mandated by |request|'s referrer policy.
   //

services/network/throttling/throttling_network_transaction.cc

@@ -304,4 +304,8 @@ void ThrottlingNetworkTransaction::GetConnectionAttempts(
   network_transaction_->GetConnectionAttempts(out);
 }
 
+void ThrottlingNetworkTransaction::CloseConnectionOnDestruction() {
+  network_transaction_->CloseConnectionOnDestruction();
+}
+
 }  // namespace network

services/network/throttling/throttling_network_transaction.h

@@ -85,6 +85,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) ThrottlingNetworkTransaction
       net::ResponseHeadersCallback callback) override;
   int ResumeNetworkStart() override;
   void GetConnectionAttempts(net::ConnectionAttempts* out) const override;
+  void CloseConnectionOnDestruction() override;
 
  protected:
   friend class ThrottlingControllerTestHelper;