From 71532a74a33965920c29b251380d51f0291da648 Mon Sep 17 00:00:00 2001
From: Adam Rice <[email protected]>
Date: Fri, 29 May 2026 09:15:50 -0700
Subject: [PATCH] Fix use-after-free in SpdyBuffer consume callbacks

Copy the consume callbacks vector to a local variable before iterating
through it.

Previously, SpdyBuffer iterated directly over its member vector of
callbacks. If a consume callback reentrantly caused the SpdyBuffer to be
destroyed, the member vector was also destroyed, leading to a
use-after-free error when the loop attempted to access the next element
or evaluate the iterator.

Iterating over a local copy of the vector ensures that the iterator
remains valid and the BindState of each callback is kept alive
throughout the execution of the loop, safely handling cases where the
buffer is freed mid-iteration.

Bug: 499182801
Change-Id: I417ec0459069e6fb271b95d28e2d9dc80a869d7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7885302
Commit-Queue: Adam Rice <[email protected]>
Auto-Submit: Adam Rice <[email protected]>
Reviewed-by: Maks Orlovich <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1638529}
---

diff --git a/net/spdy/spdy_buffer.cc b/net/spdy/spdy_buffer.cc
index 61d3900..eeea0309 100644
--- a/net/spdy/spdy_buffer.cc
+++ b/net/spdy/spdy_buffer.cc
@@ -104,10 +104,16 @@
   DCHECK_GE(consume_size, 1u);
   DCHECK_LE(consume_size, GetRemainingSize());
   offset_ += consume_size;
-  for (std::vector<ConsumeCallback>::const_iterator it =
-           consume_callbacks_.begin(); it != consume_callbacks_.end(); ++it) {
-    it->Run(consume_size, consume_source);
+  // Copy callbacks before iterating: a consume callback may cause `this` to be
+  // destroyed reentrantly. Iterating a local copy keeps the iterator valid and
+  // keeps each callback's BindState alive (via RepeatingCallback's
+  // scoped_refptr) even after `this` is freed. The callbacks themselves are
+  // WeakPtr-bound and tolerate the receiver being gone.
+  std::vector<ConsumeCallback> callbacks = consume_callbacks_;
+  for (const auto& callback : callbacks) {
+    callback.Run(consume_size, consume_source);
   }
+  // `this` may have been deleted here.
 }
 
 }  // namespace net
diff --git a/net/spdy/spdy_read_queue_unittest.cc b/net/spdy/spdy_read_queue_unittest.cc
index 03e89802c..eb6c7037 100644
--- a/net/spdy/spdy_read_queue_unittest.cc
+++ b/net/spdy/spdy_read_queue_unittest.cc
@@ -5,10 +5,12 @@
 #include "net/spdy/spdy_read_queue.h"
 
 #include <algorithm>
+#include <array>
 #include <cstddef>
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "base/containers/heap_array.h"
 #include "base/containers/span.h"
@@ -138,4 +140,55 @@
   EXPECT_TRUE(read_queue.IsEmpty());
 }
 
