0

Reland "PA: Invert MiraclePtr Platform Enablement Logic"

This reverts commit 19b5dd0c20.

Reason for reland: I re-invert the DPD condition in patchset 2

Original change's description:
> Revert "PA: Invert MiraclePtr Platform Enablement Logic"
>
> This reverts commit c3e9990871.
>
> Reason for revert: I just realize I inverted the DPD condition
> causing it to be enabled in non dcheck builds.
>
> Original change's description:
> > PA: Invert MiraclePtr Platform Enablement Logic
> >
> > This patch inverts the build flags logic from an "allowlist" of platform
> > toward a "deny list".
> >
> > - **Increased Confidence**: Guarantees MiraclePtr is active everywhere
> >   unless explicitly disabled, reducing the risk of unexpected
> >   vulnerabilities on niche platforms.
> >
> > - **Future-Proofing**: Potentially new platforms automatically inherit
> >   MiraclePtr from the beginning, eliminating the need to "ship" it one
> >   more time.
> >
> > - **Simplified Maintenance**: Focus shifts to maintaining a small,
> >   explicit list of exceptions rather than a growing list of enabled
> >   platforms.
> >
> > What is the "complementary"?
> > -----------------------------
> >
> > We know from `//build/config/BUILDCONFIG` the current_os is limited to:
> >   aix     , android , chromeos
> >   fuchsia , ios     , linux
> >   mac     , nacl    , win
> >   winuwp  , zos
> >
> > "aix" and "zos" aren't supported by Chrome, only V8. So they can be
> > ignored.
> >
> > Bug: 364466913
> > Change-Id: Ia03f8f18a4b3bb05e4c9d304980cadcf235be3c5
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5837217
> > Reviewed-by: Keishi Hattori <keishi@chromium.org>
> > Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
> > Reviewed-by: Wez <wez@chromium.org>
> > Reviewed-by: danakj <danakj@chromium.org>
> > Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1352670}
>
> Bug: 364466913
> Change-Id: I7ed0231bfa46b2091041df6ef7c09a7726cfacc1
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5844793
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Owners-Override: Arthur Sonzogni <arthursonzogni@chromium.org>
> Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1352693}

Bug: 364466913
Change-Id: I315ccb248b5d6323849883aad085c93db218d2d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5844931
Reviewed-by: danakj <danakj@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1354441}
This commit is contained in:
Arthur Sonzogni
2024-09-12 09:52:30 +00:00
committed by Chromium LUCI CQ
parent debef08655
commit 647b65eb67
4 changed files with 70 additions and 53 deletions
base
build_overrides

