[Lldb-commits] [lldb] 2a42a5a - In 'thread step-out' command, only insert a breakpoint in executable memory.

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2019-12-20T11:02:24-08:00
New Revision: 2a42a5a2f4144cd99812ad0d230480f94a1d1c92

URL: https://github.com/llvm/llvm-project/commit/2a42a5a2f4144cd99812ad0d230480f94a1d1c92
DIFF: https://github.com/llvm/llvm-project/commit/2a42a5a2f4144cd99812ad0d230480f94a1d1c92.diff

LOG: In 'thread step-out' command, only insert a breakpoint in executable memory.

Previously, if the current function had a nonstandard stack layout/ABI, and had a valid
data pointer in the location where the return address is usually located, data corruption
would occur when the breakpoint was written. This could lead to an incorrectly reported
crash or silent corruption of the program's state. Now, if the above check fails, the command safely aborts.

Differential Revision: https://reviews.llvm.org/D71372

Added: 
    lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
    lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test

Modified: 
    lldb/include/lldb/Target/ThreadPlanStepOut.h
    lldb/source/Target/ThreadPlanStepOut.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Target/ThreadPlanStepOut.h b/lldb/include/lldb/Target/ThreadPlanStepOut.h
index 00984db2dca9..576b416c3f2c 100644
--- a/lldb/include/lldb/Target/ThreadPlanStepOut.h
+++ b/lldb/include/lldb/Target/ThreadPlanStepOut.h
@@ -72,6 +72,7 @@ class ThreadPlanStepOut : public ThreadPlan, public ThreadPlanShouldStopHere {
   std::vector<lldb::StackFrameSP> m_stepped_past_frames;
   lldb::ValueObjectSP m_return_valobj_sp;
   bool m_calculate_return_value;
+  StreamString m_constructor_errors;
 
   friend lldb::ThreadPlanSP Thread::QueueThreadPlanForStepOut(
       bool abort_other_plans, SymbolContext *addr_context, bool first_insn,

diff  --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index d7dae446b229..f15a343aaa38 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -126,6 +126,25 @@ ThreadPlanStepOut::ThreadPlanStepOut(
     if (m_return_addr == LLDB_INVALID_ADDRESS)
       return;
 
+    // Perform some additional validation on the return address.
+    uint32_t permissions = 0;
+    if (!m_thread.GetProcess()->GetLoadAddressPermissions(m_return_addr,
+                                                          permissions)) {
+      m_constructor_errors.Printf("Return address (0x%" PRIx64
+                                  ") permissions not found.",
+                                  m_return_addr);
+      LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast<void *>(this),
+                m_constructor_errors.GetData());
+      return;
+    } else if (!(permissions & ePermissionsExecutable)) {
+      m_constructor_errors.Printf("Return address (0x%" PRIx64
+                                  ") did not point to executable memory.",
+                                  m_return_addr);
+      LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast<void *>(this),
+                m_constructor_errors.GetData());
+      return;
+    }
+
     Breakpoint *return_bp = m_thread.CalculateTarget()
                                 ->CreateBreakpoint(m_return_addr, true, false)
                                 .get();
@@ -238,8 +257,13 @@ bool ThreadPlanStepOut::ValidatePlan(Stream *error) {
   }
 
   if (m_return_bp_id == LLDB_INVALID_BREAK_ID) {
-    if (error)
+    if (error) {
       error->PutCString("Could not create return address breakpoint.");
+      if (m_constructor_errors.GetSize() > 0) {
+        error->PutCString(" ");
+        error->PutCString(m_constructor_errors.GetString());
+      }
+    }
     return false;
   }
 

diff  --git a/lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s b/lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
new file mode 100644
index 000000000000..d18ea24fba4d
--- /dev/null
+++ b/lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
@@ -0,0 +1,20 @@
+        .text
+        .globl  asm_main
+asm_main:
+        sub $0x8, %rsp
+        movq $0, (%rsp)
+        push %rsp
+        jmp _nonstandard_stub
+
+# Takes a single pointer argument via the stack, which is nonstandard for x64.
+# Executing 'thread step-out' here will initially attempt to write a
+# breakpoint to that stack address, but should fail because of the executable
+# memory check.
+_nonstandard_stub:
+        mov (%rsp), %rdi
+        mov (%rdi), %rsi
+        add $1, %rsi
+        mov %rsi, (%rdi)
+
+        add $0x10, %rsp
+        ret

diff  --git a/lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test b/lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test
new file mode 100644
index 000000000000..96490faa2de8
--- /dev/null
+++ b/lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test
@@ -0,0 +1,17 @@
+# Test that `thread step-out` fails when the "return address"
+# points to non-executable memory.
+
+# REQUIRES: target-x86_64, native
+
+# RUN: %clang_host %p/Inputs/call-asm.c %p/Inputs/thread-step-out-ret-addr-check.s -o %t
+# RUN: %lldb %t -s %s -b 2>&1 | FileCheck %s
+
+breakpoint set -n nonstandard_stub
+# CHECK: Breakpoint 1: where = {{.*}}`nonstandard_stub
+
+process launch
+# CHECK: stop reason = breakpoint 1.1
+
+thread step-out
+# CHECK: Could not create return address breakpoint.
+# CHECK: Return address (0x{{[a-f0-9]*}}) did not point to executable memory.