Test failures in GCC ASan testsuite on ARM Linux due to FP format mismatch between libsanitizer and GCC.

[m.ostapenko]

Summary

Fast unwind works wrong in GCC 4.10 on ARM Linux due to FP format mismatch between libsanitizer and GCC. This is related bug in GCC (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61771).

[kcc]

Can we add tests?
Evgeniy suggests that a test may be implemented with a set of assembly files that have different ABIs (GCC's and Clang's)

[rengolin]

I agree with tests, no regressions on current tests don't mean much.

cheers,
--renato

lib/sanitizer_common/sanitizer_stacktrace.cc

53

It's not clear how this will separate a GCC logic from LLVM one (or rather, different FP logics), on ARM.

It'll also make the second IsValidFrame() call redundant on anything _but_ ARM. Maybe you should move all of it inside the #ifdef block?

#ifdef __arm__
  if (IsValidFrame((uptr)bp_prev[0], stack_top, stack_bottom))
    return bp_prev;
  --bp_prev;
#endif
  return bp_prev;

[m.ostapenko]

Thanks, Renato, you are right, I've also added a small test. Should I add more tests?

[ygribov]

test/asan/TestCases/Linux/clang_gcc_abi.cc

5

Do we really need to check all optimization levels?

11

Perhaps make this local?

14

Contents of s is undefined so compiler could optimize this access out.

18

I think this should better be asm volatile.

30

Likewise.

[samsonov]

lib/sanitizer_common/sanitizer_stacktrace.cc

40

this function returns bool

65–66

This line is wrong. It assumes that FastUnwindStack() is called in the same thread for which we provide stack_top/stack_bottom. I believe this function describes a generic unwinding algorithm, and there's no place for hack like this here.

Why do you need it?

[m.ostapenko]

lib/sanitizer_common/sanitizer_stacktrace.cc

65–66

Hm, stack_bottom is simply stack_top - kStackSize (8 MB), isn't it? So, we have 8 MB interval for "valid" stack addresses. But in Linux stack grows dynamically, so if access address is, for example, in the middle of this interval, this can cause a segmentation fault, because libc hasn't mapped this memory yet. So we need something like this to prevent this error. You are right, this will work only in case if FastUnwindStack() is called in the same thread for which we provide stack_top/stack_bottom, but if it is called in another thread, how can we implement such kind of protection?

[ygribov]

lib/sanitizer_common/sanitizer_stacktrace.cc

65–66

Can we do stack_bottom = Max(stack_bottom, bp) instead?

[m.ostapenko]

Updated patch according to Renato's and Yury's comments. Regarding the issue about stack_bottom, I still believe that we need something like the protection was suggested.

[ygribov]

lib/sanitizer_common/sanitizer_stacktrace.cc

65–66

I'd still ask for bp instead of &size here.

test/asan/TestCases/Linux/clang_gcc_abi.cc

31

Broken formatting.

[ygribov]

lib/sanitizer_common/sanitizer_stacktrace.cc

41

Should this be 2 * sizeof(uhwptr)?

[samsonov]

lib/sanitizer_common/sanitizer_stacktrace.cc

65–66

Stack size is not always 8Mb, it's configurable in many ways. And, yes, it grows dynamically. But we don't assume that the whole interval from stack_top to stack_bottom is accessible. We would access unmapped memory only if there's a bug in unwinding algorithm or the frame pointers are broken. Do you actually observe such cases?

[ygribov]

lib/sanitizer_common/sanitizer_stacktrace.cc

65–66

We could also access unmapped area if code was compiled without fp support. And yes, we saw this in our ARM tests.

[m.ostapenko]

Alexey, you are right, this is a really specific situation. Removed the line. Is it OK now?

[samsonov]

lib/sanitizer_common/sanitizer_stacktrace.cc

47

Make this inline.

48

This check is not needed if we're not on ARM. Can you keep the original behavior on non-ARM platforms?

