Assert that SharedMemoryMapping is only used with lock-free atomics
It's possible to bypass this check by passing a struct with std::atomic
members, but better than no check at all.
Bug: 357945779
Change-Id: I627461015865a55792ec11d9fefc3b3fe16ec7d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5766776
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@google.com>
Cr-Commit-Position: refs/heads/main@{#1341016}
diff --git a/base/BUILD.gn b/base/BUILD.gn
index 8e82634..0ce2724 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -4209,6 +4209,7 @@
"logging_nocompile.nc",
"memory/protected_memory_nocompile.nc",
"memory/ref_counted_nocompile.nc",
+ "memory/shared_memory_mapping_nocompile.nc",
"memory/weak_ptr_nocompile.nc",
"metrics/field_trial_params_nocompile.nc",
"metrics/histogram_nocompile.nc",
diff --git a/base/memory/shared_memory_mapping.h b/base/memory/shared_memory_mapping.h
index c268af8..66e7279 100644
--- a/base/memory/shared_memory_mapping.h
+++ b/base/memory/shared_memory_mapping.h
@@ -5,6 +5,7 @@
#ifndef BASE_MEMORY_SHARED_MEMORY_MAPPING_H_
#define BASE_MEMORY_SHARED_MEMORY_MAPPING_H_
+#include <atomic>
#include <cstddef>
#include <type_traits>
@@ -18,7 +19,30 @@
namespace base {
namespace subtle {
+
class PlatformSharedMemoryRegion;
+
+// Constraints on types that are safe to copy across memory spaces.
+
+template <typename T>
+struct LockFreeIfAtomic {
+ // Non-atomics aren't synchronized, so trivially don't contain locks.
+ static constexpr bool is_lock_free = true;
+};
+
+template <typename T>
+struct LockFreeIfAtomic<std::atomic<T>> {
+ static constexpr bool is_lock_free = std::atomic<T>::is_always_lock_free;
+};
+
+template <typename T>
+concept SharedMemorySafe =
+ // Copying non-trivially-copyable object across memory spaces is dangerous.
+ std::is_trivially_copyable_v<T> &&
+ // If T is a std::atomic, it's unsafe to share across memory spaces unless
+ // it's lock-free.
+ LockFreeIfAtomic<T>::is_lock_free;
+
} // namespace subtle
// Base class for scoped handles to a shared memory mapping created from a
@@ -123,10 +147,8 @@
// Returns a pointer to a page-aligned const T if the mapping is valid and
// large enough to contain a T, or nullptr otherwise.
template <typename T>
+ requires subtle::SharedMemorySafe<T>
const T* GetMemoryAs() const {
- static_assert(std::is_trivially_copyable_v<T>,
- "Copying non-trivially-copyable object across memory spaces "
- "is dangerous");
if (!IsValid())
return nullptr;
if (sizeof(T) > size())
@@ -141,10 +163,8 @@
// will be returned. The first element, if any, is guaranteed to be
// page-aligned.
template <typename T>
+ requires subtle::SharedMemorySafe<T>
span<const T> GetMemoryAsSpan() const {
- static_assert(std::is_trivially_copyable_v<T>,
- "Copying non-trivially-copyable object across memory spaces "
- "is dangerous");
if (!IsValid())
return span<const T>();
size_t count = size() / sizeof(T);
@@ -155,10 +175,8 @@
// large enough to contain |count| elements, or an empty span otherwise. The
// first element, if any, is guaranteed to be page-aligned.
template <typename T>
+ requires subtle::SharedMemorySafe<T>
span<const T> GetMemoryAsSpan(size_t count) const {
- static_assert(std::is_trivially_copyable_v<T>,
- "Copying non-trivially-copyable object across memory spaces "
- "is dangerous");
if (!IsValid())
return span<const T>();
if (size() / sizeof(T) < count)
@@ -213,10 +231,8 @@
// Returns a pointer to a page-aligned T if the mapping is valid and large
// enough to contain a T, or nullptr otherwise.
template <typename T>
+ requires subtle::SharedMemorySafe<T>
T* GetMemoryAs() const {
- static_assert(std::is_trivially_copyable_v<T>,
- "Copying non-trivially-copyable object across memory spaces "
- "is dangerous");
if (!IsValid())
return nullptr;
if (sizeof(T) > size())
@@ -230,10 +246,8 @@
// enough to contain even one T: in that case, an empty span will be returned.
// The first element, if any, is guaranteed to be page-aligned.
template <typename T>
+ requires subtle::SharedMemorySafe<T>
span<T> GetMemoryAsSpan() const {
- static_assert(std::is_trivially_copyable_v<T>,
- "Copying non-trivially-copyable object across memory spaces "
- "is dangerous");
if (!IsValid())
return span<T>();
size_t count = size() / sizeof(T);
@@ -244,10 +258,8 @@
// enough to contain |count| elements, or an empty span otherwise. The first
// element, if any, is guaranteed to be page-aligned.
template <typename T>
+ requires subtle::SharedMemorySafe<T>
span<T> GetMemoryAsSpan(size_t count) const {
- static_assert(std::is_trivially_copyable_v<T>,
- "Copying non-trivially-copyable object across memory spaces "
- "is dangerous");
if (!IsValid())
return span<T>();
if (size() / sizeof(T) < count)
diff --git a/base/memory/shared_memory_mapping_nocompile.nc b/base/memory/shared_memory_mapping_nocompile.nc
new file mode 100644
index 0000000..7034b384
--- /dev/null
+++ b/base/memory/shared_memory_mapping_nocompile.nc
@@ -0,0 +1,62 @@
+// Copyright 2024 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// This is a "No Compile Test" suite.
+// https://dev.chromium.org/developers/testing/no-compile-tests
+
+#include <string>
+#include <type_traits>
+#include <utility>
+
+#include "base/memory/read_only_shared_memory_region.h"
+#include "base/memory/shared_memory_mapping.h"
+
+namespace base {
+
+namespace {
+
+struct NotTriviallyCopyable {
+ std::string data;
+};
+
+static_assert(!std::is_trivially_copyable_v<NotTriviallyCopyable>);
+
+struct NotLockFree {
+ int large_array[1024] = {};
+};
+
+using NotLockFreeAtomic = std::atomic<NotLockFree>;
+
+static_assert(std::is_trivially_copyable_v<NotLockFreeAtomic>);
+static_assert(!NotLockFreeAtomic::is_always_lock_free);
+
+} // namespace
+
+void RequireTriviallyCopyable() {
+ auto mapped_region =
+ ReadOnlySharedMemoryRegion::Create(sizeof(NotTriviallyCopyable));
+ WritableSharedMemoryMapping write_map = std::move(mapped_region.mapping);
+ write_map.GetMemoryAs<NotTriviallyCopyable>(); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAs'}}
+ write_map.GetMemoryAsSpan<NotTriviallyCopyable>(); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAsSpan'}}
+ write_map.GetMemoryAsSpan<NotTriviallyCopyable>(1); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAsSpan'}}
+ ReadOnlySharedMemoryMapping read_map = mapped_region.region.Map();
+ read_map.GetMemoryAs<NotTriviallyCopyable>(); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAs'}}
+ read_map.GetMemoryAsSpan<NotTriviallyCopyable>(); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAsSpan'}}
+ read_map.GetMemoryAsSpan<NotTriviallyCopyable>(1); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAsSpan'}}
+}
+
+void RequireLockFreeAtomic() {
+ auto mapped_region =
+ ReadOnlySharedMemoryRegion::Create(sizeof(NotLockFreeAtomic));
+ WritableSharedMemoryMapping write_map = std::move(mapped_region.mapping);
+ write_map.GetMemoryAs<NotLockFreeAtomic>(); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAs'}}
+ write_map.GetMemoryAsSpan<NotLockFreeAtomic>(); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAsSpan'}}
+ write_map.GetMemoryAsSpan<NotLockFreeAtomic>(1); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAsSpan'}}
+ ReadOnlySharedMemoryMapping read_map = mapped_region.region.Map();
+ read_map.GetMemoryAs<NotLockFreeAtomic>(); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAs'}}
+ read_map.GetMemoryAsSpan<NotLockFreeAtomic>(); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAsSpan'}}
+ read_map.GetMemoryAsSpan<NotLockFreeAtomic>(1); // expected-error@base/memory/shared_memory_mapping_nocompile.nc:* {{no matching member function for call to 'GetMemoryAsSpan'}}
+}
+
+} // namespace base
diff --git a/base/memory/shared_memory_mapping_unittest.cc b/base/memory/shared_memory_mapping_unittest.cc
index 24f49ee6..31dd519 100644
--- a/base/memory/shared_memory_mapping_unittest.cc
+++ b/base/memory/shared_memory_mapping_unittest.cc
@@ -6,6 +6,7 @@
#include <stdint.h>
+#include <atomic>
#include <limits>
#include "base/containers/span.h"
@@ -45,10 +46,10 @@
CreateMapping(sizeof(uint32_t));
uint32_t* write_ptr = write_mapping_.GetMemoryAs<uint32_t>();
- EXPECT_NE(nullptr, write_ptr);
+ ASSERT_NE(nullptr, write_ptr);
const uint32_t* read_ptr = read_mapping_.GetMemoryAs<uint32_t>();
- EXPECT_NE(nullptr, read_ptr);
+ ASSERT_NE(nullptr, read_ptr);
*write_ptr = 0u;
EXPECT_EQ(0u, *read_ptr);
@@ -146,6 +147,33 @@
.empty());
}
+TEST_F(SharedMemoryMappingTest, Atomic) {
+ CreateMapping(sizeof(std::atomic<uint32_t>));
+
+ auto* write_ptr = write_mapping_.GetMemoryAs<std::atomic<uint32_t>>();
+ ASSERT_NE(nullptr, write_ptr);
+
+ // Placement new to initialize the std::atomic in place.
+ new (write_ptr) std::atomic<uint32_t>;
+
+ const auto* read_ptr = read_mapping_.GetMemoryAs<std::atomic<uint32_t>>();
+ ASSERT_NE(nullptr, read_ptr);
+
+ write_ptr->store(0u, std::memory_order_relaxed);
+ EXPECT_EQ(0u, read_ptr->load(std::memory_order_relaxed));
+
+ write_ptr->store(0x12345678u, std::memory_order_relaxed);
+ EXPECT_EQ(0x12345678u, read_ptr->load(std::memory_order_relaxed));
+}
+
+TEST_F(SharedMemoryMappingTest, TooBigAtomic) {
+ CreateMapping(sizeof(std::atomic<uint8_t>));
+
+ EXPECT_EQ(nullptr, write_mapping_.GetMemoryAs<std::atomic<uint32_t>>());
+
+ EXPECT_EQ(nullptr, read_mapping_.GetMemoryAs<std::atomic<uint32_t>>());
+}
+
// TODO(dcheng): This test is temporarily disabled on iOS. iOS devices allow
// the creation of a 1GB shared memory region, but don't allow the region to be
// mapped.
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index e29449c..2b558ed 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -1802,10 +1802,6 @@
// the same host (e.g. from AgentSchedulingGroupHost::RenderProcessExited()).
// In that case, we only need to resend the read-only memory region.
if (!last_foreground_time_region_.IsValid()) {
- static_assert(
- std::atomic<base::TimeTicks>::is_always_lock_free,
- "Atomically sharing TimeTicks across processes might be unsafe");
-
last_foreground_time_region_ = base::ReadOnlySharedMemoryRegion::Create(
sizeof(std::atomic<base::TimeTicks>));
CHECK(last_foreground_time_region_.IsValid());
diff --git a/mojo/public/cpp/base/shared_memory_version.h b/mojo/public/cpp/base/shared_memory_version.h
index ed44fedf..b4b0678 100644
--- a/mojo/public/cpp/base/shared_memory_version.h
+++ b/mojo/public/cpp/base/shared_memory_version.h
@@ -18,8 +18,6 @@
using VersionType = uint64_t;
using SharedVersionType = std::atomic<VersionType>;
-static_assert(SharedVersionType::is_always_lock_free,
- "Usage of SharedVersionType across processes might be unsafe");
// This file contains classes to share a version between processes through
// shared memory. A version is a nonzero monotonically increasing integer. A
diff --git a/services/network/restricted_cookie_manager.h b/services/network/restricted_cookie_manager.h
index 9ddbf342..64e11e4 100644
--- a/services/network/restricted_cookie_manager.h
+++ b/services/network/restricted_cookie_manager.h
@@ -188,8 +188,6 @@
private:
using SharedVersionType = std::atomic<uint64_t>;
- static_assert(SharedVersionType::is_always_lock_free,
- "Usage of SharedVersionType across processes might be unsafe");
// Function to be called when an event is known to potentially invalidate
// cookies the other side could have cached.
