From eb721a86c032d2f945afb73fe45b3c101c4366d6 Mon Sep 17 00:00:00 2001
From: Kenichi Ishibashi <bashi@chromium.org>
Date: Sun, 10 May 2026 18:54:31 -0700
Subject: [PATCH] Fix potential crash in SpdyStream::QueueNextDataFrame via PrefacePing drain

This CL fixes a potential crash in SpdyStream caused by accessing a
destroyed object when a session drain is triggered by a Preface Ping
hitting the capped frames limit.

SpdyStream::QueueNextDataFrame calls SpdySession::CreateDataBuffer,
which synchronously invokes MaybeSendPrefacePing if the connection has
been idle. If the number of queued capped frames already exceeds the max
allowed limit, EnqueueSessionWrite historically called DoDrainSession
synchronously.

DoDrainSession immediately destroys all active streams. Because this
happened synchronously inside the CreateDataBuffer call stack, the
stream's data buffer reference was dropped, causing subsequent data
frame serialization to access invalid memory, resulting in a crash.

This CL changes the synchronous DoDrainSession to DoDrainSessionAsync in
EnqueueSessionWrite when the cap is hit. This ensures the session is
marked unavailable immediately, but the actual stream destruction is
deferred to the next message loop tick, allowing the call stack to
unwind safely.

Bug: 507365348
Test: SpdySessionTest.SendDataExceedsCappedFramesLimitViaPrefacePing
Change-Id: I25b82c06a7218514ea664bd43767edf2ead9bcdc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7825349
Reviewed-by: mmenke <mmenke@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1628325}
---

diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index f0775b61..3f7b9e58 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -98,7 +98,6 @@
     )");
 
 const int kReadBufferSize = 8 * 1024;
-const int kDefaultConnectionAtRiskOfLossSeconds = 10;
 const int kHungIntervalSeconds = 10;
 
 // Default initial value for HTTP/2 SETTINGS.
@@ -855,7 +854,7 @@
       is_http2_enabled_(is_http2_enabled),
       is_quic_enabled_(is_quic_enabled),
       connection_at_risk_of_loss_time_(
-          base::Seconds(kDefaultConnectionAtRiskOfLossSeconds)),
+          base::Seconds(kSpdyDefaultConnectionAtRiskOfLossSeconds)),
       hung_interval_(base::Seconds(kHungIntervalSeconds)),
       time_func_(time_func),
       network_quality_estimator_(network_quality_estimator),
@@ -2557,7 +2556,20 @@
         << "Draining session due to exceeding max queued capped frames";
     // Use ERR_CONNECTION_CLOSED to avoid sending a GOAWAY frame since that
     // frame would also exceed the cap.
-    DoDrainSession(ERR_CONNECTION_CLOSED, "Exceeded max queued capped frames");
+    // Drain the session asynchronously because this can be called from a
+    // context where a stream is actively processing data on the stack (e.g.,
+    // inside SpdyStream::QueueNextDataFrame via a Preface Ping). Synchronous
+    // draining would destroy the stream immediately, leading to Use-After-Free
+    // crashes when control returns to the stream method.
+    //
+    // Note: Skipping this write and draining asynchronously means callers might
+    // assume this write was enqueued and go on to enqueue subsequent frames
+    // that could get sent over the wire before the drain completes. However,
+    // this is generally acceptable for capped frames (RST_STREAM,
+    // WINDOW_UPDATE, PING, GOAWAY, SETTINGS) because they are mostly isolated
+    // messages without strict sequence dependencies.
+    DoDrainSessionAsync(ERR_CONNECTION_CLOSED,
+                        "Exceeded max queued capped frames");
     return;
   }
   auto buffer = std::make_unique<SpdyBuffer>(std::move(frame));
diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h
index 53d7acf..aeef8d82 100644
--- a/net/spdy/spdy_session.h
+++ b/net/spdy/spdy_session.h
@@ -102,6 +102,13 @@
 // attacker from growing this queue unboundedly.
 const int kSpdySessionMaxQueuedCappedFrames = 10000;
 
+// Default minimum time the connection must be idle before a "Preface Ping"
+// is sent upon subsequent write activity.
+// A "Preface Ping" is a PING frame proactively sent by the SPDY session
+// prior to enqueuing a DATA or HEADERS frame when the connection has been
+// idle, to verify that the network path is still alive.
+const int kSpdyDefaultConnectionAtRiskOfLossSeconds = 10;
+
 // Default time to delay sending small receive window updates (can be
 // configured through SetTimeToBufferSmallWindowUpdates()). Usually window
 // updates are sent when half of the receive window has been processed by
