[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

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?

lib/sanitizer_common/sanitizer_stacktrace.h

 #ifndef SANITIZER_STACKTRACE_H
 #define SANITIZER_STACKTRACE_H
 
 #include "sanitizer_internal_defs.h"
 
 namespace __sanitizer {
 
 static const u32 kStackTraceMax = 256;
+static const uptr kMinStackFramePcValue = 0x1000;
 
 #if SANITIZER_LINUX &&  (defined(__sparc__) || defined(__mips__))
 # define SANITIZER_CAN_FAST_UNWIND 0
 #elif SANITIZER_WINDOWS
 # define SANITIZER_CAN_FAST_UNWIND 0
 #else
 # define SANITIZER_CAN_FAST_UNWIND 1
 #endif

lib/sanitizer_common/sanitizer_stacktrace.cc

#ifdef __powerpc__
     uhwptr *caller_frame = (uhwptr*)frame[0];
     if (!IsValidFrame((uptr)caller_frame, stack_top, bottom) ||
         !IsAligned((uptr)caller_frame, sizeof(uhwptr)))
       break;
     uhwptr pc1 = caller_frame[2];
 #else
     uhwptr pc1 = frame[1];
 #endif
+    if (pc1 < kMinStackFramePcValue)
+      break;
     if (pc1 != pc) {
       trace_buffer[size++] = (uptr) pc1;
     }
     bottom = (uptr)frame;
     frame = GetCanonicFrame((uptr)frame[0], stack_top, bottom);
   }
 }