It's wrong to ever change behavior due to the bus type (session vs. system), so that's not how the access control to this method should be done.

The config file setting for session vs. system is purely to control the way environment variables are set up in activation.c:child_setup(). This probably needs to be explained in "man dbus-daemon" under that config file option since past patches have also made this mistake. It might be worth putting a comment next to bus_context_get_type() also. The config file option should be called something else probably.

The reason is that a custom bus should be able to have any of the behaviors or policies that the standard buses have.

(Or another angle on this, the difference in system and session buses is supposed to be only their configuration.)

The right fix is to either add a new config option for this specific access control setting, or maybe you can just stick a policy in the default system.conf that disables calling the new method, using existing policy stuff.

This would need fixing in the spec also. The spec should basically say "some bus instances, such as the standard system bus, may disable access to this method for some or all callers" or something like that. (Though strictly speaking, that sentence is true for all methods.)

Other more minor stuff -

+  environment = dbus_malloc0 (length * sizeof (char *));
dbus_new0(char*, length) might be better

If "environ" isn't windows-portable it should be in sysdeps-unix but I don't know if it is.

In populate_environment(), you leak earlier allocations in the "preamble" if subsequent allocations fail. Ideally the unit tests would cover this.

+      _dbus_string_append (&key, environment[i]);

Return value not checked for OOM.

+          value_length = _dbus_string_get_length (&key) - equal_position - 1;
+          if (!_dbus_string_move_len (&key,
+                                      equal_position + 1,
+                                      value_length,
+                                      &value, 0))

Adding a _dbus_string_split_on_char() or something (with unit tests) would be much preferred here; this is the kind of error-prone math DBusString is intended to avoid.

+          hash_key[equal_position] = '\0';

I don't remember for sure but is this necessary? DBusString may already do it.

+  environment = dbus_malloc0 ((length + 1) * sizeof (char *));
dbus_new0 preferred

+  if (!_dbus_hash_table_insert_string (activation->environment,
+                                       _dbus_strdup (key),
+                                       _dbus_strdup (value)))
+    {

Not checking OOM on the strdups.

+  /* first check that the caller called us with the right
+   * signature
+   */
+  msg_type = dbus_message_iter_get_arg_type (&iter);
+

Could check the entire signature with message_has_signature() rather than only the first arg's type. Then could skip some of the extra checks later. The signature is guaranteed to be match what the iterator will iterate over.

+          if (!_dbus_list_append (&values, (void *) value))

I don't think a cast to (void*) should be needed in C ... there was a cast from void* to const char* earlier that I thought was odd too.

> It's wrong to ever change behavior due to the bus type (session vs. system), so
> that's not how the access control to this method should be done.
>
> The config file setting for session vs. system is purely to control the way
> environment variables are set up in activation.c:child_setup(). This probably
> needs to be explained in "man dbus-daemon" under that config file option since
> past patches have also made this mistake. It might be worth putting a comment
> next to bus_context_get_type() also. The config file option should be called
> something else probably.
Okay, i've removed the check, added an acl in system.conf to disallow the call, updated the spec blurb, and wrote a little peice for the man page.

> If "environ" isn't windows-portable it should be in sysdeps-unix but I don't
> know if it is.
I don't know either, but there was already code using environ in dbus-sysdeps.c so I think it's probably okay.

> +          hash_key[equal_position] = '\0';
> I don't remember for sure but is this necessary? DBusString may already do it.
It does, but I needed to stamp out the equal sign.

> Could check the entire signature with message_has_signature() rather than only
> the first arg's type.
Turns out the generic driver code already does this, so I just changed it to an assert.

I think I've addressed all your other points (updated patch is at http://people.freedesktop.org/~halfline/activation-environment.patch and fixes are on the branch).

+  if (environment == NULL)
+    return FALSE;
+
+  if (!_dbus_string_init (&key))
+    return FALSE;
+
+  if (!_dbus_string_init (&value))
+    {
+      _dbus_string_free (&key);
+      return FALSE;
+    }

Need to free environment in the second two failure cases right?

Does the code now set env variables in the daemon process every time it spawns a child? I doubt the inefficiency of that ever matters, but maybe a simple invalidation serial would be worth it or something. (Keep a counter that increments whenever the environment is modified, and reset the env variables only when the counter is not equal to its value last time they were reset.) Maybe this is pointless.

update_activation_environment can fail halfway (set only some of the variables, then throw an error). I think this is probably not worth fixing on grounds that it's too hard and will never matter, but it may be worth a comment above the loop that sets all the variables just so people know we thought about it.

The <deny> rule should probably match on the recipient bus name rather than the interface; it should be fine to call a method with that name, as long as it isn't a method on the bus daemon.

I would check that the policy code properly deals with policy rules on the bus daemon, since the bus daemon's name ownership is something of a special case, though. I don't remember the details. Hopefully this case is already handled.

Anyway, patch looks good overall.

Okay, I've pushed fixes for leak cases you found.

The environment of the bus daemon never gets changed.  The activation environment is stored in a hash table and extracted just before forking.  We don't call setenv or anything.

There are a few redundant hash_table_insert calls everytime the child gets activated, but that shouldn't matter.  If it ends up causing memory fragmentation problems for the repeated allocs or something, we can just add a lookup call before doing the insert.  It's a trade off, but i think it it's pretty insignificant either way.

I've changed the acl rule to:

   <deny send_destination="org.freedesktop.DBus"
         send_interface="org.freedesktop.DBus"
         send_member="UpdateActivationEnvironment"/>

Which seems to work, given this test program:

$ cat test.py
import dbus
bus = dbus.SystemBus()
daemon = bus.get_object ("org.freedesktop.DBus", "/org/freedesktop/DBus")
i = dbus.Interface(daemon, "org.freedesktop.DBus")
try:
 daemon.UpdateActivationEnvironment({"LD_PRELOAD": "/tmp/librootshell.so"})
except Exception, e:
 print e

try:
 i.UpdateActivationEnvironment({"LD_PRELOAD": "/tmp/librootshell.so"})
except Exception, e:
 print e

$ python test.py 
org.freedesktop.DBus.Error.AccessDenied: A security policy in place prevents this sender from sending this message to this recipient, see message bus configuration file (rejected message had interface "(unset)" member "UpdateActivationEnvironment" error name "(unset)" destination "org.freedesktop.DBus")
org.freedesktop.DBus.Error.AccessDenied: A security policy in place prevents this sender from sending this message to this recipient, see message bus configuration file (rejected message had interface "org.freedesktop.DBus" member "UpdateActivationEnvironment" error name "(unset)" destination "org.freedesktop.DBus")

Okay I've pushed this out to master now and dropped the activation-environment branch, if anymore issues crop up I clean things up on master.

Off to catch a plane!