[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg created this revision.
mossberg added reviewers: labath, jingham.
mossberg added a project: LLDB.
Herald added a subscriber: lldb-commits.

During the 'thread step-out' command, check that the memory we are about to 
place a breakpoint in is 1. at an Address with a valid Section and 2. in an 
executable Section. 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 either of the above checks fail, the command 
safely aborts.

Further discussion:

- This patch doesn't include a unit test -- I'd be happy to add one, but would 
appreciate guidance on how to do so. This is my first time working with the 
lldb codebase.
- I wasn't sure if it was necessary to first check the `log` pointer before 
using it. Some parts of the code do this, and some don't.
- Should we print out the return address in the log line?

Also, I don't have commit access, so I will need some help landing it when it's 
ready.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71372

Files:
  lldb/source/Target/ThreadPlanStepOut.cpp


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Target/ThreadPlanStepOut.h"
 #include "lldb/Breakpoint/Breakpoint.h"
+#include "lldb/Core/Section.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/Symbol/Block.h"
@@ -120,6 +121,21 @@
 }
   }
 }
+
+// Perform some additional validation on the return address.
+const auto return_address_section = return_address.GetSection();
+if (!return_address_section) {
+  LLDB_LOGF(log, "Return address had no section.");
+  return;
+}
+
+const auto return_address_section_perms =
+return_address_section->GetPermissions();
+if (!(return_address_section_perms & ePermissionsExecutable)) {
+  LLDB_LOGF(log, "Return address did not point to executable memory.");
+  return;
+}
+
 m_return_addr =
 return_address.GetLoadAddress(&m_thread.GetProcess()->GetTarget());
 


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Target/ThreadPlanStepOut.h"
 #include "lldb/Breakpoint/Breakpoint.h"
+#include "lldb/Core/Section.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/Symbol/Block.h"
@@ -120,6 +121,21 @@
 }
   }
 }
+
+// Perform some additional validation on the return address.
+const auto return_address_section = return_address.GetSection();
+if (!return_address_section) {
+  LLDB_LOGF(log, "Return address had no section.");
+  return;
+}
+
+const auto return_address_section_perms =
+return_address_section->GetPermissions();
+if (!(return_address_section_perms & ePermissionsExecutable)) {
+  LLDB_LOGF(log, "Return address did not point to executable memory.");
+  return;
+}
+
 m_return_addr =
 return_address.GetLoadAddress(&m_thread.GetProcess()->GetTarget());
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg updated this revision to Diff 233442.
mossberg added a comment.

Updated to get permissions information from the Process rather than the Section.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372

Files:
  lldb/source/Target/ThreadPlanStepOut.cpp


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Target/ThreadPlanStepOut.h"
 #include "lldb/Breakpoint/Breakpoint.h"
+#include "lldb/Core/Section.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/Symbol/Block.h"
@@ -126,6 +127,17 @@
 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)) {
+  LLDB_LOGF(log, "Permissions not found for return address.");
+  return;
+} else if (!(permissions & ePermissionsExecutable)) {
+  LLDB_LOGF(log, "Return address did not point to executable memory.");
+  return;
+}
+
 Breakpoint *return_bp = m_thread.CalculateTarget()
 ->CreateBreakpoint(m_return_addr, true, false)
 .get();


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Target/ThreadPlanStepOut.h"
 #include "lldb/Breakpoint/Breakpoint.h"
+#include "lldb/Core/Section.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/Symbol/Block.h"
@@ -126,6 +127,17 @@
 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)) {
+  LLDB_LOGF(log, "Permissions not found for return address.");
+  return;
+} else if (!(permissions & ePermissionsExecutable)) {
+  LLDB_LOGF(log, "Return address did not point to executable memory.");
+  return;
+}
+
 Breakpoint *return_bp = m_thread.CalculateTarget()
 ->CreateBreakpoint(m_return_addr, true, false)
 .get();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg marked 2 inline comments as done.
