Bug 69716

Created attachment 86375 [details] [review]
[PATCH] Corret cmdline for dbus-launch started by "autolaunch:"

Comment on attachment 86375 [details] [review]
[PATCH] Corret cmdline for dbus-launch started by "autolaunch:"

Review of attachment 86375 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-unix.c
@@ +3277,2 @@
>                              dbus_bool_t path_fallback,
> +                            char      **argv,

I think "_read_subprocess_line_argv may change its argv parameter" violates the principle of least astonishment.

Wouldn't it be simpler to make _dbus_get_autolaunch_address pass the fully-qualified name (what it currently puts in argv[0]) as progpath, and "dbus-launch" in argv[0]?

(In reply to comment #2)
> Comment on attachment 86375 [details] [review] [review]
> [PATCH] Corret cmdline for dbus-launch started by "autolaunch:"
> 
> Review of attachment 86375 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-sysdeps-unix.c
> @@ +3277,2 @@
> >                              dbus_bool_t path_fallback,
> > +                            char      **argv,
> 
> I think "_read_subprocess_line_argv may change its argv parameter" violates
> the principle of least astonishment.

Yes, agreed if it's named as _read_ something.

> 
> Wouldn't it be simpler to make _dbus_get_autolaunch_address pass the
> fully-qualified name (what it currently puts in argv[0]) as progpath, and
> "dbus-launch" in argv[0]?

In fact, it already put the full path name in progpath, so as argv[0], the context here if it start the full-path progpath failed, it will fallback to launch the path segment basename($progpath) and then get the confusing result.

See the comments around there.

          /* Ok, that failed.  Now if path_fallback is given, let's
           * try unqualified.  This is mostly a hack to work
           * around systems which ship dbus-launch in /usr/bin
           * but everything else in /bin (because dbus-launch
           * depends on X11).
           */
          if (path_fallback)
            /* We must have a slash, because we checked above */
            execvp (strrchr (progpath, '/')+1, argv);

(In reply to comment #3)
> In fact, it already put the full path name in progpath, so as argv[0], the
> context here if it start the full-path progpath failed, it will fallback to
> launch the path segment basename($progpath) and then get the confusing
> result.

Yes, I understand that. My suggestion is that it would be less confusing, and avoid doing strange things with its arguments, if argv[0] was basename(progpath) in both cases.

It's already the case that argv[0] in /proc/$pid/cmdline isn't guaranteed to have the full path:

    archetype% cat /proc/$$/cmdline; printf "\n"
    zsh

(In reply to comment #4)
> (In reply to comment #3)
> > In fact, it already put the full path name in progpath, so as argv[0], the
> > context here if it start the full-path progpath failed, it will fallback to
> > launch the path segment basename($progpath) and then get the confusing
> > result.
> 
> Yes, I understand that. My suggestion is that it would be less confusing,
> and avoid doing strange things with its arguments, if argv[0] was
> basename(progpath) in both cases.

However, because the argv[0] is the full-path progpath, so in the fallback case, it's different with 'dbus-daemon', so it's cmdline in /proc is wrong and confusing.

> 
> It's already the case that argv[0] in /proc/$pid/cmdline isn't guaranteed to
> have the full path:
> 
>     archetype% cat /proc/$$/cmdline; printf "\n"
>     zsh

(In reply to comment #4)
> (In reply to comment #3)
> > In fact, it already put the full path name in progpath, so as argv[0], the
> > context here if it start the full-path progpath failed, it will fallback to
> > launch the path segment basename($progpath) and then get the confusing
> > result.
> 
> Yes, I understand that. My suggestion is that it would be less confusing,
> and avoid doing strange things with its arguments, if argv[0] was
> basename(progpath) in both cases.
> 
> It's already the case that argv[0] in /proc/$pid/cmdline isn't guaranteed to
> have the full path:
> 
>     archetype% cat /proc/$$/cmdline; printf "\n"
>     zsh

Yes, I understand your point, however, argv[0] is full-path name, not the basename(progpath).

(In reply to comment #3)
> > Wouldn't it be simpler to make _dbus_get_autolaunch_address pass the
> > fully-qualified name (what it currently puts in argv[0]) as progpath, and
> > "dbus-launch" in argv[0]?
> 
> In fact, it already put the full path name in progpath, so as argv[0], the
> context here if it start the full-path progpath failed, it will fallback to
> launch the path segment basename($progpath) and then get the confusing
> result.

Right. I'm suggesting changing the behaviour of _dbus_get_autolaunch_address() so that it always calls _read_subprocess_line_argv() with argv[0] = "dbus-launch", rather than it being a copy of progpath. Then the command seen in /proc would always be of the form "dbus-launch blah blah blah".

(In reply to comment #7)
> (In reply to comment #3)
> > > Wouldn't it be simpler to make _dbus_get_autolaunch_address pass the
> > > fully-qualified name (what it currently puts in argv[0]) as progpath, and
> > > "dbus-launch" in argv[0]?
> > 
> > In fact, it already put the full path name in progpath, so as argv[0], the
> > context here if it start the full-path progpath failed, it will fallback to
> > launch the path segment basename($progpath) and then get the confusing
> > result.
> 
> Right. I'm suggesting changing the behaviour of
> _dbus_get_autolaunch_address() so that it always calls
> _read_subprocess_line_argv() with argv[0] = "dbus-launch", rather than it
> being a copy of progpath. Then the command seen in /proc would always be of
> the form "dbus-launch blah blah blah".

Sure, that's good to me, I think I did misunderstand you, given that you said once in comment2. Will upload new patch.

Created attachment 92317 [details] [review]
Set argv[0] to dbus-launch to avoid misleading info from /proc

    Set argv[0] to dbus-launch to avoid misleading info from /proc
    
    At previous, argv[0] is the full-qualified path of program, however, if
    start dbus-launch failed, it will fall back to find one from $PATH,
    while keep the argv[0] as the full-qualified path. So we'll get
    misleading info from /proc or ps(1) about dbus-launch process.
    
    One simple solution is just hard-code argv[0] to dbus-launch, something
    like (From Simon).
    
        archetype% cat /proc/$$/cmdline; printf "\n"
        zsh

Fixed in git for 1.9.0, thanks. This is cosmetic and not 100% risk-free, so I'm not changing 1.8.