Skip to content

Commit

Permalink
Add a GUID to base::SharedMemoryHandle.
Browse files Browse the repository at this point in the history
This allows SharedMemoryHandle to be tracked across processes.
SharedMemoryHandles that point to the same region should have the same GUID.
This CL does not finish all of the GUID plumbing. The following still needs to
be done:
  * Passing GUID through Mojo. rockot@ has said that he will do this and TODOs
  have been left for him in the code.
  * Passing GUID for base::FieldTrial, which requires a SharedMemoryHandle
  shortly after proess launch, before IPC is set up.
  * Updating ArcGpuVideoDecodeAccelerator to use mojo ScopedHandles instead of
  fds.
  * Updating serialization of gfx::GpuMemoryBufferHandle to pass through shared
  buffer handles instead of generic handles.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2859843002
Cr-Commit-Position: refs/heads/master@{#469877}
  • Loading branch information
erikchen authored and Commit bot committed May 6, 2017
1 parent 469d054 commit 1452520
Show file tree
Hide file tree
Showing 24 changed files with 289 additions and 139 deletions.
1 change: 1 addition & 0 deletions base/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,7 @@ component("base") {
"memory/scoped_vector.h",
"memory/shared_memory.h",
"memory/shared_memory_android.cc",
"memory/shared_memory_handle.cc",
"memory/shared_memory_handle.h",
"memory/shared_memory_handle_mac.cc",
"memory/shared_memory_handle_win.cc",
Expand Down
6 changes: 4 additions & 2 deletions base/memory/shared_memory.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,10 @@ struct BASE_EXPORT SharedMemoryCreateOptions {
bool share_read_only = false;
};

// Platform abstraction for shared memory. Provides a C++ wrapper
// around the OS primitive for a memory mapped file.
// Platform abstraction for shared memory.
// SharedMemory consumes a SharedMemoryHandle [potentially one that it created]
// to map a shared memory OS resource into the virtual address space of the
// current process.
class BASE_EXPORT SharedMemory {
public:
SharedMemory();
Expand Down
3 changes: 2 additions & 1 deletion base/memory/shared_memory_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {

// Android doesn't appear to have a way to drop write access on an ashmem
// segment for a single descriptor. http://crbug.com/320865
readonly_shm_ = SharedMemoryHandle::ImportHandle(dup(shm_.GetHandle()));
readonly_shm_ = SharedMemoryHandle(
base::FileDescriptor(dup(shm_.GetHandle()), false), shm_.GetGUID());
if (!readonly_shm_.IsValid()) {
DPLOG(ERROR) << "dup() failed";
return false;
Expand Down
19 changes: 19 additions & 0 deletions base/memory/shared_memory_handle.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "base/memory/shared_memory_handle.h"

namespace base {

SharedMemoryHandle::SharedMemoryHandle(const SharedMemoryHandle& handle) =
default;

SharedMemoryHandle& SharedMemoryHandle::operator=(
const SharedMemoryHandle& handle) = default;

base::UnguessableToken SharedMemoryHandle::GetGUID() const {
return guid_;
}

} // namespace base
53 changes: 38 additions & 15 deletions base/memory/shared_memory_handle.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <stddef.h>

#include "base/unguessable_token.h"
#include "build/build_config.h"

#if defined(OS_WIN)
Expand All @@ -25,8 +26,12 @@

namespace base {

// SharedMemoryHandle is a platform specific type which represents
// the underlying OS handle to a shared memory segment.
// SharedMemoryHandle is the smallest possible IPC-transportable "reference" to
// a shared memory OS resource. A "reference" can be consumed exactly once [by
// base::SharedMemory] to map the shared memory OS resource into the virtual
// address space of the current process.
// TODO(erikchen): This class should have strong ownership semantics to prevent
// leaks of the underlying OS resource. https://crbug.com/640840.
class BASE_EXPORT SharedMemoryHandle {
public:
// The default constructor returns an invalid SharedMemoryHandle.
Expand Down Expand Up @@ -61,6 +66,11 @@ class BASE_EXPORT SharedMemoryHandle {
// resource.
SharedMemoryHandle Duplicate() const;

// Uniques identifies the shared memory region that the underlying OS resource
// points to. Multiple SharedMemoryHandles that point to the same shared
// memory region will have the same GUID. Preserved across IPC.
base::UnguessableToken GetGUID() const;

#if defined(OS_MACOSX) && !defined(OS_IOS)
enum Type {
// The SharedMemoryHandle is backed by a POSIX fd.
Expand All @@ -76,15 +86,23 @@ class BASE_EXPORT SharedMemoryHandle {
// common for existing code to make shallow copies of SharedMemoryHandle, and
// the one that is finally passed into a base::SharedMemory is the one that
// "consumes" the fd.
explicit SharedMemoryHandle(const base::FileDescriptor& file_descriptor);
// |guid| uniquely identifies the shared memory region pointed to by the
// underlying OS resource. If |file_descriptor| is associated with another
// SharedMemoryHandle, the caller must pass the |guid| of that
// SharedMemoryHandle. Otherwise, the caller should generate a new
// UnguessableToken.
SharedMemoryHandle(const base::FileDescriptor& file_descriptor,
const base::UnguessableToken& guid);

// Makes a Mach-based SharedMemoryHandle of the given size. On error,
// subsequent calls to IsValid() return false.
explicit SharedMemoryHandle(mach_vm_size_t size);
SharedMemoryHandle(mach_vm_size_t size, const base::UnguessableToken& guid);

// Makes a Mach-based SharedMemoryHandle from |memory_object|, a named entry
// in the current task. The memory region has size |size|.
SharedMemoryHandle(mach_port_t memory_object, mach_vm_size_t size);
SharedMemoryHandle(mach_port_t memory_object,
mach_vm_size_t size,
const base::UnguessableToken& guid);

// Exposed so that the SharedMemoryHandle can be transported between
// processes.
Expand All @@ -101,15 +119,21 @@ class BASE_EXPORT SharedMemoryHandle {
bool MapAt(off_t offset, size_t bytes, void** memory, bool read_only);
#elif defined(OS_WIN)
// Takes implicit ownership of |h|.
SharedMemoryHandle(HANDLE h);

// |guid| uniquely identifies the shared memory region pointed to by the
// underlying OS resource. If the HANDLE is associated with another
// SharedMemoryHandle, the caller must pass the |guid| of that
// SharedMemoryHandle. Otherwise, the caller should generate a new
// UnguessableToken.
SharedMemoryHandle(HANDLE h, const base::UnguessableToken& guid);
HANDLE GetHandle() const;
#else
// This constructor is deprecated, as it fails to propagate the GUID, which
// will be added in the near future.
// TODO(rockot): Remove this constructor once Mojo supports GUIDs.
// https://crbug.com/713763.
explicit SharedMemoryHandle(const base::FileDescriptor& file_descriptor);
// |guid| uniquely identifies the shared memory region pointed to by the
// underlying OS resource. If |file_descriptor| is associated with another
// SharedMemoryHandle, the caller must pass the |guid| of that
// SharedMemoryHandle. Otherwise, the caller should generate a new
// UnguessableToken.
SharedMemoryHandle(const base::FileDescriptor& file_descriptor,
const base::UnguessableToken& guid);

// Creates a SharedMemoryHandle from an |fd| supplied from an external
// service.
Expand All @@ -130,9 +154,6 @@ class BASE_EXPORT SharedMemoryHandle {
#if defined(OS_MACOSX) && !defined(OS_IOS)
friend class SharedMemory;

// Shared code between copy constructor and operator=.
void CopyRelevantData(const SharedMemoryHandle& handle);

Type type_;

// Each instance of a SharedMemoryHandle is backed either by a POSIX fd or a
Expand Down Expand Up @@ -166,6 +187,8 @@ class BASE_EXPORT SharedMemoryHandle {
#else
FileDescriptor file_descriptor_;
#endif

base::UnguessableToken guid_;
};

} // namespace base
Expand Down
46 changes: 12 additions & 34 deletions base/memory/shared_memory_handle_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,20 @@
#include "base/mac/mac_util.h"
#include "base/mac/mach_logging.h"
#include "base/posix/eintr_wrapper.h"
#include "base/unguessable_token.h"

namespace base {

SharedMemoryHandle::SharedMemoryHandle()
: type_(MACH), memory_object_(MACH_PORT_NULL) {}

SharedMemoryHandle::SharedMemoryHandle(
const base::FileDescriptor& file_descriptor)
: type_(POSIX), file_descriptor_(file_descriptor) {}
const base::FileDescriptor& file_descriptor,
const base::UnguessableToken& guid)
: type_(POSIX), file_descriptor_(file_descriptor), guid_(guid) {}

SharedMemoryHandle::SharedMemoryHandle(mach_vm_size_t size) {
SharedMemoryHandle::SharedMemoryHandle(mach_vm_size_t size,
const base::UnguessableToken& guid) {
type_ = MACH;
mach_port_t named_right;
kern_return_t kr = mach_make_memory_entry_64(
Expand All @@ -40,28 +43,17 @@ SharedMemoryHandle::SharedMemoryHandle(mach_vm_size_t size) {
memory_object_ = named_right;
size_ = size;
ownership_passes_to_ipc_ = false;
guid_ = guid;
}

SharedMemoryHandle::SharedMemoryHandle(mach_port_t memory_object,
mach_vm_size_t size)
mach_vm_size_t size,
const base::UnguessableToken& guid)
: type_(MACH),
memory_object_(memory_object),
size_(size),
ownership_passes_to_ipc_(false) {}

SharedMemoryHandle::SharedMemoryHandle(const SharedMemoryHandle& handle) {
CopyRelevantData(handle);
}

SharedMemoryHandle& SharedMemoryHandle::operator=(
const SharedMemoryHandle& handle) {
if (this == &handle)
return *this;

type_ = handle.type_;
CopyRelevantData(handle);
return *this;
}
ownership_passes_to_ipc_(false),
guid_(guid) {}

SharedMemoryHandle SharedMemoryHandle::Duplicate() const {
switch (type_) {
Expand All @@ -71,7 +63,7 @@ SharedMemoryHandle SharedMemoryHandle::Duplicate() const {
int duped_fd = HANDLE_EINTR(dup(file_descriptor_.fd));
if (duped_fd < 0)
return SharedMemoryHandle();
return SharedMemoryHandle(FileDescriptor(duped_fd, true));
return SharedMemoryHandle(FileDescriptor(duped_fd, true), guid_);
}
case MACH: {
if (!IsValid())
Expand Down Expand Up @@ -177,18 +169,4 @@ bool SharedMemoryHandle::OwnershipPassesToIPC() const {
return ownership_passes_to_ipc_;
}

void SharedMemoryHandle::CopyRelevantData(const SharedMemoryHandle& handle) {
type_ = handle.type_;
switch (type_) {
case POSIX:
file_descriptor_ = handle.file_descriptor_;
break;
case MACH:
memory_object_ = handle.memory_object_;
size_ = handle.size_;
ownership_passes_to_ipc_ = handle.ownership_passes_to_ipc_;
break;
}
}

} // namespace base
13 changes: 6 additions & 7 deletions base/memory/shared_memory_handle_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,23 @@

#include "base/logging.h"
#include "base/posix/eintr_wrapper.h"
#include "base/unguessable_token.h"

namespace base {

SharedMemoryHandle::SharedMemoryHandle() = default;
SharedMemoryHandle::SharedMemoryHandle(const SharedMemoryHandle& handle) =
default;
SharedMemoryHandle& SharedMemoryHandle::operator=(
const SharedMemoryHandle& handle) = default;

SharedMemoryHandle::SharedMemoryHandle(
const base::FileDescriptor& file_descriptor)
: file_descriptor_(file_descriptor) {}
const base::FileDescriptor& file_descriptor,
const base::UnguessableToken& guid)
: file_descriptor_(file_descriptor), guid_(guid) {}

// static
SharedMemoryHandle SharedMemoryHandle::ImportHandle(int fd) {
SharedMemoryHandle handle;
handle.file_descriptor_.fd = fd;
handle.file_descriptor_.auto_close = false;
handle.guid_ = UnguessableToken::Create();
return handle;
}

Expand Down Expand Up @@ -59,7 +58,7 @@ SharedMemoryHandle SharedMemoryHandle::Duplicate() const {
int duped_handle = HANDLE_EINTR(dup(file_descriptor_.fd));
if (duped_handle < 0)
return SharedMemoryHandle();
return SharedMemoryHandle(FileDescriptor(duped_handle, true));
return SharedMemoryHandle(FileDescriptor(duped_handle, true), GetGUID());
}

void SharedMemoryHandle::SetOwnershipPassesToIPC(bool ownership_passes) {
Expand Down
22 changes: 5 additions & 17 deletions base/memory/shared_memory_handle_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,16 @@
#include "base/memory/shared_memory_handle.h"

#include "base/logging.h"
#include "base/unguessable_token.h"

namespace base {

SharedMemoryHandle::SharedMemoryHandle()
: handle_(nullptr), ownership_passes_to_ipc_(false) {}

SharedMemoryHandle::SharedMemoryHandle(HANDLE h)
: handle_(h), ownership_passes_to_ipc_(false) {}

SharedMemoryHandle::SharedMemoryHandle(const SharedMemoryHandle& handle)
: handle_(handle.handle_),
ownership_passes_to_ipc_(handle.ownership_passes_to_ipc_) {}

SharedMemoryHandle& SharedMemoryHandle::operator=(
const SharedMemoryHandle& handle) {
if (this == &handle)
return *this;

handle_ = handle.handle_;
ownership_passes_to_ipc_ = handle.ownership_passes_to_ipc_;
return *this;
}
SharedMemoryHandle::SharedMemoryHandle(HANDLE h,
const base::UnguessableToken& guid)
: handle_(h), ownership_passes_to_ipc_(false), guid_(guid) {}

void SharedMemoryHandle::Close() const {
DCHECK(handle_ != nullptr);
Expand All @@ -45,7 +33,7 @@ SharedMemoryHandle SharedMemoryHandle::Duplicate() const {
if (!success)
return SharedMemoryHandle();

base::SharedMemoryHandle handle(duped_handle);
base::SharedMemoryHandle handle(duped_handle, GetGUID());
handle.SetOwnershipPassesToIPC(true);
return handle;
}
Expand Down
16 changes: 9 additions & 7 deletions base/memory/shared_memory_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "base/scoped_generic.h"
#include "base/strings/utf_string_conversions.h"
#include "base/threading/thread_restrictions.h"
#include "base/unguessable_token.h"
#include "build/build_config.h"

#if defined(OS_MACOSX)
Expand Down Expand Up @@ -74,7 +75,7 @@ bool MakeMachSharedMemoryHandleReadOnly(SharedMemoryHandle* new_handle,
if (kr != KERN_SUCCESS)
return false;

*new_handle = SharedMemoryHandle(named_right, size);
*new_handle = SharedMemoryHandle(named_right, size, handle.GetGUID());
return true;
}

Expand Down Expand Up @@ -149,7 +150,7 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
return false;

if (options.type == SharedMemoryHandle::MACH) {
shm_ = SharedMemoryHandle(options.size);
shm_ = SharedMemoryHandle(options.size, UnguessableToken::Create());
requested_size_ = options.size;
return shm_.IsValid();
}
Expand Down Expand Up @@ -187,9 +188,10 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
int readonly_mapped_file = -1;
result = PrepareMapFile(std::move(fp), std::move(readonly_fd), &mapped_file,
&readonly_mapped_file);
shm_ = SharedMemoryHandle(FileDescriptor(mapped_file, false));
readonly_shm_ =
SharedMemoryHandle(FileDescriptor(readonly_mapped_file, false));
shm_ = SharedMemoryHandle(FileDescriptor(mapped_file, false),
UnguessableToken::Create());
readonly_shm_ = SharedMemoryHandle(
FileDescriptor(readonly_mapped_file, false), shm_.GetGUID());
return result;
}

Expand Down Expand Up @@ -237,8 +239,8 @@ bool SharedMemory::Unmap() {
SharedMemoryHandle SharedMemory::handle() const {
switch (shm_.type_) {
case SharedMemoryHandle::POSIX:
return SharedMemoryHandle(
FileDescriptor(shm_.file_descriptor_.fd, false));
return SharedMemoryHandle(FileDescriptor(shm_.file_descriptor_.fd, false),
shm_.GetGUID());
case SharedMemoryHandle::MACH:
return shm_;
}
Expand Down
Loading

0 comments on commit 1452520

Please sign in to comment.