diff --git a/net/spdy/spdy_session_unittest.cc b/net/spdy/spdy_session_unittest.cc
index 91c548b1..a24ce29 100644
--- a/net/spdy/spdy_session_unittest.cc
+++ b/net/spdy/spdy_session_unittest.cc
@@ -133,6 +133,25 @@
       base::WeakPtr<SpdySession> spdy_session) override {}
 };
 
+// Custom delegate to wait for headers sent on a stream.
+class HeadersSentDelegate : public test::StreamDelegateDoNothing {
+ public:
+  HeadersSentDelegate(const base::WeakPtr<SpdyStream>& stream,
+                      base::OnceClosure quit_closure)
+      : StreamDelegateDoNothing(stream),
+        quit_closure_(std::move(quit_closure)) {}
+
+  void OnHeadersSent() override {
+    test::StreamDelegateDoNothing::OnHeadersSent();
+    if (quit_closure_) {
+      std::move(quit_closure_).Run();
+    }
+  }
+
+ private:
+  base::OnceClosure quit_closure_;
+};
+
 }  // namespace
 
 class SpdySessionTest : public PlatformTest, public WithTaskEnvironment {
@@ -2748,25 +2767,6 @@
   test::StreamDelegateDoNothing delegate2(stream2);
   stream2->SetDelegate(&delegate2);
 
-  // Custom delegate to wait for headers sent on stream3 (last one).
-  class HeadersSentDelegate : public test::StreamDelegateDoNothing {
-   public:
-    HeadersSentDelegate(const base::WeakPtr<SpdyStream>& stream,
-                        base::OnceClosure quit_closure)
-        : StreamDelegateDoNothing(stream),
-          quit_closure_(std::move(quit_closure)) {}
-
-    void OnHeadersSent() override {
-      test::StreamDelegateDoNothing::OnHeadersSent();
-      if (quit_closure_) {
-        std::move(quit_closure_).Run();
-      }
-    }
-
-   private:
-    base::OnceClosure quit_closure_;
-  };
-
   base::RunLoop run_loop;
   HeadersSentDelegate delegate3(stream3, run_loop.QuitClosure());
   stream3->SetDelegate(&delegate3);
@@ -2804,6 +2804,120 @@
   EXPECT_THAT(delegate3.WaitForClose(), IsError(ERR_CONNECTION_CLOSED));
 }
 
+// Tests that the session drains when the number of queued capped frames
+// exceeds the limit due to a Preface Ping triggered during SendData.
+//
+// This test sets up the following scenario:
+// 1. Set the capped frames limit to 1.
+// 2. Create 3 streams: stream1 (GET), stream2 (GET), and stream3 (POST).
+// 3. Cancel stream1 and stream2. Each cancellation enqueues a RST_STREAM frame.
+//    RST_STREAM is a "capped" frame.
+//    After 2 cancellations, there are 2 capped frames in the write queue.
+//    Since 2 > limit(1), the next attempt to enqueue a capped frame will
+//    trigger a session drain.
+//    Note: The effective limit is actually the max queue size + 1.
+// 4. Advance time to make the session appear idle.
+// 5. Call stream3->SendData(). This triggers a Preface Ping.
+// 6. The Preface Ping is a PING frame, which is also a capped frame.
+// 7. Enqueuing the Preface Ping sees that the queue already has 2 capped
+//    frames, which exceeds the limit of 1.
+// 8. This triggers DoDrainSessionAsync().
+//
+// This is a regression test for crbug.com/507365348.
+TEST_F(SpdySessionTest, SendDataExceedsCappedFramesLimitViaPrefacePing) {
+  // Set the capped frames limit to 1.
+  session_deps_.session_max_queued_capped_frames = 1;
+  session_deps_.enable_ping = true;
+  session_deps_.time_func = TheNearFuture;
+  g_time_delta = base::TimeDelta();
+
+  spdy::SpdySerializedFrame req1(spdy_util_.ConstructSpdyGet(
+      base::span<const std::string_view>(), 1, LOWEST));
+  spdy::SpdySerializedFrame req2(spdy_util_.ConstructSpdyGet(
+      base::span<const std::string_view>(), 3, LOWEST));
+  spdy::SpdySerializedFrame req3(
+      spdy_util_.ConstructSpdyPost(kDefaultUrl, 5, kUploadDataSize, LOWEST,
+                                   base::span<const std::string_view>()));
+
+  spdy::SpdySerializedFrame rst1(
+      spdy_util_.ConstructSpdyRstStream(1, spdy::ERROR_CODE_CANCEL));
+  spdy::SpdySerializedFrame rst2(
+      spdy_util_.ConstructSpdyRstStream(3, spdy::ERROR_CODE_CANCEL));
+
+  MockWrite writes[] = {
+      CreateMockWrite(req1, 1), CreateMockWrite(req2, 2),
+      CreateMockWrite(req3, 3), CreateMockWrite(rst1, 4),
+      CreateMockWrite(rst2, 5),

diff --git a/net/spdy/spdy_session_unittest.cc b/net/spdy/spdy_session_unittest.cc
index 91c548b1..a24ce29 100644
--- a/net/spdy/spdy_session_unittest.cc
+++ b/net/spdy/spdy_session_unittest.cc
@@ -133,6 +133,25 @@
       base::WeakPtr<SpdySession> spdy_session) override {}
 };
 