mossberg added inline comments.



Comment at: lldb/source/Target/ThreadPlanStepOut.cpp:127-129
+if (!return_address_section) {
+  LLDB_LOGF(log, "Return address had no section.");
+  return;

clayborg wrote:
> This will fail for JIT'ed code. Best to ask the process for memory region 
> information since it will usually be complete mappings for the process, even 
> if we have no file on disk for the region in question:
> 
> ```
> uint32_t permissions = 0;
> const addr_t return_pc = 
> return_address.GetLoadAddr(&m_thread.GetProcess()->GetTarget());
> if (!m_thread.GetProcess()->GetLoadAddressPermissions(return_pc, 
> permissions)) {
>   LLDB_LOGF(log, "No permissions for PC.");
>   return;
> } else if (!(permissions & ePermissionsExecutable)) {
>   LLDB_LOGF(log, "Return address did not point to executable memory.");
>   return;
> }
> ```
> 
> The issue is we don't always have an object file for stuff we debug. If we 
> rely on having an object file, like to require sections as your solution did, 
> then this will fail for any JIT'ed code or any code that we weren't able to 
> find object files for (remote debugging where we might not have all files).
> 
> 
Thanks @clayborg! Updated the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

Here are a few screenshots of what the bug looks like:

Memory corruption leading to a "crash":
F11025392: image.png 

Silent memory corruption:
F11025394: lldbbug.png 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

Something I noticed while updating the patch was that the 
`GetLoadedAddressPermissions` call would succeed, even if passed an address 
that is obviously not mapped. In my test case I placed an int 0x22 where the 
return address would be, expecting the validation to fail at the 
`GetLoadAddressPermissions` call because 0x22 is not mapped. However, it only 
ended up failing when the permissions (which were empty) were checked for an 
execute bit. It seems to me like this might be another bug, but I'm not sure.

F11025433: image.png 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg updated this revision to Diff 233466.
mossberg added a comment.

Remove lingering Section.h include


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372

Files:
  lldb/source/Target/ThreadPlanStepOut.cpp


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -126,6 +126,17 @@
 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)) {
+  LLDB_LOGF(log, "Permissions not found for return address.");
+  return;
+} else if (!(permissions & ePermissionsExecutable)) {
+  LLDB_LOGF(log, "Return address did not point to executable memory.");
+  return;
+}
+
 Breakpoint *return_bp = m_thread.CalculateTarget()
 ->CreateBreakpoint(m_return_addr, true, false)
 .get();


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -126,6 +126,17 @@
 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)) {
+  LLDB_LOGF(log, "Permissions not found for return address.");
+  return;
+} else if (!(permissions & ePermissionsExecutable)) {
+  LLDB_LOGF(log, "Return address did not point to executable memory.");
+  return;
+}
+
 Breakpoint *return_bp = m_thread.CalculateTarget()
 ->CreateBreakpoint(m_return_addr, true, false)
 .get();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg updated this revision to Diff 233470.
mossberg added a comment.

Output return address in logging


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372

Files:
  lldb/source/Target/ThreadPlanStepOut.cpp


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -126,6 +126,20 @@
 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)) {
+  LLDB_LOGF(log, "Permissions not found for return address (0x%llx).",
+m_return_addr);
+  return;
+} else if (!(permissions & ePermissionsExecutable)) {
+  LLDB_LOGF(log,
+"Return address (0x%llx) did not point to executable memory.",
+m_return_addr);
+  return;
+}
+
 Breakpoint *return_bp = m_thread.CalculateTarget()
 ->CreateBreakpoint(m_return_addr, true, false)
 .get();


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -126,6 +126,20 @@
 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)) {
+  LLDB_LOGF(log, "Permissions not found for return address (0x%llx).",
+m_return_addr);
+  return;
+} else if (!(permissions & ePermissionsExecutable)) {
+  LLDB_LOGF(log,
+"Return address (0x%llx) did not point to executable memory.",
+m_return_addr);
+  return;
+}
+
 Breakpoint *return_bp = m_thread.CalculateTarget()
 ->CreateBreakpoint(m_return_addr, true, false)
 .get();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

