Skip to content

Commit

Permalink
Make macOS shared memory implementation work on iOS as well
Browse files Browse the repository at this point in the history 
iOS currently uses a file-based implementation of shared memory
(platform_shared_memory_region_posix.cc), that is responsible for a
significant fraction of Chrome's disk writes overall. While Chrome
on iOS never needs to share memory between processes, cross-platform
primitives like mojo pipes use PlatformSharedMemoryRegion on iOS too.

macOS currently uses an implementation of shared memory using
mach_make_memory_entry_64. This implementation nearly works on iOS
as-is, except that mach_vm_map is not available on iOS, which instead
supports vm_map (and similarly doesn't support other mach_vm_*
functions but supports their vm_* equivalents). macOS also supports
vm_map. The main advantage of mach_vm_map and the other mach_vm_*
functions is interoperability between 32-bit and 64-bit processes
(e.g., sharing memory between such processes), which is not an issue
for Chrome on macOS, since it is 64-bit only.

This CL modifies the macOS implementation of PlatformSharedMemoryRegion
to use vm_map instead of mach_vm_map, and uses this implementation on
iOS as well. This fixes the excessive disk write issue on iOS, and also
leaves iOS in a better state for a potential future where Chrome needs
to share memory between processes.

Change-Id: I9560cba636a371e684665d59869038e4f29b957e
Bug: 1327411
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3673160
Commit-Queue: Ali Juma <ajuma@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1009355}
  • Loading branch information
