Issue in StackwalkerAMD64::GetCallerByFramePointerRecovery.

src/processor/stackwalker_amd64.cc

 // Copyright (c) 2010 Google Inc.
 // All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 // notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
@@ skipping 129 matching lines @@
   // Make sure we recovered all the essentials.
   static const int essentials = (StackFrameAMD64::CONTEXT_VALID_RIP
                                  | StackFrameAMD64::CONTEXT_VALID_RSP);
   if ((frame->context_validity & essentials) != essentials)
     return NULL;
 
   frame->trust = StackFrame::FRAME_TRUST_CFI;
   return frame.release();
 }
 
+bool StackwalkerAMD64::IsEndOfStack(uint64_t caller_rip, uint64_t caller_rsp,
+                                    uint64_t callee_rsp) {
+  // Treat an instruction address of 0 as end-of-stack.
+  if (caller_rip == 0) {
+    return true;
+  }
+
+  // If the new stack pointer is at a lower address than the old, then
+  // that's clearly incorrect. Treat this as end-of-stack to enforce
+  // progress and avoid infinite loops.
+  if (caller_rsp < callee_rsp) {
+    return true;
+  }
+
+  return false;
+}
+
 StackFrameAMD64* StackwalkerAMD64::GetCallerByFramePointerRecovery(
     const vector<StackFrame*>& frames) {
   StackFrameAMD64* last_frame = static_cast<StackFrameAMD64*>(frames.back());
   uint64_t last_rsp = last_frame->context.rsp;
   uint64_t last_rbp = last_frame->context.rbp;
 
   // Assume the presence of a frame pointer. This is not mandated by the
   // AMD64 ABI, c.f. section 3.2.2 footnote 7, though it is typical for
   // compilers to still preserve the frame pointer and not treat %rbp as a
   // general purpose register.
   //
   // With this assumption, the CALL instruction pushes the return address
   // onto the stack and sets %rip to the procedure to enter. The procedure
   // then establishes the stack frame with a prologue that PUSHes the current
   // %rbp onto the stack, MOVes the current %rsp to %rbp, and then allocates
   // space for any local variables. Using this procedure linking information,
   // it is possible to locate frame information for the callee:
   //
   // %caller_rsp = *(%callee_rbp + 16)
   // %caller_rip = *(%callee_rbp + 8)
   // %caller_rbp = *(%callee_rbp)
 
   uint64_t caller_rip, caller_rbp;
   if (memory_->GetMemoryAtAddress(last_rbp + 8, &caller_rip) &&
       memory_->GetMemoryAtAddress(last_rbp, &caller_rbp)) {
     uint64_t caller_rsp = last_rbp + 16;
 
     // Simple sanity check that the stack is growing downwards as expected.
     if (caller_rbp < last_rbp || caller_rsp < last_rsp)

[Mark Mentovai, 2015/10/16 02:44:18]

This caller_rsp < last_rsp check is duplicated in IsEndOfStack().

Perhaps you should remove it from here and combine these into

  if (IsEndOfStack(caller_rip, caller_rsp, last_rsp) || caller_rbp < last_rbp) {

[ivanpe, 2015/10/16 03:35:42]

Done.

       return NULL;
 
+    if (IsEndOfStack(caller_rip, caller_rsp, last_rsp)) {
+      // Reached end-of-stack.
+      return NULL;
+    }
+
     StackFrameAMD64* frame = new StackFrameAMD64();
     frame->trust = StackFrame::FRAME_TRUST_FP;
     frame->context = last_frame->context;
     frame->context.rip = caller_rip;
     frame->context.rsp = caller_rsp;
     frame->context.rbp = caller_rbp;
     frame->context_validity = StackFrameAMD64::CONTEXT_VALID_RIP |
                               StackFrameAMD64::CONTEXT_VALID_RSP |
                               StackFrameAMD64::CONTEXT_VALID_RBP;
     return frame;
@@ skipping 86 matching lines @@
   if (system_info_->os_short == "nacl") {
     // Apply constraints from Native Client's x86-64 sandbox.  These
     // registers have the 4GB-aligned sandbox base address (from r15)
     // added to them, and only the bottom 32 bits are relevant for
     // stack walking.
     new_frame->context.rip = static_cast<uint32_t>(new_frame->context.rip);
     new_frame->context.rsp = static_cast<uint32_t>(new_frame->context.rsp);
     new_frame->context.rbp = static_cast<uint32_t>(new_frame->context.rbp);
   }
 
-  // Treat an instruction address of 0 as end-of-stack.
-  if (new_frame->context.rip == 0)
+  if (IsEndOfStack(new_frame->context.rip, new_frame->context.rsp,
+                   last_frame->context.rsp)) {
+    // Reached end-of-stack.
     return NULL;
-
-  // If the new stack pointer is at a lower address than the old, then
-  // that's clearly incorrect. Treat this as end-of-stack to enforce
-  // progress and avoid infinite loops.
-  if (new_frame->context.rsp <= last_frame->context.rsp)
-    return NULL;
+  }
 
   // new_frame->context.rip is the return address, which is the instruction
   // after the CALL that caused us to arrive at the callee. Set
   // new_frame->instruction to one less than that, so it points within the
   // CALL instruction. See StackFrame::instruction for details, and
   // StackFrameAMD64::ReturnAddress.
   new_frame->instruction = new_frame->context.rip - 1;
 
   return new_frame.release();
 }
 
 }  // namespace google_breakpad