[ASan] Only run dlopen-mixed-c-cxx.c with static runtime

[Hahnfeld]

Summary

This is what the original bug (http://llvm.org/PR39641) and the fix
in https://reviews.llvm.org/D63877 have been about.
With the dynamic runtime the test only passes when the asan library
is linked against libstdc++: In contrast to libc++abi, it does not
implement __cxa_rethrow_primary_exception so the regex matches the
line saying that asan cannot intercept this function. Indeed, there
is no message that the runtime failed to intercept __cxa_throw.

[serge-sans-paille]

Mmmmh the whole point of the test is to test how asan behaves when meeting a symbol like __cxa_throw in a dlopened library when the calling compilation unit is not linked against libstdc++. Previous behavior was a segfault, we should now get a reasonable error. By defining __cxa_throw in the c file, you remove the origin of the bug, so sure it works, but it no longer tests the faulty situation. Maybe there's a symbol caught by asan and defined in both libcxx and libstdc++?

[Hahnfeld]

In D67298#1661672, @serge-sans-paille wrote:

Mmmmh the whole point of the test is to test how asan behaves when meeting a symbol like __cxa_throw in a dlopened library when the calling compilation unit is not linked against libstdc++. Previous behavior was a segfault, we should now get a reasonable error. By defining __cxa_throw in the c file, you remove the origin of the bug, so sure it works, but it no longer tests the faulty situation. Maybe there's a symbol caught by asan and defined in both libcxx and libstdc++?

And that's what I don't understand and what I have been trying to sort out in my comments in D63877: In the default configuration the asan shared library is linked against libstdc++. As the C file is compiled with -fsanitize=address it pulls in a definition of __cxa_throw, so what's the difference?

For me, it's unacceptable that the test is failing for more than a month on a totally legit configuration. We should get this fixed rather sooner than later, and not being the author I'm grabbing at any straw without really getting insight into what's going on.

[Hahnfeld]

In D67298#1661672, @serge-sans-paille wrote:

Mmmmh the whole point of the test is to test how asan behaves when meeting a symbol like __cxa_throw in a dlopened library when the calling compilation unit is not linked against libstdc++. Previous behavior was a segfault, we should now get a reasonable error.

1. Just noticed that the test accepts __cxa_{{.*}}throw{{.*}} which matches __cxa_rethrow_primary_exception; even if the test passes I don't have a line with __cxa_throw.
2. I don't see a segfault when I remove the added lines in interception_linux.cpp.

I think there's something fundamentally wrong and I'd ask you as the author to figure this out. I've just tried and I can reproduce the above when building a standalone version of compiler-rt with trunk clang. Let me know if you need more help.

[Hahnfeld]

I think I just figured out that the original problem is about static runtime, so it doesn't make sense to run the test with dynamic asan.

[serge-sans-paille]

And that's what I don't understand

My fault for not being clear enough. Let me elaborate the original faulty use case:

You've got an application written in C, compiled with asan, that dlopens a library written in C++, linked to libstdc++
When the application starts, asan installs a few handler based on the loaded symbols. It used to install a handler that points to itself for cxa_throw because the symbol is not present in the binary.
When the C++ library is loaded, it resolves cxa_throw to the asan implementation instead of the libstdc++ implementation.
When an exception is raised, the asam handler is called (that's normal) but it doesn't fallsback to libstdc++ (because this library wasn't loaded when the handler got initialized).

The problem with current test case is that it assumes the c++ code is compiled with libstdc++, which is an invalid assumption based on your feedback.
We could force clang to use libstdc++ when linking the C++ library, would that work in your situation?

- // RUN: %clangxx_asan -xc++ -shared -fPIC -o %t.so - < %s
+ // RUN: %clangxx_asan -xc++ -stdlib=libstdc++ -shared -fPIC -o %t.so - < %s

Assuming libstdc++ is available in the system?

[Hahnfeld]

In D67298#1662107, @serge-sans-paille wrote:

And that's what I don't understand

My fault for not being clear enough. Let me elaborate the original faulty use case:

You've got an application written in C, compiled with asan, that dlopens a library written in C++, linked to libstdc++
When the application starts, asan installs a few handler based on the loaded symbols. It used to install a handler that points to itself for __cxa_throw because the symbol is not present in the binary.

Apparently, this is only the case when the asan runtime is linked statically. Otherwise the dynamic runtime pulls in the full libstdc++ and installation of the handler succeeds.

When the C++ library is loaded, it resolves __cxa_throw to the asan implementation instead of the libstdc++ implementation.
When an exception is raised, the asam handler is called (that's normal) but it doesn't fallsback to libstdc++ (because this library wasn't loaded when the handler got initialized).

The problem with current test case is that it assumes the c++ code is compiled with libstdc++, which is an invalid assumption based on your feedback.

Where does the test assume that the C++ code is linked with libstdc++? I only observed that the test passes if the asan runtime is linked to libstdc++.

We could force clang to use libstdc++ when linking the C++ library, would that work in your situation?

- // RUN: %clangxx_asan -xc++ -shared -fPIC -o %t.so - < %s
+ // RUN: %clangxx_asan -xc++ -stdlib=libstdc++ -shared -fPIC -o %t.so - < %s

Assuming libstdc++ is available in the system?

I just tested this change and it doesn't work (at least for dynamic runtime). This matches my theory that the test expects static runtime.

Can you please try to reproduce the failing test in check-asan-dynamic on your end? Configuring a standalone build of compiler-rt with -DCOMPILER_RT_INCLUDE_TESTS=ON -DSANITIZER_CXX_ABI=libcxxabi should be enough.

[serge-sans-paille]

@Hahnfeld https://reviews.llvm.org/D67319 contains a potential fix. Can you give it a try?

[serge-sans-paille]

@Hahnfeld I missed your previous answer, sorry about that

Apparently, this is only the case when the asan runtime is linked statically

Totally makes sense! In that case the asan runtime doesn't bring the c++ library as a dependency, while with dynamic runtime it does. I didn't realize that aspect, thanks for making it clearer. Your patch totally makes sense, I'll test it.

[serge-sans-paille]

Successfully tested on my box! I'd just use

diff
- UNSUPPORTED: asan-dynamic-runtime
+REQUIRES: x86_64-target-arch && !android && !asan-dynamic-runtime

if that works on your side too.

[Hahnfeld]

In D67298#1662222, @serge-sans-paille wrote:

Successfully tested on my box! I'd just use

diff
- UNSUPPORTED: asan-dynamic-runtime
+REQUIRES: x86_64-target-arch && !android && !asan-dynamic-runtime

if that works on your side too.

Can you elaborate why you think we need to restrict this case? I don't see anything that only works on x86_64 or that cannot work per se on Android. I'd guess that these annotations were added as bots complained (because the test was running in an unintended configuration).

If there's no good reason, I'd strongly oppose to adding the REQUIRES. We should keep tests as broad as possible to maximize the likelihood that it fails if someone breaks this (or a similar) case in the future.

[serge-sans-paille]

We should keep tests as broad as possible to maximize the likelihood that it fails if someone breaks this (or a similar) case in the future.

ack on this, that's a very sane path.

compiler-rt/trunk/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c

 // RUN: %clangxx_asan -xc++ -shared -fPIC -o %t.so - < %s
 // RUN: %clang_asan %s -o %t.out -ldl
 //
 // RUN: { env ASAN_OPTIONS=verbosity=1 %t.out %t.so || : ; } 2>&1 | FileCheck %s
 //
-// CHECK: {{.*}}AddressSanitizer: failed to intercept '__cxa_{{.*}}throw{{.*}}'
+// CHECK: AddressSanitizer: failed to intercept '__cxa_throw'
 //
-// REQUIRES: x86_64-target-arch && !android
+// This tests assumes static linking of the asan runtime.
+// UNSUPPORTED: asan-dynamic-runtime
 
 #ifdef __cplusplus
 
 static void foo(void) {
   int i = 0;
   throw(i);
 }