Avoid infinite loop with asan interception

[serge-sans-paille]

Summary

This should fix https://bugs.llvm.org/show_bug.cgi?id=39641

[serge-sans-paille]

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

[serge-sans-paille]

check-asan works just fine

[kcc]

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

[vitalybuka]

In D63877#1561034, @serge-sans-paille wrote:

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]

• Test added
• Comment added
• make the change more localized, closer to the origin of the issue

[yln]

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)

Which check fails here?
Ideally, it would be this one: https://github.com/llvm/llvm-project/blob/29d05c005fa88b3a59697a2e538f46cf79413548/compiler-rt/lib/asan/asan_interceptors.h#L126

[serge-sans-paille]

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]

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'

[serge-sans-paille]

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

[yln]

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]

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.

[serge-sans-paille]

@vitalybuka / @kcc : up?

[kcc]

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?

[serge-sans-paille]

Update test according to @kcc remark.

[kcc]

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]

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]

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]

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]

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

1 ↗

(On Diff #207726)

why x86_64-target-arch?

[serge-sans-paille]

Fix test requires + asan calls

[serge-sans-paille]

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

[uweigand]

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

[uweigand]

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

[uweigand]

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!

[uweigand]

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

[Hahnfeld]

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

[Hahnfeld]

In D63877#1624224, @Hahnfeld wrote:

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]

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

[Hahnfeld]

In D63877#1624433, @serge-sans-paille wrote:

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.

[serge-sans-paille]

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

In D63877#1595638, @uweigand wrote:

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.

[uweigand]

In D63877#1624258, @Hahnfeld wrote:

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.

[Hahnfeld]

In D63877#1641054, @serge-sans-paille wrote:

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

Sure, this marks the test unsupported. The problem is that will also happen for most others because almost any default configuration of compiler-rt has the feature cxxabi which depends on SANITIZER_ALLOW_CXXABI (defaults to ON) for Linux systems.

Which CMake options did you try? I also have

• -DLLVM_ENABLE_LIBCXX=ON -DLLVM_ENABLE_LLD=ON to link with libc++ (instead of libstdc++) and use lld
• -DSANITIZER_CXX_ABI=libcxxabi -DSANITIZER_TEST_CXX=libc++ as mentioned above to use libc++ instead of libstdc++
• -DCLANG_DEFAULT_CXX_STDLIB=libc++, which you can also fake by manually passing -stdlib=libc++ when compiling the shared library for this test

Can't test right now which ones you need to reproduce the problem, but given that @uweigand also sees the problem, it's not entirely artificial.

[serge-sans-paille]

@Hahnfeld : I understand your concern. However this test case is meant to test handling of stdlib exceptions, so it doesn't look strange to disable it if stdlib is not used, right? Do you know of another feature we could use?

[Hahnfeld]

In D63877#1656135, @serge-sans-paille wrote:

@Hahnfeld : I understand your concern. However this test case is meant to test handling of stdlib exceptions, so it doesn't look strange to disable it if stdlib is not used, right? [...]

I can't follow here: As far as I understand, this test case ensures that asan can handle a runtime which is itself linked against an stdlib that provides __cxa_throw. However, exceptions are not used inside asan, and with SANITIZER_CXX_ABI=libcxxabi the runtime is not linked against a full C++ stdlib, so there's no __cxa_throw in the asan shared library. Is that summary correct?

[serge-sans-paille]

Is that summary correct?

It is :-)

[Hahnfeld]

In D63877#1656860, @serge-sans-paille wrote:

Is that summary correct?

It is :-)

Then my conclusion would be that the test incorrectly assumes asan to be linked against a C++ stdlib. Sure, we could restrict the test to the case where this assumption holds (not via cxxabi, maybe by introducing another feature). But I wonder if there are better solutions that don't lose coverage on my kind of configurations (and possibly others like @uweigand if we can't detect them during configuration time and don't want failing tests). Where exactly does __cxa_throw need to be defined in order to trigger the infinite loop in the old code?

[Hahnfeld]

I've posted a change that makes the test work for me in D67298, please take a look.

compiler-rt/trunk/lib/interception/interception_linux.cc

while (true) {
     if (*s1 == 0)
       return true;
     s1++;
     s2++;
   }
 }
 #endif
 
-static void *GetFuncAddr(const char *name) {
+static void *GetFuncAddr(const char *name, uptr wrapper_addr) {
 #if SANITIZER_NETBSD
   // FIXME: Find a better way to handle renames
   if (StrCmp(name, "sigaction"))
     name = "__sigaction14";
 #endif
   void *addr = dlsym(RTLD_NEXT, name);
   if (!addr) {
     // If the lookup using RTLD_NEXT failed, the sanitizer runtime library is
     // later in the library search order than the DSO that we are trying to
     // intercept, which means that we cannot intercept this function. We still
     // want the address of the real definition, though, so look it up using
     // RTLD_DEFAULT.
     addr = dlsym(RTLD_DEFAULT, name);
+
+    // In case `name' is not loaded, dlsym ends up finding the actual wrapper.
+    // We don't want to intercept the wrapper and have it point to itself.
+    if ((uptr)addr == wrapper_addr)
+      addr = nullptr;
   }
   return addr;
 }
 
 bool InterceptFunction(const char *name, uptr *ptr_to_real, uptr func,
                        uptr wrapper) {
-  void *addr = GetFuncAddr(name);
+  void *addr = GetFuncAddr(name, wrapper);
   *ptr_to_real = (uptr)addr;
   return addr && (func == wrapper);
 }
 
 // Android and Solaris do not have dlvsym
 #if !SANITIZER_ANDROID && !SANITIZER_SOLARIS && !SANITIZER_OPENBSD
 static void *GetFuncAddr(const char *name, const char *ver) {
   return dlvsym(RTLD_NEXT, name, ver);

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: ASAN_OPTIONS=verbosity=1 not %t.out %t.so 2>&1 | FileCheck %s
+//
+// CHECK: AddressSanitizer: failed to intercept '__cxa_throw'
+//
+// dlopen() can not be intercepted on Android
+// UNSUPPORTED: android
+#ifdef __cplusplus
+
+static void foo(void) {
+  int i = 0;
+  throw(i);
+}
+
+extern "C" {
+int bar(void);
+};
+int bar(void) {
+  try {
+    foo();
+  } catch (int i) {
+    return i;
+  }
+  return -1;
+}
+
+#else
+
+#include <assert.h>
+#include <dlfcn.h>
+
+int main(int argc, char **argv) {
+  int (*bar)(void);
+  void *handle = dlopen(argv[1], RTLD_LAZY);
+  assert(handle);
+  bar = dlsym(handle, "bar");
+  assert(bar);
+  return bar();
+}
+
+#endif