Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Linux: ProcessName: return script name instead of interpreter program #37294

Merged
wtgodbe merged 6 commits into from
May 16, 2019
Merged

Linux: ProcessName: return script name instead of interpreter program #37294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
af8ec61
Linux: ProcessName: return script name instead of interpreter program
tmds Apr 30, 2019
07835da
Add ProcessNameMatchesScriptName test
tmds Apr 30, 2019
c5077ca
ProcessNameMatchesScriptName: don't run on OSX
tmds Apr 30, 2019
19c8f2b
LongProcessNamesAreSupported: don't run on Alpine
tmds Apr 30, 2019
c83e15f
Merge branch 'master' into processname_script
tmds May 9, 2019
a34dfa1
Remove ActiveIssue attributes
tmds May 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ internal struct ParsedStat
// the MoveNext() with the appropriate ParseNext* call and assignment.

internal int pid;
// internal string comm;
internal string comm;
internal char state;
internal int ppid;
//internal int pgrp;
Expand Down Expand Up @@ -238,7 +238,7 @@ internal static bool TryParseStatFile(string statFilePath, out ParsedStat result
var results = default(ParsedStat);

results.pid = parser.ParseNextInt32();
parser.MoveAndExtractNextInOuterParens(extractValue: false); // comm
results.comm = parser.MoveAndExtractNextInOuterParens();
results.state = parser.ParseNextChar();
results.ppid = parser.ParseNextInt32();
parser.MoveNextOrFail(); // pgrp
Expand Down
4 changes: 2 additions & 2 deletions src/Common/src/System/IO/StringParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public string MoveAndExtractNext()
/// in the string. The extracted value will be everything between (not including) those parentheses.
/// </summary>
/// <returns></returns>
public string MoveAndExtractNextInOuterParens(bool extractValue = true)
public string MoveAndExtractNextInOuterParens()
{
// Move to the next position
MoveNextOrFail();
Expand All @@ -118,7 +118,7 @@ public string MoveAndExtractNextInOuterParens(bool extractValue = true)
}

// Extract the contents of the parens, then move our ending position to be after the paren
string result = extractValue ? _buffer.Substring(_startIndex + 1, lastParen - _startIndex - 1) : null;
string result = _buffer.Substring(_startIndex + 1, lastParen - _startIndex - 1);
_endIndex = lastParen + 1;

return result;
Expand Down
1 change: 1 addition & 0 deletions src/Common/tests/Tests/Interop/procfsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public static void ParseValidStatFiles_Success(
Assert.True(Interop.procfs.TryParseStatFile(path, out result, new ReusableTextReader()));

Assert.Equal(expectedPid, result.pid);
Assert.Equal(expectedComm, result.comm);
Assert.Equal(expectedState, result.state);
Assert.Equal(expectedSession, result.session);
Assert.Equal(expectedUtime, result.utime);
Expand Down
62 changes: 46 additions & 16 deletions src/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ public static Process[] GetProcessesByName(string processName, string machineNam
var processes = new List<Process>();
foreach (int pid in ProcessManager.EnumerateProcessIds())
{
if (string.Equals(processName, Process.GetProcessName(pid), StringComparison.OrdinalIgnoreCase))
Interop.procfs.ParsedStat parsedStat;
if (Interop.procfs.TryReadStatFile(pid, out parsedStat, reusableReader) &&
string.Equals(processName, Process.GetUntruncatedProcessName(ref parsedStat), StringComparison.OrdinalIgnoreCase))
{
ProcessInfo processInfo = ProcessManager.CreateProcessInfo(pid, reusableReader, processName);
ProcessInfo processInfo = ProcessManager.CreateProcessInfo(ref parsedStat, reusableReader, processName);
processes.Add(new Process(machineName, false, processInfo.ProcessId, processInfo));
}
}
Expand Down Expand Up @@ -256,12 +258,10 @@ internal static string GetExePath(int processId = -1)
}

/// <summary>Gets the name that was used to start the process, or null if it could not be retrieved.</summary>
/// <param name="processId">The pid for the target process, or -1 for the current process.</param>
internal static string GetProcessName(int processId = -1)
/// <param name="stat">The stat for the target process.</param>
internal static string GetUntruncatedProcessName(ref Interop.procfs.ParsedStat stat)
{
string cmdLineFilePath = processId == -1 ?
Interop.procfs.SelfCmdLineFilePath :
Interop.procfs.GetCmdLinePathForProcess(processId);
string cmdLineFilePath = Interop.procfs.GetCmdLinePathForProcess(stat.pid);

byte[] rentedArray = null;
try
Expand All @@ -282,7 +282,7 @@ internal static string GetProcessName(int processId = -1)
buffer.CopyTo(tmp);
byte[] toReturn = rentedArray;
buffer = rentedArray = tmp;
if (rentedArray != null)
if (toReturn != null)
{
ArrayPool<byte>.Shared.Return(toReturn);
}
Expand All @@ -293,26 +293,40 @@ internal static string GetProcessName(int processId = -1)
bytesRead += n;

// cmdline contains the argv array separated by '\0' bytes.
// we determine the process name using argv[0].
int argv0End = buffer.Slice(0, bytesRead).IndexOf((byte)'\0');
if (argv0End != -1)
// stat.comm contains a possibly truncated version of the process name.
// When the program is a native executable, the process name will be in argv[0].
// When the program is a script, argv[0] contains the interpreter, and argv[1] contains the script name.
Span<byte> argRemainder = buffer.Slice(0, bytesRead);
int argEnd = argRemainder.IndexOf((byte)'\0');
if (argEnd != -1)
{
// Strip directory names from argv[0].
int nameStart = buffer.Slice(0, argv0End).LastIndexOf((byte)'/') + 1;
// Check if argv[0] has the process name.
string name = GetUntruncatedNameFromArg(argRemainder.Slice(0, argEnd), prefix: stat.comm);
if (name != null)
{
return name;
}

return Encoding.UTF8.GetString(buffer.Slice(nameStart, argv0End - nameStart));
// Check if argv[1] has the process name.
argRemainder = argRemainder.Slice(argEnd + 1);
argEnd = argRemainder.IndexOf((byte)'\0');
if (argEnd != -1)
{
name = GetUntruncatedNameFromArg(argRemainder.Slice(0, argEnd), prefix: stat.comm);
return name ?? stat.comm;
}
}

if (n == 0)
{
return null;
return stat.comm;
}
}
}
}
catch (IOException)
{
return null;
return stat.comm;
}
finally
{
Expand All @@ -321,6 +335,22 @@ internal static string GetProcessName(int processId = -1)
ArrayPool<byte>.Shared.Return(rentedArray);
}
}

