Adding Parent extended property to System.Diagnostics.Process by powercode · Pull Request #2850 · PowerShell/PowerShell

powercode · Dec 7, 2016

Fixes #2763
System.Management.Automation:
Adding ProcessCodeMethods.cs to do the pinvokes and the mandatory timestamp check
for parent processes.
Adding 'Parent' CodeProperty to Process in Types_Ps1Xml.generates.cs
For unix, /proc/<pid>/stat is parsed.
Moving addition of process PSProperties to SMA ClrFacade.

lzybkr · Dec 7, 2016

Why native code? Isn't reading /proc/id/stat simple in C#?

But of course! Got pointed to getppid and started out on that path. Fixed.

iSazonov · Dec 7, 2016

I think that this property will rarely be used. May be better to expose the method GetParentProcessId rather than the property?

lzybkr

The code changes look good. We should have a test.

iSazonov · Jan 27, 2017

Please add Newline.

powercode · Jan 27, 2017

So what is reasonable tests for this?
Starting another version of powershell, checking that Parent.ID -eq $pid and then kill the child?
That processes in general has parents? Some doesn't, and for some we don't have access.
PowerShell seems like the only process we can know will exist on the system under test.

iSazonov · Jan 27, 2017

I would start with:

powershell.exe -Command { [???]::GetParentProcess($pid) } | Should Be $pid

lzybkr · Jan 27, 2017

Look for other tests that run PowerShell.exe to see how we run cross platform using $pshome.

lzybkr · Feb 27, 2017

@powercode - can you either give us permission to push to your branch, or add a test like:

Describe "Process Parent property" -Tags "CI" {
    It "Parent process property" {
        $powershellexe = (get-process -id $PID).mainmodule.filename
        & $powershellexe -noprofile -command '(Get-Process -Id $pid).Parent.Id' | Should Be $pid
    }
}

If you add the test yourself, please rebase on master as we've fixed an issue that was causing many hangs in CI and we'll want that to test your change in CI.

powercode · Feb 27, 2017

Sorry for the delay. On sabbatical. Added the test as suggested.

lzybkr · Feb 27, 2017

Thanks for adding a test - the test fails on Mac. If you don't have a Mac to investigate, maybe we can find someone to help track down why it works on Linux but not Mac.

iSazonov · Feb 27, 2017

Maybe security http://stackoverflow.com/questions/40218676/get-parent-process-information-at-runtime-on-ios-application

powercode · Feb 27, 2017

@iSazonov No, I parse /proc/$pid. But maybe that info reflects the same information. Anyone has access to a mac to see how the output of /proc/$pid looks?

iSazonov · Feb 27, 2017

@vors Can you help with Mac?

powercode · Feb 27, 2017

http://superuser.com/questions/631693/where-is-the-proc-folder-on-mac-os-x#631696

OS X (which is based on BSD) does not implement a procfs.

powercode · Feb 27, 2017

Is this a viable alternative?

ps -o ppid --no-headers --pid $pid

iSazonov · Feb 27, 2017

If Powershell is in sandbox we get empty as with sysctl.

I suggest don't block the PR, pending the test for Mac and open Issue to fix this problem.

powercode · Mar 1, 2017

How about something like this in libpsl-native

pid_t GetPPid(pid_t pid)
{
    const pid_t PIDUnknown = UINT_MAX;
    struct kinfo_proc info;
    size_t length = sizeof(struct kinfo_proc);
    int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PID, pid};
    if (sysctl(mib, 4, &info, &length, NULL, 0) < 0)
        return PIDUnknown;
    if (length == 0)
        return PIDUnknown;
    return info.kp_eproc.e_ppid;
}

And maybe switch the Linux version to using that implementation too?

lzybkr · Mar 1, 2017

1 implementation that works on Mac/Linux is ideal, so if it works, go for it.

powercode · Mar 2, 2017

I moved some code around (to Platform.Unix).
All sysctl's aren't implemented across FreeBSD/Linux and the kinfo_proc seems to be one of them. So we have a sysctl for OSX and parsing of procfs for Linux.

iSazonov · Mar 2, 2017

Please add new line.

iSazonov · Mar 2, 2017

Please add new line.

lzybkr · Mar 6, 2017

@andschwa - it'd be great if you can review the native Linux/Mac code here.

andyleejordan · Mar 6, 2017

+                    {
+                        return invalidPid;
+                    }
+                    return Int32.Parse(parts[3]);


Tested this out, looks good to me.

andyleejordan · Mar 6, 2017

+
+#else
+
+    return UINT_MAX;