Current output.

F11025690: image.png 

F11025692: image.png 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

In D71372#1781316 , @labath wrote:

> I'm not sure how easy it is to do that here, but it would certainly be nice 
> to include these error messages in the actual error output from the "finish" 
> command so that they are visible even without logging enabled.


Agreed!

> As for the test, you should be able to do something similar to the tests in 
> the test/Shell/Unwind folder (e.g. eh-frame-dwarf-unwind.test). I.e., you 
> could write an assembly function which uses a non-standard frame layout, but 
> do *not* describe that layout via .eh_frame. Then, stop in that function and 
> try to step out...

Thanks, will look into this.

> Are you sure there is nothing mapped at that address? I'm not a darwin 
> expert, but I have a vague knowledge that the darwin loader (or some other 
> component of the system) actually maps a couple of pages of unreadable memory 
> around the address zero...

Here's how I checked:

  (lldb) fin
   SynthesizeTailCallFrames: can't find previous function
   Return address (0x22) did not point to executable memory.
   Discarding thread plans for thread tid = 0x1bd7b6, up to 0x7fe2f165a220
  error: Could not create return address breakpoint.
  (lldb) image list -a 0x22
  error: Couldn't find module containing address: 0x22.



  $ vmmap 94213 0x22
  0x22 is not in any region.  Bytes before following region: 4294967262




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

> So just add a std::string to ThreadPlanStepOut, and cons up your error 
> message there.  Then in ThreadPlanStepOut::ValidatePlan, instead of doing:
> 
>   if (m_return_bp_id == LLDB_INVALID_BREAK_ID) {
> if (error)
>   error->PutCString("Could not create return address breakpoint.");
> return false;
>   }
>
> 
> Write the error string you made in the constructor.
> 
> It's a good idea to log it as well, since the logs don't get command output 
> so having it there will make logs easier to read.

Ok, I see that `ThreadPlanCallFunction.cpp` seems to do this. I'll plan to 
model mine after it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg updated this revision to Diff 233673.
mossberg added a comment.

Output error messages to console in addition to logging.

