Bounds-check rbp when unwinding a stack frame that uses rbp as a frame pointer.

base/profiler/native_stack_sampler_mac.cc

Index: base/profiler/native_stack_sampler_mac.cc
diff --git a/base/profiler/native_stack_sampler_mac.cc b/base/profiler/native_stack_sampler_mac.cc
index e29545a24028f0c8af70211e03b1734cd44814b0..4e5108e5daa5b9cc0a06420c6b4c316aebba9e30 100644
--- a/base/profiler/native_stack_sampler_mac.cc
+++ b/base/profiler/native_stack_sampler_mac.cc
@@ -7,6 +7,7 @@
 #include <dlfcn.h>
 #include <libkern/OSByteOrder.h>
 #include <libunwind.h>
+#include <mach-o/compact_unwind_encoding.h>
 #include <mach-o/swap.h>
 #include <mach/kern_return.h>
 #include <mach/mach.h>
@@ -113,6 +114,7 @@ void CopyStackAndRewritePointers(uintptr_t* stack_copy_bottom,
 // true.
 template <typename StackFrameCallback>
 bool WalkStackFromContext(unw_context_t* unwind_context,
+                          uintptr_t stack_top,
                           size_t* frame_count,
                           const StackFrameCallback& callback) {
   unw_cursor_t unwind_cursor;
@@ -126,6 +128,20 @@ bool WalkStackFromContext(unw_context_t* unwind_context,
 
     callback(static_cast<uintptr_t>(ip));
 
+    // If this stack frame has a frame pointer, stepping the cursor will involve

[Mike Wittman, 2017/05/19 21:21:21]

nit: this could be extracted to a separate function

[Avi (use Gerrit), 2017/05/22 19:03:37]

It could be, but it's short enough and is only used here, so I'm not sure I see
a reason to extract it.

[Mike Wittman, 2017/05/22 19:19:58]

I was thinking it would make this function a little easier to read, but the
effect is relatively marginal so feel free to leave as is.

+    // indexing memory access off of that pointer. In that case, sanity-check

[Mark Mentovai, 2017/05/22 20:29:44]

Does unw_init_remote() work instead of unw_init_local()? (Even though you’re
dealing with the same process.)

Probably not, it looks like it’s not fully/properly implemented. If it worked,
it’d get you something that accessed memory via mach_vm_read() or similar, which
will just fail when asked to read something unmapped or protected. Compare this
to the raw pointer dereference, which crashes under those circumstances.

[Avi (use Gerrit), 2017/05/22 20:32:35]

I saw that when looking at the source. Dunno why they'd spec out an API and then
neglect to actually implement it.

+    // the frame pointer register to ensure it's within bounds.
+    unw_proc_info_t proc_info;
+    unw_get_proc_info(&unwind_cursor, &proc_info);
+    if ((proc_info.format & UNWIND_X86_64_MODE_MASK) ==
+        UNWIND_X86_64_MODE_RBP_FRAME) {
+      unw_word_t rsp, rbp;
+      unw_get_reg(&unwind_cursor, UNW_X86_64_RSP, &rsp);
+      unw_get_reg(&unwind_cursor, UNW_X86_64_RBP, &rbp);
+      if (rbp < rsp || rbp > stack_top)
+        return false;
+    }
+
     step_result = unw_step(&unwind_cursor);
   } while (step_result > 0);
 
@@ -172,7 +188,8 @@ void WalkStack(const x86_thread_state64_t& thread_state,
   // over.
   unw_context_t unwind_context;
   memcpy(&unwind_context, &thread_state, sizeof(uintptr_t) * 17);
-  bool result = WalkStackFromContext(&unwind_context, &frame_count, callback);
+  bool result =
+      WalkStackFromContext(&unwind_context, stack_top, &frame_count, callback);
 
   if (!result)
     return;
@@ -194,7 +211,7 @@ void WalkStack(const x86_thread_state64_t& thread_state,
       strcmp(info.dli_fname, LibSystemKernelName()) == 0) {
       rip = *reinterpret_cast<uint64_t*>(rsp);
       rsp += 8;
-      WalkStackFromContext(&unwind_context, &frame_count, callback);
+      WalkStackFromContext(&unwind_context, stack_top, &frame_count, callback);
     }
   }
 }