chore: cherry-pick 4 changes from Release-0-M115 (#39268)

* chore: [23-x-y] cherry-pick 4 changes from Release-0-M115

* 90c9a89aa794 from chromium
* 933b9fad3a53 from chromium
* b03973561862 from chromium
* c60a1ab717c7 from chromium

* chore: update patches, remove redundent patches

* 4373801: Use rtc::scoped_refptr for webrtc::DataChannel objects

https://chromium-review.googlesource.com/c/chromium/src/+/4373801

---------

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
This commit is contained in:
Keeley Hammond
2023-08-02 06:09:58 -07:00
committed by GitHub
parent 10b49ffa12
commit b79acf50c7
4 changed files with 713 additions and 0 deletions

View File

@@ -149,3 +149,6 @@ mojoipcz_copy_incoming_messages_early.patch
base_do_not_use_va_args_twice_in_asprintf.patch
cherry-pick-85beff6fd302.patch
m114_webcodecs_fix_crash_when_changing_temporal_layer_count_in_av1.patch
cherry-pick-933b9fad3a53.patch
cherry-pick-b03973561862.patch
cherry-pick-c60a1ab717c7.patch

View File

@@ -0,0 +1,404 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Ken Rockot <rockot@google.com>
Date: Fri, 9 Jun 2023 07:49:02 +0000
Subject: Reland "ipcz: Refactor FragmentDescriptor decode"
This is a reland of commit 17dd18d1f2194089b8433e0ca334c81343b591e2
Original change's description:
> ipcz: Refactor FragmentDescriptor decode
>
> Funnels untrusted FragmentDescriptor mapping through a new
> Fragment::MappedFromDescriptor helper. See the linked bug
> for more details.
>
> Fixed: 1450899
> Change-Id: I4c7751b9f4299da4a13c0becc1b889160a0c6e66
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599218
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Ken Rockot <rockot@google.com>
> Cr-Commit-Position: refs/heads/main@{#1155133}
Change-Id: I86ee9118a30dea59d837c377a1f751b20a85a3c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4602794
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/main@{#1155397}
diff --git a/third_party/ipcz/src/BUILD.gn b/third_party/ipcz/src/BUILD.gn
index 94177b5ec335bcfa28ba47eb0e9887c991073946..5dda0f2f806936d9bd6c986afd20ec5b53e06c28 100644
--- a/third_party/ipcz/src/BUILD.gn
+++ b/third_party/ipcz/src/BUILD.gn
@@ -212,6 +212,7 @@ ipcz_source_set("impl") {
"ipcz/application_object.h",
"ipcz/block_allocator.h",
"ipcz/box.h",
+ "ipcz/buffer_id.h",
"ipcz/buffer_pool.h",
"ipcz/driver_memory.h",
"ipcz/driver_memory_mapping.h",
@@ -254,7 +255,6 @@ ipcz_source_set("impl") {
"ipcz/block_allocator_pool.cc",
"ipcz/block_allocator_pool.h",
"ipcz/box.cc",
- "ipcz/buffer_id.h",
"ipcz/buffer_pool.cc",
"ipcz/driver_memory.cc",
"ipcz/driver_memory_mapping.cc",
@@ -376,6 +376,7 @@ ipcz_source_set("ipcz_tests_sources") {
"ipcz/driver_memory_test.cc",
"ipcz/driver_object_test.cc",
"ipcz/driver_transport_test.cc",
+ "ipcz/fragment_test.cc",
"ipcz/message_test.cc",
"ipcz/node_connector_test.cc",
"ipcz/node_link_memory_test.cc",
diff --git a/third_party/ipcz/src/ipcz/block_allocator_pool.cc b/third_party/ipcz/src/ipcz/block_allocator_pool.cc
index bd464f897d1fcbde03941ee334d0e1706bf59868..1b9d50b2c77c046d815a94d7760328c8b379ecab 100644
--- a/third_party/ipcz/src/ipcz/block_allocator_pool.cc
+++ b/third_party/ipcz/src/ipcz/block_allocator_pool.cc
@@ -86,7 +86,7 @@ Fragment BlockAllocatorPool::Allocate() {
FragmentDescriptor descriptor(
entry->buffer_id, checked_cast<uint32_t>(offset),
checked_cast<uint32_t>(allocator.block_size()));
- return Fragment(descriptor, block);
+ return Fragment::FromDescriptorUnsafe(descriptor, block);
}
// Allocation from the active allocator failed. Try another if available.
diff --git a/third_party/ipcz/src/ipcz/buffer_pool.cc b/third_party/ipcz/src/ipcz/buffer_pool.cc
index 6881346d8f8532f070e5121da16f064ae4a9bdaf..27b23049848967f29f81b10ba4f8fa4ead14d2e2 100644
--- a/third_party/ipcz/src/ipcz/buffer_pool.cc
+++ b/third_party/ipcz/src/ipcz/buffer_pool.cc
@@ -26,15 +26,11 @@ Fragment BufferPool::GetFragment(const FragmentDescriptor& descriptor) {
absl::MutexLock lock(&mutex_);
auto it = mappings_.find(descriptor.buffer_id());
if (it == mappings_.end()) {
- return Fragment(descriptor, nullptr);
+ return Fragment::PendingFromDescriptor(descriptor);
}
auto& [id, mapping] = *it;
- if (descriptor.end() > mapping.bytes().size()) {
- return {};
- }
-
- return Fragment(descriptor, mapping.address_at(descriptor.offset()));
+ return Fragment::MappedFromDescriptor(descriptor, mapping);
}
bool BufferPool::AddBlockBuffer(
diff --git a/third_party/ipcz/src/ipcz/buffer_pool_test.cc b/third_party/ipcz/src/ipcz/buffer_pool_test.cc
index a009ffe1c20ade013a19b51eceee4faf334eb591..bff66c452a3e2c38b0f208cca1fa1a082f1ee871 100644
--- a/third_party/ipcz/src/ipcz/buffer_pool_test.cc
+++ b/third_party/ipcz/src/ipcz/buffer_pool_test.cc
@@ -194,9 +194,11 @@ TEST_F(BufferPoolTest, BasicBlockAllocation) {
pool.GetTotalBlockCapacity(kBlockSize));
// We can't free something that isn't a valid allocation.
- EXPECT_FALSE(pool.FreeBlock(Fragment{{}, nullptr}));
- EXPECT_FALSE(pool.FreeBlock(Fragment{{BufferId{1000}, 0, 1}, nullptr}));
- EXPECT_FALSE(pool.FreeBlock(Fragment{{BufferId{0}, 0, 1}, bytes0.data()}));
+ EXPECT_FALSE(pool.FreeBlock(Fragment::FromDescriptorUnsafe({}, nullptr)));
+ EXPECT_FALSE(pool.FreeBlock(
+ Fragment::FromDescriptorUnsafe({BufferId{1000}, 0, 1}, nullptr)));
+ EXPECT_FALSE(pool.FreeBlock(
+ Fragment::FromDescriptorUnsafe({BufferId{0}, 0, 1}, bytes0.data())));
// Allocate all available capacity.
std::vector<Fragment> fragments;
diff --git a/third_party/ipcz/src/ipcz/fragment.cc b/third_party/ipcz/src/ipcz/fragment.cc
index 651d1c2fca5fe4fb69cdf61c6062bd8804ebf704..2ef4ed8dcfa0a56a73975a0b7dcc3f86bf5a83a0 100644
--- a/third_party/ipcz/src/ipcz/fragment.cc
+++ b/third_party/ipcz/src/ipcz/fragment.cc
@@ -6,10 +6,38 @@
#include <cstdint>
+#include "ipcz/driver_memory_mapping.h"
+#include "ipcz/fragment_descriptor.h"
#include "third_party/abseil-cpp/absl/base/macros.h"
+#include "util/safe_math.h"
namespace ipcz {
+// static
+Fragment Fragment::MappedFromDescriptor(const FragmentDescriptor& descriptor,
+ DriverMemoryMapping& mapping) {
+ if (descriptor.is_null()) {
+ return {};
+ }
+
+ const uint32_t end = SaturatedAdd(descriptor.offset(), descriptor.size());
+ if (end > mapping.bytes().size()) {
+ return {};
+ }
+ return Fragment{descriptor, mapping.address_at(descriptor.offset())};
+}
+
+// static
+Fragment Fragment::PendingFromDescriptor(const FragmentDescriptor& descriptor) {
+ return Fragment{descriptor, nullptr};
+}
+
+// static
+Fragment Fragment::FromDescriptorUnsafe(const FragmentDescriptor& descriptor,
+ void* base_address) {
+ return Fragment{descriptor, base_address};
+}
+
Fragment::Fragment(const FragmentDescriptor& descriptor, void* address)
: descriptor_(descriptor), address_(address) {
// If `address` is non-null, the descriptor must also be. Note that the
diff --git a/third_party/ipcz/src/ipcz/fragment.h b/third_party/ipcz/src/ipcz/fragment.h
index c0151fdcf4b418680172a29d1c0d28b58a5807cd..de65f087b0bc27fd59ab88e23130d5ce0d345a8a 100644
--- a/third_party/ipcz/src/ipcz/fragment.h
+++ b/third_party/ipcz/src/ipcz/fragment.h
@@ -14,21 +14,32 @@
namespace ipcz {
+class DriverMemoryMapping;
+
// Represents a span of memory located within the shared memory regions owned by
// a NodeLinkMemory, via BufferPool. This is essentially a FragmentDescriptor
// plus the actual mapped address of the given buffer and offset.
struct Fragment {
constexpr Fragment() = default;
- // Constructs a new Fragment over `descriptor`, mapped to `address`. If
- // `address` is null, the Fragment is considered "pending" -- it has a
- // potentially valid descriptor, but could not be resolved to a mapped address
- // yet (e.g. because the relevant BufferPool doesn't have the identified
- // buffer mapped yet.)
- Fragment(const FragmentDescriptor& descriptor, void* address);
Fragment(const Fragment&);
Fragment& operator=(const Fragment&);
+ // Returns a new concrete Fragment corresponding to `descriptor` within the
+ // context of `mapping`. This validates that the fragment's bounds fall within
+ // the bounds of `mapping`. If `descriptor` was null or validation fails, this
+ // returns a null Fragment.
+ static Fragment MappedFromDescriptor(const FragmentDescriptor& descriptor,
+ DriverMemoryMapping& mapping);
+
+ // Returns a pending Fragment corresponding to `descriptor`.
+ static Fragment PendingFromDescriptor(const FragmentDescriptor& descriptor);
+
+ // Returns a Fragment corresponding to `descriptor`, with the starting address
+ // already mapped to `address`.
+ static Fragment FromDescriptorUnsafe(const FragmentDescriptor& descriptor,
+ void* address);
+
// A null fragment is a fragment with a null descriptor, meaning it does not
// reference a valid buffer ID.
bool is_null() const { return descriptor_.is_null(); }
@@ -66,6 +77,13 @@ struct Fragment {
}
private:
+ // Constructs a new Fragment over `descriptor`, mapped to `address`. If
+ // `address` is null, the Fragment is considered "pending" -- it has a
+ // potentially valid descriptor, but could not be resolved to a mapped address
+ // yet (e.g. because the relevant BufferPool doesn't have the identified
+ // buffer mapped yet.)
+ Fragment(const FragmentDescriptor& descriptor, void* address);
+
FragmentDescriptor descriptor_;
// The actual mapped address corresponding to `descriptor_`.
diff --git a/third_party/ipcz/src/ipcz/fragment_descriptor.h b/third_party/ipcz/src/ipcz/fragment_descriptor.h
index b247215fd5e5f7c69e521416614465b0321f5d83..aeaa7da9c82761854948d009e7f245c9c9d042c7 100644
--- a/third_party/ipcz/src/ipcz/fragment_descriptor.h
+++ b/third_party/ipcz/src/ipcz/fragment_descriptor.h
@@ -39,7 +39,6 @@ struct IPCZ_ALIGN(8) FragmentDescriptor {
BufferId buffer_id() const { return buffer_id_; }
uint32_t offset() const { return offset_; }
uint32_t size() const { return size_; }
- uint32_t end() const { return offset_ + size_; }
private:
// Identifies the shared memory buffer in which the memory resides. This ID is
diff --git a/third_party/ipcz/src/ipcz/fragment_test.cc b/third_party/ipcz/src/ipcz/fragment_test.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e6b6baa6cb2f1fbdfb89d87d644f63681c797c01
--- /dev/null
+++ b/third_party/ipcz/src/ipcz/fragment_test.cc
@@ -0,0 +1,102 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ipcz/fragment.h"
+
+#include <algorithm>
+#include <cstring>
+#include <limits>
+#include <string>
+#include <utility>
+
+#include "ipcz/buffer_id.h"
+#include "ipcz/driver_memory.h"
+#include "ipcz/driver_memory_mapping.h"
+#include "reference_drivers/sync_reference_driver.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ipcz {
+namespace {
+
+const IpczDriver& kTestDriver = reference_drivers::kSyncReferenceDriver;
+
+using FragmentTest = testing::Test;
+
+TEST_F(FragmentTest, FromDescriptorUnsafe) {
+ char kBuffer[] = "Hello, world!";
+
+ Fragment f = Fragment::FromDescriptorUnsafe({BufferId{0}, 1, 4}, kBuffer + 1);
+ EXPECT_FALSE(f.is_null());
+ EXPECT_FALSE(f.is_pending());
+ EXPECT_EQ(1u, f.offset());
+ EXPECT_EQ(4u, f.size());
+ EXPECT_EQ("ello", std::string(f.bytes().begin(), f.bytes().end()));
+
+ f = Fragment::FromDescriptorUnsafe({BufferId{0}, 7, 6}, kBuffer + 7);
+ EXPECT_FALSE(f.is_null());
+ EXPECT_FALSE(f.is_pending());
+ EXPECT_EQ(7u, f.offset());
+ EXPECT_EQ(6u, f.size());
+ EXPECT_EQ("world!", std::string(f.bytes().begin(), f.bytes().end()));
+}
+
+TEST_F(FragmentTest, PendingFromDescriptor) {
+ Fragment f = Fragment::PendingFromDescriptor({BufferId{0}, 5, 42});
+ EXPECT_TRUE(f.is_pending());
+ EXPECT_FALSE(f.is_null());
+ EXPECT_EQ(5u, f.offset());
+ EXPECT_EQ(42u, f.size());
+
+ f = Fragment::PendingFromDescriptor({kInvalidBufferId, 0, 0});
+ EXPECT_TRUE(f.is_null());
+ EXPECT_FALSE(f.is_pending());
+}
+
+TEST_F(FragmentTest, NullMappedFromDescriptor) {
+ constexpr size_t kDataSize = 32;
+ DriverMemory memory(kTestDriver, kDataSize);
+ auto mapping = memory.Map();
+
+ Fragment f =
+ Fragment::MappedFromDescriptor({kInvalidBufferId, 0, 0}, mapping);
+ EXPECT_TRUE(f.is_null());
+}
+
+TEST_F(FragmentTest, InvalidMappedFromDescriptor) {
+ constexpr size_t kDataSize = 32;
+ DriverMemory memory(kTestDriver, kDataSize);
+ auto mapping = memory.Map();
+
+ Fragment f;
+
+ // Offset out of bounds
+ f = Fragment::MappedFromDescriptor({BufferId{0}, kDataSize, 1}, mapping);
+ EXPECT_TRUE(f.is_null());
+
+ // Tail out of bounds
+ f = Fragment::MappedFromDescriptor({BufferId{0}, 0, kDataSize + 5}, mapping);
+ EXPECT_TRUE(f.is_null());
+
+ // Tail overflow
+ f = Fragment::MappedFromDescriptor(
+ {BufferId{0}, std::numeric_limits<uint32_t>::max(), 2}, mapping);
+ EXPECT_TRUE(f.is_null());
+}
+
+TEST_F(FragmentTest, ValidMappedFromDescriptor) {
+ const char kData[] = "0123456789abcdef";
+ DriverMemory memory(kTestDriver, std::size(kData));
+ auto mapping = memory.Map();
+ memcpy(mapping.bytes().data(), kData, std::size(kData));
+
+ Fragment f = Fragment::MappedFromDescriptor({BufferId{0}, 2, 11}, mapping);
+ EXPECT_FALSE(f.is_null());
+ EXPECT_FALSE(f.is_pending());
+ EXPECT_EQ(2u, f.offset());
+ EXPECT_EQ(11u, f.size());
+ EXPECT_EQ("23456789abc", std::string(f.bytes().begin(), f.bytes().end()));
+}
+
+} // namespace
+} // namespace ipcz
diff --git a/third_party/ipcz/src/ipcz/node_link_memory.cc b/third_party/ipcz/src/ipcz/node_link_memory.cc
index b4715bca8b59c1a53fa4777510c75c7aca1cdf9d..f70a6b1aee08d749a6266d3cb88f9d12f4e1f281 100644
--- a/third_party/ipcz/src/ipcz/node_link_memory.cc
+++ b/third_party/ipcz/src/ipcz/node_link_memory.cc
@@ -259,8 +259,9 @@ FragmentRef<RouterLinkState> NodeLinkMemory::GetInitialRouterLinkState(
FragmentDescriptor descriptor(kPrimaryBufferId,
ToOffset(state, primary_buffer_memory_.data()),
sizeof(RouterLinkState));
- return FragmentRef<RouterLinkState>(RefCountedFragment::kUnmanagedRef,
- Fragment(descriptor, state));
+ return FragmentRef<RouterLinkState>(
+ RefCountedFragment::kUnmanagedRef,
+ Fragment::FromDescriptorUnsafe(descriptor, state));
}
Fragment NodeLinkMemory::GetFragment(const FragmentDescriptor& descriptor) {
diff --git a/third_party/ipcz/src/ipcz/ref_counted_fragment_test.cc b/third_party/ipcz/src/ipcz/ref_counted_fragment_test.cc
index d5a2243a693597e43f87f116f80599fde383cb59..220c3556a261c5caab7194114af4cf375d9af683 100644
--- a/third_party/ipcz/src/ipcz/ref_counted_fragment_test.cc
+++ b/third_party/ipcz/src/ipcz/ref_counted_fragment_test.cc
@@ -64,7 +64,8 @@ TEST_F(RefCountedFragmentTest, SimpleRef) {
FragmentRef<TestObject> ref(
RefCountedFragment::kUnmanagedRef,
- Fragment(FragmentDescriptor(BufferId(0), 0, sizeof(object)), &object));
+ Fragment::FromDescriptorUnsafe(
+ FragmentDescriptor(BufferId(0), 0, sizeof(object)), &object));
EXPECT_EQ(1, object.ref_count_for_testing());
ref.reset();
EXPECT_EQ(0, object.ref_count_for_testing());
@@ -75,7 +76,8 @@ TEST_F(RefCountedFragmentTest, Copy) {
FragmentRef<TestObject> ref1(
RefCountedFragment::kUnmanagedRef,
- Fragment(FragmentDescriptor(BufferId(0), 0, sizeof(object1)), &object1));
+ Fragment::FromDescriptorUnsafe(
+ FragmentDescriptor(BufferId(0), 0, sizeof(object1)), &object1));
EXPECT_EQ(1, object1.ref_count_for_testing());
FragmentRef<TestObject> other1 = ref1;
@@ -88,7 +90,8 @@ TEST_F(RefCountedFragmentTest, Copy) {
TestObject object2;
auto ref2 = FragmentRef<TestObject>(
RefCountedFragment::kUnmanagedRef,
- Fragment(FragmentDescriptor(BufferId(0), 0, sizeof(object2)), &object2));
+ Fragment::FromDescriptorUnsafe(
+ FragmentDescriptor(BufferId(0), 0, sizeof(object2)), &object2));
EXPECT_EQ(1, object1.ref_count_for_testing());
EXPECT_EQ(1, object2.ref_count_for_testing());
ref2 = ref1;
@@ -115,7 +118,8 @@ TEST_F(RefCountedFragmentTest, Move) {
FragmentRef<TestObject> ref1(
RefCountedFragment::kUnmanagedRef,
- Fragment(FragmentDescriptor(BufferId(0), 0, sizeof(object1)), &object1));
+ Fragment::FromDescriptorUnsafe(
+ FragmentDescriptor(BufferId(0), 0, sizeof(object1)), &object1));
EXPECT_EQ(1, ref1.ref_count_for_testing());
FragmentRef<TestObject> other1 = std::move(ref1);
@@ -133,10 +137,12 @@ TEST_F(RefCountedFragmentTest, Move) {
TestObject object3;
FragmentRef<TestObject> ref2(
RefCountedFragment::kUnmanagedRef,
- Fragment(FragmentDescriptor(BufferId(0), 0, sizeof(object2)), &object2));
+ Fragment::FromDescriptorUnsafe(
+ FragmentDescriptor(BufferId(0), 0, sizeof(object2)), &object2));
FragmentRef<TestObject> ref3(
RefCountedFragment::kUnmanagedRef,
- Fragment(FragmentDescriptor(BufferId(0), 0, sizeof(object3)), &object3));
+ Fragment::FromDescriptorUnsafe(
+ FragmentDescriptor(BufferId(0), 0, sizeof(object3)), &object3));
EXPECT_FALSE(ref2.is_null());
EXPECT_TRUE(ref2.is_addressable());

View File

@@ -0,0 +1,119 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Tommi <tommi@chromium.org>
Date: Wed, 5 Jul 2023 10:55:53 +0000
Subject: Make RTCDataChannel's channel and observer pointers const.
This allows channel properties to be queried while the RTCDataChannel
instance exists and avoids potential null deref after entering the
kClosed state.
(cherry picked from commit 08d5ad011f53a1995bfccef6728bfa62541f7608)
Bug: 1456567, 1457421
Change-Id: I4747f9c00804b35711667d7320ec6188f20910c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4663082
Commit-Queue: Tomas Gunnarsson <tommi@chromium.org>
Reviewed-by: Elad Alon <eladalon@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1165406}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4665530
Cr-Commit-Position: refs/branch-heads/5845@{#300}
Cr-Branched-From: 5a5dff63a4a4c63b9b18589819bebb2566c85443-refs/heads/main@{#1160321}
diff --git a/third_party/blink/renderer/modules/peerconnection/rtc_data_channel.cc b/third_party/blink/renderer/modules/peerconnection/rtc_data_channel.cc
index 78d28d60822f4ce206d869846235352224378076..91c20cbcc5042373964d57545177ff06074db564 100644
--- a/third_party/blink/renderer/modules/peerconnection/rtc_data_channel.cc
+++ b/third_party/blink/renderer/modules/peerconnection/rtc_data_channel.cc
@@ -228,11 +228,12 @@ RTCDataChannel::Observer::Observer(
scoped_refptr<webrtc::DataChannelInterface> channel)
: main_thread_(main_thread),
blink_channel_(blink_channel),
- webrtc_channel_(channel) {}
+ webrtc_channel_(std::move(channel)) {
+ CHECK(webrtc_channel_.get());
+}
RTCDataChannel::Observer::~Observer() {
DCHECK(!blink_channel_) << "Reference to blink channel hasn't been released.";
- DCHECK(!webrtc_channel_.get()) << "Unregister hasn't been called.";
}
const scoped_refptr<webrtc::DataChannelInterface>&
@@ -242,13 +243,8 @@ RTCDataChannel::Observer::channel() const {
void RTCDataChannel::Observer::Unregister() {
DCHECK(main_thread_->BelongsToCurrentThread());
+ webrtc_channel_->UnregisterObserver();
blink_channel_ = nullptr;
- if (webrtc_channel_.get()) {
- webrtc_channel_->UnregisterObserver();
- // Now that we're guaranteed to not get further OnStateChange callbacks,
- // it's safe to release our reference to the channel.
- webrtc_channel_ = nullptr;
- }
}
void RTCDataChannel::Observer::OnStateChange() {
@@ -302,7 +298,7 @@ void RTCDataChannel::Observer::OnMessageImpl(
RTCDataChannel::RTCDataChannel(
ExecutionContext* context,
- scoped_refptr<webrtc::DataChannelInterface> channel,
+ scoped_refptr<webrtc::DataChannelInterface> data_channel,
RTCPeerConnectionHandler* peer_connection_handler)
: ExecutionContextLifecycleObserver(context),
state_(webrtc::DataChannelInterface::kConnecting),
@@ -317,7 +313,7 @@ RTCDataChannel::RTCDataChannel(
observer_(base::MakeRefCounted<Observer>(
context->GetTaskRunner(TaskType::kNetworking),
this,
- channel)),
+ std::move(data_channel))),
signaling_thread_(peer_connection_handler->signaling_thread()) {
DCHECK(peer_connection_handler);
@@ -340,7 +336,7 @@ RTCDataChannel::RTCDataChannel(
observer_, state_),
"RegisterObserverAndGetStateUpdate");
- IncrementCounters(*channel.get());
+ IncrementCounters(*(observer_->channel()).get());
}
RTCDataChannel::~RTCDataChannel() = default;
@@ -689,9 +685,8 @@ void RTCDataChannel::Dispose() {
if (stopped_)
return;
- // Clears the weak persistent reference to this on-heap object.
+ // Clear the weak persistent reference to this on-heap object.
observer_->Unregister();
- observer_ = nullptr;
}
void RTCDataChannel::ScheduleDispatchEvent(Event* event) {
diff --git a/third_party/blink/renderer/modules/peerconnection/rtc_data_channel.h b/third_party/blink/renderer/modules/peerconnection/rtc_data_channel.h
index 21bb39382ac0c6acbf984ffbda5f6a4e6c863432..6959b8b1e3a0b586be68cb4a8d0389b7926b98fe 100644
--- a/third_party/blink/renderer/modules/peerconnection/rtc_data_channel.h
+++ b/third_party/blink/renderer/modules/peerconnection/rtc_data_channel.h
@@ -152,7 +152,7 @@ class MODULES_EXPORT RTCDataChannel final
const scoped_refptr<base::SingleThreadTaskRunner> main_thread_;
WeakPersistent<RTCDataChannel> blink_channel_;
- scoped_refptr<webrtc::DataChannelInterface> webrtc_channel_;
+ const scoped_refptr<webrtc::DataChannelInterface> webrtc_channel_;
};
void OnStateChange(webrtc::DataChannelInterface::DataState state);
@@ -195,7 +195,11 @@ class MODULES_EXPORT RTCDataChannel final
unsigned buffered_amount_;
bool stopped_;
bool closed_from_owner_;
- scoped_refptr<Observer> observer_;
+ // Keep the `observer_` reference const to make it clear that we don't want
+ // to free the underlying channel (or callback observer) until the
+ // `RTCDataChannel` instance goes away. This allows properties to be queried
+ // after the state reaches `kClosed`.
+ const scoped_refptr<Observer> observer_;
scoped_refptr<base::SingleThreadTaskRunner> signaling_thread_;
THREAD_CHECKER(thread_checker_);
};

View File

@@ -0,0 +1,187 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Taylor Bergquist <tbergquist@chromium.org>
Date: Tue, 11 Jul 2023 01:32:22 +0000
Subject: Fix UAF when exiting a nested run loop in
TabDragContextImpl::OnGestureEvent.
OnGestureEvent may call ContinueDrag, which may run a nested run loop. After the nested run loop returns, multiple seconds of time may have passed, and the world may be in a very different state; in particular, the window that contains this TabDragContext may have closed.
This CL checks if this has happened, and returns early in that case.
(cherry picked from commit 63d6b8ba8126b16215d33670df8c67dcbc6c9bef)
Bug: 1453465
Change-Id: I6095c0afeb5aa5f422717f1bbd93b96175e52afa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4657527
Reviewed-by: Darryl James <dljames@chromium.org>
Commit-Queue: Taylor Bergquist <tbergquist@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Original-Commit-Position: refs/heads/main@{#1164449}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4676126
Reviewed-by: Shibalik Mohapatra <shibalik@chromium.org>
Cr-Commit-Position: refs/branch-heads/5845@{#410}
Cr-Branched-From: 5a5dff63a4a4c63b9b18589819bebb2566c85443-refs/heads/main@{#1160321}
diff --git a/chrome/browser/ui/views/tabs/fake_tab_slot_controller.cc b/chrome/browser/ui/views/tabs/fake_tab_slot_controller.cc
index d1f5a96ed729fde1224f596212f49e386fc6f170..c80536b0d4601c68ce279dfa122f38b39b13fb72 100644
--- a/chrome/browser/ui/views/tabs/fake_tab_slot_controller.cc
+++ b/chrome/browser/ui/views/tabs/fake_tab_slot_controller.cc
@@ -43,6 +43,12 @@ bool FakeTabSlotController::IsFocusInTabs() const {
return false;
}
+TabSlotController::Liveness FakeTabSlotController::ContinueDrag(
+ views::View* view,
+ const ui::LocatedEvent& event) {
+ return Liveness::kAlive;
+}
+
bool FakeTabSlotController::EndDrag(EndDragReason reason) {
return false;
}
diff --git a/chrome/browser/ui/views/tabs/fake_tab_slot_controller.h b/chrome/browser/ui/views/tabs/fake_tab_slot_controller.h
index 25f1c66d131189ebec2711b49f7f0c141f2c06d2..98dab21cfe2e3ecb88f70cac3868fb30828658be 100644
--- a/chrome/browser/ui/views/tabs/fake_tab_slot_controller.h
+++ b/chrome/browser/ui/views/tabs/fake_tab_slot_controller.h
@@ -60,8 +60,8 @@ class FakeTabSlotController : public TabSlotController {
TabSlotView* source,
const ui::LocatedEvent& event,
const ui::ListSelectionModel& original_selection) override {}
- void ContinueDrag(views::View* view, const ui::LocatedEvent& event) override {
- }
+ Liveness ContinueDrag(views::View* view,
+ const ui::LocatedEvent& event) override;
bool EndDrag(EndDragReason reason) override;
Tab* GetTabAt(const gfx::Point& point) override;
const Tab* GetAdjacentTab(const Tab* tab, int offset) override;
diff --git a/chrome/browser/ui/views/tabs/tab_slot_controller.h b/chrome/browser/ui/views/tabs/tab_slot_controller.h
index f729425995ffd1ee6926ef953417d2e81a1b527b..c57760f30ae515cf5ee7a021e07c8d049082e371 100644
--- a/chrome/browser/ui/views/tabs/tab_slot_controller.h
+++ b/chrome/browser/ui/views/tabs/tab_slot_controller.h
@@ -49,6 +49,8 @@ class TabSlotController {
kEvent
};
+ enum class Liveness { kAlive, kDeleted };
+
virtual const ui::ListSelectionModel& GetSelectionModel() const = 0;
// Returns the tab at |index|.
@@ -126,9 +128,10 @@ class TabSlotController {
const ui::LocatedEvent& event,
const ui::ListSelectionModel& original_selection) = 0;
- // Continues dragging a Tab.
- virtual void ContinueDrag(views::View* view,
- const ui::LocatedEvent& event) = 0;
+ // Continues dragging a Tab. May enter a nested event loop - returns
+ // Liveness::kDeleted if `this` was destroyed during this nested event loop,
+ // and Liveness::kAlive if `this` is still alive.
+ virtual Liveness ContinueDrag(views::View* view, const ui::LocatedEvent& event) = 0;
// Ends dragging a Tab. Returns whether the tab has been destroyed.
virtual bool EndDrag(EndDragReason reason) = 0;
diff --git a/chrome/browser/ui/views/tabs/tab_strip.cc b/chrome/browser/ui/views/tabs/tab_strip.cc
index 0a9ffa81d06d4bb284e8aa51758fbafc9e1b6644..cb82dc62aee1970d50a91f9692208b7303333d1d 100644
--- a/chrome/browser/ui/views/tabs/tab_strip.cc
+++ b/chrome/browser/ui/views/tabs/tab_strip.cc
@@ -186,7 +186,7 @@ class TabStrip::TabDragContextImpl : public TabDragContext,
}
bool OnMouseDragged(const ui::MouseEvent& event) override {
- ContinueDrag(this, event);
+ (void)ContinueDrag(this, event);
return true;
}
@@ -197,6 +197,7 @@ class TabStrip::TabDragContextImpl : public TabDragContext,
void OnMouseCaptureLost() override { EndDrag(END_DRAG_CAPTURE_LOST); }
void OnGestureEvent(ui::GestureEvent* event) override {
+ Liveness tabstrip_alive = Liveness::kAlive;
switch (event->type()) {
case ui::ET_GESTURE_SCROLL_END:
case ui::ET_SCROLL_FLING_START:
@@ -210,7 +211,8 @@ class TabStrip::TabDragContextImpl : public TabDragContext,
}
case ui::ET_GESTURE_SCROLL_UPDATE:
- ContinueDrag(this, *event);
+ // N.B. !! ContinueDrag may enter a nested run loop !!
+ tabstrip_alive = ContinueDrag(this, *event);
break;
case ui::ET_GESTURE_TAP_DOWN:
@@ -222,6 +224,12 @@ class TabStrip::TabDragContextImpl : public TabDragContext,
}
event->SetHandled();
+ // If tabstrip was destroyed (during ContinueDrag above), return early to
+ // avoid UAF below.
+ if (tabstrip_alive == Liveness::kDeleted) {
+ return;
+ }
+
// TabDragContext gets event capture as soon as a drag session begins, which
// precludes TabStrip from ever getting events like tap or long tap. Forward
// this on to TabStrip so it can respond to those events.
@@ -310,20 +318,20 @@ class TabStrip::TabDragContextImpl : public TabDragContext,
std::move(drag_controller_set_callback_).Run(drag_controller_.get());
}
- void ContinueDrag(views::View* view, const ui::LocatedEvent& event) {
- if (drag_controller_.get() &&
- drag_controller_->event_source() == EventSourceFromEvent(event)) {
- gfx::Point screen_location(event.location());
- views::View::ConvertPointToScreen(view, &screen_location);
+ Liveness ContinueDrag(views::View* view, const ui::LocatedEvent& event) {
+ if (!drag_controller_.get() ||
+ drag_controller_->event_source() != EventSourceFromEvent(event)) {
+ return Liveness::kAlive;
+ }
- // Note: |tab_strip_| can be destroyed during drag, also destroying
- // |this|.
- base::WeakPtr<TabDragContext> weak_ptr(weak_factory_.GetWeakPtr());
- drag_controller_->Drag(screen_location);
+ gfx::Point screen_location(event.location());
+ views::View::ConvertPointToScreen(view, &screen_location);
- if (!weak_ptr)
- return;
- }
+ // Note: `tab_strip_` can be destroyed during drag, also destroying `this`.
+ base::WeakPtr<TabDragContext> weak_ptr(weak_factory_.GetWeakPtr());
+ drag_controller_->Drag(screen_location);
+
+ return weak_ptr ? Liveness::kAlive : Liveness::kDeleted;
}
bool EndDrag(EndDragReason reason) {
@@ -1607,8 +1615,10 @@ void TabStrip::MaybeStartDrag(
drag_context_->MaybeStartDrag(source, event, original_selection);
}
-void TabStrip::ContinueDrag(views::View* view, const ui::LocatedEvent& event) {
- drag_context_->ContinueDrag(view, event);
+TabSlotController::Liveness TabStrip::ContinueDrag(
+ views::View* view,
+ const ui::LocatedEvent& event) {
+ return drag_context_->ContinueDrag(view, event);
}
bool TabStrip::EndDrag(EndDragReason reason) {
diff --git a/chrome/browser/ui/views/tabs/tab_strip.h b/chrome/browser/ui/views/tabs/tab_strip.h
index c542581822cabc26b41ab8619fd73cb15c7f8524..682a44d3d9ad10aecc28e542c0cea0c323ee0c85 100644
--- a/chrome/browser/ui/views/tabs/tab_strip.h
+++ b/chrome/browser/ui/views/tabs/tab_strip.h
@@ -278,7 +278,8 @@ class TabStrip : public views::View,
TabSlotView* source,
const ui::LocatedEvent& event,
const ui::ListSelectionModel& original_selection) override;
- void ContinueDrag(views::View* view, const ui::LocatedEvent& event) override;
+ Liveness ContinueDrag(views::View* view,
+ const ui::LocatedEvent& event) override;
bool EndDrag(EndDragReason reason) override;
Tab* GetTabAt(const gfx::Point& point) override;
const Tab* GetAdjacentTab(const Tab* tab, int offset) override;