@ -131,10 +131,7 @@ BASE_FEATURE(kPartitionAllocZappingByFreeFlags,
BASE_FEATURE(kPartitionAllocBackupRefPtr,
"PartitionAllocBackupRefPtr",
#if BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || \
BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS) || \
BUILDFLAG(IS_FUCHSIA) || (BUILDFLAG(IS_LINUX) && !BUILDFLAG(IS_CASTOS)) || \
PA_BUILDFLAG(ENABLE_BACKUP_REF_PTR_FEATURE_FLAG)
#if PA_BUILDFLAG(ENABLE_BACKUP_REF_PTR_FEATURE_FLAG)
FEATURE_ENABLED_BY_DEFAULT
#else
FEATURE_DISABLED_BY_DEFAULT

@ -90,9 +90,8 @@ assert(
!enable_allocator_shim_partition_alloc_dispatch_with_advanced_checks_support || use_partition_alloc_as_malloc,
"PartitionAlloc with advanced checks requires PartitionAlloc itself.")
assert(!use_allocator_shim || (is_android || is_apple || is_chromeos ||
is_fuchsia || is_linux || is_win),
"The allocator shim does not (yet) support the platform.")
assert(!use_allocator_shim || !is_nacl,
"The allocator shim supports every platform, except nacl")
if (use_allocator_shim && is_win) {
# It's hard to override CRT's malloc family in every case in the component
@ -216,12 +215,15 @@ declare_args() {
# Enable the feature flag required to activate backup ref pointers. That is to
# say `PartitionAllocBackupRefPtr`.
#
# This is meant to be used primarily on bots. It is much easier to override
# the feature flags using a binary flag instead of updating multiple bots's
# scripts to pass command line arguments.
# This is meant to be modified primarily on bots. It is much easier to
# override the feature flags using a binary flag instead of updating multiple
# bots's scripts to pass command line arguments.
#
# TODO(328104161): Remove this flag.
enable_backup_ref_ptr_feature_flag = false
enable_backup_ref_ptr_feature_flag =
enable_backup_ref_ptr_support && use_raw_ptr_backup_ref_impl &&
# Platforms where BackupRefPtr hasn't shipped yet:
!is_castos && !is_ios
# Build support for Dangling Ptr Detection (DPD) via BackupRefPtr (BRP),
# making the raw_ptr<T> implementation to RawPtrBackupRefImpl if active.
@ -239,9 +241,9 @@ declare_args() {
# Enable the feature flag required to check for dangling pointers. That is to
# say `PartitionAllocDanglingPtr`.
#
# This is meant to be used primarily on bots. It is much easier to override
# the feature flags using a binary flag instead of updating multiple bots's
# scripts to pass command line arguments.
# This is meant to be modified primarily on bots. It is much easier to
# override the feature flags using a binary flag instead of updating multiple
# bots's scripts to pass command line arguments.
#
# TODO(328104161): Remove this flag.
enable_dangling_raw_ptr_feature_flag = enable_dangling_raw_ptr_checks

@ -55,18 +55,20 @@ Having said that, we want to emphasize that dereferencing a dangling pointer
remains an Undefined Behavior.
`raw_ptr<T>` protection is enabled by default in all non-Renderer processes, on:
- Android (incl. AndroidWebView & Android WebEngine)
- Android (incl. AndroidWebView, Android WebEngine, & Android ChromeCast)
- Windows
- ChromeOS (incl. Ash & Lacros)
- macOS
- Linux
- Fuchsia
In particular, it isn't enabled by default on:
In particular, it isn't yet enabled by default on:
- iOS
- ChromeCast
- Aix
- Zos
- Linux CastOS (Nest hardware)
For the source of truth, both `enable_backup_ref_ptr_support` and `enable_backup_ref_ptr_feature_flag` need to enabled.
Please refer to the following files: [build_overrides/partition_alloc.gni](https://source.chromium.org/chromium/chromium/src/+/main:build_overrides/partition_alloc.gni) and [partition_alloc.gni](https://source.chromium.org/chromium/chromium/src/+/main:base/allocator/partition_allocator/partition_alloc.gni;l=5?q=partition_alloc.gni&sq=&ss=chromium)
[TOC]

@ -71,9 +71,6 @@ if (!is_debug || partition_alloc_optimized_debug) {
partition_alloc_remove_configs +=
[ "//build/config/compiler/pgo:default_pgo_flags" ]
# Sanitizers replace the allocator, don't use our own.
_is_using_sanitizers = is_asan || is_hwasan || is_lsan || is_tsan || is_msan
# - Component build support is disabled on all platforms except Linux. It is
# known to cause issues on some (e.g. Windows with shims, Android with
# non-universal symbol wrapping), and has not been validated on others.
@ -83,49 +80,68 @@ _disable_partition_alloc_everywhere =
# - NaCl: No plans to support it.
# - iOS: Depends on ios_partition_alloc_enabled.
if (is_ios) {
_is_partition_alloc_everywhere_platform = ios_partition_alloc_enabled
} else {
_is_partition_alloc_everywhere_platform = !is_nacl
_is_partition_alloc_everywhere_platform =
!is_nacl && (!is_ios || ios_partition_alloc_enabled)
use_allocator_shim_default = true # Updated below:
# Sanitizers replace the allocator, don't use our own.
if (is_asan || is_hwasan || is_lsan || is_tsan || is_msan) {
use_allocator_shim_default = false
}
# NaCl will never support the allocator shim.
if (is_nacl) {
use_allocator_shim_default = false
}
# Under Fuchsia, the allocator shim is only required for PA-E.
if (is_fuchsia && _disable_partition_alloc_everywhere) {
use_allocator_shim_default = false
}
# Under Windows debug build, the allocator shim is not compatible with CRT.
# NaCl in particular does seem to link some binaries statically
# against the debug CRT with "is_nacl=false".
# Under Fuchsia, the allocator shim is only required for PA-E.
# For all other platforms & configurations, the shim is required, to replace
# the default system allocators, e.g. with PartitionAlloc.
if ((is_linux || is_chromeos || is_android || is_apple ||
(is_fuchsia && !_disable_partition_alloc_everywhere) ||
(is_win && !is_debug &&
(!is_component_build || (use_custom_libcxx && !libcxx_is_shared)))) &&
!_is_using_sanitizers) {
_default_use_allocator_shim = true
} else {
_default_use_allocator_shim = false
# NaCl in particular does seem to link some binaries statically against the
# debug CRT with "is_nacl=false".
if (is_win && is_debug) {
use_allocator_shim_default = false
}
if (_default_use_allocator_shim && _is_partition_alloc_everywhere_platform &&
!_disable_partition_alloc_everywhere) {
_default_allocator = "partition"
} else {
_default_allocator = "none"
if (is_win && is_component_build && (!use_custom_libcxx || libcxx_is_shared)) {
use_allocator_shim_default = false
}
use_partition_alloc_as_malloc_default = _default_allocator == "partition"
use_allocator_shim_default = _default_use_allocator_shim
use_partition_alloc_as_malloc_default =
use_allocator_shim_default && _is_partition_alloc_everywhere_platform &&
!_disable_partition_alloc_everywhere
_is_brp_supported =
(is_win || is_android || is_linux || is_mac || is_ios || is_chromeos ||
is_fuchsia) && use_partition_alloc_as_malloc_default
enable_backup_ref_ptr_support_default = _is_brp_supported
enable_backup_ref_ptr_support_default = use_partition_alloc_as_malloc_default
enable_backup_ref_ptr_slow_checks_default = false
enable_dangling_raw_ptr_checks_default =
# The DanglingPointerDetector relies on BackupRefPtr:
enable_backup_ref_ptr_support_default &&
(is_linux || is_win || is_chromeos || is_mac) && !is_official_build &&
(is_debug || dcheck_always_on)
# DanglingPointerDetector is not yet enabled for official builds.
# Enabling it would require:
# - Metric-monitored dry-runs to assess real-world impact.
# - Performance re-evaluation, although past impact was neutral.
# - Additional considerations (etc.).
#
# It's also unclear if potential security issues warrant crash reports,
# as this could burden developers. Non-crashing metrics might help assess
# the value.
!is_official_build &&
# In unofficial builds, enable only for developers interested in
# DCHECKd/Debug builds. In the future, given its neutral performance impact,
# we could consider removing this restriction.
(is_debug || dcheck_always_on) &&
# Fuchsia and iOS have never been tested against DanglingPointerDetector at
# the moment.
!is_ios && !is_fuchsia &&
# Only the `android-rel` CQ bot has enforced DanglingPointerDetector checks
# at the moment. The other Android bots are not ready for it yet.
!is_android
raw_ptr_zero_on_construct_default = true
raw_ptr_zero_on_move_default = true