+// Tests that calling Dequeue() reentrantly from within a consume callback
+// does not cause a use-after-free when the SpdyBuffer is destroyed during
+// the reentrant call.
+namespace {
+
+void ReentrantDequeue(SpdyReadQueue* queue,
+                      bool* fired,
+                      size_t inner_buf_len,
+                      size_t consume_size,
+                      SpdyBuffer::ConsumeSource consume_source) {
+  if (*fired) {
+    return;
+  }
+  *fired = true;
+
+  std::vector<uint8_t> inner_buf(inner_buf_len);
+  queue->Dequeue(inner_buf);
+}
+
+}  // namespace
+
+TEST_F(SpdyReadQueueTest, ReentrantDequeue) {
+  constexpr size_t kPayloadSize = 20;
+  constexpr size_t kUserBufLen = 12;
+
+  std::array<uint8_t, kPayloadSize> payload = {};
+  SpdyReadQueue queue;
+  auto buffer =
+      std::make_unique<SpdyBuffer>(base::span<const uint8_t>(payload));
+
+  bool reentry_fired = false;
+
+  buffer->AddConsumeCallback(base::BindRepeating(&ReentrantDequeue, &queue,
+                                                 &reentry_fired, kUserBufLen));
+  // Add a second callback to ensure that the loop in ConsumeHelper continues
+  // and attempts to access the next callback after the buffer has been deleted.
+  int second_callback_called = 0;
+  buffer->AddConsumeCallback(base::BindRepeating(
+      [](int* counter, size_t, SpdyBuffer::ConsumeSource) { (*counter)++; },
+      &second_callback_called));
+
+  queue.Enqueue(std::move(buffer));
+
+  std::array<uint8_t, kUserBufLen> user_buf;
+  size_t copied = queue.Dequeue(base::span<uint8_t>(user_buf));
+
+  EXPECT_EQ(copied, kUserBufLen);
+  EXPECT_TRUE(reentry_fired);
+  EXPECT_EQ(second_callback_called, 2);
+}
+
 }  // namespace net::test

diff --git a/net/spdy/spdy_read_queue_unittest.cc b/net/spdy/spdy_read_queue_unittest.cc
index 03e89802c..eb6c7037 100644
--- a/net/spdy/spdy_read_queue_unittest.cc
+++ b/net/spdy/spdy_read_queue_unittest.cc
@@ -5,10 +5,12 @@
 #include "net/spdy/spdy_read_queue.h"
 
 #include <algorithm>
+#include <array>
 #include <cstddef>
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "base/containers/heap_array.h"
 #include "base/containers/span.h"
@@ -138,4 +140,55 @@
   EXPECT_TRUE(read_queue.IsEmpty());
 }
 
+// Tests that calling Dequeue() reentrantly from within a consume callback
+// does not cause a use-after-free when the SpdyBuffer is destroyed during
+// the reentrant call.
+namespace {
+
+void ReentrantDequeue(SpdyReadQueue* queue,
+                      bool* fired,
+                      size_t inner_buf_len,
+                      size_t consume_size,
+                      SpdyBuffer::ConsumeSource consume_source) {
+  if (*fired) {
+    return;
+  }
+  *fired = true;
+
+  std::vector<uint8_t> inner_buf(inner_buf_len);
+  queue->Dequeue(inner_buf);
+}
+
+}  // namespace
+
+TEST_F(SpdyReadQueueTest, ReentrantDequeue) {
+  constexpr size_t kPayloadSize = 20;
+  constexpr size_t kUserBufLen = 12;
+
+  std::array<uint8_t, kPayloadSize> payload = {};
+  SpdyReadQueue queue;
+  auto buffer =
+      std::make_unique<SpdyBuffer>(base::span<const uint8_t>(payload));
+
+  bool reentry_fired = false;
+
+  buffer->AddConsumeCallback(base::BindRepeating(&ReentrantDequeue, &queue,
+                                                 &reentry_fired, kUserBufLen));
+  // Add a second callback to ensure that the loop in ConsumeHelper continues
+  // and attempts to access the next callback after the buffer has been deleted.
+  int second_callback_called = 0;
+  buffer->AddConsumeCallback(base::BindRepeating(
+      [](int* counter, size_t, SpdyBuffer::ConsumeSource) { (*counter)++; },
+      &second_callback_called));
+
+  queue.Enqueue(std::move(buffer));
+
+  std::array<uint8_t, kUserBufLen> user_buf;
+  size_t copied = queue.Dequeue(base::span<uint8_t>(user_buf));
+
+  EXPECT_EQ(copied, kUserBufLen);
+  EXPECT_TRUE(reentry_fired);
+  EXPECT_EQ(second_callback_called, 2);
+}
+
 }  // namespace net::test