Added error-checking against null process name in sanitize().

[nikkitan]

Added error-reaction in WriteCoreDumpInternal() according to null process name.

[nikkitan]

@josalem But for example in sanitize(), if we save the check for NULL by feeding it "(null)", sanitize() will still parse thru "(null)" redundantly, though it will be just 6 loops each time ProcDump dumps the file.

[josalem]

But for example in sanitize(), if we save the check for NULL by feeding it "(null)", sanitize() will still parse thru "(null)" redundantly, though it will be just 6 loops each time ProcDump dumps the file.

For good code quality sanitize should still check for a null input, but it should be a catastrophic error case. I'm not concerned with sanitize "double checking" the input as its job is to sanitize input and not check that it is valid. This does make me think of something though. I'm not sure there is a limit to the length of cmdline, so we may want to put a cap in the number of characters read from /proc/<PID>/cmdline, but that can be a different PR.

[jahabibi]

@josalem we already limit the max number of characters read from /proc/<PID>/cmdline. See Line 402 where MAX_CMDLINE_LEN is passed to fread.

A simple solution to this would be when procdump fails to read from /proc/<PID>/cmdline to extract the process name we substitute a default value such as (null) or NO_CMDLINE. This will allow procdump to continue monitoring if the process does in fact exist but doesn't have a cmdline attribute in procfs.

[nikkitan]

Speaking about the cmdline file, it was my first doubt as well, but it is pretty standard under the /proc filesystem. Also I had experimented by manually removing it after my redis-server is launched, and it was automatically populated again within a split second. So now I do tend to think that there might be some steps in the issue creator's launching his redis-server that somehow removed or bypassed the cmdline file and is out of my imagination.

[nikkitan]

@josalem I don't see the misaligned indentation at all in vscode.

[nikkitan]

Taking a look at the kernel code that actually runs when you read /proc//cmdline I found that just as the man page for procfs says, a non null-terminated string is a "magical" case. You must overwrite argv[0] to rewrite your process name for this pseudo-file. If you write past the original size of argv[0] you will squash the data that comes after it and extend into the data that stores the environment. I know some programs do this to make ps print a pretty title for their program. https://github.com/torvalds/linux/blob/9e8312f5e160ade069e131d54ab8652cf0e86e1a/fs/proc/base.c#L253-L338 I think we should treat that situation as an error for now. If we determine there is enough need to follow the breadcrumbs required for parsing out the custom name, then we can add that functionality in another PR. Please see my comments on where I think we should error.

@josalem I appreciate your determination on this. But first of all I have to point out is that the original comment for the magic case states argv[], but not argv[0]. What it says with the argv[] actually matches the description of /proc/pid/cmdline that you provided before. On top of this: The magic case is apparently a foreseen magic and is expected to be handled in this way: https://github.com/torvalds/linux/blob/9e8312f5e160ade069e131d54ab8652cf0e86e1a/fs/proc/base.c#L213

[jahabibi]

@nikkitan the reason why you don't see it in VSCode is that it is interpreting the indentation and formatting the code for you. Please fix the configuration of your editor to resolve this issue.