From ebc2ac934f4c54ecaf3604551300d01ba8e0dcb9 Mon Sep 17 00:00:00 2001 From: Mitch Phillips Date: Thu, 2 May 2024 13:25:46 +0200 Subject: [PATCH] Rename GWP-ASan android_mallopt() arguments The previous names were difficult to decipher. Now, let's change this all to be more clear as it's actually one android_mallopt() call depending on where it's called from, rather than the intended behaviour. Also updated the comments so it's clear what happened across different versions of Android. Test: atest bionic-unit-tests CtsGwpAsanTestCases Bug: N/a Change-Id: I0582cab0b74aa09f4af54f9fbe5ba22697f82082 --- libc/bionic/gwp_asan_wrappers.cpp | 12 ++--- libc/platform/bionic/malloc.h | 73 +++++++++++++++++++------------ tests/malloc_test.cpp | 6 +-- 3 files changed, 55 insertions(+), 36 deletions(-) diff --git a/libc/bionic/gwp_asan_wrappers.cpp b/libc/bionic/gwp_asan_wrappers.cpp index 11f7cedd2..2124f515d 100644 --- a/libc/bionic/gwp_asan_wrappers.cpp +++ b/libc/bionic/gwp_asan_wrappers.cpp @@ -57,7 +57,7 @@ static gwp_asan::GuardedPoolAllocator GuardedAlloc; static const MallocDispatch* prev_dispatch; -using Action = android_mallopt_gwp_asan_options_t::Action; +using Mode = android_mallopt_gwp_asan_options_t::Mode; using Options = gwp_asan::options::Options; // basename() is a mess, see the manpage. Let's be explicit what handling we @@ -261,8 +261,8 @@ void SetDefaultGwpAsanOptions(Options* options, unsigned* process_sample_rate, options->Recoverable = true; GwpAsanRecoverable = true; - if (mallopt_options.desire == Action::TURN_ON_WITH_SAMPLING || - mallopt_options.desire == Action::TURN_ON_FOR_APP_SAMPLED_NON_CRASHING) { + if (mallopt_options.mode == Mode::SYSTEM_PROCESS_OR_SYSTEM_APP || + mallopt_options.mode == Mode::APP_MANIFEST_DEFAULT) { *process_sample_rate = kDefaultProcessSampling; } else { *process_sample_rate = 1; @@ -285,7 +285,7 @@ bool GetGwpAsanOptionImpl(char* value_out, // be used. Tests still continue to use the environment variable though. if (*basename != '\0') { const char* default_sysprop = system_sysprop; - if (mallopt_options.desire == Action::TURN_ON_FOR_APP) { + if (mallopt_options.mode == Mode::APP_MANIFEST_ALWAYS) { default_sysprop = app_sysprop; } async_safe_format_buffer(&program_specific_sysprop[0], kSyspropMaxLen, "%s%s", @@ -425,7 +425,7 @@ bool MaybeInitGwpAsan(libc_globals* globals, Options options; unsigned process_sample_rate = kDefaultProcessSampling; if (!GetGwpAsanOptions(&options, &process_sample_rate, mallopt_options) && - mallopt_options.desire == Action::DONT_TURN_ON_UNLESS_OVERRIDDEN) { + mallopt_options.mode == Mode::APP_MANIFEST_NEVER) { return false; } @@ -492,7 +492,7 @@ bool MaybeInitGwpAsanFromLibc(libc_globals* globals) { android_mallopt_gwp_asan_options_t mallopt_options; mallopt_options.program_name = progname; - mallopt_options.desire = Action::TURN_ON_WITH_SAMPLING; + mallopt_options.mode = Mode::SYSTEM_PROCESS_OR_SYSTEM_APP; return MaybeInitGwpAsan(globals, mallopt_options); } diff --git a/libc/platform/bionic/malloc.h b/libc/platform/bionic/malloc.h index ffc6d4a43..da85cf526 100644 --- a/libc/platform/bionic/malloc.h +++ b/libc/platform/bionic/malloc.h @@ -130,36 +130,55 @@ typedef struct { // Worth noting, the "libc.debug.gwp_asan.*.app_default" sysprops *do not* // apply to system apps. They use the "libc.debug.gwp_asan.*.system_default" // sysprops. - enum Action { - // Enable GWP-ASan. This is used by apps that have `gwpAsanMode=always` in - // the manifest. - TURN_ON_FOR_APP, - // Enable GWP-ASan, but only a small percentage of the time. This is used by - // system processes and system apps, and we use a lottery to determine which - // processes have GWP-ASan enabled. This allows us to mitigate system-wide - // memory overhead concerns, as each GWP-ASan enabled process uses ~70KiB of - // extra memory. - TURN_ON_WITH_SAMPLING, - // Don't enable GWP-ASan, unless overwritten by a system property or - // environment variable. This is used by apps that have `gwpAsanMode=never` - // in the manifest. Prior to Android 14, this also was used by non-system - // apps that didn't specify a `gwpAsanMode` in their manifest. - DONT_TURN_ON_UNLESS_OVERRIDDEN, - // Enable GWP-ASan, but only a small percentage of the time, and enable it - // in the non-crashing ("recoverable") mode. In Android 14, this is used by - // apps that don't specify `gwpAsanMode` (or use `gwpAsanMode=default`) in - // their manifest. GWP-ASan will detect heap memory safety bugs in this - // mode, and bug reports will be created by debuggerd, however the process - // will recover and continue to function as if the memory safety bug wasn't - // detected. + // + // In recoverable mode, GWP-ASan will detect heap memory safety bugs, and bug + // reports will be created by debuggerd, however the process will recover and + // continue to function as if the memory safety bug wasn't detected. This + // prevents any user-visible impact as apps and processes don't crash, and + // probably saves us some CPU time in restarting the process. + // + // Process sampling enables GWP-ASan, but only a small percentage of the time + // (~1%). This helps mitigate any recurring high-frequency problems in certain + // processes, as it's highly likely the next restart of said process won't + // have GWP-ASan. In addition, for system processes and system apps, this + // allows us to mitigate system-wide memory overhead concerns, as each + // GWP-ASan enabled process uses ~70KiB of extra memory. + enum Mode { + // Used by default for apps, or by those that have an explicit + // `gwpAsanMode=default` in the manifest. // - // In Android 15, this is the same as TURN_ON_WITH_SAMPLING, as GWP-ASan is - // only ever used in non-crashing mode (even for platform executables and - // system apps). - TURN_ON_FOR_APP_SAMPLED_NON_CRASHING, + // Result: + // - Android 13 and before: GWP-ASan is not enabled. + // - Android 14 and after: Enables GWP-ASan with process sampling in + // recoverable mode. + APP_MANIFEST_DEFAULT = 3, + // This is used by apps that have `gwpAsanMode=always` in the manifest. + // + // Result: + // - Android 14 and before: Enables GWP-ASan in non-recoverable mode, + // without process sampling. + // - Android 15 and after: Enables GWP-ASan in recoverable mode, without + // process sampling. + APP_MANIFEST_ALWAYS = 0, + // This is used by apps that have `gwpAsanMode=never` in the manifest. + // + // Result: + // - GWP-ASan is not enabled, unless it's force-enabled by a system + // property or environment variable. + APP_MANIFEST_NEVER = 2, + // Used by system processes and system apps. + // + // Result: + // - Android 14 and before: Enables GWP-ASan with process sampling in + // non-recoverable mode. + // - Android 15 and after: Enables GWP-ASan with process sampling in + // recoverable mode. + SYSTEM_PROCESS_OR_SYSTEM_APP = 1, + // Next enum value = 4. Numbered non-sequentially above to preserve ABI + // stability, but now ordered more logically. }; - Action desire = DONT_TURN_ON_UNLESS_OVERRIDDEN; + Mode mode = APP_MANIFEST_NEVER; } android_mallopt_gwp_asan_options_t; #pragma clang diagnostic pop // Manipulates bionic-specific handling of memory allocation APIs such as diff --git a/tests/malloc_test.cpp b/tests/malloc_test.cpp index 2b48d852b..26e869f46 100644 --- a/tests/malloc_test.cpp +++ b/tests/malloc_test.cpp @@ -1410,15 +1410,15 @@ TEST(android_mallopt, set_allocation_limit_multiple_threads) { } #if defined(__BIONIC__) -using Action = android_mallopt_gwp_asan_options_t::Action; +using Mode = android_mallopt_gwp_asan_options_t::Mode; TEST(android_mallopt, DISABLED_multiple_enable_gwp_asan) { android_mallopt_gwp_asan_options_t options; options.program_name = ""; // Don't infer GWP-ASan options from sysprops. - options.desire = Action::DONT_TURN_ON_UNLESS_OVERRIDDEN; + options.mode = Mode::APP_MANIFEST_NEVER; // GWP-ASan should already be enabled. Trying to enable or disable it should // always pass. ASSERT_TRUE(android_mallopt(M_INITIALIZE_GWP_ASAN, &options, sizeof(options))); - options.desire = Action::TURN_ON_WITH_SAMPLING; + options.mode = Mode::APP_MANIFEST_DEFAULT; ASSERT_TRUE(android_mallopt(M_INITIALIZE_GWP_ASAN, &options, sizeof(options))); } #endif // defined(__BIONIC__)