Page MenuHomePhabricator

Avoid infinite loop with asan interception
ClosedPublic

Authored by serge-sans-paille on Jun 27 2019, 6:42 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 27 2019, 6:42 AM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript

@kcc tested on the bug test case and it now raises a decent error, not tested on compiler-rt test suite (yet)

check-asan works just fine

I like the simplicity of the fix!
Please add a comment explaining the fix, and a test that verifies how it works.

+Vitaly for another opinion

check-asan works just fine

try also
check-asan check-tsan check-msan check-sanitizer

compiler-rt/lib/interception/interception_linux.cc
72 ↗(On Diff #206852)

same here?

serge-sans-paille updated this revision to Diff 206939.
  • Test added
  • Comment added
  • make the change more localized, closer to the origin of the issue
yln added inline comments.Jun 27 2019, 3:08 PM
compiler-rt/lib/interception/interception_linux.cc
49 ↗(On Diff #206939)

Do we understand why we need still want the pointer, even if we don't intercept functions? Or is the comment wrong?
https://reviews.llvm.org/D39779

I intended to change InterceptFunction to return true only on when "interception was successful", but I missed this case here.

If the lookup using RTLD_NEXT failed ... we cannot intercept this function.

Currently, we still return true, because we got an address addr && (func == wrapper).

The original patch added a test that checks:

// Tests that ubsan can detect errors on Android if libc appears before the
// runtime in the library search order, which means that we cannot intercept
// symbols.

It's not obvious to me why we need to lookup the pointer if none of our interceptors run!?

compiler-rt/test/asan/TestCases/dlopen-mixed-c-cxx.c
5 ↗(On Diff #206939)
serge-sans-paille marked an inline comment as done.Jun 27 2019, 10:33 PM
serge-sans-paille added inline comments.
compiler-rt/test/asan/TestCases/dlopen-mixed-c-cxx.c
5 ↗(On Diff #206939)

It's not :-/ We get

==25385==AddressSanitizer CHECK failed: /home/sguelton/sources/llvm-project/compiler-rt/lib/asan/asan_interceptors.cc:328 "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0)
    #0 0x4b84e5 in __asan::AsanCheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) /home/sguelton/sources/llvm-project/compiler-rt/lib/asan/asan_rtl.cc:73:5
    #1 0x4d2ef9 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) /home/sguelton/sources/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_termination.cc:78:24
    #2 0x433d0c in __interceptor___cxa_throw /home/sguelton/sources/llvm-project/compiler-rt/lib/asan/asan_interceptors.cc:328:3
    #3 0x7f6d8defeabe in foo() /home/sguelton/sources/llvm-project/_build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/<stdin>:10:3
    #4 0x7f6d8defe99c in bar /home/sguelton/sources/llvm-project/_build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/<stdin>:18:5
    #5 0x4e91b9 in main /home/sguelton/sources/llvm-project/compiler-rt/test/asan/TestCases/dlopen-mixed-c-cxx.c:37:10
    #6 0x7f6d916593d4 in __libc_start_main (/lib64/libc.so.6+0x223d4)
    #7 0x41ac11 in _start (/home/sguelton/sources/llvm-project/_build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/dlopen-mixed-c-cxx.c.tmp.out+0x41ac11)
serge-sans-paille marked an inline comment as done.Jun 28 2019, 1:11 AM
serge-sans-paille added inline comments.
compiler-rt/test/asan/TestCases/dlopen-mixed-c-cxx.c
5 ↗(On Diff #206939)

My bad, with the right verbosity level, I indeed get

'==17519==AddressSanitizer: failed to intercept '__cxa_throw'

Following @yln question, improve the CHECK of the associated test case

yln added inline comments.Jun 28 2019, 10:12 AM
compiler-rt/lib/interception/interception_linux.cc
49 ↗(On Diff #206939)

Just to be clear, I am suggesting to try to remove the branch with dlsym(RTLD_DEFAULT, ...) altogether to find out the reason why it is there, but I don't want to block you on this.
LGTM; please get the okay from Vitaly.

serge-sans-paille marked an inline comment as done.Jun 28 2019, 2:20 PM
serge-sans-paille added inline comments.
compiler-rt/lib/interception/interception_linux.cc
49 ↗(On Diff #206939)

Yeah, I felt that, and that's why I moved the patch inside that branch to show the fix is related to it. waiting for @vitalybuka then.

kcc added inline comments.Jul 2 2019, 11:17 AM
compiler-rt/test/asan/TestCases/dlopen-mixed-c-cxx.c
2 ↗(On Diff #207015)

Do you need -fsanitize=address here?

3 ↗(On Diff #207015)

What will happen with this test on non-linux-x86?

Update test according to @kcc remark.

serge-sans-paille marked 2 inline comments as done.Jul 2 2019, 2:38 PM
kcc added a comment.Jul 2 2019, 2:50 PM

Vitaly, please take a look as well.

compiler-rt/test/asan/TestCases/dlopen-mixed-c-cxx.c
1 ↗(On Diff #207634)

does this include non-linux?

vitalybuka added inline comments.Jul 2 2019, 3:15 PM
compiler-rt/test/asan/TestCases/dlopen-mixed-c-cxx.c
33 ↗(On Diff #207634)

Test uses dlopen, so it should go into Linux, or Posix

2 ↗(On Diff #207015)

It's marked as done, but I don't see a difference

serge-sans-paille marked 2 inline comments as done.
serge-sans-paille added inline comments.
compiler-rt/test/asan/TestCases/dlopen-mixed-c-cxx.c
33 ↗(On Diff #207634)

I've put it into the Linux/ directory, if that's not what expected (like, a special REQUIRES is needed), please advise!

2 ↗(On Diff #207015)

Both the shared library and the main executable are now compiled with -fsanitize=address, is there something else missing?

vitalybuka added inline comments.Jul 8 2019, 2:49 PM
compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
2 ↗(On Diff #207726)

"%clang_asan" should already include "-fsanitize=address"

So I would expect just

// RUN: %clangxx_asan -xc++ -shared -fPIC -o %t.so - < %s
// RUN: %clang_asan %s -o %t.out -ldl
vitalybuka added inline comments.Jul 8 2019, 2:53 PM
compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
1 ↗(On Diff #207726)

why x86_64-target-arch?

Fix test requires + asan calls

serge-sans-paille marked 3 inline comments as done.Jul 16 2019, 2:17 AM
serge-sans-paille added inline comments.
compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
1 ↗(On Diff #207726)

Updated, flagging android as unsupported, in a similar manner to other test cases involving dlopen

vitalybuka accepted this revision.Jul 16 2019, 2:20 PM
This revision is now accepted and ready to land.Jul 16 2019, 2:20 PM
Closed by commit rL366413: Fix asan infinite loop on undefined symbol (authored by serge_sans_paille, committed by ). · Explain WhyJul 18 2019, 1:09 AM
This revision was automatically updated to reflect the committed changes.

Just a quick heads-up: the new test case is failing on SystemZ. I'm not quite sure why, but the build bots show this error:

/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/projects/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c:5:11: error: CHECK: expected string not found in input
// CHECK: AddressSanitizer: failed to intercept '__cxa_throw'
          ^
<stdin>:1:1: note: scanning from here
==8377==AddressSanitizer: failed to intercept '__isoc99_printf'
^
<stdin>:10:10: note: possible intended match here
'==8377==AddressSanitizer: failed to intercept '__cxa_rethrow_primary_exception'
         ^

Running the test case manually, I see this output:

000000000007f990 T __interceptor_dlopen
[uweigand@s83lp56 llvm-head]$ ASAN_OPTIONS=verbosity=1 LD_LIBRARY_PATH=/home/uweigand/llvm/build/llvm-head/lib64/clang/10.0.0/lib/linux /home/uweigand/llvm/build/llvm-head/projects/compiler-rt/test/asan/S390XLinuxDynamicConfig/TestCases/Linux/Output/dlopen-mixed-c-cxx.c.tmp.out /home/uweigand/llvm/build/llvm-head/projects/compiler-rt/test/asan/S390XLinuxDynamicConfig/TestCases/Linux/Output/dlopen-mixed-c-cxx.c.tmp.so 
==138401==AddressSanitizer: failed to intercept '__isoc99_printf'
'==138401==AddressSanitizer: failed to intercept '__isoc99_sprintf'
'==138401==AddressSanitizer: failed to intercept '__isoc99_snprintf'
'==138401==AddressSanitizer: failed to intercept '__isoc99_fprintf'
'==138401==AddressSanitizer: failed to intercept '__isoc99_vprintf'
'==138401==AddressSanitizer: failed to intercept '__isoc99_vsprintf'
'==138401==AddressSanitizer: failed to intercept '__isoc99_vsnprintf'
'==138401==AddressSanitizer: failed to intercept '__isoc99_vfprintf'
'==138401==AddressSanitizer: failed to intercept 'xdr_quad_t'
'==138401==AddressSanitizer: failed to intercept 'xdr_u_quad_t'
'==138401==AddressSanitizer: failed to intercept 'dlopen'
'==138401==AddressSanitizer: failed to intercept '__cxa_rethrow_primary_exception'
'==138401==AddressSanitizer: libc interceptors initialized
|| `[0x14000000000000, 0x1fffffffffffff]` || HighMem    ||
|| `[0x12800000000000, 0x13ffffffffffff]` || HighShadow ||
|| `[0x12000000000000, 0x127fffffffffff]` || ShadowGap  ||
|| `[0x10000000000000, 0x11ffffffffffff]` || LowShadow  ||
|| `[0x000000000000, 0xfffffffffffff]` || LowMem     ||
MemToShadow(shadow): 0x12000000000000 0x123fffffffffff 0x12500000000000 0x127fffffffffff
redzone=16
max_redzone=2048
quarantine_size_mb=256M
thread_local_quarantine_size_kb=1024K
malloc_context_size=30
SHADOW_SCALE: 3
SHADOW_GRANULARITY: 8
SHADOW_OFFSET: 0x10000000000000
==138401==Installed the sigaction for signal 11
==138401==Installed the sigaction for signal 7
==138401==Installed the sigaction for signal 8
==138401==T0: stack [0x03ffff800000,0x040000000000) size 0x800000; local=0x03ffffffe6d0
==138401==AddressSanitizer Init done

Any suggestion what might be doing on here?

@hans : This commit went in just shortly before the LLVM 9 branch, which means that currently the test suite is failing on the branch. We'll want to backport the fix once it is available ...

I tried to run it on my Intel system for comparison, and the test case is failing there too:

Failing Tests (4):
    LLVM-Unit :: Analysis/./AnalysisTests/BasicAATest.AliasInstWithFullObjectOfImpreciseSize
    LLVM-Unit :: Analysis/./AnalysisTests/BasicAATest.AliasInstWithObjectOfImpreciseSize
    AddressSanitizer-i386-linux-dynamic :: TestCases/Linux/dlopen-mixed-c-cxx.c
    AddressSanitizer-x86_64-linux-dynamic :: TestCases/Linux/dlopen-mixed-c-cxx.c

  Expected Passes    : 52544
  Expected Failures  : 215
  Unsupported Tests  : 2326
  Unexpected Failures: 4

Looks like this patch was reverted on mainline now.

@hans : I guess it would make sense to revert the patch on the LLVM 9 branch as well now? Thanks!

I've now reverted the patch on the LLVM 9 branch as well (r366690).

This test is also failing for me on CentOS 7, Intel x86. Any clues what's wrong here?

This test is also failing for me on CentOS 7, Intel x86. Any clues what's wrong here?

Okay, I think I've found the issue for my way of building the sanitizers: I'm passing SANITIZER_CXX_ABI=libcxxabi while the default is libstdc++ for Linux. This results in the full C++ library being linked into the shared asan library (while I only have the ABI) which is "needed" for the failure to appear. @uweigand how do you build the sanitizer runtimes?

Can we rewrite the test to not break when setting SANITIZER_CXX_ABI?

serge-sans-paille marked an inline comment as done.Sat, Aug 10, 12:04 PM

ok, it's probably a regex issue for the test, I'll check that.

ok, it's probably a regex issue for the test, I'll check that.

no, the line failed to intercept '__cxa_throw' is just not there because of the reasons I outlined above.

@Hahnfeld : I failed to reproduce the issue locally, but I assume the following patch may solve your issue:

--- a/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
+++ b/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
@@ -5,7 +5,7 @@
 //
 // CHECK: {{.*}}AddressSanitizer: failed to intercept '__cxa_{{.*}}throw{{.*}}'
 //
-// REQUIRES: x86_64-target-arch && !android
+// REQUIRES: x86_64-target-arch && !android && !cxxabi

can you give it a try?

hans removed a subscriber: hans.Thu, Aug 22, 7:04 AM

I've now reverted the patch on the LLVM 9 branch as well (r366690).

un-cc'ing myself since the release branch is already taken care of.

Okay, I think I've found the issue for my way of building the sanitizers: I'm passing SANITIZER_CXX_ABI=libcxxabi while the default is libstdc++ for Linux. This results in the full C++ library being linked into the shared asan library (while I only have the ABI) which is "needed" for the failure to appear. @uweigand how do you build the sanitizer runtimes?

Sorry, only just now noticed there was a question for me ... I'm not passing any special option, so I'm assuming it will use the libstdc++ default.