Bug 92857 - Forward UpdateActivationEnvironment to systemd --user
Summary: Forward UpdateActivationEnvironment to systemd --user
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.10
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+ 1.10
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-11-07 16:07 UTC by Jan Alexander Steffens (heftig)
Modified: 2015-11-17 23:49 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/4] bus_driver_handle_update_activation_environment: Error on system buses (1.18 KB, patch)
2015-11-07 17:50 UTC, Jan Alexander Steffens (heftig)
Details | Splinter Review
[PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd (7.46 KB, patch)
2015-11-07 17:50 UTC, Jan Alexander Steffens (heftig)
Details | Splinter Review
[PATCH 3/4] test/sd-activation: Test UpdateActivationEnvironment forwarding (8.79 KB, patch)
2015-11-07 17:51 UTC, Jan Alexander Steffens (heftig)
Details | Splinter Review
[PATCH 4/4] bus-driver: Support returning org.freedesktop.DBus UID and PID (11.50 KB, patch)
2015-11-07 17:52 UTC, Jan Alexander Steffens (heftig)
Details | Splinter Review
[PATCH 1/4] bus_driver_handle_update_activation_environment: Error on system buses (1.46 KB, patch)
2015-11-14 15:36 UTC, Jan Alexander Steffens (heftig)
Details | Splinter Review
bus_driver_handle_update_activation_environment: Forward to systemd (8.02 KB, patch)
2015-11-14 15:46 UTC, Jan Alexander Steffens (heftig)
Details | Splinter Review
[PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd (8.02 KB, patch)
2015-11-14 15:47 UTC, Jan Alexander Steffens (heftig)
Details | Splinter Review
[PATCH 3/4] test/sd-activation: Test UpdateActivationEnvironment forwarding (9.23 KB, patch)
2015-11-14 15:50 UTC, Jan Alexander Steffens (heftig)
Details | Splinter Review
[PATCH 4/4] bus-driver: Support returning org.freedesktop.DBus UID and PID (12.28 KB, patch)
2015-11-14 15:57 UTC, Jan Alexander Steffens (heftig)
Details | Splinter Review
[PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd (7.76 KB, patch)
2015-11-15 15:05 UTC, Jan Alexander Steffens (heftig)
Details | Splinter Review

Description Jan Alexander Steffens (heftig) 2015-11-07 16:07:44 UTC
In order to ease transition to systemd-activated user services, dbus-daemon should forward its activation environment to systemd --user.
Comment 1 Jan Alexander Steffens (heftig) 2015-11-07 17:50:11 UTC
Created attachment 119464 [details] [review]
[PATCH 1/4] bus_driver_handle_update_activation_environment: Error on  system buses

The default policy already disallows calls on system buses. Since any
bus with a service helper cleans the environment anyway, there's no
point in allowing this to be called.
Comment 2 Jan Alexander Steffens (heftig) 2015-11-07 17:50:59 UTC
Created attachment 119465 [details] [review]
[PATCH 2/4] bus_driver_handle_update_activation_environment: Forward  to systemd

If we use systemd activation, forward all UpdateActivationEnvironment
requests to org.freedesktop.systemd1.Manager.SetEnvironment, in order
to ensure variables needed by D-Bus services are available when these
services are launched by systemd.

Since UpdateActivationEnvironment is not available on the system bus,
this only applies to user buses.
Comment 3 Jan Alexander Steffens (heftig) 2015-11-07 17:51:31 UTC
Created attachment 119466 [details] [review]
[PATCH 3/4] test/sd-activation: Test UpdateActivationEnvironment forwarding
Comment 4 Jan Alexander Steffens (heftig) 2015-11-07 17:52:09 UTC
Created attachment 119467 [details] [review]
[PATCH 4/4] bus-driver: Support returning org.freedesktop.DBus UID and PID

Attempting to call SetEnvironment on systemd causes it to inquire
about the caller's connection UID and PID. If this check fails,
the call is rejected.
Comment 5 Simon McVittie 2015-11-08 11:24:30 UTC
Comment on attachment 119464 [details] [review]
[PATCH 1/4] bus_driver_handle_update_activation_environment: Error on  system buses

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

::: bus/driver.c
@@ +1011,4 @@
>      }
>  #endif
>  
> +  {

Opening an artificial block (where we don't already have an "if", "while", etc.) just as a scope for variables is unusual style: we normally only do this inside #ifdefs, if at all. I'd normally declare BusContext at the top of the function with the rest of the local variables (retval, etc.), and assign to it just before use.

But this is fine too.
Comment 6 Simon McVittie 2015-11-08 11:48:59 UTC
Comment on attachment 119465 [details] [review]
[PATCH 2/4] bus_driver_handle_update_activation_environment: Forward  to systemd

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

::: bus/driver.c
@@ +991,5 @@
> +  _DBUS_ASSERT_ERROR_IS_CLEAR (error);
> +
> +  service_name = dbus_message_get_destination (message);
> +
> +  _dbus_assert (service_name != NULL);

I think this deserves a pseudo-doc-comment, something like

/*
 * message must already have a non-NULL destination.
 */

above the function definition.

@@ +1011,5 @@
> +          return FALSE;
> +        }
> +
> +      if (!bus_activation_activate_service (activation, connection, transaction,
> +                                            TRUE, message, service_name, error))

Hmm. I think it's incorrect to pass a non-NULL connection here: conceptually, systemd is being activated by dbus-daemon itself, not by the caller of UpdateActivationEnvironment.

(In practice it doesn't matter much in the auto_activation=TRUE code path, but I'd prefer to be consistent, in case it does matter later.)

I'd prefer this function to take the BusContext as its first argument. The caller can get it from the DBusConnection - it's essentially a container for global singletons, and has methods to get the BusActivation and BusRegistry.

If there is some reason why you *need* the DBusConnection here, it should be named something like "initiator" to distinguish it from any other connections that might be floating around, like the connection to systemd. The dbus-daemon code has a bad habit of using "connection" as a variable name where it should really be using "sender" or "destination" or something.

@@ +1189,4 @@
>  
>        if (!bus_activation_set_environment_variable (activation,
>                                                      key, value, error))
> +        {

In future please don't re-indent unless you have to change that line anyway... but this is small, and it's changing from "wrong style" to "correct style", so it's OK.

@@ +1202,5 @@
> +
> +          /* SetEnvironment wants an array of KEY=VALUE strings */
> +          if (!_dbus_string_init (&envline) ||
> +              !_dbus_string_append_printf (&envline, "%s=%s", key, value) ||
> +              (s = _dbus_string_get_data (&envline)) == NULL ||

If init'ing the string succeeded, which it did, then _dbus_string_get_data() can't actually return NULL, so there's no need for this check.

I prefer to treat assignment as a statement (like it is in Python) rather than using it as an expression:

if (!init (...) || !append_printf (...))
  {
    /* ... recover */
  }

s = _dbus_string_get_data (&envline);

if (!append_basic (&systemd_iter, &s))
  {
    /* recover */
  }
Comment 7 Simon McVittie 2015-11-08 11:55:38 UTC
Comment on attachment 119466 [details] [review]
[PATCH 3/4] test/sd-activation: Test UpdateActivationEnvironment forwarding

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

::: test/sd-activation.c
@@ +65,5 @@
>    g_assert_cmpint (dbus_message_get_reply_serial (m), ==, 0); \
>  } while (0)
>  
> +#define assert_method_call(m, sender, path, iface, \
> +    method, signature) \

Did you copy this from the monitor test? If not, please do. We should assert about the destination, not just the sender, and I think the version in the monitor test already does.

(g_assert_cmpstr accepts NULL, so it's fine to have a nullable destination parameter if needed.)
Comment 8 Simon McVittie 2015-11-08 12:22:14 UTC
Comment on attachment 119467 [details] [review]
[PATCH 4/4] bus-driver: Support returning org.freedesktop.DBus UID and PID

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

::: bus/driver.c
@@ +1754,5 @@
>    reply = dbus_message_new_method_return (message);
>    if (reply == NULL)
>      goto oom;
>  
> +  if (found != BUS_DRIVER_FOUND_PEER ||

Would benefit from a comment something like

/* We don't know how to find "ADT audit session data"
 * for the dbus-daemon itself (or even whether that would be
 * meaningful) */

@@ +1815,5 @@
>  
> +  if (found == BUS_DRIVER_FOUND_PEER)
> +    context = bus_connection_get_selinux_id (conn);
> +  else
> +    context = NULL;

Would benefit from a comment

/* Ideally we would also know how to get the security context
 * of the dbus-daemon itself, but someone who knows SELinux
 * will need to provide and test that patch */

@@ +1880,5 @@
> +  if ((found == BUS_DRIVER_FOUND_PEER &&
> +       dbus_connection_get_unix_process_id (conn, &ulong_val) &&
> +       ulong_val <= _DBUS_UINT32_MAX) ||
> +      (found == BUS_DRIVER_FOUND_SELF &&
> +       (ulong_val = _dbus_getpid ()) != DBUS_PID_UNSET))

I'd prefer to break this up, treating assignments as expressions not statements. Maybe something like

/* ... an "if/else if" or "switch" to set ulong_val */

/* we can't represent > 32-bit pids; ... */
if (ulong_val <= _DBUS_UINT32_MAX && ulong_val != DBUS_PID_UNSET)
  {
    /* ... add it */
  }

@@ +1892,5 @@
> +  if ((found == BUS_DRIVER_FOUND_PEER &&
> +       dbus_connection_get_unix_user (conn, &ulong_val) &&
> +       ulong_val <= _DBUS_UINT32_MAX) ||
> +      (found == BUS_DRIVER_FOUND_SELF &&
> +       (ulong_val = _dbus_getuid ()) != DBUS_UID_UNSET))

Similarly, I'd prefer to break this up a bit.

@@ +1899,5 @@
>          goto oom;
>      }
>  
> +  if (found == BUS_DRIVER_FOUND_PEER &&
> +      dbus_connection_get_windows_user (conn, &s))

Comment

/* ideally this would know how to get the dbus-daemon's Windows SID */

(I might implement this myself later - it's testable with Wine.)

@@ +1922,5 @@
>        dbus_free (s);
>      }
>  
> +  if (found == BUS_DRIVER_FOUND_PEER &&
> +      _dbus_connection_get_linux_security_label (conn, &s))

/* ideally this would know how to get the dbus-daemon's label too */

::: dbus/dbus-sysdeps-win.c
@@ +2138,5 @@
>    return GetCurrentProcessId ();
>  }
>  
> +/** Gets our UID
> + * @returns process UID

"Gets our Unix uid. On Windows, this just returns DBUS_UID_UNSET."

(or something)

::: dbus/dbus-sysdeps.h
@@ +641,5 @@
>  DBUS_PRIVATE_EXPORT
>  dbus_pid_t    _dbus_getpid (void);
>  
> +DBUS_PRIVATE_EXPORT
> +dbus_uid_t    _dbus_getuid (void);

Don't you also need to remove it from dbus-sysdeps-unix.h?
Comment 9 Jan Alexander Steffens (heftig) 2015-11-08 12:27:06 UTC
(In reply to Simon McVittie from comment #7)

Thanks for your reviews. I'll post adjusted patches soon.

> > +#define assert_method_call(m, sender, path, iface, \
> > +    method, signature) \
> 
> Did you copy this from the monitor test? If not, please do. We should assert
> about the destination, not just the sender, and I think the version in the
> monitor test already does.

IIRC I removed it because the destination was a unique name. Should I assert on that name?

(In reply to Simon McVittie from comment #8)
> /* Ideally we would also know how to get the security context
>  * of the dbus-daemon itself, but someone who knows SELinux
>  * will need to provide and test that patch */
  
> /* ideally this would know how to get the dbus-daemon's Windows SID */

> /* ideally this would know how to get the dbus-daemon's label too */

Should these be FIXMEs?
Comment 10 Simon McVittie 2015-11-09 12:29:20 UTC
(In reply to Jan Alexander Steffens (heftig) from comment #9)
> > We should assert
> > about the destination, not just the sender, and I think the version in the
> > monitor test already does.
> 
> IIRC I removed it because the destination was a unique name. Should I assert
> on that name?

Yes please. You can get the unique name from dbus_bus_get_unique_name(). In the monitor test, they're all cached in the Fixture to make these assertions easier.

> (In reply to Simon McVittie from comment #8)
> > /* Ideally we would also know how to get the security context
(etc.)
>
> Should these be FIXMEs?

If you like - there's no strong policy either way. I don't think writing FIXME necessarily makes it any more or less likely that they will be fixed, particularly the ones that require LSM knowledge.
Comment 11 Jan Alexander Steffens (heftig) 2015-11-14 15:36:07 UTC
Created attachment 119657 [details] [review]
[PATCH 1/4] bus_driver_handle_update_activation_environment: Error on system buses

Adjusted:
- Removed scope used only for variable
Comment 12 Jan Alexander Steffens (heftig) 2015-11-14 15:46:07 UTC
Created attachment 119658 [details] [review]
bus_driver_handle_update_activation_environment: Forward to systemd

Adjusted:
- bus_driver_send_or_activate now takes only a BusContext instead of a DBusConnection and a BusTransaction; a transaction is created internally
- Building the systemd message no longer uses assignment in a conditional
Comment 13 Jan Alexander Steffens (heftig) 2015-11-14 15:47:24 UTC
Created attachment 119659 [details] [review]
[PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd
Comment 14 Jan Alexander Steffens (heftig) 2015-11-14 15:50:07 UTC
Created attachment 119660 [details] [review]
[PATCH 3/4] test/sd-activation: Test UpdateActivationEnvironment forwarding

Adjusted:
- Copy complete asserts from the monitor test, now matching destinations

This also highlighted how bus_transaction_send_from_driver overwrites the
destination with the unique name; the activation leaves it alone.
Comment 15 Jan Alexander Steffens (heftig) 2015-11-14 15:57:51 UTC
Created attachment 119661 [details] [review]
[PATCH 4/4] bus-driver: Support returning org.freedesktop.DBus UID and PID

Adjusted:
- FIXMEs added for missing information about the org.freedesktop.DBus "connection"
- Restructured code to avoid assigning in conditions
Comment 16 Simon McVittie 2015-11-15 13:08:10 UTC
Comment on attachment 119657 [details] [review]
[PATCH 1/4] bus_driver_handle_update_activation_environment: Error on system buses

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

Makes sense. I'll probably apply this next week.
Comment 17 Simon McVittie 2015-11-15 13:13:17 UTC
Comment on attachment 119659 [details] [review]
[PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd

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

::: bus/driver.c
@@ +1002,5 @@
> +
> +  service = bus_registry_lookup (bus_context_get_registry (context),
> +                                 &service_string);
> +
> +  transaction = bus_transaction_new (context);

The existing transaction was previously a parameter, and I think it should stay a parameter, instead of you providing a new one.

The purpose of the transactions is that, as much as possible, a method call should either entirely fail with OOM or entirely succeed. In this case, if there is enough memory to send SetEnvironment to systemd *and* update the environment *and* send a success reply back to the caller, we should do all of those; but if there is insufficient memory, we should not do any of them.

The transaction methods that say they send a message do not actually send it: "send" in the method name is shorthand for "queue the message to be sent on commit, pre-allocating all the memory that you will need".
Comment 18 Simon McVittie 2015-11-15 13:14:35 UTC
Comment on attachment 119660 [details] [review]
[PATCH 3/4] test/sd-activation: Test UpdateActivationEnvironment forwarding

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

Makes sense (but I haven't reviewed in great detail, I'll do so next week)
Comment 19 Simon McVittie 2015-11-15 13:20:11 UTC
Comment on attachment 119661 [details] [review]
[PATCH 4/4] bus-driver: Support returning org.freedesktop.DBus UID and PID

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

Looks good from a brief review. I'll check this more thoroughly next week.
Comment 20 Jan Alexander Steffens (heftig) 2015-11-15 15:05:21 UTC
Created attachment 119683 [details] [review]
[PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd

Adjusted:
- bus_driver_send_or_activate takes the BusTransaction from the caller again; the context is taken from the transaction
Comment 21 Simon McVittie 2015-11-16 10:49:19 UTC
Comment on attachment 119683 [details] [review]
[PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd

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

Thanks, this looks good now. I'll check through all the patches again before merging, but hopefully they're suitable for 1.10.4.
Comment 22 Simon McVittie 2015-11-17 23:49:08 UTC
Fixed in 1.10.4. Thanks!

(I changed the error for inability to determine the pid back to DBUS_ERROR_UNIX_PROCESS_ID_UNKNOWN, to be consistent with how it was in the past.)


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.