Created attachment 84313 [details] [review]
[PATCH] Fix a typo: DBUS_ACTIVATION_ --> DBUS_STARTER_

What practical effect does this have? If it's been wrong since 2005, then perhaps it was never necessary (and I'm somewhat worried about regressions).

(In reply to comment #2)
> What practical effect does this have? If it's been wrong since 2005, then
> perhaps it was never necessary (and I'm somewhat worried about regressions).

Yeah, the support of DBUS_STARTER_X (called DBUS_ACTIVATION_X at that time) introduced in 2003, ten years ago. Not quite understand what's exactly the background, maybe it's useful to the client that which type of bus (system/session) activated it. Refer to DBus Spec: http://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-starting-services

However, seems in the past 10 years, most of all are just go and connect the other two well-known types of bus (system and session), rather than connect to the starter bus.

Anyway, from my POV, this fix should not introduce any regression because once the client initalized it's three types of connections, it's safe to clear it's environment.

It looks as though the intention of this code is that if you have this process tree:

dbus-daemon
  \- activated service com.example.MyService
     \- child process /usr/bin/myutil

then MyService gets the DBUS_STARTER_BUS_TYPE, DBUS_STARTER_ADDRESS from dbus-daemon, but myutil doesn't inherit them (which could conceivably make it behave incorrectly, e.g. if it wants to start a new session for regression tests or something like in Bug #63119).

However, if we haven't been doing this correctly for 8 years, and everyone has coped OK with that, then it can't have been all that important... and I think anyone who is starting a sub-session is responsible for sorting out their own environment, e.g. with the new dbus-run-session(1) (which, as of git master, does unset these variables).

Compounding this, setenv() is not thread-safe (e.g. Bug #65681): if you setenv() in one thread, a concurrent getenv() in other threads can return wrong pointers. So I'm very tempted to just remove these _dbus_setenv() calls, instead of making them (un)set the variable that actually has an effect.

Thiago, Colin, David, any thoughts?

GDBus doesn't seem to clear these environment variables, for what it's worth. It also mitigates the equivalent of Bug #63119 by ignoring DBUS_STARTER_ADDRESS, and just using DBUS_STARTER_BUS_TYPE to select whether it does the equivalent of dbus_bus_get (DBUS_BUS_SESSION, ...) or dbus_bus_get (DBUS_BUS_SYSTEM, ...). (Do you think we should do that too?)

(In reply to comment #4)
> Compounding this, setenv() is not thread-safe (e.g. Bug #65681)

Also Bug #27585.

(In reply to comment #4)
> It looks as though the intention of this code is that if you have this
> process tree:
> 
> dbus-daemon
>   \- activated service com.example.MyService
>      \- child process /usr/bin/myutil
> 
> then MyService gets the DBUS_STARTER_BUS_TYPE, DBUS_STARTER_ADDRESS from
> dbus-daemon, but myutil doesn't inherit them (which could conceivably make
> it behave incorrectly, e.g. if it wants to start a new session for
> regression tests or something like in Bug #63119).
> 
> However, if we haven't been doing this correctly for 8 years, and everyone
> has coped OK with that, then it can't have been all that important... and I
> think anyone who is starting a sub-session is responsible for sorting out
> their own environment, e.g. with the new dbus-run-session(1) (which, as of
> git master, does unset these variables).
> 
> Compounding this, setenv() is not thread-safe (e.g. Bug #65681): if you
> setenv() in one thread, a concurrent getenv() in other threads can return
> wrong pointers. So I'm very tempted to just remove these _dbus_setenv()
> calls, instead of making them (un)set the variable that actually has an
> effect.

Seems it's a better idea giving that it unset two never setup envs (because the typo) in the past 8 years, this is just the same effect as just doesn't do the unset.

> 
> Thiago, Colin, David, any thoughts?
> 
> GDBus doesn't seem to clear these environment variables, for what it's
> worth. It also mitigates the equivalent of Bug #63119 by ignoring
> DBUS_STARTER_ADDRESS, and just using DBUS_STARTER_BUS_TYPE to select whether
> it does the equivalent of dbus_bus_get (DBUS_BUS_SESSION, ...) or
> dbus_bus_get (DBUS_BUS_SYSTEM, ...). (Do you think we should do that too?)

Created attachment 87312 [details] [review]
[PATCH] Remove unnecessary dbus_setenv()

Comment on attachment 87312 [details] [review]
[PATCH] Remove unnecessary dbus_setenv()

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

looks good

(In reply to comment #8)
> Comment on attachment 87312 [details] [review]
> [PATCH] Remove unnecessary dbus_setenv()
...
> looks good

I'm going to give Thiago and Colin a chance to veto this, so it probably won't be in the 1.7.6 release I plan to do soon; but if they don't respond soon, I'll apply it for 1.7.8.

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 87312 [details] [review] [review]
> > [PATCH] Remove unnecessary dbus_setenv()
> ...
> > looks good
> 
> I'm going to give Thiago and Colin a chance to veto this, so it probably
> won't be in the 1.7.6 release I plan to do soon; but if they don't respond
> soon, I'll apply it for 1.7.8.

No objections here; I agree with Chengwei that basically the "starter bus" concept ended up not being useful.

Fixed in git for 1.7.8, let's see whether it breaks anything...