Perhaps we should simply not compile this GetPPid function at all when not on Mac (i.e., move the #if defined ... #endif to surround the function definition and add one around the declaration). I'm honestly not sure which would be more maintainable: a "usable" (but not implemented) function on a platform always returning an invalid PID, or a runtime error from PowerShell not finding the symbol on attempted use because the function was never compiled; @lzybkr what do you think?

An invalid PID is probably more user friendly than an obscure error

This would be a developer problem: if a Linux dev accidentally tries to use GetPPid, as is, they function will be called correctly, but only ever return UINT_MAX, which to me seems like it may be more confusing than an exception stating it couldn't call the function at all (since there are several return paths for UINT_MAX). Either way I think they'll end up looking at the native source pretty quickly, and should be able to figure out their problem (to use the /proc version instead). But I don't think a user is going to run into this.

I think an invalid pid is less confusing than an obscure error, but I can see your point of view too. The bottom line is - it shouldn't matter - if either leaks, we messed up somehow.

andyleejordan · Mar 6, 2017

+    if (length == 0)
+        return PIDUnknown;
+
+    return info.kp_eproc.e_ppid;


msftclas added the cla-already-signed label Dec 7, 2016

powercode force-pushed the process-parentpid branch from 6de7d1a to 32d657a Compare December 7, 2016 00:51

PowerShellTeam added the Review - Needed The PR is being reviewed label Dec 7, 2016

powercode force-pushed the process-parentpid branch from 32d657a to 446dde7 Compare December 7, 2016 01:27

lzybkr reviewed Dec 7, 2016

View reviewed changes

powercode force-pushed the process-parentpid branch 3 times, most recently from 7d5bb02 to 9fc9904 Compare December 7, 2016 08:10

lzybkr self-assigned this Dec 9, 2016

powercode force-pushed the process-parentpid branch from 9fc9904 to 81cc8f9 Compare December 15, 2016 10:32

powercode force-pushed the process-parentpid branch 2 times, most recently from ba2ec10 to 13be807 Compare December 31, 2016 11:32

lzybkr suggested changes Jan 26, 2017

View reviewed changes

iSazonov reviewed Jan 27, 2017

View reviewed changes

Comment thread src/System.Management.Automation/engine/ProcessCodeMethods.cs Outdated

iSazonov Jan 27, 2017

Copy link
Copy Markdown

Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add Newline.

lzybkr added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Feb 15, 2017

PowerShellTeam added Stale and removed Review - Waiting on Author labels Feb 16, 2017

powercode force-pushed the process-parentpid branch from 13be807 to f0bfc19 Compare February 27, 2017 17:08

lzybkr removed the Stale label Feb 27, 2017

powercode force-pushed the process-parentpid branch from f0bfc19 to 1c39358 Compare February 27, 2017 18:23

powercode force-pushed the process-parentpid branch 3 times, most recently from 65b9306 to 42b5d25 Compare March 2, 2017 01:57

Staffan Gustafsson added 2 commits March 1, 2017 21:03

Adding Parent extended property to System.Diagnostics.Process

e3564c7

Adding test for process parent.

fea2f00

powercode force-pushed the process-parentpid branch 4 times, most recently from 2558824 to a50c69b Compare March 2, 2017 03:01

iSazonov reviewed Mar 2, 2017

View reviewed changes

Changing implementation for unix to use sysctl call.

17441c7

powercode force-pushed the process-parentpid branch from a50c69b to 17441c7 Compare March 2, 2017 16:03

lzybkr added 2 commits March 2, 2017 08:53

replace tabs with spaces

e51230f

add newline at eof

ee1cecb

lzybkr approved these changes Mar 2, 2017

View reviewed changes

lzybkr requested a review from andyleejordan March 2, 2017 17:09

andyleejordan approved these changes Mar 6, 2017

View reviewed changes

lzybkr merged commit b404987 into PowerShell:master Mar 6, 2017

powercode deleted the process-parentpid branch April 25, 2017 18:59

joandrsn mentioned this pull request Sep 29, 2017

Added BinPath, Description, UserName and Delayed Auto Start to Get-Service #4907

Merged

Search code, repositories, users, issues, pull requests...

Conversation

powercode commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Dec 7, 2016

Uh oh!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode commented Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jan 27, 2017

Uh oh!

lzybkr commented Jan 27, 2017

Uh oh!

lzybkr commented Feb 27, 2017

Uh oh!

powercode commented Feb 27, 2017

Uh oh!

lzybkr commented Feb 27, 2017

Uh oh!

iSazonov commented Feb 27, 2017

Uh oh!

powercode commented Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Feb 27, 2017

Uh oh!

powercode commented Feb 27, 2017

Uh oh!

powercode commented Feb 27, 2017

Uh oh!

iSazonov commented Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

powercode commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lzybkr commented Mar 1, 2017

Uh oh!

powercode commented Mar 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzybkr commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyleejordan Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

powercode commented Dec 7, 2016 •

edited

Loading

powercode commented Jan 27, 2017 •

edited

Loading

powercode commented Feb 27, 2017 •

edited

Loading

iSazonov commented Feb 27, 2017 •

edited

Loading

powercode commented Mar 1, 2017 •

edited

Loading

andyleejordan Mar 6, 2017 •

edited

Loading