string GetUntruncatedNameFromArg(Span<byte> arg, string prefix)
{
// Strip directory names from arg.
int nameStart = arg.LastIndexOf((byte)'/') + 1;
string argString = Encoding.UTF8.GetString(arg.Slice(nameStart));

if (argString.StartsWith(prefix, StringComparison.OrdinalIgnoreCase))
{
return argString;
}
else
{
return null;
}
}
}

// ----------------------------------
Expand Down
8 changes: 4 additions & 4 deletions src/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Linux.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ internal static ProcessModuleCollection GetModules(int processId)
/// <summary>
/// Creates a ProcessInfo from the specified process ID.
/// </summary>
internal static ProcessInfo CreateProcessInfo(int pid, ReusableTextReader reusableReader = null, string processName = null)
internal static ProcessInfo CreateProcessInfo(int pid, ReusableTextReader reusableReader = null)
{
if (reusableReader == null)
{
Expand All @@ -117,21 +117,21 @@ internal static ProcessInfo CreateProcessInfo(int pid, ReusableTextReader reusab

Interop.procfs.ParsedStat stat;
return Interop.procfs.TryReadStatFile(pid, out stat, reusableReader) ?
CreateProcessInfo(stat, reusableReader, processName) :
CreateProcessInfo(ref stat, reusableReader) :
null;
}

/// <summary>
/// Creates a ProcessInfo from the data parsed from a /proc/pid/stat file and the associated tasks directory.
/// </summary>
internal static ProcessInfo CreateProcessInfo(Interop.procfs.ParsedStat procFsStat, ReusableTextReader reusableReader, string processName)
internal static ProcessInfo CreateProcessInfo(ref Interop.procfs.ParsedStat procFsStat, ReusableTextReader reusableReader, string processName = null)
{
int pid = procFsStat.pid;

var pi = new ProcessInfo()
{
ProcessId = pid,
ProcessName = processName ?? Process.GetProcessName(pid) ?? string.Empty,
ProcessName = processName ?? Process.GetUntruncatedProcessName(ref procFsStat) ?? string.Empty,
BasePriority = (int)procFsStat.nice,
VirtualBytes = (long)procFsStat.vsize,
WorkingSet = procFsStat.rss * Environment.SystemPageSize,
Expand Down
27 changes: 25 additions & 2 deletions src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ public void ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow()
}
}

[ActiveIssue(37198)]
[Theory, InlineData(true), InlineData(false)]
[OuterLoop("Opens program")]
public void ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled(bool isFolder)
Expand Down Expand Up @@ -157,6 +156,31 @@ public void ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled(bool
}
}

[Fact]
[PlatformSpecific(~TestPlatforms.OSX)] // On OSX, ProcessName returns the script interpreter.
public void ProcessNameMatchesScriptName()
{
string scriptName = GetTestFileName();
string filename = Path.Combine(TestDirectory, scriptName);
File.WriteAllText(filename, $"#!/bin/sh\nsleep 600\n"); // sleep 10 min.
// set x-bit
int mode = Convert.ToInt32("744", 8);
Assert.Equal(0, chmod(filename, mode));

using (var process = Process.Start(new ProcessStartInfo { FileName = filename }))
{
try
{
Assert.Equal(scriptName, process.ProcessName);
Copy link
Copy Markdown
Member

@krwq krwq Apr 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

should this actually return interpreter (whatever the shebang states) and script name in the arguments?

Copy link
Copy Markdown
Member Author

@tmds tmds Apr 30, 2019

Choose a reason for hiding this comment

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

It's debatable. I think the main use-case for this property is to help identify processes, then the script name is more useful. Also, when starting a process, scripts can be started the same way as native executables, so it may be unexpected something is actually a script and causes the interpreter to show up here.

Copy link
Copy Markdown
Member Author

@tmds tmds Apr 30, 2019

Choose a reason for hiding this comment

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

note: this has been returning the script name on Linux so far. I'm adding a test here because that regressed in #37144 (xdg-open is a script).

Copy link
Copy Markdown
Member

@krwq krwq Apr 30, 2019

Choose a reason for hiding this comment

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

Honestly I'd never expect this to return anything else than interpreter name - I think we should match what ps aux does (if it does match then I'm fine with this change), otherwise we're just asking developers to implement their own interop layer to do the right thing. I'd rather add something like ScriptName instead.

Copy link
Copy Markdown
Member Author

@tmds tmds May 1, 2019

Choose a reason for hiding this comment

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

ps aux returns the full command line, which includes both the interpreter and the script name.
gnome system monitor (something like Windows process explorer) has a 'Process Name' column which shows the script names.

The ProcessName is the property that helps a user identify a certain process. Having a bunch of bash and python processes is not that helpful (same for Process.GetProcessesByName).

Copy link
Copy Markdown
Member

@danmoseley danmoseley May 1, 2019

Choose a reason for hiding this comment

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

Is Windows a bad analog? It presumably returns cmd.exe.

Copy link
Copy Markdown
Member Author

@tmds tmds May 1, 2019

Choose a reason for hiding this comment

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

It presumably returns cmd.exe.

I assume so too.

Is Windows a bad analog?

Unix scripts are more closer to executables than Windows batch files. They are executables (+x bit set) and can be started directly from the Process class (no UseShellExecute required). For a user, there is no difference between a script 'executable' and a native executable. I think it will be unexpected and undesired that the interpreter shows up as the ProcessName.

Copy link
Copy Markdown
Member

@danmoseley danmoseley May 1, 2019

Choose a reason for hiding this comment

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

Unix scripts are more closer to executables than Windows batch files.

Fair enough. I don't feel strongly either way. @stephentoub thoughts?

Copy link
Copy Markdown
Member

@stephentoub stephentoub May 2, 2019

Choose a reason for hiding this comment

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

What do other languages / frameworks do here from their equivalent types, e.g. Java, node.js, Go, Rust, etc.?

Copy link
Copy Markdown
Member Author

@tmds tmds May 6, 2019

Choose a reason for hiding this comment

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

java's ProcessHandle provides the following:

  • arguments: array of strings (from cmdline)
  • command: string (from exe)
  • commandLine: string (from cmdline)

rust's sysinfo Process provides:

  • name: argv[0] (from cmdline)
  • cmd: string (from cmdline)

I don't think there is a standard way of getting this info in node.js or go. It is suggested to read stdout from ps ..., which includes cmdline.

imo we shouldn't change ProcessName unless additional properties are added to Process that make it possible to identify a certain script (and even then we may want to keep the current behavior).

}
finally
{
process.Kill();
process.WaitForExit();
}
}
}

[Fact]
[PlatformSpecific(TestPlatforms.Linux)] // s_allowedProgramsToRun is Linux specific
public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable()
Expand Down Expand Up @@ -294,7 +318,6 @@ public void ProcessStart_OpenFileOnLinux_UsesSpecifiedProgram(string programToOp
}
}

[ActiveIssue(37198)]
[Theory, InlineData("vi")]
[PlatformSpecific(TestPlatforms.Linux)]
[OuterLoop("Opens program")]
Expand Down
5 changes: 3 additions & 2 deletions src/System.Diagnostics.Process/tests/ProcessTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1892,15 +1892,16 @@ public void TestLongProcessIsWorking()
Assert.True(p.HasExited);
}

[ActiveIssue(37198)]
[PlatformSpecific(TestPlatforms.AnyUnix)]
[ActiveIssue(37054, TestPlatforms.OSX)]
[Fact]
public void LongProcessNamesAreSupported()
{
// Alpine implements sleep as a symlink to the busybox executable.
// If we rename it, the program will no longer sleep.
if (PlatformDetection.IsAlpine)
{
return; // https://github.com/dotnet/corefx/issues/37054
return;
}

string programPath = GetProgramPath("sleep");
Expand Down