[vm] Fix freelist interaction with dual mapping
Freelist implementation was trying to mark pages executable (RX)
which does not respect possibility that it is working with RW mapping
which can't be directly marked as executable. It needed to use RO
permission instead if dual mapping is enabled.
This CL consolidates all code mentioning RX permission in one place:
VirtualMemory::WriteProtectCode so that we avoid this mistake in the future.
R=kustermann@google.com
TEST=tested by doing large reloads which cause more churn in code space
Change-Id: If99692cac3ce3ff54b907e4c43d7f26bae7439ff
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/435860
Commit-Queue: Martin Kustermann <kustermann@google.com>
Auto-Submit: Slava Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
diff --git a/runtime/vm/code_patcher.cc b/runtime/vm/code_patcher.cc
index 43786b4..2824ada 100644
--- a/runtime/vm/code_patcher.cc
+++ b/runtime/vm/code_patcher.cc
@@ -37,8 +37,7 @@
WritableInstructionsScope::~WritableInstructionsScope() {
if (FLAG_write_protect_code) {
- VirtualMemory::Protect(reinterpret_cast<void*>(address_), size_,
- VirtualMemory::kReadExecute);
+ VirtualMemory::WriteProtectCode(reinterpret_cast<void*>(address_), size_);
}
}
#endif // defined(TARGET_ARCH_IA32)
diff --git a/runtime/vm/compiler/relocation_test.cc b/runtime/vm/compiler/relocation_test.cc
index f5b1619..754a819 100644
--- a/runtime/vm/compiler/relocation_test.cc
+++ b/runtime/vm/compiler/relocation_test.cc
@@ -263,8 +263,7 @@
ASSERT(!VirtualMemory::ShouldDualMapExecutablePages());
const uword address = UntaggedObject::ToAddr(instructions.ptr());
const auto size = instructions.ptr()->untag()->HeapSize();
- VirtualMemory::Protect(reinterpret_cast<void*>(address), size,
- VirtualMemory::kReadExecute);
+ VirtualMemory::WriteProtectCode(reinterpret_cast<void*>(address), size);
}
CPU::FlushICache(instructions.PayloadStart(), instructions.Size());
}
diff --git a/runtime/vm/ffi_callback_metadata.cc b/runtime/vm/ffi_callback_metadata.cc
index 4e78cd7..a0c8dd1 100644
--- a/runtime/vm/ffi_callback_metadata.cc
+++ b/runtime/vm/ffi_callback_metadata.cc
@@ -187,16 +187,12 @@
memcpy(new_page->address(), stub_page_->address(), // NOLINT
stub_page_->size());
+ VirtualMemory::WriteProtectCode(new_page->address(), aligned_size);
if (VirtualMemory::ShouldDualMapExecutablePages()) {
ASSERT(new_page->OffsetToExecutableAlias() != 0);
- VirtualMemory::Protect(new_page->address(), aligned_size,
- VirtualMemory::kReadOnly);
VirtualMemory::Protect(
reinterpret_cast<void*>(RXAreaStart(new_page) + RXMappingSize()),
RWMappingSize(), VirtualMemory::kReadWrite);
- } else {
- VirtualMemory::Protect(new_page->address(), aligned_size,
- VirtualMemory::kReadExecute);
}
}
diff --git a/runtime/vm/heap/freelist.cc b/runtime/vm/heap/freelist.cc
index 2292d37..a0c52a1 100644
--- a/runtime/vm/heap/freelist.cc
+++ b/runtime/vm/heap/freelist.cc
@@ -172,8 +172,8 @@
}
previous->set_next(current->next());
if (target_is_protected) {
- VirtualMemory::Protect(reinterpret_cast<void*>(target_address),
- kWordSize, VirtualMemory::kReadExecute);
+ VirtualMemory::WriteProtectCode(
+ reinterpret_cast<void*>(target_address), kWordSize);
}
}
SplitElementAfterAndEnqueue(current, size, is_protected);
@@ -266,12 +266,11 @@
if (!VirtualMemory::InSamePage(
remainder_address - 1,
remainder_address + remainder_header_size - 1)) {
- VirtualMemory::Protect(
+ VirtualMemory::WriteProtectCode(
reinterpret_cast<void*>(
Utils::RoundUp(remainder_address, VirtualMemory::PageSize())),
remainder_address + remainder_header_size -
- Utils::RoundUp(remainder_address, VirtualMemory::PageSize()),
- VirtualMemory::kReadExecute);
+ Utils::RoundUp(remainder_address, VirtualMemory::PageSize()));
}
}
}
diff --git a/runtime/vm/heap/freelist_test.cc b/runtime/vm/heap/freelist_test.cc
index 9fe8ecd..5379474 100644
--- a/runtime/vm/heap/freelist_test.cc
+++ b/runtime/vm/heap/freelist_test.cc
@@ -14,8 +14,7 @@
static uword Allocate(FreeList* free_list, intptr_t size, bool is_protected) {
uword result = free_list->TryAllocate(size, is_protected);
if ((result != 0u) && is_protected) {
- VirtualMemory::Protect(reinterpret_cast<void*>(result), size,
- VirtualMemory::kReadExecute);
+ VirtualMemory::WriteProtectCode(reinterpret_cast<void*>(result), size);
}
return result;
}
@@ -30,8 +29,7 @@
}
free_list->Free(address, size);
if (is_protected) {
- VirtualMemory::Protect(reinterpret_cast<void*>(address), size,
- VirtualMemory::kReadExecute);
+ VirtualMemory::WriteProtectCode(reinterpret_cast<void*>(address), size);
}
}
@@ -47,7 +45,7 @@
if (is_protected) {
// Write protect the whole region.
- region->Protect(VirtualMemory::kReadExecute);
+ region->WriteProtectCode();
}
// Allocate a small object. Expect it to be positioned as the first element.
@@ -122,7 +120,7 @@
free_list->Free(blob->start(), blob->size());
// Write protect the whole region.
- blob->Protect(VirtualMemory::kReadExecute);
+ blob->WriteProtectCode();
// Allocate small objects.
for (intptr_t i = 0; i < blob->size() / kObjectSize; i++) {
@@ -221,10 +219,9 @@
memcpy(other_code, ret, sizeof(ret)); // NOLINT
free_list->Free(blob->start(), alloc_size + remainder_size);
- blob->Protect(VirtualMemory::kReadExecute); // not writable
+ blob->WriteProtectCode(); // not writable
Allocate(free_list.get(), alloc_size, /*is_protected=*/true);
- VirtualMemory::Protect(blob->address(), alloc_size,
- VirtualMemory::kReadExecute);
+ VirtualMemory::WriteProtectCode(blob->address(), alloc_size);
reinterpret_cast<void (*)()>(other_code)();
}
diff --git a/runtime/vm/heap/page.cc b/runtime/vm/heap/page.cc
index 8a18ff4..5480e7e 100644
--- a/runtime/vm/heap/page.cc
+++ b/runtime/vm/heap/page.cc
@@ -293,19 +293,13 @@
void Page::WriteProtect(bool read_only) {
ASSERT(!is_image());
- VirtualMemory::Protection prot;
- if (read_only) {
- // When dual mapping code pages we don't change protection on RX page, but
- // flip RW to R and back.
- if (is_executable() && !VirtualMemory::ShouldDualMapExecutablePages()) {
- prot = VirtualMemory::kReadExecute;
- } else {
- prot = VirtualMemory::kReadOnly;
- }
+ if (is_executable() && read_only) {
+ // Handle making code executable in a special way.
+ memory_->WriteProtectCode();
} else {
- prot = VirtualMemory::kReadWrite;
+ memory_->Protect(read_only ? VirtualMemory::kReadOnly
+ : VirtualMemory::kReadWrite);
}
- memory_->Protect(prot);
}
} // namespace dart
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index c27c12b..fecf08f 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -18746,11 +18746,8 @@
// RX mapping never changes protection while RW mapping flips between
// R and RW.
uword address = UntaggedObject::ToAddr(instrs.ptr());
- VirtualMemory::Protect(reinterpret_cast<void*>(address),
- instrs.ptr()->untag()->HeapSize(),
- VirtualMemory::ShouldDualMapExecutablePages()
- ? VirtualMemory::kReadOnly
- : VirtualMemory::kReadExecute);
+ VirtualMemory::WriteProtectCode(reinterpret_cast<void*>(address),
+ instrs.ptr()->untag()->HeapSize());
}
// Hook up Code and Instructions objects.
diff --git a/runtime/vm/virtual_memory.h b/runtime/vm/virtual_memory.h
index c915286..353dc91 100644
--- a/runtime/vm/virtual_memory.h
+++ b/runtime/vm/virtual_memory.h
@@ -80,6 +80,15 @@
#endif
}
+ // Write protect a chunk of machine code which is currently writable.
+ DART_FORCE_INLINE static void WriteProtectCode(void* address, intptr_t size) {
+ Protect(address, size,
+ ShouldDualMapExecutablePages() ? kReadOnly : kReadExecute);
+ }
+ DART_FORCE_INLINE void WriteProtectCode() const {
+ WriteProtectCode(address(), size());
+ }
+
bool Contains(uword addr) const { return region_.Contains(addr); }
// Changes the protection of the virtual memory area.
diff --git a/runtime/vm/virtual_memory_posix.cc b/runtime/vm/virtual_memory_posix.cc
index 87adb69..4baad99 100644
--- a/runtime/vm/virtual_memory_posix.cc
+++ b/runtime/vm/virtual_memory_posix.cc
@@ -493,9 +493,7 @@
0xd65f03c0 // ret
};
memmove(mem->address(), kSquareFunctionCode, sizeof(kSquareFunctionCode));
- mem->Protect(VirtualMemory::ShouldDualMapExecutablePages()
- ? VirtualMemory::kReadOnly
- : VirtualMemory::kReadExecute);
+ VirtualMemory::WriteProtectCode(mem->address(), mem->size());
// Get executable entry point and check that write have succeeded.
const uword entry_point = mem->start() + mem->OffsetToExecutableAlias();
