[ASan][Windows] Honor asan config flags on windows when set through the user function #122990
Conversation
llvmbot
added
compiler-rt
compiler-rt:asan
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: David Justo (davidmrdavid) ChangesRelated to: #117925 Context: To fix this properly, we'll probably need a deep re-design of ASan initialization so that it is consistent across OS'es. This PR: refactors part of Today, Other minor changes:
Full diff: https://github.com/llvm/llvm-project/pull/122990.diff 5 Files Affected:
diff --git a/compiler-rt/lib/asan/asan_allocator.cpp b/compiler-rt/lib/asan/asan_allocator.cpp
index 9e66f77217ec6b..cf041d0a62fc20 100644
--- a/compiler-rt/lib/asan/asan_allocator.cpp
+++ b/compiler-rt/lib/asan/asan_allocator.cpp
@@ -423,10 +423,15 @@ struct Allocator {
PoisonShadow(chunk, allocated_size, kAsanHeapLeftRedzoneMagic);
}
- void ReInitialize(const AllocatorOptions &options) {
+ // Apply provided AllocatorOptions to an Allocator
+ void ApplyOptions(const AllocatorOptions &options) {
SetAllocatorMayReturnNull(options.may_return_null);
allocator.SetReleaseToOSIntervalMs(options.release_to_os_interval_ms);
SharedInitCode(options);
+ }
+
+ void ReInitialize(const AllocatorOptions &options) {
+ ApplyOptions(options);
// Poison all existing allocation's redzones.
if (CanPoisonMemory()) {
@@ -975,6 +980,11 @@ void ReInitializeAllocator(const AllocatorOptions &options) {
instance.ReInitialize(options);
}
+// Apply provided AllocatorOptions to an Allocator
+void ApplyAllocatorOptions(const AllocatorOptions &options) {
+ instance.ApplyOptions(options);
+}
+
void GetAllocatorOptions(AllocatorOptions *options) {
instance.GetOptions(options);
}
diff --git a/compiler-rt/lib/asan/asan_allocator.h b/compiler-rt/lib/asan/asan_allocator.h
index db8dc3bebfc620..a94ef958aa75ec 100644
--- a/compiler-rt/lib/asan/asan_allocator.h
+++ b/compiler-rt/lib/asan/asan_allocator.h
@@ -47,6 +47,7 @@ struct AllocatorOptions {
void InitializeAllocator(const AllocatorOptions &options);
void ReInitializeAllocator(const AllocatorOptions &options);
void GetAllocatorOptions(AllocatorOptions *options);
+void ApplyAllocatorOptions(const AllocatorOptions &options);
class AsanChunkView {
public:
diff --git a/compiler-rt/lib/asan/asan_flags.cpp b/compiler-rt/lib/asan/asan_flags.cpp
index 9cfb70bd00c786..9be2273998e40a 100644
--- a/compiler-rt/lib/asan/asan_flags.cpp
+++ b/compiler-rt/lib/asan/asan_flags.cpp
@@ -144,7 +144,8 @@ static void InitializeDefaultFlags() {
DisplayHelpMessages(&asan_parser);
}
-static void ProcessFlags() {
+// Validate flags and report incompatible configurations
+static void ValidateFlags()s() {
Flags *f = flags();
// Flag validation:
@@ -214,11 +215,11 @@ static void ProcessFlags() {
void InitializeFlags() {
InitializeDefaultFlags();
- ProcessFlags();
+ ValidateFlags();
#if SANITIZER_WINDOWS
- // On Windows, weak symbols are emulated by having the user program
- // register which weak functions are defined.
+ // On Windows, weak symbols (such as the `__asan_default_options` function)
+ // are emulated by having the user program register which weak functions are defined.
// The ASAN DLL will initialize flags prior to user module initialization,
// so __asan_default_options will not point to the user definition yet.
// We still want to ensure we capture when options are passed via
@@ -239,14 +240,8 @@ void InitializeFlags() {
asan_parser.ParseString(__asan_default_options());
DisplayHelpMessages(&asan_parser);
- ProcessFlags();
-
- // TODO: Update other globals and data structures that may need to change
- // after initialization due to new flags potentially being set changing after
- // `__asan_default_options` is registered.
- // See GH issue 'https://github.com/llvm/llvm-project/issues/117925' for
- // details.
- SetAllocatorMayReturnNull(common_flags()->allocator_may_return_null);
+ ValidateFlags();
+ ApplyFlags();
});
# if CAN_SANITIZE_UB
@@ -259,7 +254,7 @@ void InitializeFlags() {
ubsan_parser.ParseString(__ubsan_default_options());
// To match normal behavior, do not print UBSan help.
- ProcessFlags();
+ ValidateFlags();
});
# endif
@@ -273,7 +268,7 @@ void InitializeFlags() {
lsan_parser.ParseString(__lsan_default_options());
// To match normal behavior, do not print LSan help.
- ProcessFlags();
+ ValidateFlags();
});
# endif
diff --git a/compiler-rt/lib/asan/asan_internal.h b/compiler-rt/lib/asan/asan_internal.h
index 06dfc4b1773397..464faad56f32d1 100644
--- a/compiler-rt/lib/asan/asan_internal.h
+++ b/compiler-rt/lib/asan/asan_internal.h
@@ -61,6 +61,7 @@ using __sanitizer::StackTrace;
void AsanInitFromRtl();
bool TryAsanInitFromRtl();
+void ApplyFlags();
// asan_win.cpp
void InitializePlatformExceptionHandlers();
diff --git a/compiler-rt/lib/asan/asan_rtl.cpp b/compiler-rt/lib/asan/asan_rtl.cpp
index 19c6c210b564c5..65c40d8bf8b13d 100644
--- a/compiler-rt/lib/asan/asan_rtl.cpp
+++ b/compiler-rt/lib/asan/asan_rtl.cpp
@@ -390,6 +390,39 @@ void PrintAddressSpaceLayout() {
kHighShadowBeg > kMidMemEnd);
}
+// Apply most options specified either through the ASAN_OPTIONS
+// environment variable, or through the `__asan_default_options` user function.
+//
+// This function may be called multiple times, once per weak reference callback on
+// Windows, so it needs to be idempotent.
+//
+// Context:
+// For maximum compatibility on Windows, it is necessary for ASan options to be
+// configured/registered/applied inside this method (instead of in ASanInitInternal,
+// for example). That's because, on Windows, the user-provided definition for `__asan_default_opts`
+// may not be bound when `ASanInitInternal` is invoked (it is bound later).
+//
+// To work around the late binding on windows, `ApplyOptions` will be called, again,
+// after binding to the user-provided `__asan_default_opts` function. Therefore,
+// any flags not configured here are not guaranteed to be configurable
+// through `__asan_default_opts` on Windows.
+//
+//
+// For more details on this issue, see: https://github.com/llvm/llvm-project/issues/117925
+void ApplyFlags() {
+ SetCanPoisonMemory(flags()->poison_heap);
+ SetMallocContextSize(common_flags()->malloc_context_size);
+
+ __sanitizer_set_report_path(common_flags()->log_path);
+
+ __asan_option_detect_stack_use_after_return =
+ flags()->detect_stack_use_after_return;
+
+ AllocatorOptions allocator_options;
+ allocator_options.SetFrom(flags(), common_flags());
+ ApplyAllocatorOptions(allocator_options);
+}
+
static bool AsanInitInternal() {
if (LIKELY(AsanInited()))
return true;
@@ -397,10 +430,13 @@ static bool AsanInitInternal() {
CacheBinaryName();
- // Initialize flags. This must be done early, because most of the
- // initialization steps look at flags().
+ // Initialize flags and register weak function callbacks for windows.
+ // This must be done early, because most of the initialization steps look at flags().
InitializeFlags();
+ // NOTE: The sleep before/after init` flags will not work on Windows when set through
+ // `__asan_default_options`, because that function is not guaranteed to be bound
+ // this early in initialization.
WaitForDebugger(flags()->sleep_before_init, "before init");
// Stop performing init at this point if we are being loaded via
@@ -416,9 +452,6 @@ static bool AsanInitInternal() {
AsanCheckDynamicRTPrereqs();
AvoidCVE_2016_2143();
- SetCanPoisonMemory(flags()->poison_heap);
- SetMallocContextSize(common_flags()->malloc_context_size);
-
InitializePlatformExceptionHandlers();
InitializeHighMemEnd();
@@ -428,11 +461,6 @@ static bool AsanInitInternal() {
SetCheckUnwindCallback(CheckUnwind);
SetPrintfAndReportCallback(AppendToErrorMessageBuffer);
- __sanitizer_set_report_path(common_flags()->log_path);
-
- __asan_option_detect_stack_use_after_return =
- flags()->detect_stack_use_after_return;
-
__sanitizer::InitializePlatformEarly();
// Setup internal allocator callback.
@@ -460,6 +488,12 @@ static bool AsanInitInternal() {
allocator_options.SetFrom(flags(), common_flags());
InitializeAllocator(allocator_options);
+ // Apply ASan flags.
+ // NOTE: In order for options specified through `__asan_default_options` to be honored on Windows,
+ // it is necessary for those options to be configured inside the `ApplyOptions` method.
+ // See the function-level comment for `ApplyFlags` for more details.
+ ApplyFlags();
+
if (SANITIZER_START_BACKGROUND_THREAD_IN_ASAN_INTERNAL)
MaybeStartBackgroudThread();
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Converting to draft until clang-format is applied. |
…dajusto/add-win-apply-allocator-options
|
Just leaving a friendly "Ping" here. Thanks! |
compiler-rt/lib/asan/asan_rtl.cpp
Outdated
| InitializeFlags(); | ||
|
|
||
| // NOTE: The sleep before/after init` flags will not work on Windows when set |
There was a problem hiding this comment.
This comment does not belong here, maybe to WaitForDebugger implementation.
But it's irrelevant to the PR anyway
There was a problem hiding this comment.
Removed: ba740aa
compiler-rt/lib/asan/asan_rtl.cpp
Outdated
| static bool AsanInitInternal() { | ||
| if (LIKELY(AsanInited())) | ||
| return true; | ||
| SanitizerToolName = "AddressSanitizer"; | ||
|
|
||
| CacheBinaryName(); | ||
|
|
||
| // Initialize flags. This must be done early, because most of the | ||
| // initialization steps look at flags(). | ||
| // Initialize flags and register weak function callbacks for windows. |
There was a problem hiding this comment.
It reads as the function is windows specific.
Maybe something like:
Initialize flags. On Windows it also also register weak function callbacks.
There was a problem hiding this comment.
Good point, I had tunnel vision on this one. Thanks! Incorporated: 0728fb8
|
I'll mark this as 'draft until I add this test: #122990 (comment) |
|
Just leaving a friendly "Ping" here. Thank you :) |
|
Hi @vitalybuka, sorry to bother again - just wanted to keep this in your radar. Please let me know if there's a better channel/way to bump a PR as well, I want to work through the proper mechanisms here. |
|
Unless you click re-request review spinner in "reviewers" section, GitHub does shows this as waiting for review. |
|
I'll take a better look this week, and I've added folks who are more involved with Windows. |
There was a problem hiding this comment.
Is this a consequence of our choice to build the ASan RTL as a DLL? It seems like the simple case of a static executable (Chrome/Edge) with a static ASan runtime would've worked out of the box in the old model, but now that we've moved everything into a DLL, the ways that we emulate weak functions are not as transparent.
Regardless, this seems like a good change and a cost of doing business in the PE/COFF model.
I couldn't say with confidence one way or another, that decision predates me. But I think it's likely you're correct.
Thanks. Long term - I think the initialization codepath on windows would benefit from the ability to delay until the user function is available for invocation. Today's model, where we leverage a callback to correct for missing configuration, is a bit complex and leaves the door open to bugs like this one. But anyways, I'm not ready to tackle something like that at this time, but it's on my radar as a possible improvement :) . |
|
Just a friendly ping here (I hope I'm not being too noisy, trying to keep these roughly one week apart). |
|
Another friendly ping, @vitalybuka. Thanks, hope I'm not being too spammy here. |
|
Just leaving another ping, thanks a lot, and sorry for the spam. Won't ping again for another week. |
|
Another friendly ping here :-) . Thanks! |
|
@vitalybuka / @rnk just another ping here, thanks! |
|
ping @vitalybuka / @zmodem, my apologies if this is spammy, just not sure if this is falling under the radar. Thanks! |
There was a problem hiding this comment.
I think this looks good, but let @vitalybuka provide feedback. Thanks for the patch.
|
Thanks for the patch, and sorry for unnecessary delayed review. |
Related to: #117925
Follow up to: #117929
Context:
As noted in the linked issue, some ASan configuration flags are not honored on Windows when set through the
__asan_default_optionsuser function. The reason for this is that__asan_default_optionsis not available by the timeAsanInitInternalexecutes, which is responsible for applying the ASan flags.To fix this properly, we'll probably need a deep re-design of ASan initialization so that it is consistent across OS'es.
In the meantime, this PR offers a practical workaround.
This PR: refactors part of
AsanInitInternalso that idempotent flag-applying steps are extracted into a new functionApplyOptions. This function is also invoked in the "weak function callback" on Windows (which gets called when__asan_default_optionsis available) so that, if any flags were set through the user-function, they are safely applied then.Today,
ApplyOptionscontains only a subset of flags. My hope is thatApplyOptionswill over time, through incremental refactoringsAsanInitInternalso that all flags are eventually honored.Other minor changes:
ApplyAllocatorOptionshelper method, needed to implementApplyOptionsfor allocator options without re-initializing the entire allocator. Reinitializing the entire allocator is expensive, as it may do a whole pass over all the marked memory. To my knowledge, this isn't needed for the options captured inApplyAllocatorOptions.ProcessFlagstoValidateFlags, which seems like a more accurate name to what that function does, and prevents confusion when compared to the newApplyOptionsfunction.