Linux: use readlink() and prctl() in SetProcTitle() to fix "exe" showing in process listings.

Linux: use readlink() and prctl() in SetProcTitle() to fix "exe" showing in process listings.
BUG=29118
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35441

[Mike Mammarella, 2009-12-11 22:42:59]

It was not immediately obvious to me where to put a call to SetProcTitle() so
that the plugin process would invoke this new code, but that would also be
required to close bug 29118.

[Evan Martin, 2009-12-11 23:46:22]

http://codereview.chromium.org/490028/diff/1/2
File base/command_line.cc (right):

http://codereview.chromium.org/490028/diff/1/2#newcode223
base/command_line.cc:223: title = buffer;
title.assign(buffer, r);
for the above two lines, I think

[satorux1, 2009-12-13 04:16:06]

Coming from http://crbug.com/22703.

Just FYI.

top command on my Linux box (ubuntu 9.10) does not show what argv[0] says.
Instead, It seems to show what /proc/PID/status says in the Name: field. Hence,
the process would likely be still shown as "exe" in top, with this change. I
haven't tried but it may be possible to set the process name by
prctl(PR_SET_NAME).


% ps -ww $(pgrep exe)
  PID TTY      STAT   TIME COMMAND
 3421 ?        Sl   230:17 /proc/self/exe --type=plugin
--plugin-path=/usr/lib/flashplugin-installer/libflashplayer.so --lang=en-US
--plugin-data-dir=/home/satorux/.config/google-chrome/Default
--channel=3351.b03d35b0.1700324300

% grep Name: /proc/3421/status
Name:   exe

% cat /proc/3421/cmdline
/proc/self/exe^@--type=plugin^@--plugin-path=/usr/lib/flashplugin-installer/l...

% top
...
  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
 3421 satorux   20   0  529m 230m  29m S    6  1.9 229:36.87 exe

[satorux1, 2009-12-13 05:00:36]

FWIW, prctl() worked for me. We could do something like prctl(PR_SET_NAME,
basename(resolved_path)) to set the process name, if we want. One caveat is that
PR_SET_NAME is a kernel 2.6.9 feature, so it won't work and compile with older
kernels. We would need to guard it with #if defined(PR_SET_NAME). I guess it's
not worth the complexity. :)

% cat test.c
#include <stdio.h>
#include <sys/prctl.h>

int main(int argc, char **argv) {
  prctl(PR_SET_NAME, "foobar");
  int i = 0;
  for (i = 0;; ++i);
  return 0;
}

% gcc test.c
% ./a.out &
[2] 11494

% grep Name: /proc/11494/status
Name:   foobar

% top
...
  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
11494 satorux   25   5  1648  312  240 R   98  0.0   0:22.58 foobar
 

On 2009/12/13 04:16:06, satorux1 wrote:
> Coming from http://crbug.com/22703.
> 
> Just FYI.
> 
> top command on my Linux box (ubuntu 9.10) does not show what argv[0] says.
> Instead, It seems to show what /proc/PID/status says in the Name: field.
Hence,
> the process would likely be still shown as "exe" in top, with this change. I
> haven't tried but it may be possible to set the process name by
> prctl(PR_SET_NAME).
> 
> 
> % ps -ww $(pgrep exe)
>   PID TTY      STAT   TIME COMMAND
>  3421 ?        Sl   230:17 /proc/self/exe --type=plugin
> --plugin-path=/usr/lib/flashplugin-installer/libflashplayer.so --lang=en-US
> --plugin-data-dir=/home/satorux/.config/google-chrome/Default
> --channel=3351.b03d35b0.1700324300
> 
> % grep Name: /proc/3421/status
> Name:   exe
> 
> % cat /proc/3421/cmdline
>
/proc/self/exe^@--type=plugin^@--plugin-path=/usr/lib/flashplugin-installer/l...
> 
> % top
> ...
>   PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
>  3421 satorux   20   0  529m 230m  29m S    6  1.9 229:36.87 exe

[Mike Mammarella, 2009-12-14 04:42:13]

On 2009/12/13 05:00:36, satorux1 wrote:
> FWIW, prctl() worked for me. We could do something like prctl(PR_SET_NAME,
> basename(resolved_path)) to set the process name, if we want. One caveat is
that
> PR_SET_NAME is a kernel 2.6.9 feature, so it won't work and compile with older
> kernels. We would need to guard it with #if defined(PR_SET_NAME). I guess it's
> not worth the complexity. :)
> 
> On 2009/12/13 04:16:06, satorux1 wrote:
> > Coming from http://crbug.com/22703.
> > 
> > Just FYI.
> > 
> > top command on my Linux box (ubuntu 9.10) does not show what argv[0] says.
> > Instead, It seems to show what /proc/PID/status says in the Name: field.
> Hence,
> > the process would likely be still shown as "exe" in top, with this change. I
> > haven't tried but it may be possible to set the process name by
> > prctl(PR_SET_NAME).

Yeah, by default top and many other utilities show only the name, not argv[0].
But if you press 'c' in top, or specify common arguments to ps like auxw, or
configure GUI tools to show the command line, then you see argv[0]. (Also this
change is missing a necessary call to SetProcTitle() that I don't currently know
where to put.)

