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

Mon Dec 16 02:30:31 PST 2019

labath added a comment.

In D71372#1785241 <https://reviews.llvm.org/D71372#1785241>, @mossberg wrote:

> 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. The presence of instructions that manipulate the stack pointer seemed to affect this also..)

That's perfectly fine. This is all best effort -- if someone really wants us to do something bogus here, he'll always find a way. And OTOH, if he really wants us to do the right thing for functions like these, he should write unwind info for them.

================
Comment at: lldb/source/Target/ThreadPlanStepOut.cpp:136-137
+                                  m_return_addr);
+      LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast<void *>(this),
+                m_constructor_errors.GetData());
+      return;
----------------
mossberg wrote:
> 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
Yeah, sorry, I missed that part. Jim is the thread plan master, so lets do what he thinks best.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71372