[LSAN] Fix test swapcontext.cc on MIPS

[slthakur]

Summary

There is no frame validity check in the slow unwinder like there is in the fast unwinder due to which lsan reports a leak even for heap allocated coroutine in the test swapcontext.cc. Since mips/linux uses slow unwindwer instead of fast unwinder, the test fails for mips/linux. Therefore adding the checks before unwinding fixes the test for mips/linux.

[slthakur]

Hi @kcc, @samsonov, @earthdok,

Could you all please review this patch?

[slthakur]

Gentle reminder!

[kcc]

Is a test (that works on Linux x86_64) possible here?

lib/lsan/lsan.h

23

Why did you remove this check (fast &&)?

[slthakur]

Is a test (that works on Linux x86_64) possible here?

The test swapcontext.cc itself serves as a test for invalid frame on Linux x86_64 too. The difference between x86_64 and mips in case of unwinding is that x86 uses fast unwinder which checks if the frame is valid before unwinding and mips uses slow unwinder which does not check if the frame is valid before unwinding.

lib/lsan/lsan.h

23

Before this patch frame validity check was there only for fast unwinder. So stack top and bottom were fetched only in case of fast unwinder as per my understanding. Now we need it for both unwinders irrespective of whether we use the fast or slow unwinder. Thats the reason I removed this check. Is there any other significance to this check?

[kcc]

In D18690#402241, @Sagar wrote:

Is a test (that works on Linux x86_64) possible here?

The test swapcontext.cc itself serves as a test for invalid frame on Linux x86_64 too. The difference between x86_64 and mips in case of unwinding is that x86 uses fast unwinder which checks if the frame is valid before unwinding and mips uses slow unwinder which does not check if the frame is valid before unwinding.

I am not sure I understand this.
If the existing test is enough for Linux x86_64 then why we would need this patch?
I want a test that will fail with the current code and get fixed with your patch (on Linux x86_64). Is such test possible?

lib/lsan/lsan.h

23

I don't know.
This code is extremely fragile and extremely hard to test,
so any changes in this code are scary, unless you can properly explain them and test them.

[slthakur]

Hi,

The test swapconext.cc fails only for MIPS64 and this patch is intended to fix the test swapcontext.cc only for Linux/MIPS64. This patch is not intended to fix any test/bug on Linux x86_64. The test swapcontext.cc was failing only on MIPS64 because for unwinding, MIPS uses the slow unwinder. And given an invalid frame (as given in test swapcontext.cc) the slow unwinder does not detect the invalid frame and continues to unwind even though the frame is invalid which results in a false positive report.

I have tested this patch on Linux x86_64 also. It does not cause any regressions.

[kcc]

LGTM

[slthakur]

Committed in revision 266716.

@kcc, @tstellarAMD: Is it okay to merge this patch to 3.8.1 branch?

[kcc]

I have no objection as long as all the tests pass

[pcc]

I suspect that this change has caused test failures on ARM:
http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/3417

Sagar, can you please take a look and revert if the fix is non-trivial?

[slthakur]

Hi Peter,

I am unable to reproduce this failure on ARMv7 Processor.
Here is the output I got for all optimization levels (O0, O1, O2, O3):

root@debian-armhf:~# arm-linux-gnueabihf-clang/bin/clang -g -O3 -fsanitize=address null_deref.cc 
root@debian-armhf:~# ./a.out 
ASAN:DEADLYSIGNAL
=================================================================
==856==ERROR: AddressSanitizer: SEGV on unknown address 0x00000028 (pc 0x000c8fa8 bp 0x7ed69bc0 sp 0x7ed69bc0 T0)
==856==The signal is caused by a READ memory access.
==856==Hint: address points to the zero page.
    #0 0xc8fa7 in NullDeref(int*) /root/null_deref.cc:15:10
    #1 0xc8f7b in main /root/null_deref.cc:21:3
    #2 0x76d8b631 in __libc_start_main (/lib/arm-linux-gnueabihf/libc.so.6+0x17631)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /root/null_deref.cc:15:10 in NullDeref(int*)
==856==ABORTING

The build bot failure is:

error: expected string not found in input // CHECK: {{ #0 0x.* in .*NullDeref.*null_deref.cc:}}[[@LINE-3]]

[slthakur]

This CL was reverted due to a regression in arm build bot. We were thinking of putting this code under #if defined(mips) and then recommitting it so that it does not affect the arm build bot. Is this fine with everyone?

[slthakur]

Putting this code under #if defined(mips).

lib/asan/asan_stack.h

       uptr stack_top = t->stack_top();
       uptr stack_bottom = t->stack_bottom();
       ScopedUnwinding unwind_scope(t);
+#if defined(__mips__)
+      if (IsValidFrame(bp, stack_top, stack_bottom)) {
+        stack->Unwind(max_depth, pc, bp, context, stack_top, stack_bottom,
+                      fast);
+      }
+#else
       stack->Unwind(max_depth, pc, bp, context, stack_top, stack_bottom, fast);
+#endif
     } else if (!t && !fast) {
       /* If GetCurrentThread() has failed, try to do slow unwind anyways. */
       stack->Unwind(max_depth, pc, bp, context, 0, 0, false);

lib/lsan/lsan.h

 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_stacktrace.h"
 
+#if defined(__mips__)
+#define GET_STACK_TRACE(max_size, fast)                                        \
+  BufferedStackTrace stack;                                                    \
+  {                                                                            \
+    uptr stack_top = 0, stack_bottom = 0;                                      \
+    ThreadContext *t;                                                          \
+    if (t = CurrentThreadContext()) {                                          \
+      stack_top = t->stack_end();                                              \
+      stack_bottom = t->stack_begin();                                         \
+    }                                                                          \
+    if (IsValidFrame(GET_CURRENT_FRAME(), stack_top, stack_bottom)) {          \
+      stack.Unwind(max_size, StackTrace::GetCurrentPc(), GET_CURRENT_FRAME(),  \
+                 /* context */ 0, stack_top, stack_bottom, fast);              \
+    }                                                                          \
+  }
+#else
 #define GET_STACK_TRACE(max_size, fast)                                        \
   BufferedStackTrace stack;                                                    \
   {                                                                            \

     stack.Unwind(max_size, StackTrace::GetCurrentPc(), GET_CURRENT_FRAME(),    \
                  /* context */ 0, stack_top, stack_bottom, fast);              \
   }
+#endif
 
 #define GET_STACK_TRACE_FATAL \
   GET_STACK_TRACE(kStackTraceMax, common_flags()->fast_unwind_on_fatal)

lib/sanitizer_common/sanitizer_stacktrace.h

   void operator=(const BufferedStackTrace &);
 };
 
+// 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);
+}
+
 }  // namespace __sanitizer
 
 // Use this macro if you want to print stack trace with the caller

lib/sanitizer_common/sanitizer_stacktrace.cc

   top_frame_bp = 0;
 }
 
-// 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.