Skip to content

Commit

Permalink
[PartitionAlloc] Disallow committing when PageInaccessible is used
Browse files Browse the repository at this point in the history
Until now, the functionality of requesting pages that are committed and
PageInaccessible existed only on Windows.
|AllocPages(..., PageInaccessible, ..., true)| led to
|VirtualAlloc(..., MEM_RESERVE|MEM_COMMIT, PAGE_NOACCESS)| which would
allocate memory charges without really backing virtual address space
with physical pages.
https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualalloc

This is inconsistent with |base::SetSystemPagesAccess| where
|PageInaccessible| always leads to |VirtualFree(..., MEM_DECOMMIT)|

Outside of unit tests, |AllocPages(..., PageInaccessible, ..., true)|
was used only in a couple places in code, likely accidentally.

Change-Id: I657787b4785e889984c5895518e9abbc26b50a2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2522934
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Commit-Queue: Bartek Nowierski <bartekn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826105}
  • Loading branch information
bartekn-chromium authored and Commit Bot committed Nov 11, 2020
1 parent f9c9b51 commit c2f43fb
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 11 deletions.
3 changes: 3 additions & 0 deletions base/allocator/partition_allocator/page_allocator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,10 @@ void* SystemAllocPages(void* hint,
PA_DCHECK(!(length & PageAllocationGranularityOffsetMask()));
PA_DCHECK(!(reinterpret_cast<uintptr_t>(hint) &
PageAllocationGranularityOffsetMask()));
// TODO(bartekn): Remove the commit functionality, as it is equivalent to
// |accessibility != PageInaccessible|.
PA_DCHECK(commit || accessibility == PageInaccessible);
PA_DCHECK(!commit || accessibility != PageInaccessible);
void* ptr =
SystemAllocPagesInternal(hint, length, accessibility, page_tag, commit);
if (ptr)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ void* SystemAllocPagesInternal(void* hint,
PageTag page_tag,
bool commit) {
DWORD access_flag = GetAccessFlags(accessibility);
if (commit) {
PA_DCHECK(access_flag != PAGE_NOACCESS);
} else {
PA_DCHECK(access_flag == PAGE_NOACCESS);
}

const DWORD type_flags = commit ? (MEM_RESERVE | MEM_COMMIT) : MEM_RESERVE;
void* ret = VirtualAlloc(hint, length, type_flags, access_flag);
if (ret == nullptr) {
Expand Down
6 changes: 3 additions & 3 deletions base/allocator/partition_allocator/page_allocator_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ void SignalHandler(int signal, siginfo_t* info, void*) {
TEST(PageAllocatorTest, InaccessiblePages) {
void* buffer = AllocPages(nullptr, PageAllocationGranularity(),
PageAllocationGranularity(), PageInaccessible,
PageTag::kChromium, true);
PageTag::kChromium, false);
EXPECT_TRUE(buffer);

FAULT_TEST_BEGIN()
Expand Down Expand Up @@ -232,7 +232,7 @@ TEST(PageAllocatorTest, ReadExecutePages) {
TEST(PageAllocatorTest, PageTagging) {
void* buffer = AllocPages(nullptr, PageAllocationGranularity(),
PageAllocationGranularity(), PageInaccessible,
PageTag::kChromium, true);
PageTag::kChromium, false);
EXPECT_TRUE(buffer);

std::string proc_maps;
Expand Down Expand Up @@ -285,7 +285,7 @@ TEST(PageAllocatorTest, MappedPagesAccounting) {
// Ask for a large alignment to make sure that trimming doesn't change the
// accounting.
void* data = AllocPages(nullptr, size, 128 * PageAllocationGranularity(),
PageInaccessible, PageTag::kChromium, true);
PageInaccessible, PageTag::kChromium, false);
ASSERT_TRUE(data);

EXPECT_EQ(mapped_size_before + size, GetTotalMappedSize());
Expand Down
14 changes: 8 additions & 6 deletions base/allocator/partition_allocator/partition_alloc_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1294,13 +1294,15 @@ TEST_F(PartitionAllocTest, MappingCollision) {
page_base -= PartitionPageSize() - ReservedTagBitmapSize();
// Map a single system page either side of the mapping for our allocations,
// with the goal of tripping up alignment of the next mapping.
void* map1 = AllocPages(
page_base - PageAllocationGranularity(), PageAllocationGranularity(),
PageAllocationGranularity(), PageInaccessible, PageTag::kPartitionAlloc);
void* map1 =
AllocPages(page_base - PageAllocationGranularity(),
PageAllocationGranularity(), PageAllocationGranularity(),
PageInaccessible, PageTag::kPartitionAlloc, false);
EXPECT_TRUE(map1);
void* map2 = AllocPages(
page_base + kSuperPageSize, PageAllocationGranularity(),
PageAllocationGranularity(), PageInaccessible, PageTag::kPartitionAlloc);
void* map2 =
AllocPages(page_base + kSuperPageSize, PageAllocationGranularity(),
PageAllocationGranularity(), PageInaccessible,
PageTag::kPartitionAlloc, false);
EXPECT_TRUE(map2);

for (i = 0; i < num_partition_pages_needed; ++i)
Expand Down
2 changes: 1 addition & 1 deletion third_party/blink/renderer/platform/heap/impl/gc_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ void GCInfoTable::Resize() {
GCInfoTable::GCInfoTable() {
table_ = reinterpret_cast<GCInfo const**>(base::AllocPages(
nullptr, MaxTableSize(), base::PageAllocationGranularity(),
base::PageInaccessible, base::PageTag::kBlinkGC));
base::PageInaccessible, base::PageTag::kBlinkGC, false));
CHECK(table_);
Resize();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ PageMemoryRegion* PageMemoryRegion::Allocate(size_t size,
size = base::RoundUpToPageAllocationGranularity(size);
Address base = static_cast<Address>(
base::AllocPages(nullptr, size, kBlinkPageSize, base::PageInaccessible,
base::PageTag::kBlinkGC));
base::PageTag::kBlinkGC, false));
if (!base)
BlinkGCOutOfMemory();
return new PageMemoryRegion(base, size, num_pages, region_tree);
Expand Down

0 comments on commit c2f43fb

Please sign in to comment.