This is an archive of the discontinued LLVM Phabricator instance.

Add test for denied process attach by pid and fix found bugs in Process/ProcessPOSIX.cpp
ClosedPublic

Authored by ovyalov on Nov 12 2014, 4:04 PM.

Details

Reviewers
emaste
clayborg
Summary

New test test/functionalities/process_attach/attach_denied covers the case when attach is denied by potential inferior.
This test uncovered next issues that I fixed along as well:

  • lldbtest calls SBProcess::Kill for every inferior within test's tearDown. If attach fails ProcessPOSIX::DoDestroy calls m_monitor->Kill() which sends SIGKILL to pid 0, i.e. killing all processes in the current process group. Added check for invalid pid in ProcessPOSIX::DoDestroy
  • Modified ProcessPOSIX::DoDestroy made test to fail since Kill returns an error. Changed Target/Process.cpp in order to transition process to exited state in case of attach failure - so, DoDestroy exits properly since HasExited() returns true.

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 16124.Nov 12 2014, 4:04 PM
ovyalov retitled this revision from to Add test for denied process attach by pid and fix found bugs in Process/ProcessPOSIX.cpp.
ovyalov updated this object.
ovyalov edited the test plan for this revision. (Show Details)
ovyalov added a reviewer: clayborg.
ovyalov added a subscriber: Unknown Object (MLST).
ovyalov updated this revision to Diff 16126.Nov 12 2014, 4:58 PM

Removed unused imports from TestAttachDenied.py.

Hi Ed,

since you monitor POSIX code changes let me add you as reviewer.

ovyalov updated this revision to Diff 16301.Nov 17 2014, 12:21 PM
  • Added synchronization between test and inferior via named pipe.
  • Restricted test run conditions - either Darwin or Linux.

Please take a look.

emaste edited edge metadata.Nov 17 2014, 12:38 PM

For future would you provide full context, i.e. diff -U999999 if manually uploading diffs?

This looks OK to me, but as I think we lack an appropriate interface in FreeBSD right now I can't test, so hope one of the Linux or Darwin developers can comment.

ovyalov updated this revision to Diff 16302.Nov 17 2014, 1:08 PM
ovyalov edited edge metadata.

Thanks for suggestion - re-did patch with full context.

Yes, I tried to run the test on FreeBSD but prctl isn't available - so, for now BSD will be skipped.

Some of my FreeBSD colleagues recommended trigger the failure by trying to attach to a process that already has a debugger attached, as that's the common cause of the failure we're handling here anyway.

Perhaps attach_denied/main.c can fork(), PT_ATTACH to the child, and then return the child's pid for LLDB to try to attach.

ovyalov updated this revision to Diff 16359.Nov 18 2014, 5:38 PM

I reworked test according to Ed's suggestion with fork to make the test to cover OSX, Linux and FreeBSD.
Along the way FreeBSD/ProcessMonitor bug was discovered which led LLDB to hang when Attach fails - fixed this.
Checked against OSX 10.9.5, Ubuntu 14.0.4 and FreeBSD 10 - no regressions found.

Please take another look.

ovyalov updated this revision to Diff 16362.Nov 18 2014, 5:41 PM

Removed unneeded file from patch.

emaste accepted this revision.Nov 18 2014, 6:58 PM
emaste edited edge metadata.

overall lgtm

source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
1143 ↗(On Diff #16362)

we probably ought to clean this up to just return; now - the goto is just extra obfuscation

test/functionalities/process_attach/attach_denied/main.cpp
60 ↗(On Diff #16362)

just 1 probably. -ve return values from main end up with implementation-specific results.

This revision is now accepted and ready to land.Nov 18 2014, 6:58 PM
ovyalov updated this revision to Diff 16365.Nov 18 2014, 10:02 PM
ovyalov edited edge metadata.

Fixed according to your suggestions.

Thank you for the review!

clayborg accepted this revision.Nov 20 2014, 12:23 PM
clayborg edited edge metadata.

lgtm