+// Custom delegate to wait for headers sent on a stream.
+class HeadersSentDelegate : public test::StreamDelegateDoNothing {
+ public:
+  HeadersSentDelegate(const base::WeakPtr<SpdyStream>& stream,
+                      base::OnceClosure quit_closure)
+      : StreamDelegateDoNothing(stream),
+        quit_closure_(std::move(quit_closure)) {}
+
+  void OnHeadersSent() override {
+    test::StreamDelegateDoNothing::OnHeadersSent();
+    if (quit_closure_) {
+      std::move(quit_closure_).Run();
+    }
+  }
+
+ private:
+  base::OnceClosure quit_closure_;
+};
+
 }  // namespace
 
 class SpdySessionTest : public PlatformTest, public WithTaskEnvironment {
@@ -2748,25 +2767,6 @@
   test::StreamDelegateDoNothing delegate2(stream2);
   stream2->SetDelegate(&delegate2);
 
-  // Custom delegate to wait for headers sent on stream3 (last one).
-  class HeadersSentDelegate : public test::StreamDelegateDoNothing {
-   public:
-    HeadersSentDelegate(const base::WeakPtr<SpdyStream>& stream,
-                        base::OnceClosure quit_closure)
-        : StreamDelegateDoNothing(stream),
-          quit_closure_(std::move(quit_closure)) {}
-
-    void OnHeadersSent() override {
-      test::StreamDelegateDoNothing::OnHeadersSent();
-      if (quit_closure_) {
-        std::move(quit_closure_).Run();
-      }
-    }
-
-   private:
-    base::OnceClosure quit_closure_;
-  };
-
   base::RunLoop run_loop;
   HeadersSentDelegate delegate3(stream3, run_loop.QuitClosure());
   stream3->SetDelegate(&delegate3);
@@ -2804,6 +2804,120 @@
   EXPECT_THAT(delegate3.WaitForClose(), IsError(ERR_CONNECTION_CLOSED));
 }
 