@alijuma
alijuma authored and Chromium LUCI CQ committed May 31, 2022
1 parent 7c98072 commit dff201a
Show file tree
Hide file tree
Showing 18 changed files with 102 additions and 101 deletions.
4 changes: 3 additions & 1 deletion base/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2213,6 +2213,8 @@ mixed_component("base") {
"mac/scoped_objc_class_swizzler.h",
"mac/scoped_objc_class_swizzler.mm",
"mac/scoped_typeref.h",
"memory/platform_shared_memory_mapper_mac.cc",
"memory/platform_shared_memory_region_mac.cc",
"message_loop/message_pump_mac.h",
"message_loop/message_pump_mac.mm",
"power_monitor/power_monitor_device_source_ios.mm",
Expand Down Expand Up @@ -2266,7 +2268,7 @@ mixed_component("base") {

# Android and MacOS have their own custom shared memory handle
# implementations. e.g. due to supporting both POSIX and native handles.
if (is_posix && !is_android && !is_mac) {
if (is_posix && !is_android && !is_apple) {
sources += [
"memory/platform_shared_memory_mapper_posix.cc",
"memory/platform_shared_memory_region_posix.cc",
Expand Down
2 changes: 1 addition & 1 deletion base/memory/platform_shared_memory_handle.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

namespace base::subtle {

#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_MAC) && !BUILDFLAG(IS_ANDROID)
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE) && !BUILDFLAG(IS_ANDROID)
ScopedFDPair::ScopedFDPair() = default;

ScopedFDPair::ScopedFDPair(ScopedFDPair&&) = default;
Expand Down
6 changes: 3 additions & 3 deletions base/memory/platform_shared_memory_handle.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

#include "build/build_config.h"

#if BUILDFLAG(IS_MAC)
#if BUILDFLAG(IS_APPLE)
#include <mach/mach.h>
#include "base/mac/scoped_mach_port.h"
#elif BUILDFLAG(IS_FUCHSIA)
Expand All @@ -22,7 +22,7 @@

namespace base::subtle {

#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_MAC) && !BUILDFLAG(IS_ANDROID)
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE) && !BUILDFLAG(IS_ANDROID)
// Helper structs to keep two descriptors on POSIX. It's needed to support
// ConvertToReadOnly().
struct BASE_EXPORT FDPair {
Expand All @@ -49,7 +49,7 @@ struct BASE_EXPORT ScopedFDPair {
#endif

// Platform-specific shared memory type used by the shared memory system.
#if BUILDFLAG(IS_MAC)
#if BUILDFLAG(IS_APPLE)
using PlatformSharedMemoryHandle = mach_port_t;
using ScopedPlatformSharedMemoryHandle = mac::ScopedMachSendRight;
#elif BUILDFLAG(IS_FUCHSIA)
Expand Down
31 changes: 15 additions & 16 deletions base/memory/platform_shared_memory_mapper_mac.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

#include "base/logging.h"

#include <mach/mach_vm.h>
#include <mach/vm_map.h>
#include "base/mac/mach_logging.h"

namespace base {
Expand All @@ -17,30 +17,29 @@ absl::optional<span<uint8_t>> PlatformSharedMemoryMapper::Map(
uint64_t offset,
size_t size) {
vm_prot_t vm_prot_write = write_allowed ? VM_PROT_WRITE : 0;
mach_vm_address_t address = 0;
kern_return_t kr =
mach_vm_map(mach_task_self(),
&address, // Output parameter
size,
0, // Alignment mask
VM_FLAGS_ANYWHERE, handle, offset,
FALSE, // Copy
VM_PROT_READ | vm_prot_write, // Current protection
VM_PROT_READ | vm_prot_write, // Maximum protection
VM_INHERIT_NONE);
vm_address_t address = 0;
kern_return_t kr = vm_map(mach_task_self(),
&address, // Output parameter
size,
0, // Alignment mask
VM_FLAGS_ANYWHERE, handle, offset,
FALSE, // Copy
VM_PROT_READ | vm_prot_write, // Current protection
VM_PROT_READ | vm_prot_write, // Maximum protection
VM_INHERIT_NONE);
if (kr != KERN_SUCCESS) {
MACH_DLOG(ERROR, kr) << "mach_vm_map";
MACH_DLOG(ERROR, kr) << "vm_map";
return absl::nullopt;
}

return make_span(reinterpret_cast<uint8_t*>(address), size);
}

void PlatformSharedMemoryMapper::Unmap(span<uint8_t> mapping) {
kern_return_t kr = mach_vm_deallocate(
mach_task_self(), reinterpret_cast<mach_vm_address_t>(mapping.data()),
kern_return_t kr = vm_deallocate(
mach_task_self(), reinterpret_cast<vm_address_t>(mapping.data()),
mapping.size());
MACH_DLOG_IF(ERROR, kr != KERN_SUCCESS, kr) << "mach_vm_deallocate";
MACH_DLOG_IF(ERROR, kr != KERN_SUCCESS, kr) << "vm_deallocate";
}

} // namespace base
6 changes: 3 additions & 3 deletions base/memory/platform_shared_memory_region.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ class BASE_EXPORT PlatformSharedMemoryRegion {
Mode mode,
size_t size,
const UnguessableToken& guid);
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_MAC)
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_APPLE)
// Specialized version of Take() for POSIX that takes only one file descriptor
// instead of pair. Cannot be used with kWritable |mode|.
static PlatformSharedMemoryRegion Take(ScopedFD handle,
Expand Down Expand Up @@ -173,13 +173,13 @@ class BASE_EXPORT PlatformSharedMemoryRegion {
// kWritable mode, all other modes will CHECK-fail. The object will have
// kReadOnly mode after this call on success.
bool ConvertToReadOnly();
#if BUILDFLAG(IS_MAC)
#if BUILDFLAG(IS_APPLE)
// Same as above, but |mapped_addr| is used as a hint to avoid additional
// mapping of the memory object.
// |mapped_addr| must be mapped location of |memory_object_|. If the location
// is unknown, |mapped_addr| should be |nullptr|.
bool ConvertToReadOnly(void* mapped_addr);
#endif // BUILDFLAG(IS_MAC)
#endif // BUILDFLAG(IS_APPLE)

// Converts the region to unsafe. Returns whether the operation succeeded.
// Makes the current instance invalid on failure. Can be called only in
Expand Down
32 changes: 14 additions & 18 deletions base/memory/platform_shared_memory_region_mac.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,14 @@

#include "base/memory/platform_shared_memory_region.h"

#include <mach/mach_vm.h>
#include <mach/vm_map.h>

#include "base/mac/mach_logging.h"
#include "base/mac/scoped_mach_vm.h"
#include "base/metrics/histogram_functions.h"
#include "base/metrics/histogram_macros.h"
#include "build/build_config.h"

#if BUILDFLAG(IS_IOS)
#error "MacOS only - iOS uses platform_shared_memory_region_posix.cc"
#endif

namespace base {
namespace subtle {

Expand Down Expand Up @@ -88,12 +84,12 @@ bool PlatformSharedMemoryRegion::ConvertToReadOnly(void* mapped_addr) {
mac::ScopedMachVM scoped_memory;
if (!temp_addr) {
// Intentionally lower current prot and max prot to |VM_PROT_READ|.
kern_return_t kr = mach_vm_map(
mach_task_self(), reinterpret_cast<mach_vm_address_t*>(&temp_addr),
size_, 0, VM_FLAGS_ANYWHERE, handle_copy.get(), 0, FALSE, VM_PROT_READ,
VM_PROT_READ, VM_INHERIT_NONE);
kern_return_t kr =
vm_map(mach_task_self(), reinterpret_cast<vm_address_t*>(&temp_addr),
size_, 0, VM_FLAGS_ANYWHERE, handle_copy.get(), 0, FALSE,
VM_PROT_READ, VM_PROT_READ, VM_INHERIT_NONE);
if (kr != KERN_SUCCESS) {
MACH_DLOG(ERROR, kr) << "mach_vm_map";
MACH_DLOG(ERROR, kr) << "vm_map";
return false;
}
scoped_memory.reset(reinterpret_cast<vm_address_t>(temp_addr),
Expand Down Expand Up @@ -143,7 +139,7 @@ PlatformSharedMemoryRegion PlatformSharedMemoryRegion::Create(Mode mode,
CHECK_NE(mode, Mode::kReadOnly) << "Creating a region in read-only mode will "
"lead to this region being non-modifiable";

mach_vm_size_t vm_size = size;
memory_object_size_t vm_size = size;
mac::ScopedMachSendRight named_right;
kern_return_t kr = mach_make_memory_entry_64(
mach_task_self(), &vm_size,
Expand All @@ -165,19 +161,19 @@ bool PlatformSharedMemoryRegion::CheckPlatformHandlePermissionsCorrespondToMode(
PlatformSharedMemoryHandle handle,
Mode mode,
size_t size) {
mach_vm_address_t temp_addr = 0;
vm_address_t temp_addr = 0;
kern_return_t kr =
mach_vm_map(mach_task_self(), &temp_addr, size, 0, VM_FLAGS_ANYWHERE,
handle, 0, FALSE, VM_PROT_READ | VM_PROT_WRITE,
VM_PROT_READ | VM_PROT_WRITE, VM_INHERIT_NONE);
vm_map(mach_task_self(), &temp_addr, size, 0, VM_FLAGS_ANYWHERE, handle,
0, FALSE, VM_PROT_READ | VM_PROT_WRITE,
VM_PROT_READ | VM_PROT_WRITE, VM_INHERIT_NONE);
if (kr == KERN_SUCCESS) {
kern_return_t kr_deallocate =
mach_vm_deallocate(mach_task_self(), temp_addr, size);
vm_deallocate(mach_task_self(), temp_addr, size);
// TODO(crbug.com/838365): convert to DLOG when bug fixed.
MACH_LOG_IF(ERROR, kr_deallocate != KERN_SUCCESS, kr_deallocate)
<< "mach_vm_deallocate";
<< "vm_deallocate";
} else if (kr != KERN_INVALID_RIGHT) {
MACH_LOG(ERROR, kr) << "mach_vm_map";
MACH_LOG(ERROR, kr) << "vm_map";
return false;
}

Expand Down
28 changes: 16 additions & 12 deletions base/memory/platform_shared_memory_region_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
#include "build/build_config.h"
#include "testing/gtest/include/gtest/gtest.h"

#if BUILDFLAG(IS_MAC)
#include <mach/mach_vm.h>
#if BUILDFLAG(IS_APPLE)
#include <mach/vm_map.h>
#include <sys/mman.h>
#elif BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_IOS)
#elif BUILDFLAG(IS_POSIX)
#include <sys/mman.h>
#include "base/debug/proc_maps_linux.h"
#elif BUILDFLAG(IS_WIN)
Expand Down Expand Up @@ -212,7 +212,7 @@ TEST_F(PlatformSharedMemoryRegionTest, MapAtWithOverflowTest) {
EXPECT_FALSE(mapping.IsValid());
}

#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_MAC)
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_APPLE)
// Tests that the second handle is closed after a conversion to read-only on
// POSIX.
TEST_F(PlatformSharedMemoryRegionTest,
Expand All @@ -238,17 +238,21 @@ TEST_F(PlatformSharedMemoryRegionTest, ConvertToUnsafeInvalidatesSecondHandle) {
#endif

void CheckReadOnlyMapProtection(void* addr) {
#if BUILDFLAG(IS_MAC)
#if BUILDFLAG(IS_APPLE)
vm_region_basic_info_64 basic_info;
mach_vm_size_t dummy_size = 0;
void* temp_addr = addr;
MachVMRegionResult result = GetBasicInfo(
mach_task_self(), &dummy_size,
reinterpret_cast<mach_vm_address_t*>(&temp_addr), &basic_info);
ASSERT_EQ(result, MachVMRegionResult::Success);
vm_size_t dummy_size = 0;
mach_msg_type_number_t info_count = VM_REGION_BASIC_INFO_COUNT_64;
mach_port_t object_name;
kern_return_t kr = vm_region_64(
mach_task_self(), reinterpret_cast<vm_address_t*>(&addr), &dummy_size,
VM_REGION_BASIC_INFO_64, reinterpret_cast<vm_region_info_t>(&basic_info),
&info_count, &object_name);
mach_port_deallocate(mach_task_self(), object_name);

ASSERT_EQ(kr, KERN_SUCCESS);
EXPECT_EQ(basic_info.protection & VM_PROT_ALL, VM_PROT_READ);
EXPECT_EQ(basic_info.max_protection & VM_PROT_ALL, VM_PROT_READ);
#elif BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_IOS)
#elif BUILDFLAG(IS_POSIX)
std::string proc_maps;
ASSERT_TRUE(base::debug::ReadProcMaps(&proc_maps));
std::vector<base::debug::MappedMemoryRegion> regions;
Expand Down
4 changes: 2 additions & 2 deletions base/metrics/field_trial.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -846,7 +846,7 @@ void FieldTrialList::PopulateLaunchOptionsWithFieldTrialState(
}
#endif // !BUILDFLAG(IS_IOS)

#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_MAC) && !BUILDFLAG(IS_NACL)
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE) && !BUILDFLAG(IS_NACL)
// static
int FieldTrialList::GetFieldTrialDescriptor() {
InstantiateFieldTrialAllocatorIfNeeded();
Expand All @@ -859,7 +859,7 @@ int FieldTrialList::GetFieldTrialDescriptor() {
return global_->readonly_allocator_region_.GetPlatformHandle().fd;
#endif
}
#endif // BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_MAC) && !BUILDFLAG(IS_NACL)
#endif // BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE) && !BUILDFLAG(IS_NACL)

// static
ReadOnlySharedMemoryRegion
Expand Down
4 changes: 2 additions & 2 deletions base/metrics/field_trial.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -572,13 +572,13 @@ class BASE_EXPORT FieldTrialList {
LaunchOptions* launch_options);
#endif // !BUILDFLAG(IS_IOS)

#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_MAC) && !BUILDFLAG(IS_NACL)
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE) && !BUILDFLAG(IS_NACL)
// On POSIX, we also need to explicitly pass down this file descriptor that
// should be shared with the child process. Returns -1 if it was not
// initialized properly. The current process remains the onwer of the passed
// descriptor.
static int GetFieldTrialDescriptor();
#endif // BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_MAC) && !BUILDFLAG(IS_NACL)
#endif // BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE) && !BUILDFLAG(IS_NACL)

static ReadOnlySharedMemoryRegion DuplicateFieldTrialSharedMemoryForTesting();

Expand Down
26 changes: 13 additions & 13 deletions base/test/test_shared_memory_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@
#include <zircon/rights.h>
#endif

#if BUILDFLAG(IS_MAC)
#include <mach/mach_vm.h>
#if BUILDFLAG(IS_APPLE)
#include <mach/vm_map.h>
#endif

#if BUILDFLAG(IS_WIN)
Expand All @@ -41,7 +41,7 @@ static const size_t kDataSize = 1024;
// Common routine used with Posix file descriptors. Check that shared memory
// file descriptor |fd| does not allow writable mappings. Return true on
// success, false otherwise.
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_MAC)
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE)
static bool CheckReadOnlySharedMemoryFdPosix(int fd) {
// Note that the error on Android is EPERM, unlike other platforms where
// it will be EACCES.
Expand All @@ -66,7 +66,7 @@ static bool CheckReadOnlySharedMemoryFdPosix(int fd) {
}
return true;
}
#endif // BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_MAC)
#endif // BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE)

#if BUILDFLAG(IS_FUCHSIA)
// Fuchsia specific implementation.
Expand All @@ -88,16 +88,16 @@ bool CheckReadOnlySharedMemoryFuchsiaHandle(zx::unowned_vmo handle) {
return true;
}

#elif BUILDFLAG(IS_MAC)
#elif BUILDFLAG(IS_APPLE)
bool CheckReadOnlySharedMemoryMachPort(mach_port_t memory_object) {
mach_vm_address_t memory;
const kern_return_t kr = mach_vm_map(
mach_task_self(), &memory, kDataSize, 0, VM_FLAGS_ANYWHERE, memory_object,
0, FALSE, VM_PROT_READ | VM_PROT_WRITE,
VM_PROT_READ | VM_PROT_WRITE | VM_PROT_IS_MASK, VM_INHERIT_NONE);
vm_address_t memory;
const kern_return_t kr =
vm_map(mach_task_self(), &memory, kDataSize, 0, VM_FLAGS_ANYWHERE,
memory_object, 0, FALSE, VM_PROT_READ | VM_PROT_WRITE,
VM_PROT_READ | VM_PROT_WRITE | VM_PROT_IS_MASK, VM_INHERIT_NONE);
if (kr == KERN_SUCCESS) {
LOG(ERROR) << "mach_vm_map() should have failed!";
mach_vm_deallocate(mach_task_self(), memory, kDataSize); // Cleanup.
LOG(ERROR) << "vm_map() should have failed!";
vm_deallocate(mach_task_self(), memory, kDataSize); // Cleanup.
return false;
}
return true;
Expand Down Expand Up @@ -126,7 +126,7 @@ bool CheckReadOnlyPlatformSharedMemoryRegionForTesting(
return false;
}

#if BUILDFLAG(IS_MAC)
#if BUILDFLAG(IS_APPLE)
return CheckReadOnlySharedMemoryMachPort(region.GetPlatformHandle());
#elif BUILDFLAG(IS_FUCHSIA)
return CheckReadOnlySharedMemoryFuchsiaHandle(region.GetPlatformHandle());
Expand Down
2 changes: 1 addition & 1 deletion ipc/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ component("message_support") {
]
}

if (is_mac) {
if (is_apple) {
sources += [
"mach_port_attachment_mac.cc",
"mach_port_attachment_mac.h",
Expand Down
Loading

0 comments on commit dff201a

Please sign in to comment.