I wasn't sure, but decided to unconditionally output the  `Could not create 
return address breakpoint.` message for UX reasons. That message provides a bit 
of context for these new, lower level error messages.

Implementation is based on the one in `ThreadPlanCallFunction.cpp`.
I also slightly altered the error message for the "permissions" not found case 
to make both error messages more consistent.

Still working on adding the unit test, but I thought I'd submit this for early 
feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372

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


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -126,6 +126,25 @@
 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(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(this),
+m_constructor_errors.GetData());
+  return;
+}
+
 Breakpoint *return_bp = m_thread.CalculateTarget()
 ->CreateBreakpoint(m_return_addr, true, false)
 .get();
@@ -238,8 +257,13 @@
   }
 
   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;
   }
 
Index: lldb/include/lldb/Target/ThreadPlanStepOut.h
===
--- lldb/include/lldb/Target/ThreadPlanStepOut.h
+++ lldb/include/lldb/Target/ThreadPlanStepOut.h
@@ -72,6 +72,7 @@
   std::vector 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,


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -126,6 +126,25 @@
 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(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(this),
+m_constructor_errors.GetData());
+  return;
+}
+
 Breakpoint *return_bp = m_thread.CalculateTarget()
 ->CreateBreakpoint(m_return_addr, true, false)
 .get();
@@ -238,8 +257,13 @@
   }
 
   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;
   }
 
Index: lldb

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

Screenshots with new error messages.

F11034423: image.png 

F11034433: image.png 

(I artificially forced this branch to execute, still unable to make it execute 
at runtime)
F11034446: image.png 

I replaced my 0x22 example with 0x2, which is unmapped *and* outside 
the low 32 bit range. `GetLoadAddressPermissions` still seems to succeed.

F11034470: image.png 
F11034475: image.png 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

In D71372#1782536 , @clayborg wrote:

> I believe it is ok for permissions to succeed as long as they don't return 
> invalid permissions. For any address outside any mapped ranges, we should 
> have zero as the permissions. Looking up address mappings for zero will return
>
> [0x - 0x1) ---
>
> no permisssions are represented as "---". Then you can take the end address 
> and look it up, and continue. So as long as we aren't getting bogus values 
> back, we are good.


Where is that example output from? I don't see that kind of formatting in 
`image list` or from `vmmap`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg updated this revision to Diff 233708.
mossberg added a comment.

Add test.

I wasn't sure where to put it, so I left it in the `Unwind/` directory since 
the `call-asm.c` infrastructure is already there, and this fix is *kind of* 
vaguely related to unwinding. Please let me know if there's a better location.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372

Files:
  lldb/include/lldb/Target/ThreadPlanStepOut.h
  lldb/source/Target/ThreadPlanStepOut.cpp
  lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
  lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test

Index: lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test
===
--- /dev/null
+++ 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.
Index: lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
===
--- /dev/null
+++ 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
Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -126,6 +126,25 @@
 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(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(this),
+m_constructor_errors.GetData());
+  return;
+}
+
 Breakpoint *return_bp = m_thread.CalculateTarget()
 ->CreateBreakpoint(m_return_addr, true, false)
 .get();
@@ -238,8 +257,13 @@
   }
 
   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;
   }
 
Index: lldb/include/lldb/Target/ThreadPlanStepOut.h
===
--- lldb/include/lldb/Target/ThreadPlanStepOut.h
+++ lldb/include/lldb/Target/ThreadPlanStepOut.h
@@ -72,6 +72,7 @@
   std::vector 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,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

I noticed some other tests specify the OS's they apply to-- I wasn't sure if 
there were any restrictions I should put on this one. I only tested on MacOS, 
and I imagine it should work on Linux. Not sure about Windows.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg marked an inline comment as done.
mossberg added inline comments.



Comment at: lldb/source/Target/ThreadPlanStepOut.cpp:136-137
+  m_return_addr);
+  LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this),
+m_constructor_errors.GetData());
+  return;

labath wrote:
> I'd probably remove the log statements now that we have the real error output.
I included both based on @jingham's feedback here: 
https://reviews.llvm.org/D71372#1782270


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

I wanted to also mention that this patch won't address buggy behavior if, for 
example, the stub takes a function pointer on the stack (vs a normal data 
pointer). In this case, the executable check will succeed, and the breakpoint 
will be written, but to a potentially arbitrary code address. This may or may 
not lead to the debugger "randomly" breaking out later in a debug session if or 
when that code happens to be executed (I've reproduced this, but also seen some 
very confusing behavior where in some situations the breakpoint logging will 
say a breakpoint is written, but it doesn't seem like this was actually the 
case.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

I believe all the comments should be addressed at this point; thanks very much 
for your reviews so far.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

Took a quick look, and I think the good news is that the failure looks fairly 
superficial.

> clang-10: warning: argument unused during compilation: 
> '-fmodules-cache-path=/home/worker/lldb-x86_64-debian/lldb-x86_64-debian/build/lldb-test-build.noindex/module-cache-clang/lldb-shell'
>  [-Wunused-command-line-argument]
>  
> /home/worker/lldb-x86_64-debian/lldb-x86_64-debian/llvm-project/lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test:10:10:
>  error: CHECK: expected string not found in input
> 
> 1. CHECK: Breakpoint 1: where = {{.*}}`nonstandard_stub

The failing check was the one merely to see if the breakpoint was set, which I 
don't believe is related to the actual code changes that were made. Will 
continue to look. I also will also look at D71784 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71784: Fedora Linux fails `Unwind/thread-step-out-ret-addr-check.test`

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

