commit d5f0d5c090198500aa8f46c3586403bd7e89f508
parent 6cf0a635af6bec239e165d05d306e54aeb6a62ae
Author: Yannis Juglaret <yjuglaret@mozilla.com>
Date: Wed, 12 Nov 2025 10:56:04 +0000
Bug 1999243 - Remove redundant read/execute memory commits. r=bobowen
As identified by :bobowen, when using a shared mapping it is redundant
to commit memory through multiple views. Committing memory in one of the
views propagates commitment to the other views.
This patch removes the redundant commits and adds a test that ensures
that we can rely on this behavior.
Differential Revision: https://phabricator.services.mozilla.com/D271988
Diffstat:
6 files changed, 146 insertions(+), 20 deletions(-)
diff --git a/mozglue/tests/TestSharedMappingCommit.cpp b/mozglue/tests/TestSharedMappingCommit.cpp
@@ -0,0 +1,129 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#include <cstdint>
+#include <cstdio>
+#include <windows.h>
+
+// Tests that when there are multiple views to a unique mapping, committing
+// memory pages for one of the views actually commits them for all views. We
+// rely on this behavior in `mozglue/interceptor/MMPolicies.h` and in
+// `security/sandbox/chromium/sandbox/win/src/interception.cc`.
+bool TestSharedMappingCommit() {
+ constexpr size_t kMappingSize = 64 * 1024;
+ constexpr size_t kCommitOffset = 32 * 1024;
+ constexpr size_t kCommitSize = 4 * 1024;
+
+ HANDLE mapping = ::CreateFileMappingW(INVALID_HANDLE_VALUE, nullptr,
+ PAGE_EXECUTE_READWRITE | SEC_RESERVE, 0,
+ kMappingSize, nullptr);
+ if (!mapping) {
+ printf(
+ "TEST-FAIL | SharedMappingCommit | Failed to create a pagefile-backed "
+ "mapping\n");
+ return false;
+ }
+
+ void* rwView =
+ ::MapViewOfFile(mapping, FILE_MAP_READ | FILE_MAP_WRITE, 0, 0, 0);
+ if (!rwView) {
+ printf(
+ "TEST-FAIL | SharedMappingCommit | Failed to get a read/write view\n");
+ return false;
+ }
+
+ void* rxView =
+ ::MapViewOfFile(mapping, FILE_MAP_READ | FILE_MAP_EXECUTE, 0, 0, 0);
+ if (!rxView) {
+ printf(
+ "TEST-FAIL | SharedMappingCommit | Failed to get a read/execute "
+ "view\n");
+ return false;
+ }
+
+ auto* rwCommitAddress =
+ static_cast<void*>(static_cast<uint8_t*>(rwView) + kCommitOffset);
+ auto* rxCommitAddress =
+ static_cast<void*>(static_cast<uint8_t*>(rxView) + kCommitOffset);
+
+ {
+ MEMORY_BASIC_INFORMATION info{};
+ if (::VirtualQuery(rxCommitAddress, &info, sizeof(info)) != sizeof(info)) {
+ printf(
+ "TEST-FAIL | SharedMappingCommit | Failed to query basic information "
+ "about the read/execute memory pages\n");
+ return false;
+ }
+ if (info.State != MEM_RESERVE) {
+ printf(
+ "TEST-FAIL | SharedMappingCommit | Unexpected initial state for the "
+ "read/execute memory pages: %lu\n",
+ info.State);
+ return false;
+ }
+ }
+
+ rwCommitAddress = ::VirtualAlloc(static_cast<void*>(rwCommitAddress),
+ kCommitSize, MEM_COMMIT, PAGE_READWRITE);
+ if (!rwCommitAddress) {
+ printf(
+ "TEST-FAIL | SharedMappingCommit | Failed to commit read/write "
+ "memory pages\n");
+ return false;
+ }
+
+ {
+ MEMORY_BASIC_INFORMATION info{};
+ if (::VirtualQuery(rxCommitAddress, &info, sizeof(info)) != sizeof(info)) {
+ printf(
+ "TEST-FAIL | SharedMappingCommit | Failed to query basic information "
+ "about the read/execute memory pages\n");
+ return false;
+ }
+ if (info.State != MEM_COMMIT) {
+ printf(
+ "TEST-FAIL | SharedMappingCommit | Read/write commit hasn't changed "
+ "the state of the read/execute memory pages: %lu\n",
+ info.State);
+ return false;
+ }
+ if (info.Protect != PAGE_EXECUTE_READ) {
+ printf(
+ "TEST-FAIL | SharedMappingCommit | Unexpected protection for the "
+ "read/execute memory pages: %lu\n",
+ info.Protect);
+ return false;
+ }
+ }
+
+ constexpr uint32_t kMagic = 0xdeadbeef;
+
+ uint32_t* rwMagicAddr = static_cast<uint32_t*>(rwCommitAddress);
+ *rwMagicAddr = kMagic;
+
+ const uint32_t* rxMagicAddr = static_cast<const uint32_t*>(rxCommitAddress);
+ if (*rxMagicAddr != kMagic) {
+ printf(
+ "TEST-FAIL | SharedMappingCommit | Read a different value through the "
+ "read/execute view than was written through the read/write view\n");
+ return false;
+ }
+
+ printf(
+ "TEST-PASS | SharedMappingCommit | Committing memory pages through the "
+ "read/write view also commited the corresponding pages in the "
+ "read/execute view\n");
+ return true;
+}
+
+int wmain(int argc, wchar_t* argv[]) {
+ if (!TestSharedMappingCommit()) {
+ return 1;
+ }
+
+ printf("TEST-PASS | SharedMappingCommit | All tests ran successfully\n");
+ return 0;
+}
diff --git a/mozglue/tests/moz.build b/mozglue/tests/moz.build
@@ -32,6 +32,7 @@ if CONFIG["OS_ARCH"] == "WINNT":
[
"TestNativeNt",
"TestPEExportSection",
+ "TestSharedMappingCommit",
"TestStackCookie",
"TestTimeStampWin",
],
diff --git a/security/sandbox/chromium-shim/patches/50_use_rw_rx_mapping_for_child_init.patch b/security/sandbox/chromium-shim/patches/50_use_rw_rx_mapping_for_child_init.patch
@@ -13,14 +13,14 @@ We also delete ServiceResolverThunk::CopyThunk which is dead code.
---
sandbox/win/src/eat_resolver.cc | 9 ++-
sandbox/win/src/eat_resolver.h | 1 +
- sandbox/win/src/interception.cc | 57 ++++++++++++-------
+ sandbox/win/src/interception.cc | 56 +++++++++++--------
sandbox/win/src/interception_agent.cc | 2 +-
sandbox/win/src/resolver.cc | 4 +-
sandbox/win/src/resolver.h | 2 +
sandbox/win/src/service_resolver.h | 10 +---
- sandbox/win/src/service_resolver_32.cc | 50 ++--------------
+ sandbox/win/src/service_resolver_32.cc | 50 ++---------------
sandbox/win/src/service_resolver_64.cc | 49 ++++------------
- 9 files changed, 65 insertions(+), 119 deletions(-)
+ 9 files changed, 64 insertions(+), 119 deletions(-)
diff --git a/sandbox/win/src/eat_resolver.cc b/sandbox/win/src/eat_resolver.cc
index 97f77c1849d2..e1dc18a241bb 100644
@@ -58,7 +58,7 @@ index 924354e447a3..9d45c2863942 100644
size_t storage_bytes,
size_t* storage_used) override;
diff --git a/sandbox/win/src/interception.cc b/sandbox/win/src/interception.cc
-index 04f32b099646..9fef6728eb3e 100644
+index 04f32b099646..4a5e06b35d84 100644
--- a/sandbox/win/src/interception.cc
+++ b/sandbox/win/src/interception.cc
@@ -28,6 +28,8 @@
@@ -100,7 +100,7 @@ index 04f32b099646..9fef6728eb3e 100644
// Find an aligned, random location within the reserved range.
size_t thunk_bytes =
-@@ -381,19 +402,24 @@ ResultCode InterceptionManager::PatchNtdll(bool hot_patch_needed) {
+@@ -381,19 +402,23 @@ ResultCode InterceptionManager::PatchNtdll(bool hot_patch_needed) {
size_t thunk_offset = internal::GetGranularAlignedRandomOffset(thunk_bytes);
// Split the base and offset along page boundaries.
@@ -115,12 +115,11 @@ index 04f32b099646..9fef6728eb3e 100644
- ::VirtualAllocEx(child, thunk_base, thunk_bytes_padded, MEM_COMMIT,
- PAGE_EXECUTE_READWRITE));
- CHECK(thunk_base); // If this fails we'd crash anyway on an invalid access.
++ // MOZ: Committing RW pages in the parent also commits the corresponding RX
++ // pages in the child (see TestSharedMappingCommit).
+ local_thunk_base = reinterpret_cast<BYTE*>(::VirtualAlloc(
+ local_thunk_base, thunk_bytes_padded, MEM_COMMIT, PAGE_READWRITE));
+ CHECK(local_thunk_base);
-+ thunk_base = reinterpret_cast<BYTE*>(::VirtualAllocEx(
-+ child, thunk_base, thunk_bytes_padded, MEM_COMMIT, PAGE_EXECUTE_READ));
-+ CHECK(thunk_base);
+
DllInterceptionData* thunks =
reinterpret_cast<DllInterceptionData*>(thunk_base + thunk_offset);
@@ -131,7 +130,7 @@ index 04f32b099646..9fef6728eb3e 100644
dll_data.data_bytes = thunk_bytes;
dll_data.num_thunks = 0;
dll_data.used_bytes = offsetof(DllInterceptionData, thunks);
-@@ -407,20 +433,6 @@ ResultCode InterceptionManager::PatchNtdll(bool hot_patch_needed) {
+@@ -407,20 +432,6 @@ ResultCode InterceptionManager::PatchNtdll(bool hot_patch_needed) {
if (rc != SBOX_ALL_OK)
return rc;
@@ -152,7 +151,7 @@ index 04f32b099646..9fef6728eb3e 100644
ResultCode ret =
child_->TransferVariable("g_originals", g_originals, sizeof(g_originals));
return ret;
-@@ -475,6 +487,7 @@ ResultCode InterceptionManager::PatchClientFunctions(
+@@ -475,6 +486,7 @@ ResultCode InterceptionManager::PatchClientFunctions(
NTSTATUS ret = thunk.Setup(
ntdll_base, interceptor_base, interception.function.c_str(),
interception.interceptor.c_str(), interception.interceptor_address,
diff --git a/security/sandbox/chromium/sandbox/win/src/interception.cc b/security/sandbox/chromium/sandbox/win/src/interception.cc
@@ -408,12 +408,11 @@ ResultCode InterceptionManager::PatchNtdll(bool hot_patch_needed) {
// Make an aligned, padded allocation, and move the pointer to our chunk.
size_t thunk_bytes_padded = base::bits::AlignUp(thunk_bytes, kPageSize);
+ // MOZ: Committing RW pages in the parent also commits the corresponding RX
+ // pages in the child (see TestSharedMappingCommit).
local_thunk_base = reinterpret_cast<BYTE*>(::VirtualAlloc(
local_thunk_base, thunk_bytes_padded, MEM_COMMIT, PAGE_READWRITE));
CHECK(local_thunk_base);
- thunk_base = reinterpret_cast<BYTE*>(::VirtualAllocEx(
- child, thunk_base, thunk_bytes_padded, MEM_COMMIT, PAGE_EXECUTE_READ));
- CHECK(thunk_base);
DllInterceptionData* thunks =
reinterpret_cast<DllInterceptionData*>(thunk_base + thunk_offset);
diff --git a/testing/cppunittest.toml b/testing/cppunittest.toml
@@ -159,6 +159,9 @@ run-if = ["os == 'win'"]
["TestSegmentedVector"]
+["TestSharedMappingCommit"]
+run-if = ["os == 'win'"]
+
["TestSmallPointerArray"]
["TestSplayTree"]
diff --git a/toolkit/xre/dllservices/mozglue/interceptor/MMPolicies.h b/toolkit/xre/dllservices/mozglue/interceptor/MMPolicies.h
@@ -977,19 +977,14 @@ class MMPolicyOutOfProcess : public MMPolicyBase {
return false;
}
+ // Committing RW pages in the parent also commits the corresponding RX pages
+ // in the child (see TestSharedMappingCommit).
PVOID local = ::VirtualAlloc(mLocalView + mCommitOffset, GetPageSize(),
MEM_COMMIT, PAGE_READWRITE);
if (!local) {
return false;
}
- PVOID remote = ::VirtualAllocEx(
- mProcess, static_cast<uint8_t*>(mRemoteView) + mCommitOffset,
- GetPageSize(), MEM_COMMIT, PAGE_EXECUTE_READ);
- if (!remote) {
- return false;
- }
-
mCommitOffset += GetPageSize();
return true;
}