#ifdef __arm__
  if (!IsValidFrame(bp, stack_top, stack_bottom) return 0;
  uhwptr *bp_prev = (uhwptr *)bp;
  if (IsValidFrame((uptr)bp_prev[0], stack_top, stack_bottom) return bp_prev;
  return bp_prev - 1;
#else
  return (uhwptr*)bp;
#endif

66–68

Move this line upper (before calling GetCanonicFrame for the first time).

test/asan/TestCases/Linux/clang_gcc_abi.cc

41

Put CHECK-lines closer to the actual source code lines

[m.ostapenko]

Update according to last review notes.

[samsonov]

Looks reasonable. Please fix the test case and let me know if I should land this for you.

test/asan/TestCases/Linux/clang_gcc_abi.cc

15

[[@LINE]] directives don't look right now.

[m.ostapenko]

Thanks, fixed. Could you apply it when you have a time?

[samsonov]

test/asan/TestCases/Linux/clang_gcc_abi.cc

15

Just sanity checking: doesn't ASan output line 13 in the error report? (the one with the BOOM)

28

This can be [[@LINE+1]] if you put it immediately after the asm blob (which would probably be more appropriate/clear).

[m.ostapenko]

test/asan/TestCases/Linux/clang_gcc_abi.cc

15

Yes, it does.

=================================================================
==12410==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x422007f3 at pc 0x000c1444 bp 0x40800118 sp 0x40800110
READ of size 1 at 0x422007f3 thread T0
    #0 0xc1443 in boom /home/max/workspace/downloads/llvm/projects/compiler-rt/test/asan/TestCases/Linux/clang_gcc_abi.cc:13:3
    #1 0xc1453 in gcc_abi /home/max/workspace/downloads/llvm/projects/compiler-rt/test/asan/TestCases/Linux/clang_gcc_abi.cc:19:3
    #2 0xc1453 in gcc_abi /home/max/workspace/downloads/llvm/projects/compiler-rt/test/asan/TestCases/Linux/clang_gcc_abi.cc:19:3
    #3 0xc1453 in gcc_abi /home/max/workspace/downloads/llvm/projects/compiler-rt/test/asan/TestCases/Linux/clang_gcc_abi.cc:19:3

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/max/workspace/downloads/llvm/projects/compiler-rt/test/asan/TestCases/Linux/clang_gcc_abi.cc:13 boom

However, ASan unable to unwind the stack deeper then gcc_abi function. Should I do something about it?

[ygribov]

test/asan/TestCases/Linux/clang_gcc_abi.cc

15

That's expected - your inline asm lacks frame info so assembler can't generate proper unwind instructions which puzzles slow unwinder.

[m.ostapenko]

Moved check lines.

[samsonov]

Landed in r217079. Thanks for working on this!

lib/sanitizer_common/sanitizer_stacktrace.cc

#else
   return pc - 1;
 #endif
 }
 
 uptr StackTrace::GetCurrentPc() {
   return GET_CALLER_PC();
 }
 
+// Check if given pointer points into allocated stack area.
+static inline bool IsValidFrame(uptr frame, uptr stack_top, uptr stack_bottom) {
+  return frame > stack_bottom && frame < stack_top - 2 * sizeof (uhwptr);
+}
+
+// In GCC on ARM bp points to saved lr, not fp, so we should check the next
+// cell in stack to be a saved frame pointer. GetCanonicFrame returns the
+// pointer to saved frame pointer in any case.
+static inline uhwptr *GetCanonicFrame(uptr bp,
+                                      uptr stack_top,
+                                      uptr stack_bottom) {
+#ifdef __arm__
+  if (!IsValidFrame(bp, stack_top, stack_bottom)) return 0;
+  uhwptr *bp_prev = (uhwptr *)bp;
+  if (IsValidFrame((uptr)bp_prev[0], stack_top, stack_bottom)) return bp_prev;
+  return bp_prev - 1;
+#else
+  return (uhwptr*)bp;
+#endif
+}
+
 void StackTrace::FastUnwindStack(uptr pc, uptr bp,
                                  uptr stack_top, uptr stack_bottom,
                                  uptr max_depth) {
   CHECK_GE(max_depth, 2);
   trace[0] = pc;
   size = 1;
-  uhwptr *frame = (uhwptr *)bp;
-  uhwptr *prev_frame = frame - 1;
   if (stack_top < 4096) return;  // Sanity check for stack top.
+  uhwptr *frame = GetCanonicFrame(bp, stack_top, stack_bottom);
+  uhwptr *prev_frame = 0;
   // Avoid infinite loop when frame == frame[0] by using frame > prev_frame.
   while (frame > prev_frame &&
-         frame < (uhwptr *)stack_top - 2 &&
-         frame > (uhwptr *)stack_bottom &&
+         IsValidFrame((uptr)frame, stack_top, stack_bottom) &&
          IsAligned((uptr)frame, sizeof(*frame)) &&
          size < max_depth) {
     uhwptr pc1 = frame[1];
     if (pc1 != pc) {
       trace[size++] = (uptr) pc1;
     }
     prev_frame = frame;
-    frame = (uhwptr *)frame[0];
+    frame = GetCanonicFrame((uptr)frame[0], stack_top, stack_bottom);
   }
 }
 
 static bool MatchPc(uptr cur_pc, uptr trace_pc, uptr threshold) {
   return cur_pc - trace_pc <= threshold || trace_pc - cur_pc <= threshold;
 }
 
 void StackTrace::PopStackFrames(uptr count) {

test/asan/TestCases/Linux/clang_gcc_abi.cc

+// RUN: %clangxx_asan -O0 -x c %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O1 -x c %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O2 -x c %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O3 -x c %s -o %t && not %run %t 2>&1 | FileCheck %s
+
+// REQUIRES: arm-supported-target
+
+#include <stdlib.h>
+
+int boom() {
+  volatile int three = 3;
+  char *s = (char *)malloc(three);
+// CHECK: #1 0x{{.*}} in boom {{.*}}clang_gcc_abi.cc:[[@LINE-1]]
+  return s[three]; //BOOM
+}
+
+__attribute__((naked, noinline)) void gcc_abi() {
+// CHECK: #2 0x{{.*}} in gcc_abi {{.*}}clang_gcc_abi.cc:[[@LINE+1]]
+  asm volatile("str fp, [sp, #-8]!\n\t"
+               "str lr, [sp, #4]\n\t"
+               "add fp, sp, #4\n\t"
+               "bl  boom\n\t"
+               "sub sp, fp, #4\n\t"
+               "ldr fp, [sp]\n\t"
+               "add sp, sp, #4\n\t"
+               "ldr pc, [sp], #4\n\t"
+              );
+}
+
+__attribute__((naked, noinline)) void clang_abi() {
+// CHECK: #3 0x{{.*}} in clang_abi {{.*}}clang_gcc_abi.cc:[[@LINE+1]]
+  asm volatile("push {r11, lr}\n\t"
+               "mov r11, sp\n\t"
+               "bl  gcc_abi\n\t"
+               "add r0, r0, #1\n\t"
+               "pop {r11, pc}\n\t"
+              );
+}
+
+int main() {
+  clang_abi();
+// CHECK: #4 0x{{.*}} in main {{.*}}clang_gcc_abi.cc:[[@LINE-1]]
+}