This is an archive of the discontinued LLVM Phabricator instance.

Optional support for dynamic Asan runtime
ClosedPublic

Authored by ygribov on Mar 11 2014, 7:34 AM.

Details

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
ygribov updated this revision to Unknown Object (????).Mar 11 2014, 7:41 AM

Forgot to add new files (as usual).

One more thing that would be useful to verify:

  • make sure that libasan is preloaded

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.

ygribov updated this revision to Unknown Object (????).Mar 13 2014, 9:35 AM

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.

ygribov updated this revision to Unknown Object (????).Mar 13 2014, 9:41 AM

Removed redundant code.

ygribov updated this revision to Unknown Object (????).Mar 14 2014, 2:11 AM

Resolved afore-mentioned problem with deactivation tests. Dlerror now uses same preinit hack as dlsym.

ygribov updated this revision to Unknown Object (????).Mar 19 2014, 12:28 AM

Improved tests.

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]).
which would build the shared runtime if SHARED is specified, and static runtime otherwise?

lib/asan/CMakeLists.txt
135

get rid of this and ASAN_DYNAMIC_RUNTIME_OBJECTS, just write all objects you need inside add_compiler_rt_runtime.

142–159

accidental duplication?

152

ldl or lpthread may not be available, see COMPILER_RT_HAS_LIBDL for example.

164

Do you use symbols from dynamic runtime anywhere?

lib/asan/asan_rtl.cc
715

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

samsonov added inline comments.Mar 19 2014, 7:49 AM
CMakeLists.txt
234

ASAN_BUILD_SHARED_RUNTIME

