Page MenuHomePhabricator

mossberg (Mark Mossberg)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 24 2019, 5:12 PM (18 w, 1 d)

Recent Activity

Dec 21 2019

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

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

Dec 21 2019, 9:50 AM · Restricted Project

Dec 20 2019

mossberg added a comment to D71784: Fedora Linux fails `Unwind/thread-step-out-ret-addr-check.test`.

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

Dec 20 2019, 3:31 PM · Restricted Project
mossberg removed a reviewer for D71789: [lldb] Refactor thread-step-out-ret-addr-check test to use .data instead of stack variable: stella.stamenova.
Dec 20 2019, 3:18 PM · Restricted Project
mossberg created D71789: [lldb] Refactor thread-step-out-ret-addr-check test to use .data instead of stack variable.
Dec 20 2019, 3:11 PM · Restricted Project
mossberg added a comment to D71784: Fedora Linux fails `Unwind/thread-step-out-ret-addr-check.test`.

Thanks for looking into this @jankratochvil!

Dec 20 2019, 2:48 PM · Restricted Project
mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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

Dec 20 2019, 2:08 PM · Restricted Project

Dec 19 2019

mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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

Dec 19 2019, 1:42 PM · Restricted Project

Dec 15 2019

mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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.)

Dec 15 2019, 1:31 PM · Restricted Project

Dec 13 2019

mossberg added inline comments to D71372: [lldb] Add additional validation on return address in 'thread step-out'.
Dec 13 2019, 9:10 AM · Restricted Project

Dec 12 2019

mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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.

Dec 12 2019, 7:37 PM · Restricted Project
mossberg updated the summary of D71372: [lldb] Add additional validation on return address in 'thread step-out'.
Dec 12 2019, 4:40 PM · Restricted Project
mossberg updated the diff for D71372: [lldb] Add additional validation on return address in 'thread step-out'.

Add test.

Dec 12 2019, 4:40 PM · Restricted Project
mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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

[0x00000000 - 0x100000000) ---

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.

Dec 12 2019, 2:15 PM · Restricted Project
mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

Screenshots with new error messages.

Dec 12 2019, 1:39 PM · Restricted Project
mossberg updated the diff for D71372: [lldb] Add additional validation on return address in 'thread step-out'.

Output error messages to console in addition to logging.

Dec 12 2019, 1:33 PM · Restricted Project
mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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.

Dec 12 2019, 11:40 AM · Restricted Project
mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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.

Dec 12 2019, 10:51 AM · Restricted Project
mossberg updated the summary of D71372: [lldb] Add additional validation on return address in 'thread step-out'.
Dec 12 2019, 10:41 AM · Restricted Project

Dec 11 2019

mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

Current output.

Dec 11 2019, 4:37 PM · Restricted Project
mossberg updated the diff for D71372: [lldb] Add additional validation on return address in 'thread step-out'.

Output return address in logging

Dec 11 2019, 4:37 PM · Restricted Project
mossberg updated the diff for D71372: [lldb] Add additional validation on return address in 'thread step-out'.

Remove lingering Section.h include

Dec 11 2019, 4:03 PM · Restricted Project
mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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.

Dec 11 2019, 2:19 PM · Restricted Project
mossberg updated the summary of D71372: [lldb] Add additional validation on return address in 'thread step-out'.
Dec 11 2019, 2:10 PM · Restricted Project
mossberg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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

Dec 11 2019, 1:51 PM · Restricted Project
mossberg added inline comments to D71372: [lldb] Add additional validation on return address in 'thread step-out'.
Dec 11 2019, 1:42 PM · Restricted Project
mossberg updated the diff for D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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

Dec 11 2019, 1:42 PM · Restricted Project
mossberg created D71372: [lldb] Add additional validation on return address in 'thread step-out'.
Dec 11 2019, 11:38 AM · Restricted Project