Diff Detail
Event Timeline
First try on Konstantin's suggestions in ML (https://groups.google.com/forum/#!topic/address-sanitizer/XJXOrSvN8vg).
Stuff that hasn't been addressed:
- platforms other than Linux
- support for other dynamic runtimes (UBSan, MSan, etc.)
- detection of static/dynamic rt conflict
- detection of dlopen
I think I found a reasonably robust way to detect that dynamic rt was either linked to exectable or preloaded (and thus can be used safely). I can simply check that it preceedes other dynamic libraries in list returned by dl_iterate_phdr. This would also prohibit dlopens.
Still not sure how to detect static/shared rt conflicts though.
Still not sure how to detect static/shared rt conflicts though.
One option would be to export some marker from executable linked with static libasan (e.g. local_asan_preinit symbol) and then check if it's present via dlsym() in asan_init if ASAN_DYNAMIC is defined.
Added support for
- detection of static/dynamic rt conflicts
- detection of dlopen
- make sure that libasan is preloaded
(all features have unit tests).
One huge problem is that currently Asan always starts in active mode because dlsym's in AsanCheckIncompatibleRT call dlerror, this calls malloc and malloc activates Asan. I'm not sure how to address the issue (I tried the calloc-style hack but this would cause errors when dlerror-allocated regions are freed).
As before, I haven't really checked anything besides Linux x64.
I decided to leave dynamic runtimes for UBSan, etc. for future. IMHO this would be a trivial piece of work anyway.
Resolved afore-mentioned problem with deactivation tests. Dlerror now uses same preinit hack as dlsym.
First round of comments.
cmake/Modules/AddCompilerRT.cmake | ||
---|---|---|
42 | Can you avoid duplicating the function, and instead generalize it? add_compiler_rt_runtime (<name> <arch> [SOURCES <sources>] [CFLAGS <cflags>] [DEFS <defs>] [SHARED]). | |
lib/asan/CMakeLists.txt | ||
132 | get rid of this and ASAN_DYNAMIC_RUNTIME_OBJECTS, just write all objects you need inside add_compiler_rt_runtime. | |
140–161 | accidental duplication? | |
150 | ldl or lpthread may not be available, see COMPILER_RT_HAS_LIBDL for example. | |
166 | Do you use symbols from dynamic runtime anywhere? | |
lib/asan/asan_rtl.cc | ||
632 | We generally assume that all ASAN_xxx compiler definitions should be defined. If they are not, we set the default value in the source file, see asan_internal.h |
CMakeLists.txt | ||
---|---|---|
233 | ASAN_BUILD_SHARED_RUNTIME | |
lib/asan/CMakeLists.txt | ||
77 | define this right after ASAN_COMMON_DEFINITIONS | |
lib/asan/asan_win.cc | ||
77 | Why don't we use UNIMPLEMENTED() here? | |
lib/sanitizer_common/sanitizer_internal_defs.h | ||
151 | Please don't do this. We should really specfiy this for each variable instead. |
lib/asan/asan_rtl.cc | ||
---|---|---|
563 | Can't you simply call __asan_init() here? It also calls AsanCheckIncompatibleRT, checks for asan_inited, and you may add a call to AsanCheckDynamicRTPrereqs under #if ASAN_DYNAMIC |
CMakeLists.txt | ||
---|---|---|
233 | Hm, are you sure? I thought all compiler-rt configs should start with COMPILER_RT... | |
cmake/Modules/AddCompilerRT.cmake | ||
42 | Well, there is already so much code dup (e.g. compare add_compiler_rt_static_runtime against add_compiler_rt_osx_static_runtime). Perhaps it should be addressed in a separate patch? | |
lib/asan/CMakeLists.txt | ||
77 | Yeah, already done locally. | |
132 | Right. | |
140–161 | Most probably result of code rewrite. | |
150 | Ok, I'll add checks then. What about adding these for static rt as well (in Clang driver)? | |
166 | No, probably not. I'll remove this. | |
lib/asan/asan_rtl.cc | ||
563 | No, because then __asan_init may then get called twice:
which will force ASan in activated state. Actually ability to start in deactivated state greatly complicates startup. | |
632 | Will do. | |
lib/asan/asan_win.cc | ||
77 | Well, they are still called from initialization code (like other empty functions in this file). | |
lib/sanitizer_common/sanitizer_internal_defs.h | ||
151 | This was Kostya's request and I think it makes sense. We require ASan runtime to be the first dynamic library to load so we may safely assume that initial-exec works. |
CMakeLists.txt | ||
---|---|---|
233 | Alright, let's then use COMPILER_RT_BUILD_SHARED_ASAN, "include" is a bit confusing to me (yep, I know we have COMPILER_RT_INCLUDE_TESTS named after LLVM_INCLUDE_TESTS, but maybe I'll rename it soon). | |
cmake/Modules/AddCompilerRT.cmake | ||
42 | the diff between the two macro you mention is much larger. In your case the difference is only in: SHARED/STATIC, LIBRARY_OUTPUT_DIRECTORY/ARCHIVE_OUTPUT_DIRECTORY and LIBRARY DESTINATION / ARCHIVE DESTINATION. | |
lib/asan/CMakeLists.txt | ||
150 | We already started to add toolchain-specific logic to adding -l... flags to the Clang driver (for FreeBSD port) | |
lib/asan/asan_rtl.cc | ||
563 | Then we should be able to somehow use AsanInitFromRtl(). | |
lib/asan/asan_win.cc | ||
77 | Looks like AsanCheckDynamicRTPrereqs() is called only in shared-libasan. It makes sense to print "UNIMPLEMENTED" on platforms where it's not available. Which reminds me of Mac - we use dynamic runtime there as well. Can you make sure that your changes will play well with it? Does it make sense to build it with -DASAN_DYNAMIC=1 as well? | |
lib/sanitizer_common/sanitizer_internal_defs.h | ||
151 | This is still confusing. sanitizer_internal_defs.h is included not only in ASan source files, but also in sanitizer_common source files, and you don't compile the latter with -DASAN_DYNAMIC=1 (and in general you shouldn't add sanitizer-specific defines to sanitizer_common code). |
CMakeLists.txt | ||
---|---|---|
233 | Ok. | |
cmake/Modules/AddCompilerRT.cmake | ||
42 |
Ok, then take a look osx/darwin. Anyway this isn't a big deal, I can refactor my macro. | |
lib/asan/CMakeLists.txt | ||
150 | Ok, will check. | |
lib/asan/asan_rtl.cc | ||
563 | I think it makes sense to detect this error ASAP and not wait till AsanInitFromRtl() gets called. By this time rt inconsistencies may already cause mysterious jumps to NULL, etc. | |
lib/asan/asan_win.cc | ||
77 |
Makes sense.
Yeah and it also uses a cool reexec trick which may be useful for other platforms as well (Linux, Android).
I think there is quite some potential for unification. I could work on this but I have practically no experience with Mac so I'd prefer to do this separately to avoid huge delays. | |
lib/sanitizer_common/sanitizer_internal_defs.h | ||
151 | Ok,I see the point but I'd still prefer not to save performance. Do you think building runtime with -ftls-model=initial-exec would work? |
Addressed Alexey's concerns. Remaining issues:
- need to test non-lit tests with shared runtime
- need to agree with Alex on where to check for incompatible runtimes
cmake/Modules/AddCompilerRT.cmake | ||
---|---|---|
82 | Use a single call: set_target_properties(${name} PROPERTIES ARCHIVE_OUTPUT_DIRECTORY ... LIBRARY_OUTPUT_DIRECTORY ...) | |
101 | Is it possible to use install(TARGETS ${name} ARCHIVE DESTINATION ... LIBRARY DESTINATION ...) in the add_compiler_rt_runtime above and get rid of add_compiler_rt_{static,shared}_runtime rules? | |
lib/asan/CMakeLists.txt | ||
57 | set(ASAN_DYNAMIC_LIBS stdc++ c m) | |
lib/asan/asan_linux.cc | ||
56 | Use more specific name (FindFirstDynamicLibraryCallback or some such). | |
81 | remove newline | |
89 | Weird indentation. | |
94 | Consider running tools/clang/tools/clang-format/clang-format-diff.py on your patch. | |
lib/asan/asan_malloc_linux.cc | ||
64 ↗ | (On Diff #8083) | Rename the variables - they are used not only in calloc now. |
66 ↗ | (On Diff #8083) | Use ALIGNED before the type (see comments in sanitizer_internal_defs.h) |
73 ↗ | (On Diff #8083) | uptr size_aligned = RoundUpTo(size, kWordSize); |
76 ↗ | (On Diff #8083) | CHECK_LT |
115 ↗ | (On Diff #8083) | This is just wrong. Is realloc() called before asan_inited or on PreinitAllocated pointers? |
test/asan/CMakeLists.txt | ||
44 | Can you instead just unconditionally provide the basename of dynamic ASan runtime (for i386, x86_64, powerpc64) when configuring a dynamic test suite (and not deal with this asan_rt_suffix)? | |
test/asan/TestCases/Linux/interception_malloc_test.cc | ||
13 | So, how does attribute((no_sanitize_address)) help here? | |
test/asan/lit.cfg | ||
43 | Why do you need this? | |
45 | and this? |
cmake/Modules/AddCompilerRT.cmake | ||
---|---|---|
101 | Yeah, this failed with separate installs but cumulative one seems to work. Renaming add_compiler_rt_static_runtime is going to cause some churn though... | |
lib/asan/CMakeLists.txt | ||
57 | I wonder whether I need 'c' at all. | |
lib/asan/asan_linux.cc | ||
89 | Yeah, moved from other place. | |
94 | Ok, good to know. | |
lib/asan/asan_malloc_linux.cc | ||
115 ↗ | (On Diff #8083) | AFAIR realloc() is called before asan_inited on PreinitAlloc-ated pointers (dlerror calls it to allocate string). So what's wrong exactly? |
test/asan/lit.cfg | ||
43 | To make a copy of clang_asan_cflags (they are changed below). | |
45 | Likewise. |
lib/asan/CMakeLists.txt | ||
---|---|---|
57 | I believe we don't | |
lib/asan/asan_malloc_linux.cc | ||
115 ↗ | (On Diff #8083) | if realloc(ptr, new_size) != ptr, you have to copy the contents to the newly allocated buffer. It's so sad we need to appease dlerror() with hacks like this. Can you clarify why you need to call it in AsanCheckIncompatibleRT? |
test/asan/lit.cfg | ||
35 | Can you instead rename this to clang_asan_static_cflags, and define clang_asan_cflags depending on config.asan_dynamic (so that these lists are immutable and defined in a single place)? |
lib/asan/asan_malloc_linux.cc | ||
---|---|---|
115 ↗ | (On Diff #8083) |
Agreed, won't work in general case. Was enough for dlerror because I was ignoring them anyway.
I search for __asan_static via dlsym() and this fails and calls dlerror().
It is. Unfortunately this seems to be the only reliable way to detect incompatible rts (which is super-important). |
test/asan/TestCases/Linux/interception_malloc_test.cc | ||
13 | I think malloc's frame gets poisoned which caused segfault because shadow is not initialized (malloc is called at the beginning of asan_init in dlsym). |
lib/asan/asan_malloc_linux.cc | ||
---|---|---|
115 ↗ | (On Diff #8083) | Can you search for ASan runtime (.so) in the contents of /proc/self/maps when initializing a static runtime? I think we can read it w/o calling malloc... Or to add asan_runtime_type .bss global, and initialize it in __asan_init(). Set it to 1 if ASAN_DYNAMIC is set, and to 2 otherwise, and detect inconsistencies. |
lib/asan/asan_linux.cc | ||
---|---|---|
94 | Are you sure the tool is usable? I see some strange changes like +extern "C" attribute(( and misplaced comments. |
lib/asan/asan_malloc_linux.cc | ||
---|---|---|
115 ↗ | (On Diff #8083) |
No, this won't protect from dlopening dynamic rt.
Yup, this might work. I'll see how it goes tomorrow. |
lib/asan/asan_linux.cc | ||
---|---|---|
94 | I'll try to get something from it but really noise ratio is just too high. | |
lib/asan/asan_malloc_linux.cc | ||
115 ↗ | (On Diff #8083) |
Ok, now I remember. We will get segfault instead of proper error message if we try to preload dynamic rt to statically sanitized executable. Here is what happens: interceptors in static runtime will be initialized using dlsym(RTLD_NEXT, ...) but they will find dynamic runtime's symbols instead of true libc symbols. So __interception::real_* pointers will remain NULLs. This will cause segfaults in e.g. MaybeInstallSigaction, etc. |
Updated based on comments. Stuff that was not addressed:
- ASAN_RT_SUFFIX
- asan_static/asan_dynamic are still there (I still don't see any alternative solution)
lib/asan/asan_linux.cc | ||
---|---|---|
71 | I could probably also use DumpListOfModules here. What's your opinion? | |
82 | I think we can do this:
But all this looks too complicated compared to current approach (dlsym + preinit allocator). |
lib/asan/asan_linux.cc | ||
---|---|---|
71 | Although the order of modules in address space does not necessarily match the order in which they were loaded by ld.so. Dl_iterate_phdr is probably more reliable... |
cmake/Modules/AddCompilerRT.cmake | ||
---|---|---|
73 | FYI I've submitted this renaming as r205183, so you can rebase your patch to minimize the diff. | |
77 | Why is the default OUTPUT_NAME not suitable here? | |
lib/asan/asan_linux.cc | ||
71 | Yeah. | |
82 | I think this would be easier to digest than these preinit-allocator hooks. I'd try to avoid calling functions from system libraries before ASan is initialized, by all means possible. Could you please try if this approach works? Does anyone else have other opinions about it? |
lib/asan/asan_linux.cc | ||
---|---|---|
82 | If we call AsanCheckIncompatibleRT later in initialization (after we initialized the interceptors, allocator, etc. and after we set asan_inited=1), will this still work and will it make preinit-allocator hooks unnecessary? |
cmake/Modules/AddCompilerRT.cmake | ||
---|---|---|
77 | Ok, got it. |
lib/asan/asan_linux.cc | ||
---|---|---|
82 |
We can't do this without sacrificing usability. As I pointed out in one of earlier comments this may cause segfaults long before initialization finishes. |
lib/asan/asan_linux.cc | ||
---|---|---|
82 | Ah, I see, this is the comment about MaybeInstallSigaction() problems. |
Changes in this revision:
- rebased
- get rid of preinit allocator (yay!)
- unittests now know about dynamic runtime
Awesome.
lib/asan/asan_linux.cc | ||
---|---|---|
88 | Could you use if (ASAN_DYNAMIC) { //... else { //... } instead of #ifdef here? Also please add a comment describing why you need /proc/self/maps iteration. | |
lib/asan/asan_rtl.cc | ||
630 | Do you still need these? | |
test/asan/TestCases/Linux/interception_malloc_test.cc | ||
13 | Is this still relevant? |
Addressed Alexey's concerns. Now this fails two tests:
AddressSanitizer-Unit :: Asan-i386-Dynamic-Test/AddressSanitizer.asm_rw AddressSanitizer-Unit :: Asan-i386-Dynamic-Test/AddressSanitizer.asm_rw ThreadSanitizer :: ignore_lib0.cc
because of unrelated errors (I've filed separate issues for these: #285 and #286.
LGTM
I think this looks good now. I will land this patch (maybe, in a series of commits) shortly.
I've landed this change as r205308, with a few fixes/enhancements. However,
if I configure/build the tree with -DCOMPILER_RT_BUILD_SHARED_ASAN=ON and
run "make check-asan",
some of the tests fail with the following linker errors:
llvm/projects/compiler-rt/test/asan/TestCases/stack-overflow.cc:105: error:
undefined reference to 'pthread_join'
I presume, this happens because we don't explicitly provide -lpthread in
the clang invocation in RUN: line. "-lpthread" is added automatically if we
link with static ASan runtime, but in your patch
to the Clang driver you explicitly changed this behavior for
-shared-libasan. I had to add "-lpthread" to CMake rules for building
instrumented ASan unit test as well. What am I doing wrong?
Why linking with dynamic ASan runtime doesn't pulls in libpthread?
ASAN_BUILD_SHARED_RUNTIME