Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process.GetProcesses returns truncated 15 char string on Linux #34437

Closed
2E0PGS opened this issue Jan 8, 2019 · 31 comments

Comments

@2E0PGS
Copy link
Contributor

commented Jan 8, 2019

When using Process.GetProcesses() in dotnet core it returns the Process.ProcessName property fine under Windows as expected.

However when ran under Linux (Tested on Ubuntu 17.04) it seems to return process names as 15 char long strings. It truncates the process names to all be 15 char long strings.

My applogies if there is an issue for this somewhere under another repo. I found there was such an issue with the mono framework when doing research prior to submitting this issue: https://bugzilla.xamarin.com/show_bug.cgi?id=32539

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

Thanks for the pointer to the MacOS bug fix. It seems Linux has a similar limit. Here is a process with a 30 character name, only 15 show:

dan@danmose3:~/git/corefx/src/Native$ cat /proc/10725/stat
10725 (123456789012345) S 10116 10725 10115 1025 0 0 0 0 0 0 0 0 0 0 20 0 1 0 17132735 97915326464 204 18446744073709551615 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0

This is because /proc/<PID>/stat gets the value from task_struct which is TASK_COMM_LEN chars (sched.h) which is 16 including null.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

It looks like /proc/<PID>/exe is one way to get the full file name

dan@danmose3:~/git/corefx/src/Native$ ls -l /proc/10725/exe
lrwxrwxrwx 1 dan dan 0 Jan  8 14:38 /proc/10725/exe -> /home/dan/123456789012345678901234567890
@danmosemsft

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

It looks like we already get the full exe path from this file. We just need to use it a few lines higher up (cropping off the path) instead of the stat entry

ModuleName = Path.GetFileName(entry.FileName),
BaseAddress = new IntPtr(unchecked((void*)entry.AddressRange.Key)),
ModuleMemorySize = sizeOfImage,
EntryPointAddress = IntPtr.Zero // unknown
});
}
}
// Move the main executable module to be the first in the list if it's not already
string exePath = Process.GetExePath(processId);

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

@2E0PGS any interest in making this change, with a unit test?

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

Thanks for pulling out those lines and doing a few basic tests.

I will have a further look at the code base and check what unit tests are in place at the moment.

I will try give it a shot if I get some free time. Otherwise I will let you know if I am too busy to make the change.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

OK, thanks. As for Mac it seems we do not use /proc/../stat but it would be nice to have a test (need not be you of course)

@danmosemsft danmosemsft added this to To Do in Tracking for next Preview via automation Jan 30, 2019

@wtgodbe wtgodbe added this to the Future milestone Jan 31, 2019

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 2, 2019

@2E0PGS do you think you might give this a go? Let me know if you need pointers getting set up?

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

Sorry been very busy. Yes I was just reading the dotnet core wiki for setting up the development environment. Hopefully I can give it a go soon.

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

I don't quite know know why the solution doesn't pickup the distro specific cs files.

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

Got a build done, just need to see how I am going to run this inplace of regular dotnet core on the system to check it against my test program.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 2, 2019

Have you written a unit test? Take a look at existing tests under Systrm.Diagnostics.Process/tests. There are some that launch processes. You could follow the pattern but rename the file to a long name before launching. Or possibly copy "sleep" to a long name version then run it with a large value, read the name, then terminate it and delete it. This assumes all distros have sleep.

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

I got this so far: 2E0PGS@691e55e

Just need to test this on Linux.

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

OK so implementing those changes: 2E0PGS@e1e308c

Using my test program the output we had before any of my changes looked like this and it had a name for every process.

Process name: systemd
Process name: kthreadd
Process name: kworker/0:0
Process name: kworker/0:0H

Using the recent commit I linked above the test program has quite a few processes without names.

Process name: systemd
Process name: 
Process name: 
Process name: mate-session
Process name: dbus-daemon
@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

However the names are now longer than 15 chars. We need to investigate why some are blank.

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

It appears Process.GetExePath(pid) doesn't always return a string.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

If the exe has already terminated, it seems that /proc/nnn can exist, but /proc/nnn/exe won't exist: https://linux.die.net/man/5/proc

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

In such a case you could stick with procFsStat.comm as the best we can do.

The other place where GetExePath happens

            // Move the main executable module to be the first in the list if it's not already
            string exePath = Process.GetExePath(processId);

..would be blank, but in simple experiments, it seems like when /proc/nnn/exe is missing, so is /proc/nnn/maps, so this line would not be reached.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

In a really quick look, it seems like mono gets the path from the first line of /proc/nnn/maps, so it doesn't have a better way either.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

As for a test to copy, ProcessStart_DirectoryNameInCurDirectorySameAsFileNameInExecDirectory_Success is not far off. I suggest you use sleep as the test program, suitably renamed to a long name. You can use the logic in IsProgramInstalled to find it (perhaps extract a GetProgramPath). On some distros it doens't exist by default: I would skip the test in that case, unless you thnk of some other scheme. The test should run for macOS as well as Linux.

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

Are you suggesting we try Process.GetExePath(processId); and if that's null or blank fall back to procFsStat.comm? Sorry if I didn't understand what you are suggesting correctly.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Are you suggesting we try Process.GetExePath(processId); and if that's null or blank fall back to procFsStat.comm

Was out of office -- @2E0PGS yes, I think so. Try that, plus a test?

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

I will give that a try soon thanks.

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

Just building a new corefx now to test it against a CLI app.

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

Ok this commit works well: 2E0PGS@56e7385

Test CLI program output:

Process name: indicator-application-service
Process name: indicator-session-service
Process name: indicator-messages-service
Process name: indicator-sound-service
Process name: indicator-power-service
Process name: mate-terminal
Process name: bash
Process name: obexd
Process name: bash
Process name: htop

No missing process names now.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Great, do you want to create a test and put up a PR?

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Thanks @2E0PGS

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@2E0PGS hello, are you still working on this one? I think only test remains? You could throw up a PR just for work in progress, that others could comment on.

@2E0PGS

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@2E0PGS totally fine. It's just us keeping the pipes flowing rather than any particular urgency for this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants
You can’t perform that action at this time.