Skip to content

Commit

Permalink
Support read-only duplicates of Mojo shared buffers.
Browse files Browse the repository at this point in the history
BUG=556587

Review URL: https://codereview.chromium.org/1689053003

Cr-Commit-Position: refs/heads/master@{#385634}
  • Loading branch information
akmistry authored and Commit bot committed Apr 7, 2016
1 parent 763ebe0 commit ee191a4
Show file tree
Hide file tree
Showing 13 changed files with 189 additions and 33 deletions.
4 changes: 0 additions & 4 deletions mojo/edk/embedder/embedder.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,6 @@ PassWrappedPlatformHandle(MojoHandle platform_handle_wrapper_handle,
// |read_only| is whether the handle is a read-only handle to shared memory.
// This |MojoHandle| is a Mojo shared buffer and can be manipulated using the
// shared buffer functions and transferred over a message pipe.
// TODO(crbug.com/556587): Support read-only handles. Currently, |read_only|
// must be false.
MOJO_SYSTEM_IMPL_EXPORT MojoResult
CreateSharedBufferWrapper(base::SharedMemoryHandle shared_memory_handle,
size_t num_bytes,
Expand All @@ -109,8 +107,6 @@ CreateSharedBufferWrapper(base::SharedMemoryHandle shared_memory_handle,
// Note: The value of |shared_memory_handle| may be
// base::SharedMemory::NULLHandle(), even if this function returns success.
// Callers should perform appropriate checks.
// TODO(crbug.com/556587): Support read-only handles. Currently, |read_only|
// will always return |false|.
MOJO_SYSTEM_IMPL_EXPORT MojoResult
PassSharedMemoryHandle(MojoHandle mojo_handle,
base::SharedMemoryHandle* shared_memory_handle,
Expand Down
41 changes: 31 additions & 10 deletions mojo/edk/embedder/platform_shared_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ ScopedPlatformHandle SharedMemoryToPlatformHandle(
PlatformSharedBuffer* PlatformSharedBuffer::Create(size_t num_bytes) {
DCHECK_GT(num_bytes, 0u);

PlatformSharedBuffer* rv = new PlatformSharedBuffer(num_bytes);
PlatformSharedBuffer* rv = new PlatformSharedBuffer(num_bytes, false);
if (!rv->Init()) {
// We can't just delete it directly, due to the "in destructor" (debug)
// check.
Expand All @@ -58,10 +58,11 @@ PlatformSharedBuffer* PlatformSharedBuffer::Create(size_t num_bytes) {
// static
PlatformSharedBuffer* PlatformSharedBuffer::CreateFromPlatformHandle(
size_t num_bytes,
bool read_only,
ScopedPlatformHandle platform_handle) {
DCHECK_GT(num_bytes, 0u);

PlatformSharedBuffer* rv = new PlatformSharedBuffer(num_bytes);
PlatformSharedBuffer* rv = new PlatformSharedBuffer(num_bytes, read_only);
if (!rv->InitFromPlatformHandle(std::move(platform_handle))) {
// We can't just delete it directly, due to the "in destructor" (debug)
// check.
Expand All @@ -78,9 +79,8 @@ PlatformSharedBuffer* PlatformSharedBuffer::CreateFromSharedMemoryHandle(
bool read_only,
base::SharedMemoryHandle handle) {
DCHECK_GT(num_bytes, 0u);
DCHECK(!read_only);

PlatformSharedBuffer* rv = new PlatformSharedBuffer(num_bytes);
PlatformSharedBuffer* rv = new PlatformSharedBuffer(num_bytes, read_only);
rv->InitFromSharedMemoryHandle(handle);

return rv;
Expand All @@ -90,6 +90,10 @@ size_t PlatformSharedBuffer::GetNumBytes() const {
return num_bytes_;
}

bool PlatformSharedBuffer::IsReadOnly() const {
return read_only_;
}

scoped_ptr<PlatformSharedBufferMapping> PlatformSharedBuffer::Map(
size_t offset,
size_t length) {
Expand Down Expand Up @@ -125,7 +129,7 @@ scoped_ptr<PlatformSharedBufferMapping> PlatformSharedBuffer::MapNoCheck(
return nullptr;

scoped_ptr<PlatformSharedBufferMapping> mapping(
new PlatformSharedBufferMapping(handle, offset, length));
new PlatformSharedBufferMapping(handle, read_only_, offset, length));
if (mapping->Map())
return make_scoped_ptr(mapping.release());

Expand Down Expand Up @@ -164,16 +168,34 @@ base::SharedMemoryHandle PlatformSharedBuffer::DuplicateSharedMemoryHandle() {
return base::SharedMemory::DuplicateHandle(shared_memory_->handle());
}

PlatformSharedBuffer::PlatformSharedBuffer(size_t num_bytes)
: num_bytes_(num_bytes) {}
PlatformSharedBuffer* PlatformSharedBuffer::CreateReadOnlyDuplicate() {
DCHECK(shared_memory_);
base::SharedMemoryHandle handle;
bool success;
{
base::AutoLock locker(lock_);
success = shared_memory_->ShareReadOnlyToProcess(
base::GetCurrentProcessHandle(), &handle);
}
if (!success || handle == base::SharedMemory::NULLHandle())
return nullptr;

return CreateFromSharedMemoryHandle(num_bytes_, true, handle);
}

PlatformSharedBuffer::PlatformSharedBuffer(size_t num_bytes, bool read_only)
: num_bytes_(num_bytes), read_only_(read_only) {}

PlatformSharedBuffer::~PlatformSharedBuffer() {}

bool PlatformSharedBuffer::Init() {
DCHECK(!shared_memory_);
DCHECK(!read_only_);

base::SharedMemoryCreateOptions options;
options.size = num_bytes_;
// By default, we can share as read-only.
options.share_read_only = true;
#if defined(OS_MACOSX) && !defined(OS_IOS)
options.type = base::SharedMemoryHandle::MACH;
#endif
Expand Down Expand Up @@ -201,16 +223,15 @@ bool PlatformSharedBuffer::InitFromPlatformHandle(
base::SharedMemoryHandle handle(platform_handle.release().handle, false);
#endif

shared_memory_.reset(new base::SharedMemory(handle, false));
shared_memory_.reset(new base::SharedMemory(handle, read_only_));
return true;
}

void PlatformSharedBuffer::InitFromSharedMemoryHandle(
base::SharedMemoryHandle handle) {
DCHECK(!shared_memory_);

// TODO(crbug.com/556587): Support read-only handles.
shared_memory_.reset(new base::SharedMemory(handle, false));
shared_memory_.reset(new base::SharedMemory(handle, read_only_));
}

PlatformSharedBufferMapping::~PlatformSharedBufferMapping() {
Expand Down
19 changes: 13 additions & 6 deletions mojo/edk/embedder/platform_shared_buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,6 @@ class PlatformSharedBufferMapping;
// - Sizes/offsets (of the shared memory and mappings) are arbitrary, and not
// restricted by page size. However, more memory may actually be mapped than
// requested.
//
// It currently does NOT support the following:
// - Sharing read-only. (This will probably eventually be supported.)
class MOJO_SYSTEM_IMPL_EXPORT PlatformSharedBuffer
: public base::RefCountedThreadSafe<PlatformSharedBuffer> {
public:
Expand All @@ -45,6 +42,7 @@ class MOJO_SYSTEM_IMPL_EXPORT PlatformSharedBuffer
// handle |platform_handle|. Returns null on failure.
static PlatformSharedBuffer* CreateFromPlatformHandle(
size_t num_bytes,
bool read_only,
ScopedPlatformHandle platform_handle);

// Creates a shared buffer of size |num_bytes| from the existing shared memory
Expand All @@ -57,6 +55,9 @@ class MOJO_SYSTEM_IMPL_EXPORT PlatformSharedBuffer
// Gets the size of shared buffer (in number of bytes).
size_t GetNumBytes() const;

// Returns whether this shared buffer is read-only.
bool IsReadOnly() const;

// Maps (some) of the shared buffer into memory; [|offset|, |offset + length|]
// must be contained in [0, |num_bytes|], and |length| must be at least 1.
// Returns null on failure.
Expand All @@ -71,7 +72,6 @@ class MOJO_SYSTEM_IMPL_EXPORT PlatformSharedBuffer
size_t length);

// Duplicates the underlying platform handle and passes it to the caller.
// TODO(vtl): On POSIX, we'll need two FDs to support sharing read-only.
ScopedPlatformHandle DuplicatePlatformHandle();

// Duplicates the underlying shared memory handle and passes it to the caller.
Expand All @@ -83,10 +83,15 @@ class MOJO_SYSTEM_IMPL_EXPORT PlatformSharedBuffer
// be disposed of.
ScopedPlatformHandle PassPlatformHandle();

// Create and return a read-only duplicate of this shared buffer. If this
// shared buffer isn't capable of returning a read-only duplicate, then
// nullptr will be returned.
PlatformSharedBuffer* CreateReadOnlyDuplicate();

private:
friend class base::RefCountedThreadSafe<PlatformSharedBuffer>;

explicit PlatformSharedBuffer(size_t num_bytes);
PlatformSharedBuffer(size_t num_bytes, bool read_only);
~PlatformSharedBuffer();

// This is called by |Create()| before this object is given to anyone.
Expand All @@ -100,6 +105,7 @@ class MOJO_SYSTEM_IMPL_EXPORT PlatformSharedBuffer
void InitFromSharedMemoryHandle(base::SharedMemoryHandle handle);

const size_t num_bytes_;
const bool read_only_;

base::Lock lock_;
scoped_ptr<base::SharedMemory> shared_memory_;
Expand All @@ -126,12 +132,13 @@ class MOJO_SYSTEM_IMPL_EXPORT PlatformSharedBufferMapping {
friend class PlatformSharedBuffer;

PlatformSharedBufferMapping(base::SharedMemoryHandle handle,
bool read_only,
size_t offset,
size_t length)
: offset_(offset),
length_(length),
base_(nullptr),
shared_memory_(handle, false) {}
shared_memory_(handle, read_only) {}

bool Map();
void Unmap();
Expand Down
3 changes: 2 additions & 1 deletion mojo/edk/system/broker_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ scoped_refptr<PlatformSharedBuffer> Broker::GetSharedBuffer(size_t num_bytes) {
BrokerMessageType::BUFFER_RESPONSE, 1,
&incoming_platform_handles)) {
return PlatformSharedBuffer::CreateFromPlatformHandle(
num_bytes, ScopedPlatformHandle(incoming_platform_handles.front()));
num_bytes, false /* read_only */,
ScopedPlatformHandle(incoming_platform_handles.front()));
}

return nullptr;
Expand Down
3 changes: 1 addition & 2 deletions mojo/edk/system/core.cc
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ MojoResult Core::CreateSharedBufferWrapper(
bool read_only,
MojoHandle* mojo_wrapper_handle) {
DCHECK(num_bytes);
CHECK(!read_only);
scoped_refptr<PlatformSharedBuffer> platform_buffer =
PlatformSharedBuffer::CreateFromSharedMemoryHandle(num_bytes, read_only,
shared_memory_handle);
Expand Down Expand Up @@ -214,7 +213,7 @@ MojoResult Core::PassSharedMemoryHandle(
if (num_bytes)
*num_bytes = platform_shared_buffer->GetNumBytes();
if (read_only)
*read_only = false;
*read_only = platform_shared_buffer->IsReadOnly();
*shared_memory_handle = platform_shared_buffer->DuplicateSharedMemoryHandle();

shm_dispatcher->Close();
Expand Down
1 change: 1 addition & 0 deletions mojo/edk/system/data_pipe_consumer_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ DataPipeConsumerDispatcher::Deserialize(const void* data,
scoped_refptr<PlatformSharedBuffer> ring_buffer =
PlatformSharedBuffer::CreateFromPlatformHandle(
state->options.capacity_num_bytes,
false /* read_only */,
ScopedPlatformHandle(buffer_handle));
if (!ring_buffer) {
DLOG(ERROR) << "Failed to deserialize shared buffer handle.";
Expand Down
1 change: 1 addition & 0 deletions mojo/edk/system/data_pipe_producer_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ DataPipeProducerDispatcher::Deserialize(const void* data,
scoped_refptr<PlatformSharedBuffer> ring_buffer =
PlatformSharedBuffer::CreateFromPlatformHandle(
state->options.capacity_num_bytes,
false /* read_only */,
ScopedPlatformHandle(buffer_handle));
if (!ring_buffer) {
DLOG(ERROR) << "Failed to deserialize shared buffer handle.";
Expand Down
31 changes: 28 additions & 3 deletions mojo/edk/system/shared_buffer_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,12 @@ namespace {

struct SerializedState {
uint64_t num_bytes;
uint32_t flags;
uint32_t padding;
};

const uint32_t kSerializedStateFlagsReadOnly = 1 << 0;

#pragma pack(pop)

static_assert(sizeof(SerializedState) % 8 == 0,
Expand Down Expand Up @@ -141,9 +145,10 @@ scoped_refptr<SharedBufferDispatcher> SharedBufferDispatcher::Deserialize(

// Wrapping |platform_handle| in a |ScopedPlatformHandle| means that it'll be
// closed even if creation fails.
bool read_only = (serialization->flags & kSerializedStateFlagsReadOnly);
scoped_refptr<PlatformSharedBuffer> shared_buffer(
PlatformSharedBuffer::CreateFromPlatformHandle(
static_cast<size_t>(serialization->num_bytes),
static_cast<size_t>(serialization->num_bytes), read_only,
ScopedPlatformHandle(platform_handle)));
if (!shared_buffer) {
LOG(ERROR)
Expand Down Expand Up @@ -190,6 +195,21 @@ MojoResult SharedBufferDispatcher::DuplicateBufferHandle(
base::AutoLock lock(lock_);
if (in_transit_)
return MOJO_RESULT_INVALID_ARGUMENT;

if ((validated_options.flags &
MOJO_DUPLICATE_BUFFER_HANDLE_OPTIONS_FLAG_READ_ONLY) &&
(!shared_buffer_->IsReadOnly())) {
// If a read-only duplicate is requested and |shared_buffer_| is not
// read-only, make a read-only duplicate of |shared_buffer_|.
scoped_refptr<PlatformSharedBuffer> read_only_buffer =
shared_buffer_->CreateReadOnlyDuplicate();
if (!read_only_buffer)
return MOJO_RESULT_FAILED_PRECONDITION;
DCHECK(read_only_buffer->IsReadOnly());
*new_dispatcher = CreateInternal(std::move(read_only_buffer));
return MOJO_RESULT_OK;
}

*new_dispatcher = CreateInternal(shared_buffer_);
return MOJO_RESULT_OK;
}
Expand All @@ -215,8 +235,10 @@ MojoResult SharedBufferDispatcher::MapBuffer(
DCHECK(mapping);
*mapping = shared_buffer_->MapNoCheck(static_cast<size_t>(offset),
static_cast<size_t>(num_bytes));
if (!*mapping)
if (!*mapping) {
LOG(ERROR) << "Unable to map: read_only" << shared_buffer_->IsReadOnly();
return MOJO_RESULT_RESOURCE_EXHAUSTED;
}

return MOJO_RESULT_OK;
}
Expand All @@ -237,6 +259,9 @@ bool SharedBufferDispatcher::EndSerialize(void* destination,
base::AutoLock lock(lock_);
serialization->num_bytes =
static_cast<uint64_t>(shared_buffer_->GetNumBytes());
serialization->flags =
(shared_buffer_->IsReadOnly() ? kSerializedStateFlagsReadOnly : 0);
serialization->padding = 0;

handle_for_transit_ = shared_buffer_->DuplicatePlatformHandle();
if (!handle_for_transit_.is_valid()) {
Expand Down Expand Up @@ -283,7 +308,7 @@ MojoResult SharedBufferDispatcher::ValidateDuplicateOptions(
const MojoDuplicateBufferHandleOptions* in_options,
MojoDuplicateBufferHandleOptions* out_options) {
const MojoDuplicateBufferHandleOptionsFlags kKnownFlags =
MOJO_DUPLICATE_BUFFER_HANDLE_OPTIONS_FLAG_NONE;
MOJO_DUPLICATE_BUFFER_HANDLE_OPTIONS_FLAG_READ_ONLY;
static const MojoDuplicateBufferHandleOptions kDefaultOptions = {
static_cast<uint32_t>(sizeof(MojoDuplicateBufferHandleOptions)),
MOJO_DUPLICATE_BUFFER_HANDLE_OPTIONS_FLAG_NONE};
Expand Down
6 changes: 6 additions & 0 deletions mojo/edk/system/shared_buffer_dispatcher_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,19 @@ TEST_F(SharedBufferDispatcherTest, DuplicateBufferHandleOptionsValid) {
MojoDuplicateBufferHandleOptions options[] = {
{sizeof(MojoDuplicateBufferHandleOptions),
MOJO_DUPLICATE_BUFFER_HANDLE_OPTIONS_FLAG_NONE},
{sizeof(MojoDuplicateBufferHandleOptions),
MOJO_DUPLICATE_BUFFER_HANDLE_OPTIONS_FLAG_READ_ONLY},
{sizeof(MojoDuplicateBufferHandleOptionsFlags), ~0u}};
for (size_t i = 0; i < arraysize(options); i++) {
scoped_refptr<Dispatcher> dispatcher2;
EXPECT_EQ(MOJO_RESULT_OK, dispatcher1->DuplicateBufferHandle(
&options[i], &dispatcher2));
ASSERT_TRUE(dispatcher2);
EXPECT_EQ(Dispatcher::Type::SHARED_BUFFER, dispatcher2->GetType());
{
scoped_ptr<PlatformSharedBufferMapping> mapping;
EXPECT_EQ(MOJO_RESULT_OK, dispatcher2->MapBuffer(0, 100, 0, &mapping));
}
EXPECT_EQ(MOJO_RESULT_OK, dispatcher2->Close());
}

Expand Down
Loading

0 comments on commit ee191a4

Please sign in to comment.