Thanks for looking into this @jankratochvil!

> Another problem is that Fedora Linux has executable stack by default and all 
> programs indicate non-executable stack by PT_GNU_STACK, after fixing the 
> underscore I was getting:
> 
> (lldb) thread step-out
>  Process 22294 exited with status = 0 (0x)
>  (lldb) _
>  Is the .section harmless for non-Linux platforms or will it need some 
> conditional compilation? (#ifdef is not available in .s file)

Ok, I guess I was incorrectly assuming the stack should be non-executable by 
default on all platforms. In this case, let's rewrite the test to have a 
variable that is actually in the .data section (therefore guaranteed to be 
non-executable), and pass a pass a pointer to it to `nonstandard_stub`. The 
core requirement is that the pointer passed to `nonstandard_stub` must simply 
point to some kind of non-executable memory. I can do this.




Comment at: lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s:13
 # memory check.
+nonstandard_stub:
 _nonstandard_stub:

Maybe we can just remove the underscore version, and simply use 
`nonstandard_stub` if it will work everywhere?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71784/new/

https://reviews.llvm.org/D71784



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71789: [lldb] Refactor thread-step-out-ret-addr-check test to use .data instead of stack variable

[Mark Mossberg via Phabricator via lldb-commits]

mossberg created this revision.
mossberg added reviewers: jingham, jankratochvil, labath, clayborg, 
stella.stamenova.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Original diff: D71372 
Related diff: D71784 

D71784  shows that we cannot assume the stack 
is not executable across platforms. Refactor the test to use a variable from 
the .data section which should be guaranteed non-executable.

I would appreciate guidance regarding the `var@GOTPCREL(%rip)`. Directly 
referencing `var` would not assemble on my darwin machine due to an error about 
absolute addressing, and I used this to workaround it. I'm not sure if it's 
even portable to other platforms?

@jankratochvil Would you be able to test this on a Fedora machine, please?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71789

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


Index: lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
===
--- lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
+++ lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
@@ -1,14 +1,13 @@
 .text
 .globl  asm_main
 asm_main:
-sub $0x8, %rsp
-movq $0, (%rsp)
-push %rsp
+mov var@GOTPCREL(%rip), %rcx
+push %rcx
 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
+# breakpoint to that .data address, but should fail because of the executable
 # memory check.
 _nonstandard_stub:
 mov (%rsp), %rdi
@@ -18,3 +17,7 @@
 
 add $0x10, %rsp
 ret
+
+.data
+var:
+.long 0


Index: lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
===
--- lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
+++ lldb/test/Shell/Unwind/Inputs/thread-step-out-ret-addr-check.s
@@ -1,14 +1,13 @@
 .text
 .globl  asm_main
 asm_main:
-sub $0x8, %rsp
-movq $0, (%rsp)
-push %rsp
+mov var@GOTPCREL(%rip), %rcx
+push %rcx
 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
+# breakpoint to that .data address, but should fail because of the executable
 # memory check.
 _nonstandard_stub:
 mov (%rsp), %rdi
@@ -18,3 +17,7 @@
 
 add $0x10, %rsp
 ret
+
+.data
+var:
+.long 0
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71784: Fedora Linux fails `Unwind/thread-step-out-ret-addr-check.test`

[Mark Mossberg via Phabricator via lldb-commits]

mossberg added a comment.

The Windows failure (https://reviews.llvm.org/D71372#1793371) seems like it may 
also be caused by the underscore symbol issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71784/new/

https://reviews.llvm.org/D71784



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71789: [lldb] Refactor thread-step-out-ret-addr-check test to use .data instead of stack variable

[Mark Mossberg via Phabricator via lldb-commits]

mossberg abandoned this revision.
mossberg added a comment.

Thanks all for testing this! I had no idea it was this hard to get a 
non-executable section :)

Superseded by D71784 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71789/new/

https://reviews.llvm.org/D71789



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits