[PA] Introduce use_partition_alloc_as_malloc (2 of N)

This is one in the series of CLs that aims at replacing use_allocator and use_partition_alloc_as_malloc with use_partition_alloc_as_malloc. use_allocator == "partition" <=> use_partition_alloc_as_malloc == true use_allocator == "none" <=> use_partition_alloc_as_malloc == false This CL switches from enable_partition_alloc_as_malloc_support and use_allocator to use_partition_alloc_as_malloc everywhere. Once landed, use_allocator will be useless and can be removed from GN args. enable_partition_alloc_as_malloc_support too, but that doesn't seem to be set anywhere anyway. Bug: 1151236 Change-Id: Ie1c99b92e0f533513a20d56d69cecb1c903dd320 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3965990 Commit-Queue: Bartek Nowierski <bartekn@chromium.org> Reviewed-by: Takashi Sakamoto <tasak@google.com> Auto-Submit: Bartek Nowierski <bartekn@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> Cr-Commit-Position: refs/heads/main@{#1062311}
Denger-Network · Oct 21, 2022 · 07f51c6 · 07f51c6
1 parent 1242220
commit 07f51c6
Show file tree

Hide file tree

Showing 12 changed files with 51 additions and 73 deletions.
diff --git a/base/BUILD.gn b/base/BUILD.gn
@@ -1607,7 +1607,7 @@ mixed_component("base") {
       ]
     }
 
