This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Hahnfeld on Sep 6 2019, 12:24 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Sep 6 2019, 12:24 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 6 2019, 12:24 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
serge-sans-paille added a comment.EditedSep 6 2019, 2:58 PM

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++?

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.

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 updated this revision to Diff 219227.Sep 7 2019, 3:32 AM
Hahnfeld retitled this revision from [ASan] Fix test case dlopen-mixed-c-cxx.c without C++ stdlib to [ASan] Only run dlopen-mixed-c-cxx.c with static runtime.
Hahnfeld edited the summary of this revision. (Show Details)

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.

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?

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.

Hahnfeld edited the summary of this revision. (Show Details)Sep 7 2019, 6:23 AM

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

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

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.

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 accepted this revision.Sep 8 2019, 6:38 AM

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.

This revision is now accepted and ready to land.Sep 8 2019, 6:38 AM
This revision was automatically updated to reflect the committed changes.