I wasn't aware of PR_SET_NAME, that's spiffy. It's unfortunate that it's so new
that we can't count on its presence. On the other hand we could conditionally
compile it, to support compiling on older systems, and ignore errors like
EINVAL/ENOSYS at runtime that indicate the current kernel doesn't support it
even if it was compiled in. Thoughts on something like that?

[satorux1, 2009-12-14 05:20:08]

On 2009/12/14 04:42:13, Mike Mammarella wrote:
> On 2009/12/13 05:00:36, satorux1 wrote:
> > FWIW, prctl() worked for me. We could do something like prctl(PR_SET_NAME,
> > basename(resolved_path)) to set the process name, if we want. One caveat is
> that
> > PR_SET_NAME is a kernel 2.6.9 feature, so it won't work and compile with
older
> > kernels. We would need to guard it with #if defined(PR_SET_NAME). I guess
it's
> > not worth the complexity. :)
> > 
> > On 2009/12/13 04:16:06, satorux1 wrote:
> > > Coming from http://crbug.com/22703.
> > > 
> > > Just FYI.
> > > 
> > > top command on my Linux box (ubuntu 9.10) does not show what argv[0] says.
> > > Instead, It seems to show what /proc/PID/status says in the Name: field.
> > Hence,
> > > the process would likely be still shown as "exe" in top, with this change.
I
> > > haven't tried but it may be possible to set the process name by
> > > prctl(PR_SET_NAME).
> 
> Yeah, by default top and many other utilities show only the name, not argv[0].
> But if you press 'c' in top, or specify common arguments to ps like auxw, or
> configure GUI tools to show the command line, then you see argv[0]. (Also this
> change is missing a necessary call to SetProcTitle() that I don't currently
know
> where to put.)
> 
> I wasn't aware of PR_SET_NAME, that's spiffy. It's unfortunate that it's so
new
> that we can't count on its presence. On the other hand we could conditionally
> compile it, to support compiling on older systems, and ignore errors like
> EINVAL/ENOSYS at runtime that indicate the current kernel doesn't support it
> even if it was compiled in. Thoughts on something like that?

Sounds good to me. I would appreciate it if you could add some code for prctl(),
but I guess it's not a must-have, if it's adding too much complexity to the
code.

basename() seems to be a tricky function per man 3, so we should probably use
FileUtil::BaseName(), to get the binary name.

[satorux1, 2010-01-02 01:47:27]

LGTM, but please wait for lgtm from Evan just in case.

Minor comments below:

http://codereview.chromium.org/490028/diff/8001/8002
File base/command_line.cc (right):

http://codereview.chromium.org/490028/diff/8001/8002#newcode224
base/command_line.cc:224: if (r > 0 && r < static_cast<ssize_t>(sizeof(buffer)))
{
I think you can get rid of

  r < static_cast<ssize_t>(sizeof(buffer)

or make it

  r <= static_cast<ssize_t>(sizeof(buffer)

as you no longer adds '\0' at the end to null-terminate the string in buffer.

http://codereview.chromium.org/490028/diff/8001/8002#newcode228
base/command_line.cc:228: r -= 10;
Not a big deal, but it may be easier to copy it to title first then chop it,
like  

const std::string kDeletedSuffix = " (deleted)";
if (EndsWith(title, kDeletedSuffix)) {
  title.resize(title.size() - kDeletedSuffix.size());
}

http://codereview.chromium.org/490028/diff/8001/8002#newcode245
base/command_line.cc:245: setproctitle(have_argv0 ? "-%s" : "%s",
title.c_str());
I was wondering why prefixing '-' here. Would be nice to add a one-line comment
about why.

Looking at the code in setproctitle_linux.cc, it seems to me that setproctitle()
uses the original argv[0], preserved at the initialization time, by default, and
adding the '-' prefix disables the default behavior. Would be also nice to
document the behavior in the header file setproctitle_linux.h.

[Evan Martin, 2010-01-03 01:52:32]

LGTM with some suggestions, also adding on Ambrose since he had some thoughts on
this prctl business (but don't wait for him to respond)

http://codereview.chromium.org/490028/diff/11001/6006
File base/command_line.cc (right):

http://codereview.chromium.org/490028/diff/11001/6006#newcode10
base/command_line.cc:10: #elif defined(OS_LINUX) || defined(OS_FREEBSD)
seems harmless to use this on osx, so why not just OS_POSIX

http://codereview.chromium.org/490028/diff/11001/6006#newcode217
base/command_line.cc:217: // feature, but this makes us show up as "exe" in
process listings. Read
not a security feature, it's so updates don't swap ourselves out from under us. 
Since we also exec ourselves for renderers (though through a different
mechanism) I would just drop the "as a security feature".

http://codereview.chromium.org/490028/diff/11001/6006#newcode223
base/command_line.cc:223: ssize_t r = readlink("/proc/self/exe", buffer,
sizeof(buffer));
google style frowns on one-letter variable names.  why not "length" ?