[sanitizer] Stop unwinding the stack when a close-to-zero PC is found

[kubamracek]

Summary

On OS X, we often get stack trace in a report that ends with a 0x0 frame:

=================================================================
==56615==ERROR: AddressSanitizer: heap-use-after-free on address 0x60200000eed0 at pc 0x00010aa33359 bp 0x7fff552057f0 sp 0x7fff552023a0
READ of size 2 at 0x60200000eed0 thread T0
#0 0x10aa33358 in printf_common(void*, char const*, __va_list_tag*) sanitizer_common_interceptors_format.inc:545
#1 0x10aa31e24 in wrap_vprintf sanitizer_common_interceptors.inc:1099
#2 0x7fff8c4375ac in start (libdyld.dylib+0x35ac)
#3 0x0  (<unknown module>)

To get rid of it, let's trim the stack trace when we find a close-to-zero value, which is obviously not a valid PC.

[glider]

Makes sense, LGTM

lib/sanitizer_common/sanitizer_stacktrace.h

21 ↗

(On Diff #40148)

OOC, have you seen values other than 0x0 for the bottom frame?

[samsonov]

This doesn't look like a good change. Why is this fixed only in fast, but not in slow unwinder? Are you sure we should consider any address 0x1000 as bad, not only 0x0?

[kubamracek]

In D14656#289133, @samsonov wrote:

This doesn't look like a good change. Why is this fixed only in fast, but not in slow unwinder? Are you sure we should consider any address 0x1000 as bad, not only 0x0?

Ignoring 0x0 is not enough. The result 0x0 printed in the stack trace is actually a 0x1 at the bottom of the stack (because we subtract 1 from return addresses in GetPreviousInstructionPc).

[glider]

In D14656#289133, @samsonov wrote:

This doesn't look like a good change. Why is this fixed only in fast, but not in slow unwinder? Are you sure we should consider any address 0x1000 as bad, not only 0x0?

It should be safe to assume that the first kPageSize bytes of the address space (not necessarily 0x1000) do not contain any code.

[kubamracek]

Updating patch.

[glider]

So how about doing the same in the slow unwinder?

[kubamracek]

Adding the check into slow unwinder as well.

[glider]

I think this is fine, but let's wait for Alexey.

[samsonov]

Please add a test case to lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc
I'm slightly worried about extra function call you add to each fast stack unwind. Maybe it makes sense to inline that function in the header.

[kubamracek]

Adding a unit test. Changing GetPageSizeCached into an inlined function.

[kubamracek]

ping

[kubamracek]

Is this okay to land?

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h

 INLINE void SetVerbosity(int verbosity) {
   atomic_store(&current_verbosity, verbosity, memory_order_relaxed);
 }
 INLINE int Verbosity() {
   return atomic_load(&current_verbosity, memory_order_relaxed);
 }
 
 uptr GetPageSize();
-uptr GetPageSizeCached();
+extern uptr PageSizeCached;
+INLINE uptr GetPageSizeCached() {
+  if (!PageSizeCached)
+    PageSizeCached = GetPageSize();
+  return PageSizeCached;
+}
 uptr GetMmapGranularity();
 uptr GetMaxVirtualAddress();
 // Threads
 uptr GetTid();
 uptr GetThreadSelf();
 void GetThreadStackTopAndBottom(bool at_initialization, uptr *stack_top,
                                 uptr *stack_bottom);
 void GetThreadStackAndTls(bool main, uptr *stk_addr, uptr *stk_size,

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.cc

 #include "sanitizer_stacktrace_printer.h"
 #include "sanitizer_symbolizer.h"
 
 namespace __sanitizer {
 
 const char *SanitizerToolName = "SanitizerTool";
 
 atomic_uint32_t current_verbosity;
-
-uptr GetPageSizeCached() {
-  static uptr PageSize;
-  if (!PageSize)
-    PageSize = GetPageSize();
-  return PageSize;
-}
+uptr PageSizeCached;
 
 StaticSpinMutex report_file_mu;
 ReportFile report_file = {&report_file_mu, kStderrFd, "", "", 0};
 
 void RawWrite(const char *buffer) {
   report_file.Write(buffer, internal_strlen(buffer));
 }
 

compiler-rt/trunk/lib/sanitizer_common/sanitizer_stacktrace.cc

#ifdef __arm__
   return bp_prev;
 #else
   return (uhwptr*)bp;
 #endif
 }
 
 void BufferedStackTrace::FastUnwindStack(uptr pc, uptr bp, uptr stack_top,
                                          uptr stack_bottom, u32 max_depth) {
+  const uptr kPageSize = GetPageSizeCached();
   CHECK_GE(max_depth, 2);
   trace_buffer[0] = pc;
   size = 1;
   if (stack_top < 4096) return;  // Sanity check for stack top.
   uhwptr *frame = GetCanonicFrame(bp, stack_top, stack_bottom);
   // Lowest possible address that makes sense as the next frame pointer.
   // Goes up as we walk the stack.
   uptr bottom = stack_bottom;
if (!IsValidFrame((uptr)caller_frame, stack_top, bottom) ||
         !IsAligned((uptr)caller_frame, sizeof(uhwptr)))
       break;
     uhwptr pc1 = caller_frame[2];
 #elif defined(__s390__)
     uhwptr pc1 = frame[14];
 #else
     uhwptr pc1 = frame[1];
 #endif
+    if (pc1 < kPageSize)
+      break;
     if (pc1 != pc) {
       trace_buffer[size++] = (uptr) pc1;
     }
     bottom = (uptr)frame;
     frame = GetCanonicFrame((uptr)frame[0], stack_top, bottom);
   }
 }
 

compiler-rt/trunk/lib/sanitizer_common/sanitizer_unwind_linux_libcdep.cc

struct UnwindTraceArg {
   BufferedStackTrace *stack;
   u32 max_depth;
 };
 
 _Unwind_Reason_Code Unwind_Trace(struct _Unwind_Context *ctx, void *param) {
   UnwindTraceArg *arg = (UnwindTraceArg*)param;
   CHECK_LT(arg->stack->size, arg->max_depth);
   uptr pc = Unwind_GetIP(ctx);
+  const uptr kPageSize = GetPageSizeCached();
+  if (pc < kPageSize) return UNWIND_STOP;
   arg->stack->trace_buffer[arg->stack->size++] = pc;
   if (arg->stack->size == arg->max_depth) return UNWIND_STOP;
   return UNWIND_CONTINUE;
 }
 
 void BufferedStackTrace::SlowUnwindStack(uptr pc, u32 max_depth) {
   CHECK_GE(max_depth, 2);
   size = 0;

compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc

TEST_F(FastUnwindTest, FPBelowPrevFP) {
   fake_stack[1] = PC(1);
   if (!TryFastUnwind(3))
     return;
   EXPECT_EQ(2U, trace.size);
   EXPECT_EQ(PC(0), trace.trace[0]);
   EXPECT_EQ(PC(1), trace.trace[1]);
 }
 
+TEST_F(FastUnwindTest, CloseToZeroFrame) {
+  // Make one pc a NULL pointer.
+  fake_stack[5] = 0x0;
+  if (!TryFastUnwind(kStackTraceMax))
+    return;
+  // The stack should be truncated at the NULL pointer (and not include it).
+  EXPECT_EQ(3U, trace.size);
+  EXPECT_EQ(start_pc, trace.trace[0]);
+  for (uptr i = 1; i < 3U; i++) {
+    EXPECT_EQ(PC(i*2 - 1), trace.trace[i]);
+  }
+}
+
 TEST(SlowUnwindTest, ShortStackTrace) {
   if (StackTrace::WillUseFastUnwind(false))
     return;
   BufferedStackTrace stack;
   uptr pc = StackTrace::GetCurrentPc();
   uptr bp = GET_CURRENT_FRAME();
   stack.Unwind(0, pc, bp, 0, 0, 0, false);
   EXPECT_EQ(0U, stack.size);
   EXPECT_EQ(0U, stack.top_frame_bp);
   stack.Unwind(1, pc, bp, 0, 0, 0, false);
   EXPECT_EQ(1U, stack.size);
   EXPECT_EQ(pc, stack.trace[0]);
   EXPECT_EQ(bp, stack.top_frame_bp);
 }
 
 }  // namespace __sanitizer