Skip to content

Commit

Permalink
Bug 1550900 - Support "frozen" shared memory where the parent process…
Browse files Browse the repository at this point in the history
… retains write access. r=froydnj

This patch extends shared memory freezing to support the use case where
the parent process retains write access for incremental updates, while
other processes receive read-only access.

Note that, while some OSes allow independent read-only and read/write
capabilities for the same object, all we have on Android is an operation
that prevents future write mappings.  Therefore, this allows an existing
writeable mapping to be retained, but if that is unmapped then even the
parent process can't re-create it.

As with freezing, the read-only restriction may not be enforceable if
the recipient process isn't adequately sandboxed (e.g., on Linux, if it
can use /proc/self/fd to reopen the inode for writing).

Differential Revision: https://phabricator.services.mozilla.com/D67187

--HG--
extra : moz-landing-system : lando
  • Loading branch information
jld committed Mar 20, 2020
1 parent 0e85250 commit f9c2aef
Show file tree
Hide file tree
Showing 4 changed files with 174 additions and 10 deletions.
21 changes: 20 additions & 1 deletion ipc/chromium/src/base/shared_memory.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,26 @@ class SharedMemory {
//
// (See bug 1479960 comment #0 for OS-specific implementation
// details.)
MOZ_MUST_USE bool Freeze();
MOZ_MUST_USE bool Freeze() {
Unmap();
return ReadOnlyCopy(this);
}

// Similar to Freeze(), but assigns the read-only capability to
// another SharedMemory object and leaves this object's mapping in
// place and writeable. This can be used to broadcast data to
// several untrusted readers without copying.
//
// The other constraints of Freeze() still apply: this object closes
// its handle (as if by `Close(false)`), it cannot have been
// previously shared, and the read-only handle cannot be used to
// write the memory even by a malicious process.
//
// (The out parameter can currently be the same as `this` if and
// only if the memory was unmapped, but this is an implementation
// detail and shouldn't be relied on; for that use case Freeze()
// should be used instead.)
MOZ_MUST_USE bool ReadOnlyCopy(SharedMemory *ro_out);

// Closes the open shared memory segment.
// It is safe to call Close repeatedly.
Expand Down
23 changes: 18 additions & 5 deletions ipc/chromium/src/base/shared_memory_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -261,26 +261,39 @@ bool SharedMemory::CreateInternal(size_t size, bool freezeable) {
return true;
}

bool SharedMemory::Freeze() {
bool SharedMemory::ReadOnlyCopy(SharedMemory* ro_out) {
DCHECK(!read_only_);
CHECK(freezeable_);
Unmap();

if (ro_out == this) {
DCHECK(!memory_);
}

int ro_file = -1;
#ifdef ANDROID
if (mozilla::android::ashmem_setProt(mapped_file_, PROT_READ) != 0) {
CHROMIUM_LOG(WARNING) << "failed to freeze shm: " << strerror(errno);
CHROMIUM_LOG(WARNING) << "failed to set ashmem read-only: " << strerror(errno);
return false;
}
ro_file = mapped_file_;
#else
DCHECK(frozen_file_ >= 0);
DCHECK(mapped_file_ >= 0);
close(mapped_file_);
mapped_file_ = frozen_file_;
ro_file = frozen_file_;
frozen_file_ = -1;
#endif

read_only_ = true;
DCHECK(ro_file >= 0);
mapped_file_ = -1;
freezeable_ = false;

ro_out->Close();
ro_out->mapped_file_ = ro_file;
ro_out->max_size_ = max_size_;
ro_out->read_only_ = true;
ro_out->freezeable_ = false;

return true;
}

Expand Down
22 changes: 18 additions & 4 deletions ipc/chromium/src/base/shared_memory_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -157,19 +157,33 @@ bool SharedMemory::CreateInternal(size_t size, bool freezeable) {
return true;
}

bool SharedMemory::Freeze() {
bool SharedMemory::ReadOnlyCopy(SharedMemory* ro_out) {
DCHECK(!read_only_);
CHECK(freezeable_);
Unmap();

if (ro_out == this) {
DCHECK(!memory_);
}

HANDLE ro_handle;
if (!::DuplicateHandle(GetCurrentProcess(), mapped_file_, GetCurrentProcess(),
&mapped_file_, GENERIC_READ | FILE_MAP_READ, false,
&ro_handle, GENERIC_READ | FILE_MAP_READ, false,
DUPLICATE_CLOSE_SOURCE)) {
// DUPLICATE_CLOSE_SOURCE applies even if there is an error.
mapped_file_ = nullptr;
return false;
}

read_only_ = true;
mapped_file_ = nullptr;
freezeable_ = false;

ro_out->Close();
ro_out->mapped_file_ = ro_handle;
ro_out->max_size_ = max_size_;
ro_out->read_only_ = true;
ro_out->freezeable_ = false;
ro_out->external_section_ = external_section_;

return true;
}

Expand Down
118 changes: 118 additions & 0 deletions ipc/gtest/TestSharedMemory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,4 +143,122 @@ TEST(IPCSharedMemory, WinUnfreeze)
}
#endif

// Test that a read-only copy sees changes made to the writeable
// mapping in the case that the page wasn't accessed before the copy.
TEST(IPCSharedMemory, ROCopyAndWrite)
{
base::SharedMemory shmRW, shmRO;

// Create and initialize
ASSERT_TRUE(shmRW.CreateFreezeable(1));
ASSERT_TRUE(shmRW.Map(1));
auto memRW = reinterpret_cast<char*>(shmRW.memory());
ASSERT_TRUE(memRW);

// Create read-only copy
ASSERT_TRUE(shmRW.ReadOnlyCopy(&shmRO));
EXPECT_FALSE(shmRW.IsValid());
ASSERT_EQ(shmRW.memory(), memRW);
ASSERT_EQ(shmRO.max_size(), size_t(1));

// Map read-only
ASSERT_TRUE(shmRO.IsValid());
ASSERT_TRUE(shmRO.Map(1));
auto memRO = reinterpret_cast<const char*>(shmRO.memory());
ASSERT_TRUE(memRO);
ASSERT_NE(memRW, memRO);

// Check
*memRW = 'A';
EXPECT_EQ(*memRO, 'A');
}

// Test that a read-only copy sees changes made to the writeable
// mapping in the case that the page was accessed before the copy
// (and, before that, sees the state as of when the copy was made).
TEST(IPCSharedMemory, ROCopyAndRewrite)
{
base::SharedMemory shmRW, shmRO;

// Create and initialize
ASSERT_TRUE(shmRW.CreateFreezeable(1));
ASSERT_TRUE(shmRW.Map(1));
auto memRW = reinterpret_cast<char*>(shmRW.memory());
ASSERT_TRUE(memRW);
*memRW = 'A';

// Create read-only copy
ASSERT_TRUE(shmRW.ReadOnlyCopy(&shmRO));
EXPECT_FALSE(shmRW.IsValid());
ASSERT_EQ(shmRW.memory(), memRW);
ASSERT_EQ(shmRO.max_size(), size_t(1));

// Map read-only
ASSERT_TRUE(shmRO.IsValid());
ASSERT_TRUE(shmRO.Map(1));
auto memRO = reinterpret_cast<const char*>(shmRO.memory());
ASSERT_TRUE(memRO);
ASSERT_NE(memRW, memRO);

// Check
ASSERT_EQ(*memRW, 'A');
EXPECT_EQ(*memRO, 'A');
*memRW = 'X';
EXPECT_EQ(*memRO, 'X');
}

// See FreezeAndMapRW.
TEST(IPCSharedMemory, ROCopyAndMapRW)
{
base::SharedMemory shmRW, shmRO;

// Create and initialize
ASSERT_TRUE(shmRW.CreateFreezeable(1));
ASSERT_TRUE(shmRW.Map(1));
auto memRW = reinterpret_cast<char*>(shmRW.memory());
ASSERT_TRUE(memRW);
*memRW = 'A';

// Create read-only copy
ASSERT_TRUE(shmRW.ReadOnlyCopy(&shmRO));
ASSERT_TRUE(shmRO.IsValid());

// Re-create as writeable
auto handle = base::SharedMemory::NULLHandle();
ASSERT_TRUE(shmRO.GiveToProcess(base::GetCurrentProcId(), &handle));
ASSERT_TRUE(shmRO.IsHandleValid(handle));
ASSERT_FALSE(shmRO.IsValid());
ASSERT_TRUE(shmRO.SetHandle(handle, /* read-only */ false));
ASSERT_TRUE(shmRO.IsValid());

// This should fail
EXPECT_FALSE(shmRO.Map(1));
}

// See FreezeAndReprotect
TEST(IPCSharedMemory, ROCopyAndReprotect)
{
base::SharedMemory shmRW, shmRO;

// Create and initialize
ASSERT_TRUE(shmRW.CreateFreezeable(1));
ASSERT_TRUE(shmRW.Map(1));
auto memRW = reinterpret_cast<char*>(shmRW.memory());
ASSERT_TRUE(memRW);
*memRW = 'A';

// Create read-only copy
ASSERT_TRUE(shmRW.ReadOnlyCopy(&shmRO));
ASSERT_TRUE(shmRO.IsValid());

// Re-map
ASSERT_TRUE(shmRO.Map(1));
auto memRO = reinterpret_cast<char*>(shmRO.memory());
ASSERT_EQ(*memRO, 'A');

// Try to alter protection; should fail
EXPECT_FALSE(ipc::SharedMemory::SystemProtectFallible(
memRO, 1, ipc::SharedMemory::RightsReadWrite));
}

} // namespace mozilla

0 comments on commit f9c2aef

Please sign in to comment.