Fix a socket NULL-dereference in ProcessReadResult.

The QuicChromiumPacketReader upon finishing the socket read, will dereference
the socket to get local and peer addresses and report packet back to the
visitor unconditionally even if the connection is closed. The patch landed in
crrev.com/c/1900569 nulls out the socket upon connection close, leading to null
dereference when fetching local and peer addresses.

Bug: 1014092, 1021938
Change-Id: I9af79f03b3e2ffbe1ed036af8980efa6b6d1478c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1901946
Commit-Queue: Zhongyi Shi <zhongyi@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Auto-Submit: Zhongyi Shi <zhongyi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713287}

net/quic/quic_chromium_client_session_test.cc

@@ -618,6 +618,76 @@ TEST_P(QuicChromiumClientSessionTest, AsyncStreamRequest) {
   EXPECT_TRUE(quic_data.AllWriteDataConsumed());
 }
 
+// Regression test for https://crbug.com/1021938.
+// When the connection is closed, there may be tasks queued in the message loop
+// to read the last packet, reading that packet should not crash.
+TEST_P(QuicChromiumClientSessionTest, ReadAfterConnectionClose) {
+  MockQuicData quic_data(version_);
+  if (version_.transport_version == quic::QUIC_VERSION_99) {
+    quic_data.AddWrite(SYNCHRONOUS, client_maker_.MakeInitialSettingsPacket(1));
+    // The open stream limit is set to 50 by
+    // MockCryptoClientStream::SetConfigNegotiated() so when the 51st stream is
+    // requested, a STREAMS_BLOCKED will be sent, indicating that it's blocked
+    // at the limit of 50.
+    quic_data.AddWrite(SYNCHRONOUS, client_maker_.MakeStreamsBlockedPacket(
+                                        2, true, 50,
+                                        /*unidirectional=*/false));
+    quic_data.AddWrite(SYNCHRONOUS, client_maker_.MakeStreamsBlockedPacket(
+                                        3, true, 50,
+                                        /*unidirectional=*/false));
+  }
+  quic_data.AddRead(ASYNC, ERR_IO_PENDING);
+  // This packet will be read after connection is closed.
+  quic_data.AddRead(
+      ASYNC,
+      server_maker_.MakeConnectionClosePacket(
+          1, false, quic::QUIC_CRYPTO_VERSION_NOT_SUPPORTED, "Time to panic!"));
+  quic_data.AddSocketDataToFactory(&socket_factory_);
+
+  Initialize();
+  CompleteCryptoHandshake();
+
+  // Open the maximum number of streams so that a subsequent request
+  // can not proceed immediately.
+  const size_t kMaxOpenStreams = GetMaxAllowedOutgoingBidirectionalStreams();
+  for (size_t i = 0; i < kMaxOpenStreams; i++) {
+    QuicChromiumClientSessionPeer::CreateOutgoingStream(session_.get());
+  }
+  EXPECT_EQ(kMaxOpenStreams, session_->GetNumOpenOutgoingStreams());
+
+  // Request two streams which will both be pending.
+  // In V99 each will generate a max stream id for each attempt.
+  std::unique_ptr<QuicChromiumClientSession::Handle> handle =
+      session_->CreateHandle(destination_);
+  std::unique_ptr<QuicChromiumClientSession::Handle> handle2 =
+      session_->CreateHandle(destination_);
+
+  ASSERT_EQ(
+      ERR_IO_PENDING,
+      handle->RequestStream(
+          /*requires_confirmation=*/false,
+          base::BindOnce(&QuicChromiumClientSessionTest::ResetHandleOnError,
+                         base::Unretained(this), &handle2),
+          TRAFFIC_ANNOTATION_FOR_TESTS));
+
+  TestCompletionCallback callback2;
+  ASSERT_EQ(ERR_IO_PENDING,
+            handle2->RequestStream(/*requires_confirmation=*/false,
+                                   callback2.callback(),
+                                   TRAFFIC_ANNOTATION_FOR_TESTS));
+
+  session_->connection()->CloseConnection(
+      quic::QUIC_NETWORK_IDLE_TIMEOUT, "Timed out",
+      quic::ConnectionCloseBehavior::SILENT_CLOSE);
+
+  // Pump the message loop to read the connection close packet.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(handle2.get());
+  quic_data.Resume();
+  EXPECT_TRUE(quic_data.AllReadDataConsumed());
+  EXPECT_TRUE(quic_data.AllWriteDataConsumed());
+}
+
 TEST_P(QuicChromiumClientSessionTest, ClosedWithAsyncStreamRequest) {
   MockQuicData quic_data(version_);
   if (version_.transport_version == quic::QUIC_VERSION_99) {

net/quic/quic_chromium_packet_reader.cc

@@ -116,10 +116,17 @@ bool QuicChromiumPacketReader::ProcessReadResult(int result) {
   quic::QuicReceivedPacket packet(read_buffer_->data(), result, clock_->Now());
   IPEndPoint local_address;
   IPEndPoint peer_address;
-  socket_->GetLocalAddress(&local_address);
-  socket_->GetPeerAddress(&peer_address);
-  return visitor_->OnPacket(packet, ToQuicSocketAddress(local_address),
-                            ToQuicSocketAddress(peer_address));
+  // TODO(zhongyi): once crbug.com/1014092 is root caused, consider early return
+  // false if |socket_| is nulled. For debugging purpose, still report up to
+  // avoid introducing behavior change.
+  // If the socket has been nulled, the connection is already closed. Reporting
+  // packet up to the visitor is a no-op.
+  if (socket_ != nullptr) {
+    socket_->GetLocalAddress(&local_address_);
+    socket_->GetPeerAddress(&peer_address_);
+  }
+  return visitor_->OnPacket(packet, ToQuicSocketAddress(local_address_),
+                            ToQuicSocketAddress(peer_address_));
 }
 
 void QuicChromiumPacketReader::OnReadComplete(int result) {

net/quic/quic_chromium_packet_reader.h

@@ -16,6 +16,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "net/base/io_buffer.h"
+#include "net/base/ip_endpoint.h"
 #include "net/base/net_export.h"
 #include "net/log/net_log_with_source.h"
 #include "net/socket/datagram_client_socket.h"
@@ -65,6 +66,9 @@ class NET_EXPORT_PRIVATE QuicChromiumPacketReader {
   void SetShouldStopReading() {
     DCHECK(!should_stop_reading_);
     should_stop_reading_ = true;
+    // Cache local and peer addresses before null out the socket.
+    socket_->GetLocalAddress(&local_address_);
+    socket_->GetPeerAddress(&peer_address_);
     socket_ = nullptr;
   }
 
@@ -85,6 +89,8 @@ class NET_EXPORT_PRIVATE QuicChromiumPacketReader {
   bool ProcessReadResult(int result);
 
   DatagramClientSocket* socket_;
+  IPEndPoint local_address_;
+  IPEndPoint peer_address_;
   // Set to true if |this| should no longer attempt read.
   bool should_stop_reading_;