Avoid crash in FindAvailableSession when changing socket tag of alias.

Bug: 954503
Change-Id: Ie52549316bb71e476335f63e95c6c178245cdd4f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1575842
Auto-Submit: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Bence Béky <bnc@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652834}

net/http/http_stream_factory_unittest.cc

@@ -2992,6 +2992,179 @@ TEST_F(HttpStreamFactoryTest, ChangeSocketTag) {
 
   waiter3.stream()->Close(/* not_reusable = */ true);
 }
+
+TEST_F(HttpStreamFactoryTest, ChangeSocketTagAvoidOverwrite) {
+  SpdySessionDependencies session_deps;
+  MockTaggingClientSocketFactory* socket_factory =
+      new MockTaggingClientSocketFactory();
+  session_deps.socket_factory.reset(socket_factory);
+
+  // Prepare for two HTTPS connects.
+  MockRead mock_read(SYNCHRONOUS, ERR_IO_PENDING);
+  SequencedSocketData socket_data(base::make_span(&mock_read, 1),
+                                  base::span<MockWrite>());
+  socket_data.set_connect_data(MockConnect(ASYNC, OK));
+  session_deps.socket_factory->AddSocketDataProvider(&socket_data);
+  MockRead mock_read2(SYNCHRONOUS, ERR_IO_PENDING);
+  SequencedSocketData socket_data2(base::make_span(&mock_read2, 1),
+                                   base::span<MockWrite>());
+  socket_data2.set_connect_data(MockConnect(ASYNC, OK));
+  session_deps.socket_factory->AddSocketDataProvider(&socket_data2);
+  SSLSocketDataProvider ssl_socket_data(ASYNC, OK);
+  // Use cert for *.example.org
+  ssl_socket_data.ssl_info.cert =
+      ImportCertFromFile(GetTestCertsDirectory(), "wildcard.pem");
+  ssl_socket_data.next_proto = kProtoHTTP2;
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_socket_data);
+  SSLSocketDataProvider ssl_socket_data2(ASYNC, OK);
+  // Use cert for *.example.org
+  ssl_socket_data2.ssl_info.cert =
+      ImportCertFromFile(GetTestCertsDirectory(), "wildcard.pem");
+  ssl_socket_data2.next_proto = kProtoHTTP2;
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_socket_data2);
+
+  std::unique_ptr<HttpNetworkSession> session(
+      SpdySessionDependencies::SpdyCreateSession(&session_deps));
+
+  // Prepare three different tags and corresponding HttpRequestInfos.
+  SocketTag tag1(SocketTag::UNSET_UID, 2);
+  HttpRequestInfo request_info1;
+  request_info1.method = "GET";
+  request_info1.url = GURL("https://www.example.org");
+  request_info1.load_flags = 0;
+  request_info1.socket_tag = tag1;
+  request_info1.traffic_annotation =
+      MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS);
+
+  SocketTag tag2(SocketTag::UNSET_UID, 1);
+  HttpRequestInfo request_info2 = request_info1;
+  request_info2.socket_tag = tag2;
+
+  HttpRequestInfo request_info3 = request_info1;
+  SocketTag tag3(SocketTag::UNSET_UID, 3);
+  request_info3.socket_tag = tag3;
+
+  // Prepare another HttpRequestInfo with tag3 and a different host name.
+  HttpRequestInfo request_info4 = request_info1;
+  request_info4.socket_tag = tag3;
+  request_info4.url = GURL("https://foo.example.org");
+
+  // Verify one stream with one tag results in one session, group and
+  // socket.
+  SSLConfig ssl_config;
+  StreamRequestWaiter waiter1;
+  std::unique_ptr<HttpStreamRequest> request1(
+      session->http_stream_factory()->RequestStream(
+          request_info1, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter1,
+          /* enable_ip_based_pooling = */ true,
+          /* enable_alternative_services = */ true, NetLogWithSource()));
+  waiter1.WaitForStream();
+  EXPECT_TRUE(waiter1.stream_done());
+  EXPECT_FALSE(waiter1.websocket_stream());
+  ASSERT_TRUE(waiter1.stream());
+
+  EXPECT_EQ(1, GetSpdySessionCount(session.get()));
+  EXPECT_EQ(
+      1, GetSocketPoolGroupCount(session->GetSocketPool(
+             HttpNetworkSession::NORMAL_SOCKET_POOL, ProxyServer::Direct())));
+  EXPECT_EQ(
+      1, GetHandedOutSocketCount(session->GetSocketPool(
+             HttpNetworkSession::NORMAL_SOCKET_POOL, ProxyServer::Direct())));
+  // Verify socket tagged appropriately.
+  MockTaggingStreamSocket* socket = socket_factory->GetLastProducedTCPSocket();
+  EXPECT_TRUE(tag1 == socket->tag());
+  EXPECT_TRUE(socket->tagged_before_connected());
+
+  // Initialize the first stream, thus marking the session active, so it cannot
+  // have its socket tag changed and be reused for the second session.
+  TestCompletionCallback callback1;
+  EXPECT_EQ(OK,
+            waiter1.stream()->InitializeStream(
+                &request_info1, /* can_send_early = */ false, DEFAULT_PRIORITY,
+                NetLogWithSource(), callback1.callback()));
+
+  // Create a second stream with a new tag.
+  StreamRequestWaiter waiter2;
+  std::unique_ptr<HttpStreamRequest> request2(
+      session->http_stream_factory()->RequestStream(
+          request_info2, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter2,
+          /* enable_ip_based_pooling = */ true,
+          /* enable_alternative_services = */ true, NetLogWithSource()));
+  waiter2.WaitForStream();
+  EXPECT_TRUE(waiter2.stream_done());
+  EXPECT_FALSE(waiter2.websocket_stream());
+  ASSERT_TRUE(waiter2.stream());
+  // Verify we now have two sessions.
+  EXPECT_EQ(2, GetSpdySessionCount(session.get()));
+  EXPECT_EQ(
+      1, GetSocketPoolGroupCount(session->GetSocketPool(
+             HttpNetworkSession::NORMAL_SOCKET_POOL, ProxyServer::Direct())));
+  EXPECT_EQ(
+      2, GetHandedOutSocketCount(session->GetSocketPool(
+             HttpNetworkSession::NORMAL_SOCKET_POOL, ProxyServer::Direct())));
+  // Verify a new socket was created.
+  MockTaggingStreamSocket* socket2 = socket_factory->GetLastProducedTCPSocket();
+  EXPECT_NE(socket, socket2);
+  // Verify tag set appropriately.
+  EXPECT_TRUE(tag2 == socket2->tag());
+  EXPECT_TRUE(socket2->tagged_before_connected());
+  // Verify tag on original socket is unchanged.
+  EXPECT_TRUE(tag1 == socket->tag());
+
+  // Initialize the second stream, thus marking the session active, so it cannot
+  // have its socket tag changed and be reused for the third session.
+  TestCompletionCallback callback2;
+  EXPECT_EQ(OK,
+            waiter2.stream()->InitializeStream(
+                &request_info2, /* can_send_early = */ false, DEFAULT_PRIORITY,
+                NetLogWithSource(), callback2.callback()));
+
+  // Release first stream so first session can be retagged for third request.
+  waiter1.stream()->Close(/* not_reusable = */ true);
+
+  // Verify the first session can be retagged for a third request.
+  StreamRequestWaiter waiter3;
+  std::unique_ptr<HttpStreamRequest> request3(
+      session->http_stream_factory()->RequestStream(
+          request_info3, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter3,
+          /* enable_ip_based_pooling = */ true,
+          /* enable_alternative_services = */ true, NetLogWithSource()));
+  waiter3.WaitForStream();
+  EXPECT_TRUE(waiter3.stream_done());
+  EXPECT_FALSE(waiter3.websocket_stream());
+  ASSERT_TRUE(waiter3.stream());
+  // Verify still have two sessions.
+  EXPECT_EQ(2, GetSpdySessionCount(session.get()));
+  EXPECT_EQ(
+      1, GetSocketPoolGroupCount(session->GetSocketPool(
+             HttpNetworkSession::NORMAL_SOCKET_POOL, ProxyServer::Direct())));
+  EXPECT_EQ(
+      2, GetHandedOutSocketCount(session->GetSocketPool(
+             HttpNetworkSession::NORMAL_SOCKET_POOL, ProxyServer::Direct())));
+  // Verify no new sockets created.
+  EXPECT_EQ(socket2, socket_factory->GetLastProducedTCPSocket());
+  // Verify socket tag changed.
+  EXPECT_TRUE(tag3 == socket->tag());
+  EXPECT_FALSE(socket->tagged_before_connected());
+
+  // Release second stream so second session can be retagged for fourth request.
+  waiter2.stream()->Close(/* not_reusable = */ true);
+
+  // Request a stream with a new tag and a different host that aliases existing
+  // sessions.
+  StreamRequestWaiter waiter4;
+  std::unique_ptr<HttpStreamRequest> request4(
+      session->http_stream_factory()->RequestStream(
+          request_info4, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter4,
+          /* enable_ip_based_pooling = */ true,
+          /* enable_alternative_services = */ true, NetLogWithSource()));
+  waiter4.WaitForStream();
+  EXPECT_TRUE(waiter4.stream_done());
+  EXPECT_FALSE(waiter4.websocket_stream());
+  ASSERT_TRUE(waiter4.stream());
+  // Verify no new sockets created.
+  EXPECT_EQ(socket2, socket_factory->GetLastProducedTCPSocket());
+}
 #endif
 
 // Test that when creating a stream all sessions that alias an IP are tried,

net/spdy/spdy_session_pool.cc

@@ -258,11 +258,19 @@ base::WeakPtr<SpdySession> SpdySessionPool::FindAvailableSession(
       // If socket tags differ, see if session's socket tag can be changed.
       if (alias_key.socket_tag() != key.socket_tag()) {
         SpdySessionKey old_key = available_session->spdy_session_key();
+        SpdySessionKey new_key(old_key.host_port_pair(), old_key.proxy_server(),
+                               old_key.privacy_mode(),
+                               old_key.is_proxy_session(), key.socket_tag());
+
+        // If there is already a session with |new_key|, skip this one.
+        // It will be found in |aliases_| in a future iteration.
+        if (available_sessions_.find(new_key) != available_sessions_.end())
+          continue;
 
         if (!available_session->ChangeSocketTag(key.socket_tag()))
           continue;
 
-        const SpdySessionKey& new_key = available_session->spdy_session_key();
+        DCHECK(available_session->spdy_session_key() == new_key);
 
         // This isn't a pooled alias, it's the actual session.
         adding_pooled_alias = false;