[asan][windows] Eliminate the static asan runtime on windows #81677
Conversation
barcharcraz
added
compiler-rt:asan
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-compiler-rt-sanitizer Author: Charlie Barto (barcharcraz) ChangesThis is one of the major changes we (Microsoft) have made in the version of asan we ship with Visual Studio. Here's the description of these changes from our internal PR
In addition:
I have tried to separate things into reasonable commits to make reviewing easier. Currently this is marked as a draft because there are a few test failures that popped up when I rebased, as well as some failures on mingw. Patch is 154.09 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81677.diff 71 Files Affected:
diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index 56d497eb4c32b8..c631e5d828ccfd 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -900,10 +900,16 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
DiagnoseErrors);
}
- SharedRuntime =
- Args.hasFlag(options::OPT_shared_libsan, options::OPT_static_libsan,
- TC.getTriple().isAndroid() || TC.getTriple().isOSFuchsia() ||
- TC.getTriple().isOSDarwin());
+ SharedRuntime = Args.hasFlag(
+ options::OPT_shared_libsan, options::OPT_static_libsan,
+ TC.getTriple().isAndroid() || TC.getTriple().isOSFuchsia() ||
+ TC.getTriple().isOSDarwin() || TC.getTriple().isOSWindows());
+ if (!SharedRuntime && TC.getTriple().isOSWindows()) {
+ Arg *A =
+ Args.getLastArg(options::OPT_shared_libsan, options::OPT_static_libsan);
+ D.Diag(clang::diag::err_drv_unsupported_opt_for_target)
+ << A->getSpelling() << TC.getTriple().str();
+ }
ImplicitCfiRuntime = TC.getTriple().isAndroid();
diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index 396522225158de..3812246da1d25e 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -196,10 +196,10 @@ void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
if (TC.getSanitizerArgs(Args).needsAsanRt()) {
CmdArgs.push_back(Args.MakeArgString("-debug"));
CmdArgs.push_back(Args.MakeArgString("-incremental:no"));
- if (TC.getSanitizerArgs(Args).needsSharedRt() ||
- Args.hasArg(options::OPT__SLASH_MD, options::OPT__SLASH_MDd)) {
- for (const auto &Lib : {"asan_dynamic", "asan_dynamic_runtime_thunk"})
- CmdArgs.push_back(TC.getCompilerRTArgString(Args, Lib));
+ CmdArgs.push_back(TC.getCompilerRTArgString(Args, "asan_dynamic"));
+ auto defines = Args.getAllArgValues(options::OPT_D);
+ if (Args.hasArg(options::OPT__SLASH_MD, options::OPT__SLASH_MDd) ||
+ find(begin(defines), end(defines), "_DLL") != end(defines)) {
// Make sure the dynamic runtime thunk is not optimized out at link time
// to ensure proper SEH handling.
CmdArgs.push_back(Args.MakeArgString(
@@ -208,19 +208,15 @@ void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
: "-include:__asan_seh_interceptor"));
// Make sure the linker consider all object files from the dynamic runtime
// thunk.
- CmdArgs.push_back(Args.MakeArgString(std::string("-wholearchive:") +
+ CmdArgs.push_back(Args.MakeArgString(
+ std::string("-wholearchive:") +
TC.getCompilerRT(Args, "asan_dynamic_runtime_thunk")));
- } else if (DLL) {
- CmdArgs.push_back(TC.getCompilerRTArgString(Args, "asan_dll_thunk"));
} else {
- for (const auto &Lib : {"asan", "asan_cxx"}) {
- CmdArgs.push_back(TC.getCompilerRTArgString(Args, Lib));
- // Make sure the linker consider all object files from the static lib.
- // This is necessary because instrumented dlls need access to all the
- // interface exported by the static lib in the main executable.
- CmdArgs.push_back(Args.MakeArgString(std::string("-wholearchive:") +
- TC.getCompilerRT(Args, Lib)));
- }
+ // Make sure the linker consider all object files from the static runtime
+ // thunk.
+ CmdArgs.push_back(Args.MakeArgString(
+ std::string("-wholearchive:") +
+ TC.getCompilerRT(Args, "asan_static_runtime_thunk")));
}
}
diff --git a/compiler-rt/CMakeLists.txt b/compiler-rt/CMakeLists.txt
index bbb4e8d7c333e4..ed0a13c10be962 100644
--- a/compiler-rt/CMakeLists.txt
+++ b/compiler-rt/CMakeLists.txt
@@ -375,8 +375,12 @@ if("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "s390x")
endif()
if(MSVC)
- # FIXME: In fact, sanitizers should support both /MT and /MD, see PR20214.
- set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded)
+
+ # asan on windows only supports the release dll version of the runtimes, in the interest of
+ # only having one asan dll to support/test, having asan statically linked
+ # with the runtime might be possible, but it multiplies the number of scenerios to test.
+ # the program USING sanitizers can use whatever version of the runtime it wants to.
+ set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreadedDLL)
# Remove any /M[DT][d] flags, and strip any definitions of _DEBUG.
# Since we're using CMAKE_MSVC_RUNTIME_LIBRARY (CMP0091 set to NEW),
diff --git a/compiler-rt/lib/asan/CMakeLists.txt b/compiler-rt/lib/asan/CMakeLists.txt
index f83ae82d42935a..bd973479fe9724 100644
--- a/compiler-rt/lib/asan/CMakeLists.txt
+++ b/compiler-rt/lib/asan/CMakeLists.txt
@@ -32,6 +32,20 @@ set(ASAN_SOURCES
asan_win.cpp
)
+if(WIN32)
+ set(ASAN_DYNAMIC_RUNTIME_THUNK_SOURCES
+ asan_globals_win.cpp
+ asan_win_common_runtime_thunk.cpp
+ asan_win_dynamic_runtime_thunk.cpp
+ )
+ set(ASAN_STATIC_RUNTIME_THUNK_SOURCES
+ asan_globals_win.cpp
+ asan_malloc_win_thunk.cpp
+ asan_win_common_runtime_thunk.cpp
+ asan_win_static_runtime_thunk.cpp
+ )
+endif()
+
if (NOT WIN32 AND NOT APPLE)
list(APPEND ASAN_SOURCES
asan_interceptors_vfork.S
@@ -85,6 +99,9 @@ SET(ASAN_HEADERS
include_directories(..)
set(ASAN_CFLAGS ${SANITIZER_COMMON_CFLAGS})
+
+append_list_if(MSVC /Zl ASAN_CFLAGS)
+
set(ASAN_COMMON_DEFINITIONS ${COMPILER_RT_ASAN_SHADOW_SCALE_DEFINITION})
append_rtti_flag(OFF ASAN_CFLAGS)
@@ -133,7 +150,7 @@ append_list_if(MINGW "${MINGW_LIBRARIES}" ASAN_DYNAMIC_LIBS)
add_compiler_rt_object_libraries(RTAsan_dynamic
OS ${SANITIZER_COMMON_SUPPORTED_OS}
ARCHS ${ASAN_SUPPORTED_ARCH}
- SOURCES ${ASAN_SOURCES} ${ASAN_CXX_SOURCES}
+ SOURCES ${ASAN_SOURCES}
ADDITIONAL_HEADERS ${ASAN_HEADERS}
CFLAGS ${ASAN_DYNAMIC_CFLAGS}
DEFS ${ASAN_DYNAMIC_DEFINITIONS})
@@ -218,46 +235,52 @@ else()
RTSanitizerCommonSymbolizerInternal
RTLSanCommon
RTUbsan)
+ if (NOT WIN32)
+ add_compiler_rt_runtime(clang_rt.asan
+ STATIC
+ ARCHS ${ASAN_SUPPORTED_ARCH}
+ OBJECT_LIBS RTAsan_preinit
+ RTAsan
+ ${ASAN_COMMON_RUNTIME_OBJECT_LIBS}
+ CFLAGS ${ASAN_CFLAGS}
+ DEFS ${ASAN_COMMON_DEFINITIONS}
+ PARENT_TARGET asan)
- add_compiler_rt_runtime(clang_rt.asan
- STATIC
- ARCHS ${ASAN_SUPPORTED_ARCH}
- OBJECT_LIBS RTAsan_preinit
- RTAsan
- ${ASAN_COMMON_RUNTIME_OBJECT_LIBS}
- CFLAGS ${ASAN_CFLAGS}
- DEFS ${ASAN_COMMON_DEFINITIONS}
- PARENT_TARGET asan)
-
- add_compiler_rt_runtime(clang_rt.asan_cxx
- STATIC
- ARCHS ${ASAN_SUPPORTED_ARCH}
- OBJECT_LIBS RTAsan_cxx
- RTUbsan_cxx
- CFLAGS ${ASAN_CFLAGS}
- DEFS ${ASAN_COMMON_DEFINITIONS}
- PARENT_TARGET asan)
+ add_compiler_rt_runtime(clang_rt.asan_cxx
+ STATIC
+ ARCHS ${ASAN_SUPPORTED_ARCH}
+ OBJECT_LIBS RTAsan_cxx
+ RTUbsan_cxx
+ CFLAGS ${ASAN_CFLAGS}
+ DEFS ${ASAN_COMMON_DEFINITIONS}
+ PARENT_TARGET asan)
- add_compiler_rt_runtime(clang_rt.asan_static
- STATIC
- ARCHS ${ASAN_SUPPORTED_ARCH}
- OBJECT_LIBS RTAsan_static
- CFLAGS ${ASAN_CFLAGS}
- DEFS ${ASAN_COMMON_DEFINITIONS}
- PARENT_TARGET asan)
+ add_compiler_rt_runtime(clang_rt.asan_static
+ STATIC
+ ARCHS ${ASAN_SUPPORTED_ARCH}
+ OBJECT_LIBS RTAsan_static
+ CFLAGS ${ASAN_CFLAGS}
+ DEFS ${ASAN_COMMON_DEFINITIONS}
+ PARENT_TARGET asan)
- add_compiler_rt_runtime(clang_rt.asan-preinit
- STATIC
- ARCHS ${ASAN_SUPPORTED_ARCH}
- OBJECT_LIBS RTAsan_preinit
- CFLAGS ${ASAN_CFLAGS}
- DEFS ${ASAN_COMMON_DEFINITIONS}
- PARENT_TARGET asan)
+ add_compiler_rt_runtime(clang_rt.asan-preinit
+ STATIC
+ ARCHS ${ASAN_SUPPORTED_ARCH}
+ OBJECT_LIBS RTAsan_preinit
+ CFLAGS ${ASAN_CFLAGS}
+ DEFS ${ASAN_COMMON_DEFINITIONS}
+ PARENT_TARGET asan)
+ endif()
foreach(arch ${ASAN_SUPPORTED_ARCH})
if (COMPILER_RT_HAS_VERSION_SCRIPT)
+ if(WIN32)
+ set(SANITIZER_RT_VERSION_LIST_LIBS clang_rt.asan-${arch})
+ else()
+ set(SANITIZER_RT_VERSION_LIST_LIBS clang_rt.asan-${arch} clang_rt.asan_cxx-${arch})
+ endif()
add_sanitizer_rt_version_list(clang_rt.asan-dynamic-${arch}
- LIBS clang_rt.asan-${arch} clang_rt.asan_cxx-${arch}
+ LIBS ${SANITIZER_RT_VERSION_LIST_LIBS}
EXTRA asan.syms.extra)
set(VERSION_SCRIPT_FLAG
-Wl,--version-script,${CMAKE_CURRENT_BINARY_DIR}/clang_rt.asan-dynamic-${arch}.vers)
@@ -275,25 +298,11 @@ else()
endif()
set(ASAN_DYNAMIC_WEAK_INTERCEPTION)
- if (WIN32)
- add_compiler_rt_object_libraries(AsanWeakInterception
- ${SANITIZER_COMMON_SUPPORTED_OS}
- ARCHS ${arch}
- SOURCES
- asan_win_weak_interception.cpp
- CFLAGS ${ASAN_CFLAGS} -DSANITIZER_DYNAMIC
- DEFS ${ASAN_COMMON_DEFINITIONS})
- set(ASAN_DYNAMIC_WEAK_INTERCEPTION
- AsanWeakInterception
- UbsanWeakInterception
- SancovWeakInterception
- SanitizerCommonWeakInterception)
- endif()
-
add_compiler_rt_runtime(clang_rt.asan
SHARED
ARCHS ${arch}
OBJECT_LIBS ${ASAN_COMMON_RUNTIME_OBJECT_LIBS}
+ RTAsan_cxx
RTAsan_dynamic
# The only purpose of RTAsan_dynamic_version_script_dummy is to
# carry a dependency of the shared runtime on the version script.
@@ -321,36 +330,12 @@ else()
endif()
if (WIN32)
- add_compiler_rt_object_libraries(AsanDllThunk
- ${SANITIZER_COMMON_SUPPORTED_OS}
- ARCHS ${arch}
- SOURCES asan_globals_win.cpp
- asan_win_dll_thunk.cpp
- CFLAGS ${ASAN_CFLAGS} -DSANITIZER_DLL_THUNK
- DEFS ${ASAN_COMMON_DEFINITIONS})
-
- add_compiler_rt_runtime(clang_rt.asan_dll_thunk
- STATIC
- ARCHS ${arch}
- OBJECT_LIBS AsanDllThunk
- UbsanDllThunk
- SancovDllThunk
- SanitizerCommonDllThunk
- SOURCES $<TARGET_OBJECTS:RTInterception.${arch}>
- PARENT_TARGET asan)
-
set(DYNAMIC_RUNTIME_THUNK_CFLAGS "-DSANITIZER_DYNAMIC_RUNTIME_THUNK")
- if(MSVC)
- list(APPEND DYNAMIC_RUNTIME_THUNK_CFLAGS "-Zl")
- elseif(CMAKE_C_COMPILER_ID MATCHES Clang)
- list(APPEND DYNAMIC_RUNTIME_THUNK_CFLAGS "-nodefaultlibs")
- endif()
add_compiler_rt_object_libraries(AsanDynamicRuntimeThunk
${SANITIZER_COMMON_SUPPORTED_OS}
ARCHS ${arch}
- SOURCES asan_globals_win.cpp
- asan_win_dynamic_runtime_thunk.cpp
+ SOURCES ${ASAN_DYNAMIC_RUNTIME_THUNK_SOURCES}
CFLAGS ${ASAN_CFLAGS} ${DYNAMIC_RUNTIME_THUNK_CFLAGS}
DEFS ${ASAN_COMMON_DEFINITIONS})
@@ -358,12 +343,35 @@ else()
STATIC
ARCHS ${arch}
OBJECT_LIBS AsanDynamicRuntimeThunk
- UbsanDynamicRuntimeThunk
- SancovDynamicRuntimeThunk
- SanitizerCommonDynamicRuntimeThunk
+ UbsanRuntimeThunk
+ SancovRuntimeThunk
+ SanitizerRuntimeThunk
CFLAGS ${ASAN_CFLAGS} ${DYNAMIC_RUNTIME_THUNK_CFLAGS}
DEFS ${ASAN_COMMON_DEFINITIONS}
PARENT_TARGET asan)
+
+ # mingw does not support static linkage of the CRT
+ if(NOT MINGW)
+ set(STATIC_RUNTIME_THUNK_CFLAGS "-DSANITIZER_STATIC_RUNTIME_THUNK")
+
+ add_compiler_rt_object_libraries(AsanStaticRuntimeThunk
+ ${SANITIZER_COMMON_SUPPORTED_OS}
+ ARCHS ${arch}
+ SOURCES ${ASAN_STATIC_RUNTIME_THUNK_SOURCES}
+ CFLAGS ${ASAN_DYNAMIC_CFLAGS} ${STATIC_RUNTIME_THUNK_CFLAGS}
+ DEFS ${ASAN_DYNAMIC_DEFINITIONS})
+
+ add_compiler_rt_runtime(clang_rt.asan_static_runtime_thunk
+ STATIC
+ ARCHS ${arch}
+ OBJECT_LIBS AsanStaticRuntimeThunk
+ UbsanRuntimeThunk
+ SancovRuntimeThunk
+ SanitizerRuntimeThunk
+ CFLAGS ${ASAN_DYNAMIC_CFLAGS} ${STATIC_RUNTIME_THUNK_CFLAGS}
+ DEFS ${ASAN_DYNAMIC_DEFINITIONS}
+ PARENT_TARGET asan)
+ endif()
endif()
endforeach()
endif()
diff --git a/compiler-rt/lib/asan/asan_flags.cpp b/compiler-rt/lib/asan/asan_flags.cpp
index 23989843323211..56deb1b0d082b8 100644
--- a/compiler-rt/lib/asan/asan_flags.cpp
+++ b/compiler-rt/lib/asan/asan_flags.cpp
@@ -11,14 +11,16 @@
// ASan flag parsing logic.
//===----------------------------------------------------------------------===//
-#include "asan_activation.h"
#include "asan_flags.h"
+
+#include "asan_activation.h"
#include "asan_interface_internal.h"
#include "asan_stack.h"
#include "lsan/lsan_common.h"
#include "sanitizer_common/sanitizer_common.h"
-#include "sanitizer_common/sanitizer_flags.h"
#include "sanitizer_common/sanitizer_flag_parser.h"
+#include "sanitizer_common/sanitizer_flags.h"
+#include "sanitizer_common/sanitizer_win_interception.h"
#include "ubsan/ubsan_flags.h"
#include "ubsan/ubsan_platform.h"
@@ -47,7 +49,21 @@ static void RegisterAsanFlags(FlagParser *parser, Flags *f) {
#undef ASAN_FLAG
}
-void InitializeFlags() {
+static void DisplayHelpMessages(FlagParser *parser) {
+ // TODO(eugenis): dump all flags at verbosity>=2?
+ if (Verbosity()) {
+ ReportUnrecognizedFlags();
+ }
+
+ if (common_flags()->help) {
+ parser->PrintFlagDescriptions();
+ }
+}
+
+static void InitializeDefaultFlags() {
+ Flags *f = flags();
+ FlagParser asan_parser;
+
// Set the default values and prepare for parsing ASan and common flags.
SetCommonFlagsDefaults();
{
@@ -60,10 +76,8 @@ void InitializeFlags() {
cf.exitcode = 1;
OverrideCommonFlags(cf);
}
- Flags *f = flags();
f->SetDefaults();
- FlagParser asan_parser;
RegisterAsanFlags(&asan_parser, f);
RegisterCommonFlags(&asan_parser);
@@ -126,13 +140,12 @@ void InitializeFlags() {
InitializeCommonFlags();
- // TODO(eugenis): dump all flags at verbosity>=2?
- if (Verbosity()) ReportUnrecognizedFlags();
+ // TODO(samsonov): print all of the flags (ASan, LSan, common).
+ DisplayHelpMessages(&asan_parser);
+}
- if (common_flags()->help) {
- // TODO(samsonov): print all of the flags (ASan, LSan, common).
- asan_parser.PrintFlagDescriptions();
- }
+static void ProcessFlags() {
+ Flags *f = flags();
// Flag validation:
if (!CAN_SANITIZE_LEAKS && common_flags()->detect_leaks) {
@@ -199,6 +212,67 @@ void InitializeFlags() {
}
}
+void InitializeFlags() {
+ InitializeDefaultFlags();
+ ProcessFlags();
+
+#if SANITIZER_WINDOWS
+ // On Windows, weak symbols 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
+ // __asan_default_options, so we add a callback to be run
+ // when it is registered with the runtime.
+
+ // There is theoretically time between the initial ProcessFlags and
+ // registering the weak callback where a weak function could be added and we
+ // would miss it, but in practice, InitializeFlags will always happen under
+ // the loader lock (if built as a DLL) and so will any calls to
+ // __sanitizer_register_weak_function.
+ AddRegisterWeakFunctionCallback(
+ reinterpret_cast<uptr>(__asan_default_options), []() {
+ FlagParser asan_parser;
+
+ RegisterAsanFlags(&asan_parser, flags());
+ RegisterCommonFlags(&asan_parser);
+ asan_parser.ParseString(__asan_default_options());
+
+ DisplayHelpMessages(&asan_parser);
+ ProcessFlags();
+ });
+
+# if CAN_SANITIZE_UB
+ AddRegisterWeakFunctionCallback(
+ reinterpret_cast<uptr>(__ubsan_default_options), []() {
+ FlagParser ubsan_parser;
+
+ __ubsan::RegisterUbsanFlags(&ubsan_parser, __ubsan::flags());
+ RegisterCommonFlags(&ubsan_parser);
+ ubsan_parser.ParseString(__ubsan_default_options());
+
+ // To match normal behavior, do not print UBSan help.
+ ProcessFlags();
+ });
+# endif
+
+# if CAN_SANITIZE_LEAKS
+ AddRegisterWeakFunctionCallback(
+ reinterpret_cast<uptr>(__lsan_default_options), []() {
+ FlagParser lsan_parser;
+
+ __lsan::RegisterLsanFlags(&lsan_parser, __lsan::flags());
+ RegisterCommonFlags(&lsan_parser);
+ lsan_parser.ParseString(__lsan_default_options());
+
+ // To match normal behavior, do not print LSan help.
+ ProcessFlags();
+ });
+# endif
+
+#endif
+}
+
} // namespace __asan
SANITIZER_INTERFACE_WEAK_DEF(const char*, __asan_default_options, void) {
diff --git a/compiler-rt/lib/asan/asan_globals_win.cpp b/compiler-rt/lib/asan/asan_globals_win.cpp
index 19af88ab12b40a..8267f07b9cce49 100644
--- a/compiler-rt/lib/asan/asan_globals_win.cpp
+++ b/compiler-rt/lib/asan/asan_globals_win.cpp
@@ -28,7 +28,9 @@ static void call_on_globals(void (*hook)(__asan_global *, uptr)) {
__asan_global *end = &__asan_globals_end;
uptr bytediff = (uptr)end - (uptr)start;
if (bytediff % sizeof(__asan_global) != 0) {
-#if defined(SANITIZER_DLL_THUNK) || defined(SANITIZER_DYNAMIC_RUNTIME_THUNK)
+# if defined(SANITIZER_DLL_THUNK) || \
+ defined(SANITIZER_DYNAMIC_RUNTIME_THUNK) || \
+ defined(SANITIZER_STATIC_RUNTIME_THUNK)
__debugbreak();
#else
CHECK("corrupt asan global array");
diff --git a/compiler-rt/lib/asan/asan_interceptors.cpp b/compiler-rt/lib/asan/asan_interceptors.cpp
index 4de2fa356374a6..1ca54d404bcaa3 100644
--- a/compiler-rt/lib/asan/asan_interceptors.cpp
+++ b/compiler-rt/lib/asan/asan_interceptors.cpp
@@ -558,6 +558,17 @@ INTERCEPTOR(char *, strcpy, char *to, const char *from) {
return REAL(strcpy)(to, from);
}
+// Windows doesn't always define the strdup identifier,
+// and when it does it's a macro defined to either _strdup
+// or _strdup_dbg, _strdup_dbg ends up calling _strdup, so
+// we want to intercept that. push/pop_macro are used to avoid problems
+// if this file ends up including <string.h> in the future.
+# if SANITIZER_WINDOWS
+# pragma push_macro("strdup")
+# undef strdup
+# define strdup _strdup
+# endif
+
INTERCEPTOR(char*, strdup, const char *s) {
void *ctx;
ASAN_INTERCEPTOR_ENTER(ctx, strdup);
@@ -575,7 +586,7 @@ INTERCEPTOR(char*, strdup, const char *s) {
return reinterpret_cast<char*>(new_mem);
}
-#if ASAN_INTERCEPT___STRDUP
+# if ASAN_INTERCEPT___STRDUP
INTERCEPTOR(char*, __strdup, const char *s) {
void *ctx;
ASAN_INTERCEPTOR_ENTER(ctx, strdup);
@@ -758,7 +769,7 @@ void InitializeAsanInterceptors() {
ASAN_INTERCEPT_FUNC(strncat);
ASAN_INTERCEPT_FUNC(strncpy);
ASAN_INTERCEPT_FUNC(strdup);
-#if ASAN_INTERCEPT___STRDUP
+# if ASAN_INTERCEPT___STRDUP
ASAN_INTERCEPT_FUNC(__strdup);
#endif
#if ASAN_INTERCEPT_INDEX && ASAN_USE_ALIAS_ATTRIBUTE_FOR_INDEX
@@ -849,6 +860,10 @@ void InitializeAsanInterceptors() {
VReport(1, "AddressSanitizer: libc interceptors initialized\n");
}
+# if SANITIZER_WINDOWS
+# pragma pop_macro("strdup")
+# endif
+
} // namespace __asan
#endif // !SANITIZER_FUCHSIA
diff --git a/compiler-rt/lib/asan/asan_malloc_win.cpp b/compiler-rt/lib/asan/asan_malloc_win.cpp
index 7e1d04c36dd580..3278f072198769 100644
--- a/compiler-rt/lib/asan/asan_malloc_win.cpp
+++ b/compiler-rt/lib/asan/asan_malloc_win.cpp
@@ -58,97 +58,69 @@ using namespace __asan;
// MD: Memory allocation functions are defined in the CRT .dll,
// so we have to intercept them before they are called for the first time.
-#if ASAN_DYNAMIC
-# define ALLOCATION_FUNCTION_ATTRIBUTE
-#else
-# define ALLOCATION_FUNCTION_ATTRIBUTE SANITIZER_INTERFACE_ATTRIBUTE
-#endif
-
extern "C" {
-ALLOCATION_FUNCTION_ATTRIBUTE
-size_t _msize(void *ptr) {
+__declspec(noinline) size_t _msize(void *ptr) {
GET_CURRENT_PC_BP_SP;
(void)sp;
return asan_malloc_usable_size(ptr, pc, bp);
}
-ALLOCATION_FUNCTION_ATTRIBUTE
-size_t _msize_base(void *ptr) {
- return _msize...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
Added some folks who may know how Asan in used by Chromium on Windows. |
Sure, the reason is to greatly simplify how asan works on windows, and to fix a bunch of bugs with asan+static CRT. Before this change there was a bunch of code that would redirect calls from various dlls into the executable if the asan runtime was statically linked, this code has never worked all that well, and it was a nightmare to fix. Additionally, statically linked DLLs didn't work with the previous scheme and weak symbols didn't work with /MD. |
|
I think @rnk has the deepest knowledge here. Just to double check, when you say "Eliminate the static asan runtime", you mean the use case it was supporting will still keep working, but in a different way right? |
Yes. The static asan runtime didn't work very well in the first place, so this actually improves support for those use-cases. The static runtime is still supported on non-windows systems, however. The whole static linked asan runtime library is gone on windows, but you can still use the dynamic asan runtime with a statically linked CRT. The core reasoning is that asan is a "only one allowed per process" type thing (you can't have one copy of the asan runtime handling a malloc while a different one handles the corresponding free). To get this on windows you really need to use a DLL or some really, really horrible hacks that essentially amounted to doing yourself what the loader already does for DLLs. |
barcharcraz
force-pushed
the
dev/chbarto/static_with_dynamic
branch
from
c7eaa51 to
e7126d5
Compare
Not to distract from the direction taken here (which I do agree seems reasonable, even if I haven't had time to look closer at the PR yet) - but, when using the static CRT in general, doesn't the same concern apply there as well? I.e. when using the static CRT, care must be taken that the same CRT instances does frees as did the allocation. But I guess there are other asan-related aspects that makes it even more of a mess. Also secondly, when discussing these details - how the asan runtime is built in one, specific way, while it is used for applications that can use a different CRT configuration - would you like to chime in on microsoft/vcpkg#34123 (comment)? There, some people involved questioned and weren't convinced that one part of a project (compiler-rt in llvm) should be allowed to override the CRT choice that is made for the whole vcpkg build. I tried to argue that compiler-rt is a very special case and doesn't interact with the rest of the libraries built in vcpkg like that, and compiler-rt is free to override the CRT to use internally. |
Yes, it does, but my understanding is that this requirement is slightly weaker than what asan needs. As long as you do match mallocs and frees things work fine, because the data that's shared is shared through kernel32.dll, ntdll.dll, or the kernel and page table. If asan allowed this it would need some way of mapping memory to the correct global structures, and some way of figuring out who gets to handle the shadow memory area and where to get the stack information for a given allocation upon an invalid access. We can't rely on "matching" allocations with invalid accesses because it's far from uncommon for a memory error to relate to memory allocated in another DLL. Maybe the static-linked asans could have some voting mechanism to settle on a "lead" copy of asan or something. It's not impossible but making things work well is a ton of work for something the loader can do anyway. Before this change we still relied on only one copy of asan being around, but the functions would be exported from the main exe with the "dll thunk" using GetProcAddress to call them.
It might be possible to support copies of the asan runtime built with the static CRT, it's just not that useful and we'd rather keep one configuration and spend time making it work perfectly for all instrumented programs. Edit: I forgot to mention, things do get more complicated. In some cases we should be keeping separate interceptors for each CRT instance and for the debug vs release CRTs. By intercepting both the release and debug version of certain functions we break errno. Internally we have a fix for this internally, but it's a bit messy as it involves stamping out many more interceptors. |
|
It occurs to me that this probably requires changes to the gyp build files. |
Ah, right, these are all very good explanations for why this is harder than the preexisting cases of multiple statically linked CRTs within the same process. Thanks!
Yeah, I totally agree, there's no practical need for allowing asan to be linked in another way. Especially as it works for both MSVC and mingw environments.
No, the extra build systems doesn't need to be updated in general, that's up to whoever maintains them to sync them afterwards. Regular contributors only need to make sure the cmake build works. (And unless one is able to test that the changes to the third party build systems actually is entirely right, it's usually better to leave them untouched, so it can be done cleanly in one commit, rather than do a half-broken update to the files.) |
|
The buildkite failure is just windows defender freaking out about one of cmake's test exes. Presumably it'll get sorted out next time it runs. |
|
I'm short on time, so I can't respond to all the discussion here, but at a high level, I agree with this direction and it's something I discussed with Microsoft folks working on ASan in the past, so I'm happy to see it come together. There is a great deal of build configuration complexity involved in supporting the static/dynamic variants. As mentioned, there should only ever be one copy of the ASan runtime active in a process address space. Making the static ASan build work properly required linking against different libraries depending on whether an EXE or a DLL was being produced, and this knowledge had to be encoded in every build system using ASan on Windows, since the linker is invoked directly, rather than running through the Clang driver. Ultimately, there is no real gain to be had from statically linking ASan. You ultimately have to make something equivalent to a shim import library for any DLL containing ASan instrumented code, so you might as well use the dynamic model and standardize. I think the biggest risk here is, as usual, portability of weak symbol registration schemes. I'll look at this some more and provide more feedback and hopefully other next steps. |
There was a problem hiding this comment.
Is it feasible to create a migration path from the static runtime to the dynamic runtime? As written, this requires users to update their ASan builds at the same time that they take this update. Is it possible to leave the static runtime behind, create a migration path to the dynamic runtime (add all the new weak symbol registration functionality), push that, and then follow up by removing the static runtime 2-4 weeks later?
The sanitizer runtimes are not ABI-stable, so I think we're free to remove the static runtimes without leaving behind some kind of compatibility layer. This isn't libc++.
I also don't want to overindex on Google's or Chromium's needs. Many folks use the open source releases produced every six months, so to really accommodate those users, we'd have to delay the removal across a release boundary. I'm not advocating for that, but it's worth considering depending on the cost.
I believe the first four patches can land separately if we agree on the direction.
compiler-rt/lib/sanitizer_common/sanitizer_win_thunk_interception.h
Outdated
Show resolved
Hide resolved
The new symbol registration mechanism depends on how the loader imports multiple DLLexports of the same symbol, maybe it could be adapted to work with the static runtime, but it's a bunch of work for something that will be immediately removed. We could also just ship a copy of the dynamic runtime import library under the same name as the old static runtime, perhaps emitting a warning someplace to let people know what's going on. So, for context: MSVC's link.exe uses a special /INFERASANLIBS option that picks the correct asan library automatically, and this PR should (hopefully, I haven't tested it yet) make that flag work with clang's version of asan as well, at least for release builds (MSVC ships with debug builds of asan as well, they're still linked to the release-dll variant of the CRT, they just have optimization turned off). When you pass /fsanitize=address to cl.exe it adds the equivalent of I'm not quite sure I understand the problem scenario. Is the trouble that if you're building with mingw's gcc then you need to invoke the linker manually because gcc doesn't know how to deal with link.exe (and you want to use link.exe over ld.exe because ld is slow and presumably not that well maintained on windows?). I think clang can invoke link.exe for you, just like any other linker, or is that only lld-link? cl can also invoke link for you, although I understand going through the compiler like that is less common on windows that on other platforms. |
|
@vitalybuka Here's a more detailed explination on the motivations behind this change from @amyw-msft, who is the original author of these changes on our side. (And who I'll add as a co-author using fixup commits) |
Thanks. Would be possible to extract summary paragraph and put on top of the main patch, with this URL. Also, to submit, it would be nice to do with smaller patches with some time between them to let bots test them separately. Or it's going to be annoying to revert/reland such large patch. Almost every item from description looks like a candidate for a patch. Relaxing |
Instead build only ONE asan runtime on windows. File Overview: * asan_malloc_win_thunk.cpp Contains interceptors for malloc/free for applications using the static CRT. These are intercepted with an oldnames style library that takes precedence over the CRT because of linker search order. This is used instead of the library interception routines used for other functions so that we can intercept mallocs that happen before our interceptors run. Some other CRT functions are also included here, because they are provided by the same objects as allocation functions in the debug CRT. * asan_win_common_runtime_thunk.cpp just the common init code for both static and dynamic CRTs * asan_win_static_runtime_thunk.cpp static CRT specific initialization * asan_win_dynamic_runtime_thunk.cpp dynamic crt initialization, most of the content that was here has been moved to the common runtime thunk * asan_win_dll_thunk.cpp This used to provide functionality to redirect calls from DLLs to asan instrumented functions in the main library, but this never worked that well and was a nightmare. It's gone now * sanitizer_common/sanitizer_common_interface.inc: The added functions are for the thunks to be able to delegate to the asan runtime DLL in order to override functions that live in the application executable at initialization. The ASAN dll can't do this itself because it doesn't have a way to get the addresses of these functions. * sanitizer_common/sanitizer_win_immortalize: This is just an implementation of call_once that doens't require the CRT or C++ stdlib. We need this because we need to do this sort of thing before the CRT has initialized. This infrastructure is kinda ugly, we're sorry. * sanitizer_common/sanitizer_win_interception.cpp: Provides the interface inside the sanitizer runtime DLL that instramented apps call to intercept stuff. * sanitizer_common/sanitizer_win_thunk_interception.cpp: this is the code to setup and run the static initializers and/or TLS initializers, implemented basically how any initializers are on windows, these ones are registered before all the CRT initializers. * sanitizer_common/sanitizer_win_thunk_interception.h INTERCEPT_LIBRARY_FUNCTION and REGISTER_WEAK_FUNCTION are the called initializers for each relevant function inside the instrumented binary. Note the optimizer is disabled for weak function registration routines because it detects that the two functions being compared have different names and deduces they must be the same, and no actual codegen for the if is required, causing an infinite loop. Clang does this in debug mode as well as release mode, and the cast to uintptr_t is required to suppress it in debug mode. Co-Authored-By: Amy Wishnousky <[email protected]>
Now that the static runtime is gone the required librares are different. Note that /MD or -D_DLL causes the dynamic runtime to get linked, this is a little gross but emulates the behavior of MSVC. "Real" msvc will link the debug build of asan if you specify /DEBUG or _DEBUG, but that's not really necessary and requires building multiple configurations from the same tree.
weak functions are registered after the asan runtime initializes. This means __asan_default_options isn't available during asan runtime initialization. Thus we split asan flag processing into two parts and register a callback for the second part that's executed after __asan_default_options is registered. Co-Authored-By: Amy Wishnousky <[email protected]>
ALLOCATION_FUNCTION_ATTRIBUTE wasn't used elsewhere, and was just one attribute, so abstracting it through a macro wasn't doing much good now that it's not conditional on runtime type. We're always in the dynamic runtime now so eliminate the preprocessor conditional. The new exported functions are the interface used by the intercepted malloc/free family in the instrumented binary to call the asan versions inside the dll runtime. Co-authored-by: Amy Wishnousky <[email protected]>
…runtime thunks and the fact we're "always" using the dynamic asan runtime. python formatting
… config After the windows static runtime is removed these static=static CRT and dynamic=dynamic CRT, both using the dynamic asan runtime.
This is required because now that there's only one asan runtime dll the dynamic/static CRT wholearchived runtime thunk "flavor" is determined by passing the /MD or /MDd options, or defining -D_DLL.
barcharcraz
force-pushed
the
dev/chbarto/static_with_dynamic
branch
from
2ef27cf to
7a46519
Compare
Re-Apply: 246234a Originally #81677 The static asan runtime on windows had various buggy hacks to ensure loaded dlls got the executable's copy of asan, these never worked all that well, so we have eliminated the static runtime altogether and made the dynamic runtime work for applications linking any flavor of the CRT. Among other things this allows non-asan-instrumented applications to load asan-instrumented dlls that link against the static CRT. Co-authored-by: Amy Wishnousky <[email protected]>
Contains test changes from llvm#81677 that are simple test changes. For the most part just makes the tests allow more flexibility in the callstacks produced by asan. Note: this PR has the exact changes from llvm#81677 in addition to a revert commit to remove the ones that require other things from that PR. This will be squashed, ofc. I just left things unsquashed for reviewing pleasure :). This is a non-functional-change
This is one of the major changes we (Microsoft) have made in the version of asan we ship with Visual Studio.
@amyw-msft wrote a blog post outlining this work at https://devblogs.microsoft.com/cppblog/msvc-address-sanitizer-one-dll-for-all-runtime-configurations/
Here's the description of these changes from our internal PR
In addition: