mirror
/
electron
mirror of https://github.com/electron/electron
1
0
Fork 0
Code Issues Releases Activity
electron/patches/chromium/cherry-pick-933b9fad3a53.patch

405 lines
16 KiB
Diff
Raw Permalink Blame History

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 4b3bbaf262815573249648b56c03442757c098ae..3c1cd26c65e5be543e24f51ad0d89b482d177a79 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",
@@ -378,6 +378,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 ed5229a392ca8f20ef092fec78c878a53babfa95..d8dbec5333b52f977d56571965ffbee79dff288c 100644
--- a/third_party/ipcz/src/ipcz/fragment_descriptor.h
+++ b/third_party/ipcz/src/ipcz/fragment_descriptor.h
@@ -36,7 +36,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 480c756f48f8367d8adacdda4867e8d5766fa760..e424192e77f2bc51215320a9ca7489931dcaf8ba 100644
--- a/third_party/ipcz/src/ipcz/node_link_memory.cc
+++ b/third_party/ipcz/src/ipcz/node_link_memory.cc
@@ -260,8 +260,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 Git Blame Copy Permalink