-    if (use_allocator == "none") {
+    if (!use_partition_alloc_as_malloc) {
       if (is_android) {
         sources += [ "allocator/partition_allocator/shim/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc" ]
       }
@@ -2513,7 +2513,7 @@ mixed_component("base") {
       ]
     }
 
-    if (use_allocator == "partition") {
+    if (use_partition_alloc_as_malloc) {
       sources += [
         "trace_event/address_space_dump_provider.cc",
         "trace_event/address_space_dump_provider.h",

diff --git a/base/allocator/BUILD.gn b/base/allocator/BUILD.gn
@@ -9,18 +9,14 @@ import("//build/config/dcheck_always_on.gni")
 
 buildflag_header("buildflags") {
   header = "buildflags.h"
-  _use_partition_alloc_as_malloc = use_allocator == "partition"
-  assert(use_allocator_shim || !_use_partition_alloc_as_malloc,
-         "Partition alloc requires the allocator shim")
-  assert(
-      !_use_partition_alloc_as_malloc ||
-          enable_partition_alloc_as_malloc_support,
-      "Partition alloc as malloc requires enable_partition_alloc_as_malloc_support=true")
+
+  assert(use_allocator_shim || !use_partition_alloc_as_malloc,
+         "PartitionAlloc-Everywhere requires the allocator shim")
 
   flags = [
-    "USE_ALLOCATOR_SHIM=$use_allocator_shim",
     "USE_PARTITION_ALLOC=$use_partition_alloc",
-    "USE_PARTITION_ALLOC_AS_MALLOC=$_use_partition_alloc_as_malloc",
+    "USE_ALLOCATOR_SHIM=$use_allocator_shim",
+    "USE_PARTITION_ALLOC_AS_MALLOC=$use_partition_alloc_as_malloc",
 
     "USE_BACKUP_REF_PTR=$use_backup_ref_ptr",
     "USE_ASAN_BACKUP_REF_PTR=$use_asan_backup_ref_ptr",

diff --git a/base/allocator/README.md b/base/allocator/README.md
@@ -15,29 +15,13 @@ Background
 ----------
 The `allocator` target defines at compile-time the platform-specific choice of
 the allocator and extra-hooks which services calls to malloc/new. The relevant
-build-time flags involved are `use_allocator` and `use_allocator_shim`.
+build-time flags involved are `use_allocator_shim` and
+`use_partition_alloc_as_malloc`.
 
-The default choices are as follows:
-
-**Windows**
-`use_allocator: winheap`, the default Windows heap.
-Additionally, `static_library` (i.e. non-component) builds have a shim
-layer wrapping malloc/new, which is controlled by `use_allocator_shim`.
-The shim layer provides extra security features, such as preventing large
-allocations that can hit signed vs. unsigned bugs in third_party code.
-
-**Android**
-`use_allocator: none`, always use the allocator symbols coming from Android's
-libc (Bionic). As it is developed as part of the OS, it is considered to be
-optimized for small devices and more memory-efficient than other choices.
-The actual implementation backing malloc symbols in Bionic is up to the board
-config and can vary (typically *dlmalloc* or *jemalloc* on most Nexus devices).
-
-**Mac/iOS**
-`use_allocator: none`, we always use the system's allocator implementation.
-
-In addition, when building for `asan` / `msan` both the allocator and the shim
-layer are disabled.
+By default, these are true on all platforms except iOS (not yet supported) and
+NaCl (no plan to support).
+Furthermore, when building with a sanitizer (e.g. `asan`, `msan`, ...) both the
+allocator and the shim layer are disabled.
 
 
 Layering and build deps

diff --git a/base/allocator/allocator.gni b/base/allocator/allocator.gni
@@ -18,7 +18,7 @@ declare_args() {
   # Whether PartitionAlloc should be available for use or not.
   # true makes PartitionAlloc linked to the executable or shared library and
   # makes it available for use. It doesn't mean that the default allocator
-  # is PartitionAlloc, which is governed by |use_allocator|.
+  # is PartitionAlloc, which is governed by |use_partition_alloc_as_malloc|.
   #
   # This flag is currently set to false only on Cronet bots, because Cronet
   # doesn't use PartitionAlloc at all, and doesn't wish to incur the library
@@ -30,16 +30,14 @@ declare_args() {
   force_enable_raw_ptr_exclusion = false
 }
 
-if (!use_partition_alloc && use_allocator == "partition") {
+if (!use_partition_alloc && use_partition_alloc_as_malloc) {
   # If there is a conflict, prioritize |use_partition_alloc| over
-  # |use_allocator|.
+  # |use_partition_alloc_as_malloc|.
   # TODO(bartekn): Move |use_partition_alloc| to PA lib, since we'd need to
-  # override |use_allocator| there.
-  use_allocator = "none"
+  # override |use_partition_alloc_as_malloc| there.
+  use_partition_alloc_as_malloc = false
 }
 
-assert(use_allocator == "none" || use_allocator == "partition")
-
 assert(
     !use_allocator_shim || is_linux || is_chromeos || is_android || is_win ||
         is_fuchsia || is_apple,
@@ -73,9 +71,9 @@ if (is_win && use_allocator_shim) {
 #     that are invoked explicitly (not via malloc). These are only Blink
 #     partition, where we currently don't even use raw_ptr<T>.
 use_backup_ref_ptr = use_partition_alloc && enable_backup_ref_ptr_support &&
-                     enable_partition_alloc_as_malloc_support && !is_nacl
+                     use_partition_alloc_as_malloc && !is_nacl
 use_mte_checked_ptr = use_partition_alloc && enable_mte_checked_ptr_support &&
-                      enable_partition_alloc_as_malloc_support && !is_nacl
+                      use_partition_alloc_as_malloc && !is_nacl
 
 assert(!(use_backup_ref_ptr && use_mte_checked_ptr),
        "MTECheckedPtr conflicts with BRP.")

diff --git a/base/allocator/partition_allocator/BUILD.gn b/base/allocator/partition_allocator/BUILD.gn
@@ -388,7 +388,7 @@ buildflag_header("partition_alloc_buildflags") {
   # defines and partition alloc includes the header file. For chrome,
   # gen/base/allocator/buildflags.h defines and chrome includes.
   flags = [
-    "ENABLE_PARTITION_ALLOC_AS_MALLOC_SUPPORT=$enable_partition_alloc_as_malloc_support",
+    "ENABLE_PARTITION_ALLOC_AS_MALLOC_SUPPORT=$use_partition_alloc_as_malloc",
 
     "ENABLE_BACKUP_REF_PTR_SUPPORT=$_enable_backup_ref_ptr_support",
     "ENABLE_BACKUP_REF_PTR_SLOW_CHECKS=$_enable_backup_ref_ptr_slow_checks",

diff --git a/base/allocator/partition_allocator/build_config.md b/base/allocator/partition_allocator/build_config.md
@@ -48,18 +48,17 @@ PartitionAlloc-Everywhere and must be `true` as a prerequisite for
 enabling PA-E.
 ***
 
-### `use_allocator`
+### `use_partition_alloc_as_malloc`
 
-Does nothing special when value is `"none"`. Enables
-[PartitionAlloc-Everywhere (PA-E)][pae-public-doc] when value is
-`"partition"`.
+Does nothing special when value is `false`. Enables
+[PartitionAlloc-Everywhere (PA-E)][pae-public-doc] when value is `true`.
 
 *** note
 * While "everywhere" (in "PartitionAlloc-Everywhere") tautologically
   includes Blink where PartitionAlloc originated, setting
-  `use_allocator = "none"` does not disable PA usage in Blink.
-* `use_allocator = "partition"` internally sets
-  `use_partition_alloc_as_malloc = true`, which must not be confused
+  `use_partition_alloc_as_malloc = false` does not disable PA usage in Blink,
+  which invokes PA explicitly (not via malloc).
+* `use_partition_alloc_as_malloc = true` must not be confused
   with `use_partition_alloc` (see above).
 ***
 

diff --git a/base/allocator/partition_allocator/partition_alloc.gni b/base/allocator/partition_allocator/partition_alloc.gni
@@ -12,6 +12,8 @@ if (use_partition_alloc_as_malloc_default) {
 }
 
 declare_args() {
+  # PartitionAlloc-Everywhere (PA-E).
+  #
   # Temporarily move |use_allocator| to partition_alloc.gni, because
   # some bots use |use_allocator|="none" with
   # |use_partition_alloc_as_malloc_default|=true. This causes PA_CHECK()
@@ -26,6 +28,13 @@ declare_args() {
   use_partition_alloc_as_malloc = use_partition_alloc_as_malloc_default
 }
 
+assert(use_allocator == "partition" || use_allocator == "none",
+       "use_allocator can either be 'partition' or 'none'")
+
+assert(
+    use_partition_alloc_as_malloc == (use_allocator == "partition"),
+    "use_partition_alloc_as_malloc must be true iff use_allocator=='partition'")
+
 declare_args() {
   use_freeslot_bitmap = false
 
@@ -35,10 +44,6 @@ declare_args() {
 }
 
 declare_args() {
-  # PartitionAlloc-Everywhere (PA-E).
-  enable_partition_alloc_as_malloc_support =
-      use_partition_alloc_as_malloc_default && use_allocator == "partition"
-
   # Build support for Use-after-Free protection via BackupRefPtr (BRP) or
   # MTECheckedPtr. To be effective, these need to be paired with raw_ptr<>.
   #
@@ -49,10 +54,6 @@ declare_args() {
   enable_mte_checked_ptr_support = enable_mte_checked_ptr_support_default
 }
 
-assert(
-    !enable_partition_alloc_as_malloc_support || use_allocator == "partition",
-    "PA-E support can be enabled only if use_allocator == \"partition\"")
-
 assert(!(enable_backup_ref_ptr_support && enable_mte_checked_ptr_support),
        "MTECheckedPtrSupport conflicts with BRPSupport.")
 

diff --git a/base/process/memory.cc b/base/process/memory.cc
@@ -25,9 +25,9 @@
 
 namespace base {
 
-// Defined in memory_mac.mm for macOS + use_allocator="none".  In case of
-// USE_PARTITION_ALLOC_AS_MALLOC, no need to route the call to the system
-// default calloc of macOS.
+// Defined in memory_mac.mm for macOS + use_partition_alloc_as_malloc=false.
+// In case of use_partition_alloc_as_malloc=true, no need to route the call to
+// the system default calloc of macOS.
 #if !BUILDFLAG(IS_APPLE) || BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
 
 bool UncheckedCalloc(size_t num_items, size_t size, void** result) {

diff --git a/base/process/memory_mac.mm b/base/process/memory_mac.mm
@@ -27,8 +27,8 @@ void EnableTerminationOnHeapCorruption() {
 
 bool UncheckedMalloc(size_t size, void** result) {
 #if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
-  // Unlike use_allocator="none", the default malloc zone is replaced with
-  // PartitionAlloc, so the allocator shim functions work best.
+  // Unlike use_partition_alloc_as_malloc=false, the default malloc zone is
+  // replaced with PartitionAlloc, so the allocator shim functions work best.
   *result = allocator_shim::UncheckedAlloc(size);
   return *result != nullptr;
 #else   // BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)

diff --git a/build_overrides/partition_alloc.gni b/build_overrides/partition_alloc.gni
@@ -9,9 +9,10 @@
 # this file and will use the defined values as default build configuration.
 #
 # partition_alloc.gni declares the following variables:
+# - use_allocator_shim
+# - use_partition_alloc_as_malloc
 # - enable_backup_ref_ptr_support
 # - enable_mte_checked_ptr_support
-# - enable_partition_alloc_as_malloc_support
 # - put_ref_count_in_previous_slot
 # - enable_backup_ref_ptr_slow_checks
 # - enable_dangling_raw_ptr_checks
@@ -36,11 +37,11 @@ _is_using_sanitizers = is_asan || is_hwasan || is_lsan || is_tsan || is_msan
 #   issues on some (e.g. Windows with shims, Android with non-universal symbol
 #   wrapping), and has not been validated on others.
 # - Windows: debug CRT is not compatible, see below.
-_disable_partition_alloc = is_component_build || (is_win && is_debug)
+_disable_partition_alloc_everywhere = is_component_build || (is_win && is_debug)
 
 # - NaCl: No plans to support it.
 # - iOS: not done yet.
-_is_partition_alloc_platform = !is_nacl && !is_ios
+_is_partition_alloc_everywhere_platform = !is_nacl && !is_ios
 
 # Under Windows debug build, the allocator shim is not compatible with CRT.
 #     NaCl in particular does seem to link some binaries statically
@@ -49,15 +50,15 @@ _is_partition_alloc_platform = !is_nacl && !is_ios
 # For all other platforms & configurations, the shim is required, to replace
 #     the default system allocators, e.g. with Partition Alloc.
 if ((is_linux || is_chromeos || is_android || is_apple ||
-     (is_fuchsia && !_disable_partition_alloc) ||
+     (is_fuchsia && !_disable_partition_alloc_everywhere) ||
      (is_win && !is_component_build && !is_debug)) && !_is_using_sanitizers) {
   _default_use_allocator_shim = true
 } else {
   _default_use_allocator_shim = false
 }
 
-if (_default_use_allocator_shim && _is_partition_alloc_platform &&
-    !_disable_partition_alloc) {
+if (_default_use_allocator_shim && _is_partition_alloc_everywhere_platform &&
+    !_disable_partition_alloc_everywhere) {
   _default_allocator = "partition"
 } else {
   _default_allocator = "none"

diff --git a/docs/dangling_ptr.md b/docs/dangling_ptr.md
@@ -44,7 +44,7 @@ disabled PartitionAlloc-Everywhere. `enable_backup_ref_ptr_support = true` can't
 be used without PartitionAlloc-Everywhere, leading to error:
 ```
 ERROR at //base/allocator/allocator.gni:126:3: Assertion failed.
-  assert(!use_backup_ref_ptr || use_allocator == "partition",
+  assert(!use_backup_ref_ptr || use_partition_alloc_as_malloc,
 ```
 
 ## Runtime flags

diff --git a/tools/memory/partition_allocator/BUILD.gn b/tools/memory/partition_allocator/BUILD.gn
@@ -6,9 +6,8 @@ import("//base/allocator/allocator.gni")
 
 # Only support x86_64 Linux as the system calls used are Linux-specific. No
 # point in building this if PartitionAlloc is not the allocator used.
-_tcache_tool_supported =
-    ((target_cpu == "x64" && is_linux) || is_mac) &&
-    (use_allocator == "partition" || use_partition_alloc_as_malloc)
+_tcache_tool_supported = ((target_cpu == "x64" && is_linux) || is_mac) &&
+                         use_partition_alloc_as_malloc
 
 if (_tcache_tool_supported) {
   source_set("pa_tool_utils") {