Adding Parent extended property to System.Diagnostics.Process #2850

powercode · 2016-12-07T00:40:05Z

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 · 2016-12-07T02:40:17Z

src/libpsl-native/src/getparentprocessid.cpp

+/// Read /proc/pid/stat at fourth column to determine
+/// the parent process id
+///
+pid_t GetParentProcessId(pid_t pid)


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 · 2016-12-07T14:09:47Z

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 · 2017-01-27T04:48:50Z

src/System.Management.Automation/engine/ProcessCodeMethods.cs

+#endif
+
+        }
+}


Please add Newline.

powercode · 2017-01-27T13:58:55Z

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 · 2017-01-27T14:23:08Z

I would start with:

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

lzybkr · 2017-01-27T15:13:15Z

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

lzybkr · 2017-02-27T05:25:16Z

@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 · 2017-02-27T17:16:37Z

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

lzybkr · 2017-02-27T17:53:48Z

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 · 2017-02-27T18:15:54Z

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

powercode · 2017-02-27T18:26:35Z

@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 · 2017-02-27T18:28:02Z

@vors Can you help with Mac?

powercode · 2017-02-27T18:29:32Z

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 · 2017-02-27T19:08:52Z

Is this a viable alternative?

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

iSazonov · 2017-02-27T19:19:49Z

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 · 2017-03-01T03:43:42Z

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 · 2017-03-01T04:00:17Z

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

powercode · 2017-03-02T03:28:50Z

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 · 2017-03-02T03:37:54Z

src/libpsl-native/src/getppid.cpp

+#else
+    return UINT_MAX;
+#endif
+}


Please add new line.

iSazonov · 2017-03-02T03:38:04Z

src/libpsl-native/src/getppid.h

+
+pid_t GetPPid(pid_t pid);
+
+PAL_END_EXTERNC


Please add new line.

lzybkr · 2017-03-06T06:06:41Z

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

andyleejordan · 2017-03-06T18:55:00Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

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


Tested this out, looks good to me.

andyleejordan · 2017-03-06T19:01:41Z

src/libpsl-native/src/getppid.cpp

+
+#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 · 2017-03-06T19:03:55Z

src/libpsl-native/src/getppid.cpp

+    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

src/System.Management.Automation/engine/ProcessCodeMethods.cs

#endif

}

}

Copy link

Collaborator

iSazonov Jan 27, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Parent extended property to System.Diagnostics.Process #2850

Adding Parent extended property to System.Diagnostics.Process #2850

+              #endif
+                      }
+              }

Adding Parent extended property to System.Diagnostics.Process #2850

Adding Parent extended property to System.Diagnostics.Process #2850

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment