New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 887875 link

Starred by 4 users

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Set linux compatible commandline for Chrome.

Project Member Reported by hidehiko@chromium.org, Sep 21

Issue description

Chrome currently sets command line by overwriting the given argv.

https://cs.chromium.org/chromium/src/services/service_manager/embedder/set_process_title_linux.cc

Specifically, the args are concat'ed by white space (0x20).

This is incompatible with linux:

http://man7.org/linux/man-pages/man5/proc.5.html
"The command-line arguments appear in this file as a set of strings separated by null bytes ('\0'), with a further null byte after the last string."

This confuses some tools, e.g. gopsutil.Process.CmdlineSlice() family. Nice to be fixed.

mdm@, please let us know if there is special requirements that needs to be concat'ed by whitespace instead of null byte.

cf) https://bugs.gentoo.org/477538
 
Cc: thestig@chromium.org phajdan.jr@chromium.org
Components: Internals>Core
Labels: OS-Chrome OS-Linux
Hey, so, I totally don't work on Chromium anymore, but I can provide a bit of help here. See the comment at the beginning of set_process_title_linux.cc - it explains how the arguments and environment are laid out in memory, and how the kernel treats that memory when it's reading it to expose data via /proc and other such places. In particular, because of the behavior explained in the paragraph on lines 21-27, we must use a single space-separated string, since otherwise it's possible that we'll get unlucky and end up with a 0 byte at just the wrong place and the argument list will be truncated.
thanks, that comment block is very helpful to provide background.

i believe that limitation is outdated as of linux-3.5 (July 2012).  that's when PR_SET_MM_{ARG,ENV}_{START,END} knobs were introduced to prctl.  although it was under the CONFIG_CHECKPOINT_RESTORE knob, and wasn't made generally available until linux-3.10 (June 2013).
  http://man7.org/linux/man-pages/man2/prctl.2.html

so if that is the only concern wrt formatting, i think we can handle that:
- create a NUL delimited buffer and point at it using prctl
- if prctl fails with EINVAL, fallback to the whitespace form we have today and rewriting the argv page
That sounds like it should work to me.
hidehiko@: did you feel like picking this up ?  i'm happy to review :).
It's a nice idea; however, prctl(PR_SET_MM, PR_SET_MM_ARG_START, ...) requires CAP_SYS_RESOURCE. You get EPERM otherwise.

prctl(PR_SET_MM, PR_SET_MM_ARG_START, 0x5594979de260, 0, 0) = -1 EPERM (Operation not permitted)
Would it be acceptable to simply not overwrite the command line on Linux? Or only do it when the replacement string fits in the initially allocated memory?

Is the only purpose behind this to get rid of references to /proc/self/exe? If that's the case, we could just replace argv[0] with a short string ("chrome" or "chromium" without clobbering the environ memory.
It also has the nice effect of making it so you can see which processes are the renderers, and what arguments are actually passed to them. Otherwise you'd always see "/proc/self/exe --type=zygote" and that's pretty much it. (At least, that'd have been the case years ago, last time I looked at this.)
This is causing test failures in tast.ui.MashLogin that look for --mash-service-name in the command line to detect certain process types (similar to looking for --type=renderer, etc.). The code can't use --type because these are actually utility processes. See  issue 891470 

I think there's another bug on this issue as well.

Cc: piman@chromium.org marc...@chromium.org
 Issue 772099  has been merged into this issue.
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/f95075b921b5041b81dc53cdae911c5bfb83239e

commit f95075b921b5041b81dc53cdae911c5bfb83239e
Author: James Cook <jamescook@chromium.org>
Date: Fri Oct 05 06:00:17 2018

tast-tests: Use arg prefix for mash processes in ui.MashLogin

Chrome sometimes truncates arguments in its command line.
Instead of looking for "mash-service-name", just look for
"mash-". The command line problem is issue 887875

BUG= chromium:891470 ,chromium:887875
TEST=this is the test

Change-Id: I812ffd147ec36148cd6185b73510dfef40b70135
Reviewed-on: https://chromium-review.googlesource.com/c/1260054
Tested-by: James Cook <jamescook@chromium.org>
Trybot-Ready: James Cook <jamescook@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>

[modify] https://crrev.com/f95075b921b5041b81dc53cdae911c5bfb83239e/src/chromiumos/tast/local/bundles/cros/ui/mash_login.go

I tried calling prctl(PR_SET_MM, PR_SET_MM_ARG_START, buffer, 0, 0) on-device with buffers in the heap, data area, and stack. All return -1 EPERM, consistent with comment 6 re: CAP_SYS_RESOURCE.

I'm guessing we can't or shouldn't grant CAP_SYS_RESOURCE to the browser? 

Sign in to add a comment