Check stack pointer to be inside stack when unwinding.

base/debug/stack_trace.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/debug/stack_trace.h"
 
 #include <string.h>
 
 #include <algorithm>
 #include <sstream>
 
 #include "base/macros.h"
 
+#if HAVE_TRACE_STACK_FRAME_POINTERS && \
+    (defined(OS_ANDROID) || defined(OS_LINUX))
+#define HAVE_MINCORE
+#include <sys/mman.h>
+#include <unistd.h>
+#include "base/posix/eintr_wrapper.h"
+#include "base/process/process_metrics.h"
+#endif
+
+
 namespace base {
 namespace debug {
 
 StackTrace::StackTrace(const void* const* trace, size_t count) {
   count = std::min(count, arraysize(trace_));
   if (count)
     memcpy(trace_, trace, count * sizeof(trace_[0]));
   count_ = count;
 }
 
@@ skipping 10 matching lines @@
 std::string StackTrace::ToString() const {
   std::stringstream stream;
 #if !defined(__UCLIBC__)
   OutputToStream(&stream);
 #endif
   return stream.str();
 }
 
 #if HAVE_TRACE_STACK_FRAME_POINTERS
 
+#ifdef HAVE_MINCORE
+
+// Attempts to advance |address| by PageCount pages, checking that all
+// pages are mapped.
+// Returns true if no unmapped pages were found, and adds PageCount
+// pages to |address| and rounds result down to a page.
+// Returns false if unmapped pages were detected, and sets |address| to
+// the address of the lowest unmapped page.
+template <size_t PageCount>

[Primiano Tucci (use gerrit), 2016/05/31 16:13:06]

why do you need a template function with a lambda inside? Can this function be
simpler?

+bool AdvanceMappedPages(uintptr_t* address) {
+  auto probe = [](uintptr_t address, size_t size) -> bool {
+    uint8_t vec[PageCount];
+    int result = HANDLE_EINTR(

[Primiano Tucci (use gerrit), 2016/05/31 16:13:07]

accordin to its manpage mincore doesn't EINTR (vm operations are typically not
interruptible because they take the map_sem). At most can EAGAIN if the kernel
is temporarily OOM.So maybe you want to do something like
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/p...

[Dmitry Skiba, 2016/05/31 21:52:18]

Done.

+        mincore(reinterpret_cast<void*>(address), size, vec));
+    if (result == 0) {
+      return true;
+    }
+    // mincore() returns ENOMEM if address range contains unmapped pages.
+    CHECK_EQ(errno, ENOMEM);

[Primiano Tucci (use gerrit), 2016/05/31 16:13:06]

I'd remove this CHECK or make it a DCHECK.
1) you can get a EAGAIN
2) If you fail this estimation worst case you'll end up scanning a reduced stack
which is not undefined behavior (which is what CHECK should protect against).

[Dmitry Skiba, 2016/05/31 21:52:18]

Done.

+    return false;
+  };
+
+  size_t page_size = GetPageSize();
+
+  // Rewind address to the page boundary.
+  *address -= *address % page_size;
+
+  // Probe the whole range first.
+  if (probe(*address, PageCount * page_size)) {
+    *address += PageCount * page_size;
+    return true;
+  }
+
+  // Binary search leftmost page that is not mapped.
+  for (size_t pages = PageCount; pages != 1;) {

[Primiano Tucci (use gerrit), 2016/05/31 16:13:06]

I think this is a bit hard to follow as you do a binary search here and an outer
while below.
Could this be just a single loop in one place so it's easier to read? 

[Dmitry Skiba, 2016/05/31 21:52:18]

I don't see how it can be a single loop. When binary searching I half the range
on each iteration. So the first check that checks the whole range doens't really
belong here. Besides, I need to return true / false depending on the first
check. 

+    size_t half_pages = pages / 2;
+    if (!probe(*address, half_pages * page_size)) {
+      pages = half_pages;
+      continue;
+    }
+    *address += half_pages * page_size;
+    pages -= half_pages;
+  }
+  return false;
+}
+
+#endif  // HAVE_MINCORE
+
+PerThreadStackInfo::PerThreadStackInfo()
+  : start_address(0), start_address_final(false) {}
+
 size_t TraceStackFramePointers(const void** out_trace,
                                size_t max_depth,
-                               size_t skip_initial) {
+                               size_t skip_initial,
+                               PerThreadStackInfo* stack_info) {
   // Usage of __builtin_frame_address() enables frame pointers in this
   // function even if they are not enabled globally. So 'sp' will always
   // be valid.
   uintptr_t sp = reinterpret_cast<uintptr_t>(__builtin_frame_address(0));
 
   size_t depth = 0;
   while (depth < max_depth) {
 #if defined(__arm__) && defined(__GNUC__) && !defined(__clang__)
     // GCC and LLVM generate slightly different frames on ARM, see
     // https://llvm.org/bugs/show_bug.cgi?id=18505 - LLVM generates
     // x86-compatible frame, while GCC needs adjustment.
     sp -= sizeof(uintptr_t);
 #endif
 
+#ifdef HAVE_MINCORE
+    if (stack_info) {
+      // Both sp[0] and s[1] must be valid.
+      uintptr_t max_sp = sp + 2 * sizeof(uintptr_t);
+      if (!stack_info->start_address) {
+        // Initialize start_address so that it satisfies while() below.
+        stack_info->start_address = max_sp - 1;
+      }
+      while (max_sp > stack_info->start_address &&
+             !stack_info->start_address_final) {
+        stack_info->start_address_final = !AdvanceMappedPages<32>(
+            &stack_info->start_address);
+      }
+      if (max_sp > stack_info->start_address &&
+          stack_info->start_address_final) {
+        break;
+      }
+    }
+#endif
+
     if (skip_initial != 0) {
       skip_initial--;
     } else {
       out_trace[depth++] = reinterpret_cast<const void**>(sp)[1];
     }
 
     // Find out next frame pointer
     // (heuristics are from TCMalloc's stacktrace functions)
     {
       uintptr_t next_sp = reinterpret_cast<const uintptr_t*>(sp)[0];
@@ skipping 12 matching lines @@
     }
   }
 
   return depth;
 }
 
 #endif  // HAVE_TRACE_STACK_FRAME_POINTERS
 
 }  // namespace debug
 }  // namespace base