lib/asan/CMakeLists.txt
87

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 ↗(On Diff #7936)

Please don't do this. We should really specfiy this for each variable instead.

samsonov added inline comments.Mar 19 2014, 8:06 AM
lib/asan/asan_rtl.cc
647

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

ygribov added inline comments.Mar 20 2014, 9:24 AM
CMakeLists.txt
234

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
87

Yeah, already done locally.

135

Right.

142–159

Most probably result of code rewrite.

152

Ok, I'll add checks then. What about adding these for static rt as well (in Clang driver)?

164

No, probably not. I'll remove this.

lib/asan/asan_rtl.cc
647

No, because then __asan_init may then get called twice:

  1. once from .preinit_array
  2. once from this ctor

which will force ASan in activated state. Actually ability to start in deactivated state greatly complicates startup.

715

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 ↗(On Diff #7936)

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.

samsonov added inline comments.Mar 21 2014, 2:09 AM
CMakeLists.txt
234

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.
The first choice may be controlled by an input parameter, the second and third may just be combined (you can set both _OUTPUT_DIRECTORY) w/o any harm.

lib/asan/CMakeLists.txt
152

We already started to add toolchain-specific logic to adding -l... flags to the Clang driver (for FreeBSD port)

lib/asan/asan_rtl.cc
647

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 ↗(On Diff #7936)

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).

ygribov added inline comments.Mar 21 2014, 4:31 AM
CMakeLists.txt
234

Ok.

cmake/Modules/AddCompilerRT.cmake
42

the diff between the two macro you mention is much larger

Ok, then take a look osx/darwin. Anyway this isn't a big deal, I can refactor my macro.

lib/asan/CMakeLists.txt
152

Ok, will check.

lib/asan/asan_rtl.cc
647

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

It makes sense to print "UNIMPLEMENTED" on platforms where it's not available.

Makes sense.

Which reminds me of Mac - we use dynamic runtime there as well.

Yeah and it also uses a cool reexec trick which may be useful for other platforms as well (Linux, Android).

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?

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 ↗(On Diff #7936)

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?

ygribov added inline comments.Mar 21 2014, 12:44 PM
lib/asan/asan_rtl.cc
647

Also keep in mind that I'd like to detect dlopen.

lib/sanitizer_common/sanitizer_internal_defs.h
151 ↗(On Diff #7936)

s/prefer not to save performance/prefer to save performance/

ygribov updated this revision to Unknown Object (????).Mar 24 2014, 7:32 AM

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
ygribov updated this revision to Unknown Object (????).Mar 25 2014, 7:27 AM

Fixed based on Alexey's comments.

samsonov added inline comments.Mar 26 2014, 7:41 AM
cmake/Modules/AddCompilerRT.cmake
58

Use a single call:

set_target_properties(${name} PROPERTIES
  ARCHIVE_OUTPUT_DIRECTORY ...
  LIBRARY_OUTPUT_DIRECTORY ...)
83

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
79

Use more specific name (FindFirstDynamicLibraryCallback or some such).

104

remove newline

112

Weird indentation.

117

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
50

Why do you need this?

52

and this?

ygribov added inline comments.Mar 26 2014, 9:28 AM
cmake/Modules/AddCompilerRT.cmake
83

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
112

Yeah, moved from other place.

117

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
50

To make a copy of clang_asan_cflags (they are changed below).

52

Likewise.

samsonov added inline comments.Mar 26 2014, 11:51 AM
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
34–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)?

ygribov added inline comments.Mar 26 2014, 12:24 PM
lib/asan/asan_malloc_linux.cc
115 ↗(On Diff #8083)

you have to copy the contents

Agreed, won't work in general case. Was enough for dlerror because I was ignoring them anyway.

Can you clarify why you need to call it in AsanCheckIncompatibleRT?

I search for __asan_static via dlsym() and this fails and calls dlerror().

It's so sad we need to appease dlerror() with hacks like this

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).

samsonov added inline comments.Mar 27 2014, 3:46 AM
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.

ygribov added inline comments.Mar 27 2014, 5:15 AM
test/asan/CMakeLists.txt
44

I'm not sure how to provide it for unit tests below.

66

Here.

ygribov added inline comments.Mar 27 2014, 6:11 AM
lib/asan/asan_linux.cc
117

Are you sure the tool is usable? I see some strange changes like

+extern "C" attribute((
+ no_sanitize_address)) // Malloc may be called from dlsym in __asan_init
+void *malloc(size_t size) {

and misplaced comments.

samsonov added inline comments.Mar 27 2014, 8:08 AM
lib/asan/asan_linux.cc
117

Hm. If this happens with fresh clang-format in your PATH, feel free to disregard it then.

test/asan/CMakeLists.txt
66

Yeah, running unit tests with dynamic runtime may be... challenging. Feel free to drop this part from the patch for now.

ygribov added inline comments.Mar 27 2014, 8:16 AM
lib/asan/asan_malloc_linux.cc
115 ↗(On Diff #8083)

Can you search for ASan runtime (.so) in the contents of /proc/self/maps

No, this won't protect from dlopening dynamic rt.

Or to add asan_runtime_type .bss global

Yup, this might work. I'll see how it goes tomorrow.

ygribov added inline comments.Mar 28 2014, 1:48 AM
lib/asan/asan_linux.cc
117

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)

Or to add asan_runtime_type .bss global

Yup, this might work. I'll see how it goes tomorrow.

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.

ygribov updated this revision to Unknown Object (????).Mar 28 2014, 3:16 AM

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)
ygribov added inline comments.Mar 28 2014, 12:41 PM
lib/asan/asan_linux.cc
94

I could probably also use DumpListOfModules here. What's your opinion?

105

I think we can do this:

  • for static rt:
    • check if dynamic rt in /proc/maps (and die if it's there)
    • set __asan_rt_version
  • for dynamic rt:
    • check __asan_rt_version (and die if it's already initialized by static rt)

But all this looks too complicated compared to current approach (dlsym + preinit allocator).

ygribov added inline comments.Mar 28 2014, 11:47 PM
lib/asan/asan_linux.cc
94

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...

samsonov added inline comments.Mar 31 2014, 7:01 AM
cmake/Modules/AddCompilerRT.cmake
41

FYI I've submitted this renaming as r205183, so you can rebase your patch to minimize the diff.

45

Why is the default OUTPUT_NAME not suitable here?

lib/asan/asan_linux.cc
94

Yeah.

105

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?

ygribov added inline comments.Mar 31 2014, 7:11 AM
cmake/Modules/AddCompilerRT.cmake
45

I want to use same name for static and dynamic runtimes but default name for dynamic is clang_rt.asan-dynamic-${arch}.

lib/asan/asan_linux.cc
105

Sure I'll give it a try.

samsonov added inline comments.Mar 31 2014, 7:13 AM
lib/asan/asan_linux.cc
105

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?

samsonov added inline comments.Mar 31 2014, 7:15 AM
cmake/Modules/AddCompilerRT.cmake
45

Ok, got it.

ygribov added inline comments.Mar 31 2014, 7:22 AM
lib/asan/asan_linux.cc
105

If we call AsanCheckIncompatibleRT later in initialization

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.

samsonov added inline comments.Mar 31 2014, 7:24 AM
lib/asan/asan_linux.cc
105

Ah, I see, this is the comment about MaybeInstallSigaction() problems.

ygribov updated this revision to Unknown Object (????).Apr 1 2014, 2:15 AM

Changes in this revision:

  • rebased
  • get rid of preinit allocator (yay!)
  • unittests now know about dynamic runtime

Awesome.

lib/asan/asan_linux.cc
111

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
713

Do you still need these?

test/asan/TestCases/Linux/interception_malloc_test.cc
13

Is this still relevant?

ygribov updated this revision to Unknown Object (????).Apr 1 2014, 4:17 AM

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.

samsonov accepted this revision.Apr 1 2014, 4:39 AM

LGTM

I think this looks good now. I will land this patch (maybe, in a series of commits) shortly.

Cool! BTW when will the next merge to GCC happen?

samsonov closed this revision.Apr 1 2014, 6:41 AM

Landed as r205308.

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?

test/asan/TestCases/Linux/interception_malloc_test.cc