+// Tests that the session drains when the number of queued capped frames
+// exceeds the limit due to a Preface Ping triggered during SendData.
+//
+// This test sets up the following scenario:
+// 1. Set the capped frames limit to 1.
+// 2. Create 3 streams: stream1 (GET), stream2 (GET), and stream3 (POST).
+// 3. Cancel stream1 and stream2. Each cancellation enqueues a RST_STREAM frame.
+//    RST_STREAM is a "capped" frame.
+//    After 2 cancellations, there are 2 capped frames in the write queue.
+//    Since 2 > limit(1), the next attempt to enqueue a capped frame will
+//    trigger a session drain.
+//    Note: The effective limit is actually the max queue size + 1.
+// 4. Advance time to make the session appear idle.
+// 5. Call stream3->SendData(). This triggers a Preface Ping.
+// 6. The Preface Ping is a PING frame, which is also a capped frame.
+// 7. Enqueuing the Preface Ping sees that the queue already has 2 capped
+//    frames, which exceeds the limit of 1.
+// 8. This triggers DoDrainSessionAsync().
+//
+// This is a regression test for crbug.com/507365348.
+TEST_F(SpdySessionTest, SendDataExceedsCappedFramesLimitViaPrefacePing) {
+  // Set the capped frames limit to 1.
+  session_deps_.session_max_queued_capped_frames = 1;
+  session_deps_.enable_ping = true;
+  session_deps_.time_func = TheNearFuture;
+  g_time_delta = base::TimeDelta();
+
+  spdy::SpdySerializedFrame req1(spdy_util_.ConstructSpdyGet(
+      base::span<const std::string_view>(), 1, LOWEST));
+  spdy::SpdySerializedFrame req2(spdy_util_.ConstructSpdyGet(
+      base::span<const std::string_view>(), 3, LOWEST));
+  spdy::SpdySerializedFrame req3(
+      spdy_util_.ConstructSpdyPost(kDefaultUrl, 5, kUploadDataSize, LOWEST,
+                                   base::span<const std::string_view>()));
+
+  spdy::SpdySerializedFrame rst1(
+      spdy_util_.ConstructSpdyRstStream(1, spdy::ERROR_CODE_CANCEL));
+  spdy::SpdySerializedFrame rst2(
+      spdy_util_.ConstructSpdyRstStream(3, spdy::ERROR_CODE_CANCEL));
+
+  MockWrite writes[] = {
+      CreateMockWrite(req1, 1), CreateMockWrite(req2, 2),
+      CreateMockWrite(req3, 3), CreateMockWrite(rst1, 4),
+      CreateMockWrite(rst2, 5),
+  };
+
+  MockRead reads[] = {
+      MockRead(SYNCHRONOUS, ERR_IO_PENDING, 0),
+  };
+
+  SequencedSocketData data(reads, writes);
+  session_deps_.socket_factory->AddSocketDataProvider(&data);
+
+  AddSSLSocketData();
+
+  CreateNetworkSession();
+  CreateSpdySession();
+
+  base::WeakPtr<SpdyStream> stream1 =
+      CreateStreamSynchronously(SPDY_REQUEST_RESPONSE_STREAM, session_,
+                                test_url_, LOWEST, NetLogWithSource());
+  ASSERT_TRUE(stream1);
+
+  base::WeakPtr<SpdyStream> stream2 =
+      CreateStreamSynchronously(SPDY_REQUEST_RESPONSE_STREAM, session_,
+                                test_url_, LOWEST, NetLogWithSource());
+  ASSERT_TRUE(stream2);
+
+  base::WeakPtr<SpdyStream> stream3 =
+      CreateStreamSynchronously(SPDY_REQUEST_RESPONSE_STREAM, session_,
+                                test_url_, LOWEST, NetLogWithSource());
+  ASSERT_TRUE(stream3);
+
+  test::StreamDelegateDoNothing delegate1(stream1);
+  stream1->SetDelegate(&delegate1);
+
+  test::StreamDelegateDoNothing delegate2(stream2);
+  stream2->SetDelegate(&delegate2);
+
+  base::RunLoop run_loop;
+  HeadersSentDelegate delegate3(stream3, run_loop.QuitClosure());
+  stream3->SetDelegate(&delegate3);
+
+  quiche::HttpHeaderBlock headers1(
+      spdy_util_.ConstructGetHeaderBlock(kDefaultUrl));
+  stream1->SendRequestHeaders(std::move(headers1), NO_MORE_DATA_TO_SEND);
+
+  quiche::HttpHeaderBlock headers2(
+      spdy_util_.ConstructGetHeaderBlock(kDefaultUrl));
+  stream2->SendRequestHeaders(std::move(headers2), NO_MORE_DATA_TO_SEND);
+
+  quiche::HttpHeaderBlock headers3(
+      spdy_util_.ConstructPostHeaderBlock(kDefaultUrl, kUploadDataSize));
+  stream3->SendRequestHeaders(std::move(headers3), MORE_DATA_TO_SEND);
+
+  run_loop.Run();
+
+  EXPECT_EQ(1u, stream1->stream_id());
+  EXPECT_EQ(3u, stream2->stream_id());
+  EXPECT_EQ(5u, stream3->stream_id());
+
+  stream1->Cancel(ERR_ABORTED);
+  stream2->Cancel(ERR_ABORTED);
+
+  // Advance time to simulate connection idleness, forcing the next SendData
+  // call to trigger a Preface Ping.
+  g_time_delta += base::Seconds(kSpdyDefaultConnectionAtRiskOfLossSeconds + 1);
+
+  auto body = base::MakeRefCounted<StringIOBuffer>(kUploadData);
+  stream3->SendData(body.get(), kUploadDataSize, NO_MORE_DATA_TO_SEND);
+
+  EXPECT_THAT(delegate3.WaitForClose(), IsError(ERR_CONNECTION_CLOSED));
+}
+
 TEST_F(SpdySessionTest, VerifyDomainAuthentication) {
   SequencedSocketData data;
   session_deps_.socket_factory->AddSocketDataProvider(&data);