[base] Check the mprotect return code directly in SetSystemPagesAccess

SetSystemPagesAccess is more or less a wrapper around mprotect on posix
systems, and VirtualFree and VirtualAlloc on Windows.  We see crashes,
e.g. in the referenced issue, where the return value of mprotect is not
equal to 0. However, because we do not CHECK the return value of
mprotect directly, we cannot see the reason why mprotect failed. With
this CL I move the CHECK of the return value of mprotect to the
mprotect, so if the CHECK fails, we also see the error reason.

Some caller of SetSystemPagesAccess do not CHECK the result of
SetSystemPagesAccess but deal with it or forward it. For these callers I
introduced a new function, TrySetSystemPagesAccess, which does exactly
the same as SetSystemPagesAccess did until now.

Bug: chromium:839036
Change-Id: I774e648cc6968202805a495fc0b3c3b7d9974b02
Reviewed-on: https://chromium-review.googlesource.com/c/1336130Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608310}

base/allocator/partition_allocator/page_allocator.cc

@@ -199,11 +199,18 @@ void FreePages(void* address, size_t length) {
   FreePagesInternal(address, length);
 }
 
-bool SetSystemPagesAccess(void* address,
+bool TrySetSystemPagesAccess(void* address,
+                             size_t length,
+                             PageAccessibilityConfiguration accessibility) {
+  DCHECK(!(length & kSystemPageOffsetMask));
+  return TrySetSystemPagesAccessInternal(address, length, accessibility);
+}
+
+void SetSystemPagesAccess(void* address,
                           size_t length,
                           PageAccessibilityConfiguration accessibility) {
   DCHECK(!(length & kSystemPageOffsetMask));
-  return SetSystemPagesAccessInternal(address, length, accessibility);
+  SetSystemPagesAccessInternal(address, length, accessibility);
 }
 
 void DecommitSystemPages(void* address, size_t length) {

base/allocator/partition_allocator/page_allocator.h

@@ -68,7 +68,16 @@ BASE_EXPORT void FreePages(void* address, size_t length);
 //
 // Returns true if the permission change succeeded. In most cases you must
 // |CHECK| the result.
-BASE_EXPORT WARN_UNUSED_RESULT bool SetSystemPagesAccess(
+BASE_EXPORT WARN_UNUSED_RESULT bool TrySetSystemPagesAccess(
+    void* address,
+    size_t length,
+    PageAccessibilityConfiguration page_accessibility);
+
+// Mark one or more system pages, starting at |address| with the given
+// |page_accessibility|. |length| must be a multiple of |kSystemPageSize| bytes.
+//
+// Performs a CHECK that the operation succeeds.
+BASE_EXPORT void SetSystemPagesAccess(
     void* address,
     size_t length,
     PageAccessibilityConfiguration page_accessibility);

base/allocator/partition_allocator/page_allocator_internals_posix.h

@@ -117,13 +117,20 @@ void* TrimMappingInternal(void* base,
   return ret;
 }
 
-bool SetSystemPagesAccessInternal(
+bool TrySetSystemPagesAccessInternal(
     void* address,
     size_t length,
     PageAccessibilityConfiguration accessibility) {
   return 0 == mprotect(address, length, GetAccessFlags(accessibility));
 }
 
+void SetSystemPagesAccessInternal(
+    void* address,
+    size_t length,
+    PageAccessibilityConfiguration accessibility) {
+  CHECK_EQ(0, mprotect(address, length, GetAccessFlags(accessibility)));
+}
+
 void FreePagesInternal(void* address, size_t length) {
   CHECK(!munmap(address, length));
 

base/allocator/partition_allocator/page_allocator_internals_win.h

@@ -65,7 +65,7 @@ void* TrimMappingInternal(void* base,
   return ret;
 }
 
-bool SetSystemPagesAccessInternal(
+bool TrySetSystemPagesAccessInternal(
     void* address,
     size_t length,
     PageAccessibilityConfiguration accessibility) {
@@ -75,18 +75,30 @@ bool SetSystemPagesAccessInternal(
                                  GetAccessFlags(accessibility));
 }
 
+void SetSystemPagesAccessInternal(
+    void* address,
+    size_t length,
+    PageAccessibilityConfiguration accessibility) {
+  if (accessibility == PageInaccessible) {
+    CHECK_NE(0, VirtualFree(address, length, MEM_DECOMMIT));
+  } else {
+    CHECK_NE(nullptr, VirtualAlloc(address, length, MEM_COMMIT,
+                                   GetAccessFlags(accessibility)));
+  }
+}
+
 void FreePagesInternal(void* address, size_t length) {
   CHECK(VirtualFree(address, 0, MEM_RELEASE));
 }
 
 void DecommitSystemPagesInternal(void* address, size_t length) {
-  CHECK(SetSystemPagesAccess(address, length, PageInaccessible));
+  SetSystemPagesAccess(address, length, PageInaccessible);
 }
 
 bool RecommitSystemPagesInternal(void* address,
                                  size_t length,
                                  PageAccessibilityConfiguration accessibility) {
-  return SetSystemPagesAccess(address, length, accessibility);
+  return TrySetSystemPagesAccess(address, length, accessibility);
 }
 
 void DiscardSystemPagesInternal(void* address, size_t length) {

base/allocator/partition_allocator/partition_alloc.cc

@@ -222,15 +222,13 @@ bool PartitionReallocDirectMappedInPlace(PartitionRootGeneric* root,
     // Shrink by decommitting unneeded pages and making them inaccessible.
     size_t decommit_size = current_size - new_size;
     root->DecommitSystemPages(char_ptr + new_size, decommit_size);
-    CHECK(SetSystemPagesAccess(char_ptr + new_size, decommit_size,
-                               PageInaccessible));
+    SetSystemPagesAccess(char_ptr + new_size, decommit_size, PageInaccessible);
   } else if (new_size <=
              internal::PartitionDirectMapExtent::FromPage(page)->map_size) {
     // Grow within the actually allocated memory. Just need to make the
     // pages accessible again.
     size_t recommit_size = new_size - current_size;
-    CHECK(SetSystemPagesAccess(char_ptr + current_size, recommit_size,
-                               PageReadWrite));
+    SetSystemPagesAccess(char_ptr + current_size, recommit_size, PageReadWrite);
     root->RecommitSystemPages(char_ptr + current_size, recommit_size);
 
 #if DCHECK_IS_ON()

base/allocator/partition_allocator/partition_alloc_unittest.cc

@@ -1185,10 +1185,10 @@ TEST_F(PartitionAllocTest, MappingCollision) {
   map2 = AllocPages(page_base + kSuperPageSize, kPageAllocationGranularity,
                     kPageAllocationGranularity, PageReadWrite);
   EXPECT_TRUE(map2);
-  EXPECT_TRUE(
-      SetSystemPagesAccess(map1, kPageAllocationGranularity, PageInaccessible));
-  EXPECT_TRUE(
-      SetSystemPagesAccess(map2, kPageAllocationGranularity, PageInaccessible));
+  EXPECT_TRUE(TrySetSystemPagesAccess(map1, kPageAllocationGranularity,
+                                      PageInaccessible));
+  EXPECT_TRUE(TrySetSystemPagesAccess(map2, kPageAllocationGranularity,
+                                      PageInaccessible));
 
   PartitionPage* page_in_third_super_page = GetFullPage(kTestAllocSize);
   FreePages(map1, kPageAllocationGranularity);

base/allocator/partition_allocator/partition_bucket.cc

@@ -49,12 +49,12 @@ ALWAYS_INLINE PartitionPage* PartitionDirectMap(PartitionRootBase* root,
   root->IncreaseCommittedPages(committed_page_size);
 
   char* slot = ptr + kPartitionPageSize;
-  CHECK(SetSystemPagesAccess(ptr + (kSystemPageSize * 2),
-                             kPartitionPageSize - (kSystemPageSize * 2),
-                             PageInaccessible));
+  SetSystemPagesAccess(ptr + (kSystemPageSize * 2),
+                       kPartitionPageSize - (kSystemPageSize * 2),
+                       PageInaccessible);
 #if !defined(ARCH_CPU_64_BITS)
-  CHECK(SetSystemPagesAccess(ptr, kSystemPageSize, PageInaccessible));
-  CHECK(SetSystemPagesAccess(slot + size, kSystemPageSize, PageInaccessible));
+  SetSystemPagesAccess(ptr, kSystemPageSize, PageInaccessible);
+  SetSystemPagesAccess(slot + size, kSystemPageSize, PageInaccessible);
 #endif
 
   PartitionSuperPageExtentEntry* extent =
@@ -207,7 +207,7 @@ ALWAYS_INLINE void* PartitionBucket::AllocNewSlotSpan(
 
     // Fresh System Pages in the SuperPages are decommited. Commit them
     // before vending them back.
-    CHECK(SetSystemPagesAccess(ret, total_size, PageReadWrite));
+    SetSystemPagesAccess(ret, total_size, PageReadWrite);
 
     root->next_partition_page += total_size;
     root->IncreaseCommittedPages(total_size);
@@ -240,22 +240,22 @@ ALWAYS_INLINE void* PartitionBucket::AllocNewSlotSpan(
   // hole in the middle.
   // This is where we put page metadata and also a tiny amount of extent
   // metadata.
-  CHECK(SetSystemPagesAccess(super_page, kSystemPageSize, PageInaccessible));
-  CHECK(SetSystemPagesAccess(super_page + (kSystemPageSize * 2),
-                             kPartitionPageSize - (kSystemPageSize * 2),
-                             PageInaccessible));
-  //  CHECK(SetSystemPagesAccess(super_page + (kSuperPageSize -
+  SetSystemPagesAccess(super_page, kSystemPageSize, PageInaccessible);
+  SetSystemPagesAccess(super_page + (kSystemPageSize * 2),
+                       kPartitionPageSize - (kSystemPageSize * 2),
+                       PageInaccessible);
+  //  SetSystemPagesAccess(super_page + (kSuperPageSize -
   //  kPartitionPageSize),
-  //                             kPartitionPageSize, PageInaccessible));
+  //                             kPartitionPageSize, PageInaccessible);
   // All remaining slotspans for the unallocated PartitionPages inside the
   // SuperPage are conceptually decommitted. Correctly set the state here
   // so they do not occupy resources.
   //
   // TODO(ajwong): Refactor Page Allocator API so the SuperPage comes in
   // decommited initially.
-  CHECK(SetSystemPagesAccess(super_page + kPartitionPageSize + total_size,
-                             (kSuperPageSize - kPartitionPageSize - total_size),
-                             PageInaccessible));
+  SetSystemPagesAccess(super_page + kPartitionPageSize + total_size,
+                       (kSuperPageSize - kPartitionPageSize - total_size),
+                       PageInaccessible);
 
   // If we were after a specific address, but didn't get it, assume that
   // the system chose a lousy address. Here most OS'es have a default

gin/v8_platform.cc

@@ -245,8 +245,8 @@ class PageAllocator : public v8::PageAllocator {
       base::DecommitSystemPages(address, length);
       return true;
     } else {
-      return base::SetSystemPagesAccess(address, length,
-                                        GetPageConfig(permissions));
+      return base::TrySetSystemPagesAccess(address, length,
+                                           GetPageConfig(permissions));
     }
   }
 

third_party/blink/renderer/platform/heap/gc_info.cc

@@ -89,11 +89,10 @@ void GCInfoTable::Resize() {
   // Commit the new size and allow read/write.
   // TODO(ajwong): SetSystemPagesAccess should be part of RecommitSystemPages to
   // avoid having two calls here.
-  bool ok = base::SetSystemPagesAccess(current_table_end, table_size_delta,
-                                       base::PageReadWrite);
-  CHECK(ok);
-  ok = base::RecommitSystemPages(current_table_end, table_size_delta,
-                                 base::PageReadWrite);
+  base::SetSystemPagesAccess(current_table_end, table_size_delta,
+                             base::PageReadWrite);
+  bool ok = base::RecommitSystemPages(current_table_end, table_size_delta,
+                                      base::PageReadWrite);
   CHECK(ok);
 
 #if DCHECK_IS_ON()

third_party/blink/renderer/platform/heap/page_memory.cc

@@ -19,13 +19,13 @@ void MemoryRegion::Release() {
 
 bool MemoryRegion::Commit() {
   CHECK(base::RecommitSystemPages(base_, size_, base::PageReadWrite));
-  return base::SetSystemPagesAccess(base_, size_, base::PageReadWrite);
+  return base::TrySetSystemPagesAccess(base_, size_, base::PageReadWrite);
 }
 
 void MemoryRegion::Decommit() {
   ASAN_UNPOISON_MEMORY_REGION(base_, size_);
   base::DecommitSystemPages(base_, size_);
-  CHECK(base::SetSystemPagesAccess(base_, size_, base::PageInaccessible));
+  base::SetSystemPagesAccess(base_, size_, base::PageInaccessible);
 }
 
 PageMemoryRegion::PageMemoryRegion(Address base,