Merge lp:~jamesodhunt/upstart/bugs-1235649+1203595 into lp:upstart

[James Hunt]

Make a Session Init connect to the D-Bus session bus as well as the private socket.

[James Hunt (jamesodhunt)]

This branch makes the Session Init attempt to connect to the D-Bus session bus on startup and retry when sent SIGUSR1. This behaviour is consistent with Upstart running as PID 1 and connecting to the D-Bus system bus.

The branch achieves the required behaviour by setting all global job environment variables in the Session Inits own environment too. This is specifically so that the Session Init will have knowledge of DBUS_SESSION_BUS_ADDRESS but avoids hard-coding a whitelist of variables to set. Note that since the Session Inits environment will change with each 'initctl (un)set-env -g ...', a subsequent call to 'initctl reset-env -g' will set the environment back to contain:

1) The default variables (PATH, TERM).
2) The Session Inits initial environment.
3) Any variables set via 'set-env -g'.

This is a subtle behavioural change which although not ideal can be rectified by manually running 'unsetenv $var' for all job global variables that should be removed from job environments.

Note that Upstart running as PID 1 will not have its environment modified.

Tested to ensure that bug 1235649 is resolved when a client connects to Upstart via the D-Bus session bus without a main loop and the spam.sh (attached to the bug) is run.

[James Hunt (jamesodhunt)]

Discussion with slangasek has resulted in a better plan to solve this issue.

[Steve Langasek (vorlon)]

I was assuming you would replace this with a fresh branch, based on our discussion, since it will have almost nothing in common with the current code. :) So rejecting this mp.

As we discussed, a better approach is to make this explicit rather than implicit by having a new initctl interface that takes the dbus address as an argument. When a session init receives this call, it should connect to the dbus session bus at the specified address if it isn't already connected to the session bus. (Requests to connect to a session bus if there's already a session bus should be ignored.) This way, we don't have to fiddle with the environment at all.

review: Disapprove

Unmerged revisions

1545. By James Hunt on 2013-10-22

* init/control.c: control_bus_open(): Don't call nih_dbus_bus() if
  DBUS_SESSION_BUS_ADDRESS is not set to avoid D-bus auto-launching a
  dbus-daemon.
* init/environ.c: Comments.
* init/job_class.c:
  - job_class_environment_init(): Superior check on whether job_environ
    is not empty.
  - job_class_environment_reset(): Only reset job_environ if not NULL
    already.
  - job_class_environment_set(): Set variable in Upstarts environment
    too (required to allow Upstart to be aware of the D-Bus session bus
    address when the dbus-daemon is available).
  - job_class_environment_unset(): Unset variable from Upstarts
    environment, but only if it is not a default variable.
* init/job_process.c: Formatting.
* init/test_control.c: Updated strings used by tests which check error
  messages to include 'D-Bus'.
* init/test_environ.c:
  - test_add(): New test:
    - "using bare word with no corresponding variable set in environment"
  - test_remove(): New function ("the missing test") containing 8 new tests:
    - "remove name=value pair with empty table"
    - "remove bare name with empty table"
    - "remove name=value from table of size 1"
    - "remove bare name from table of size 1"
    - "remove first name=value entry from table of size 2"
    - "remove first bare name entry from table of size 2"
    - "remove last name=value entry from table of size 2"
    - "remove last bare name entry from table of size 2"
* test/test_util_common.c:
  - Formatting.
  - get_initctl(): Added environment checks.
* util/initctl.c:
  - Formatting.
  - Removed testing comment from option text for '--session'.
* util/man/initctl.8: Removed testing comment for '--session'.
* util/tests/test_initctl.c:
  - test_session_init(): New test that checks the Session Init now
    connects to the D-Bus session bus.

1544. By James Hunt on 2013-10-18

* Connect to the D-Bus session bus as well as using a private socket
  when running as a Session Init (LP: #1203595, #1235649).

=== modified file 'ChangeLog'
--- ChangeLog	2013-10-04 21:34:25 +0000
+++ ChangeLog	2013-10-22 10:15:27 +0000
@@ -1,4 +1,51 @@
-2013-01-04  Steve Langasek  <[e-mail]
[phone]  James Hunt  <[e-mail]>
+
+	* init/control.c: control_bus_open(): Don't call nih_dbus_bus() if
+	  DBUS_SESSION_BUS_ADDRESS is not set to avoid D-bus auto-launching a
+	  dbus-daemon.
+	* init/environ.c: Comments.
+	* init/job_class.c:
+	  - job_class_environment_init(): Superior check on whether job_environ
+	    is not empty.
+	  - job_class_environment_reset(): Only reset job_environ if not NULL
+	    already.
+	  - job_class_environment_set(): Set variable in Upstarts environment
+	    too (required to allow Upstart to be aware of the D-Bus session bus
+	    address when the dbus-daemon is available).
+	  - job_class_environment_unset(): Unset variable from Upstarts
+	    environment, but only if it is not a default variable.
+	* init/job_process.c: Formatting.
+	* init/test_control.c: Updated strings used by tests which check error
+	  messages to include 'D-Bus'.
+	* init/test_environ.c:
+	  - test_add(): New test:
+	    - "using bare word with no corresponding variable set in environment"
+	  - test_remove(): New function ("the missing test") containing 8 new tests:
+	    - "remove name=value pair with empty table"
+	    - "remove bare name with empty table"
+	    - "remove name=value from table of size 1"
+	    - "remove bare name from table of size 1"
+	    - "remove first name=value entry from table of size 2"
+	    - "remove first bare name entry from table of size 2"
+	    - "remove last name=value entry from table of size 2"
+	    - "remove last bare name entry from table of size 2"
+	* test/test_util_common.c:
+	  - Formatting.
+	  - get_initctl(): Added environment checks.
+	* util/initctl.c:
+	  - Formatting.
+	  - Removed testing comment from option text for '--session'.
+	* util/man/initctl.8: Removed testing comment for '--session'.
+	* util/tests/test_initctl.c:
+	  - test_session_init(): New test that checks the Session Init now
+	    connects to the D-Bus session bus.
+
[phone]  James Hunt  <[e-mail]>
+
+	* Connect to the D-Bus session bus as well as using a private socket
+	  when running as a Session Init (LP: #1203595, #1235649).
+
[phone]  Steve Langasek  <[e-mail]
 
 	* extra/upstart-local-bridge.c: use SOCKET_PATH in our event
 	  environment, instead of clobbering PATH.  (LP: #1235480)
 
=== modified file 'init/control.c'
--- init/control.c	2013-04-22 10:30:09 +0000
+++ init/control.c	2013-10-22 10:15:27 +0000
@@ -81,11 +81,19 @@
  *
  * If TRUE, connect to the D-Bus session bus rather than the system bus.
  *
- * Used for testing.
+ * Used for testing to simulate (as far as possible) a system-like init
+ * when running as a non-priv user.
  **/
 int use_session_bus = FALSE;
 
 /**
+ * dbus_bus_type:
+ *
+ * Type of D-Bus bus to connect to.
+ **/
+DBusBusType dbus_bus_type;
+
+/**
  * control_server_address:
  *
  * Address on which the control server may be reached.
@@ -258,16 +266,35 @@
 
 	control_init ();
 
-	control_handle_bus_type ();
-
-	/* Connect to the D-Bus System Bus and hook everything up into
+	dbus_bus_type = control_get_bus_type ();
+
+	/* Avoid D-Bus attempting to autolaunch a dbus-daemon since:
+	 *
+	 * - the behaviour is undesirable: launching a dbus-daemon
+	 *   should be handled by a job.
+	 *
+	 * - if autolaunch fails (for example if DISPLAY is not set, or
+	 *   dbus-uuidgen has not been run (or the generated file is
+	 *   unreadable)), libdbus may call abort(), depending on how
+	 *   libdbus has been built.
+	 */
+	if (dbus_bus_type == DBUS_BUS_SESSION && ! getenv ("DBUS_SESSION_BUS_ADDRESS")) {
+		nih_dbus_error_raise ("dbus session bus error",
+				"DBUS_SESSION_BUS_ADDRESS not set");
+		return -1;
+	}
+
+	/* Connect to the appropriate D-Bus bus and hook everything up into
 	 * our own main loop automatically.
 	 */
-	conn = nih_dbus_bus (use_session_bus ? DBUS_BUS_SESSION : DBUS_BUS_SYSTEM,
-			     control_disconnected);
+	conn = nih_dbus_bus (dbus_bus_type, control_disconnected);
 	if (! conn)
 		return -1;
 
+	nih_debug ("Connected to D-Bus %s bus",
+			dbus_bus_type == DBUS_BUS_SESSION
+			? "session" : "system");
+
 	/* Register objects on the bus. */
 	control_register_all (conn);
 
@@ -345,7 +372,9 @@
 
 		dbus_error_init (&error);
 
-		nih_warn (_("Disconnected from system bus"));
+		nih_warn (_("Disconnected from D-Bus %s bus"),
+				dbus_bus_type == DBUS_BUS_SESSION
+				? "session" : "system");
 
 		control_bus = NULL;
 	}
@@ -817,19 +846,23 @@
 }
 
 /**
- * control_handle_bus_type:
+ * control_get_bus_type:
  *
  * Determine D-Bus bus type to connect to.
+ *
+ * Returns: Type of D-Bus bus to connect to.
  **/
-void
-control_handle_bus_type (void)
+DBusBusType
+control_get_bus_type (void)
 {
 	if (getenv (USE_SESSION_BUS_ENV))
 		use_session_bus = TRUE;
 
-	if (use_session_bus)
-		nih_debug ("Using session bus");
+	return (use_session_bus || user_mode) 
+		? DBUS_BUS_SESSION
+		: DBUS_BUS_SYSTEM;
 }
+
 /**
  * control_notify_disk_writeable:
  * @data: not used,
 
=== modified file 'init/control.h'
--- init/control.h	2013-05-08 16:21:08 +0000
+++ init/control.h	2013-10-22 10:15:27 +0000
@@ -134,7 +134,11 @@
 				   const char *log_priority)
 	__attribute__ ((warn_unused_result));
 
-void control_handle_bus_type      (void);
+DBusBusType control_get_bus_type (void)
+	__attribute__ ((warn_unused_result));
+
+const char *control_get_bus_name (void)
+	__attribute__ ((warn_unused_result));
 
 void control_prepare_reexec       (void);
 
 
=== modified file 'init/environ.c'
--- init/environ.c	2013-01-08 15:57:31 +0000
+++ init/environ.c	2013-10-22 10:15:27 +0000
@@ -202,6 +202,10 @@
 	for (e = *env; e && *e; e++) {
 		keylen = strcspn (*e, "=");
 
+		/* Found @str in the existing environment (either as a
+		 * name=value pair, or a bare name), so don't copy it to
+		 * the new environment.
+		 */
 		if (! strncmp (str, *e, keylen))
 			continue;
 
 
=== modified file 'init/job_class.c'
--- init/job_class.c	2013-07-22 00:15:43 +0000
+++ init/job_class.c	2013-10-22 10:15:27 +0000
@@ -89,6 +89,14 @@
 NihHash *job_classes = NULL;
 
 /**
+ * default_environ:
+ *
+ * Minimal set of environment variables all jobs are provided with by
+ * default.
+ */
+static char * const default_environ[] = { JOB_DEFAULT_ENVIRONMENT, NULL };
+
+/**
  * job_environ:
  *
  * Array of environment variables that will be set in the jobs
@@ -116,8 +124,6 @@
 void
 job_class_environment_init (void)
 {
-	char * const default_environ[] = { JOB_DEFAULT_ENVIRONMENT, NULL };
-
 	if (job_environ)
 		return;
 
@@ -125,7 +131,7 @@
 	NIH_MUST (environ_append (&job_environ, NULL, 0, TRUE, default_environ));
 
 	if (user_mode && ! no_inherit_env)
-		NIH_MUST(environ_append (&job_environ, NULL, 0, TRUE, environ));
+		NIH_MUST (environ_append (&job_environ, NULL, 0, TRUE, environ));
 }
 
 /**
@@ -138,10 +144,10 @@
 void
 job_class_environment_reset (void)
 {
-	if (job_environ)
+	if (job_environ) {
 		nih_free (job_environ);
-
-	job_environ = NULL;
+		job_environ = NULL;
+	}
 
 	job_class_environment_init ();
 }
@@ -160,9 +166,31 @@
 int
 job_class_environment_set (const char *var, int replace)
 {
+	nih_local char  *name = NULL;
+	char            *value;
+
 	nih_assert (var);
 	nih_assert (job_environ);
 
+	name = nih_strdup (NULL, var);
+	if (! name)
+		return -1;
+
+	value = strchr (name, '=');
+	nih_assert (value);
+	*value = '\0';
+
+	/* Jump over delimiter */
+	value++;
+
+	/* Set variable in Upstarts environment.
+	 *
+	 * This isn't necessary for jobs (due to job_environ), but is
+	 * required to allow the Session Init to connect to the D-Bus
+	 * session bus.
+	 */
+	setenv (name, value, replace);
+
 	if (! environ_add (&job_environ, NULL, NULL, replace, var))
 		return -1;
 
@@ -193,9 +221,20 @@
 int
 job_class_environment_unset (const char *name)
 {
+	char * const *e;
+	int           keep = FALSE;
+
 	nih_assert (name);
 	nih_assert (job_environ);
 
+	/* Determine if @name is a default variable */
+	for (e = default_environ; e && *e; e++) {
+		if (! strcmp (*e, name)) {
+			keep = TRUE;
+			break;
+		}
+	}
+
 	if (! environ_remove (&job_environ, NULL, NULL, name))
 		return -1;
 
@@ -211,6 +250,17 @@
 		}
 	}
 
+	if (! keep) {
+		/* Remove variable from Upstarts environment, but only if it is
+		 * not part of the default environment; if we did this,
+		 * it would not be possible to reset the job environment
+		 * since adding a variable by name would *not* be added
+		 * to the job environment table since the environment
+		 * lookup would fail.
+		 */
+		unsetenv (name);
+	}
+
 	return 0;
 }
 
 
=== modified file 'init/job_process.c'
--- init/job_process.c	2013-10-03 14:43:24 +0000
+++ init/job_process.c	2013-10-22 10:15:27 +0000
@@ -278,7 +278,7 @@
 	env = NIH_MUST (nih_str_array_new (NULL));
 
 	if (job->env)
-		NIH_MUST(environ_append (&env, NULL, &envc, TRUE, job->env));
+		NIH_MUST (environ_append (&env, NULL, &envc, TRUE, job->env));
 
 	if (job->stop_env
 	    && ((process == PROCESS_PRE_STOP)
 
=== modified file 'init/main.c'
--- init/main.c	2013-07-31 09:28:48 +0000
+++ init/main.c	2013-10-22 10:15:27 +0000
@@ -128,7 +128,7 @@
 extern int          default_console;
 extern int          write_state_file;
 extern char        *log_dir;
-
+extern DBusBusType  dbus_bus_type;
 
 /**
  * options:
@@ -213,7 +213,8 @@
 	if (disable_job_logging)
 		nih_debug ("Job logging disabled");
 
-	control_handle_bus_type ();
+	if (getenv (USE_SESSION_BUS_ENV))
+		use_session_bus = TRUE;
 
 	if (! user_mode)
 		no_inherit_env = TRUE;
@@ -933,14 +934,20 @@
 	      NihSignal *signal)
 {
 	if (! control_bus) {
-		nih_info (_("Reconnecting to system bus"));
+		char *dbus_bus_name;
+
+		dbus_bus_name = dbus_bus_type == DBUS_BUS_SESSION
+			? "session" : "system";
+
+		nih_info (_("Reconnecting to D-Bus %s bus"),
+				dbus_bus_name);
 
 		if (control_bus_open () < 0) {
 			NihError *err;
 
 			err = nih_error_get ();
-			nih_warn ("%s: %s", _("Unable to connect to the system bus"),
-				  err->message);
+			nih_warn (_("Unable to connect to the D-Bus %s bus: %s"),
+					dbus_bus_name, err->message);
 			nih_free (err);
 		}
 	}
 
=== modified file 'init/tests/test_control.c'
--- init/tests/test_control.c	2012-09-09 21:27:24 +0000
+++ init/tests/test_control.c	2013-10-22 10:15:27 +0000
@@ -821,7 +821,7 @@
 
 	TEST_LIST_EMPTY (control_conns);
 
-	TEST_FILE_EQ (output, "test: Disconnected from system bus\n");
+	TEST_FILE_EQ (output, "test: Disconnected from D-Bus system bus\n");
 	TEST_FILE_END (output);
 	TEST_FILE_RESET (output);
 
@@ -879,7 +879,7 @@
 
 	TEST_LIST_EMPTY (control_conns);
 
-	TEST_FILE_EQ (output, "test: Disconnected from system bus\n");
+	TEST_FILE_EQ (output, "test: Disconnected from D-Bus system bus\n");
 	TEST_FILE_END (output);
 	TEST_FILE_RESET (output);
 
 
=== modified file 'init/tests/test_environ.c'
--- init/tests/test_environ.c	2011-06-06 17:05:11 +0000
+++ init/tests/test_environ.c	2013-10-22 10:15:27 +0000
@@ -572,6 +572,462 @@
 	}
 
 	unsetenv ("BAR");
+
+	/* Check that attempting to add a variable by name fails if
+	 * there is no corresponding environment variable set.
+	 */
+	TEST_FEATURE ("using bare word with no corresponding variable set in environment");
+
+	/* Ensure variable not set initially */
+	TEST_EQ_P (getenv ("UPSTART_TEST_VARIABLE"), NULL);
+
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			len = 0;
+			env = nih_str_array_new (NULL);
+			assert (nih_str_array_add (&env, NULL, &len,
+						   "FOO=BAR"));
+		}
+
+		ret = environ_add (&env, NULL, &len, FALSE, "UPSTART_TEST_VARIABLE");
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (ret, NULL);
+
+			TEST_EQ (len, 1);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+			TEST_EQ_P (env[1], NULL);
+
+			nih_free (env);
+			continue;
+		}
+
+		/* XXX: Attempting to add an unset variable results in
+		 * no change to the table (it is not an error!)
+		 */
+		TEST_EQ_P (ret, env);
+
+		TEST_EQ (len, 1);
+		TEST_EQ_STR (env[0], "FOO=BAR");
+		TEST_EQ_P (env[1], NULL);
+
+		nih_free (env);
+	}
+}
+
+void
+test_remove (void)
+{
+	char   **env = NULL, **ret;
+	size_t   len = 0;
+
+	TEST_FUNCTION ("environ_remove");
+
+	TEST_FEATURE ("remove name=value pair with empty table");
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			len = 0;
+			env = nih_str_array_new (NULL);
+		}
+
+		ret = environ_remove (&env, NULL, &len, "FOO=BAR");
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (ret, NULL);
+
+			TEST_EQ (len, 0);
+			TEST_EQ_P (env[0], NULL);
+
+			nih_free (env);
+			continue;
+		}
+
+		TEST_EQ_P (ret, NULL);
+		TEST_EQ (len, 0);
+		TEST_EQ_P (env[0], NULL);
+
+		nih_free (env);
+	}
+
+	TEST_FEATURE ("remove bare name with empty table");
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			len = 0;
+			env = nih_str_array_new (NULL);
+		}
+
+		ret = environ_remove (&env, NULL, &len, "FOO");
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (ret, NULL);
+
+			TEST_EQ (len, 0);
+			TEST_EQ_P (env[0], NULL);
+
+			nih_free (env);
+			continue;
+		}
+
+		TEST_EQ_P (ret, NULL);
+		TEST_EQ (len, 0);
+		TEST_EQ_P (env[0], NULL);
+
+		nih_free (env);
+	}
+
+	TEST_FEATURE ("remove name=value from table of size 1");
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			len = 0;
+			env = nih_str_array_new (NULL);
+
+			ret = environ_add (&env, NULL, &len, TRUE, "FOO=BAR");
+			TEST_NE_P (ret, NULL);
+
+			TEST_EQ (len, 1);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+			TEST_EQ_P (env[1], NULL);
+		}
+
+		ret = environ_remove (&env, NULL, &len, "FOO=BAR");
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (ret, NULL);
+
+			TEST_EQ (len, 1);
+
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+
+			TEST_NE_P (env[0], NULL);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+
+			TEST_EQ_P (env[1], NULL);
+
+			nih_free (env);
+			continue;
+		}
+
+		TEST_NE_P (ret, NULL);
+		TEST_EQ (len, 0);
+		TEST_EQ_P (env[0], NULL);
+
+		nih_free (env);
+	}
+
+	TEST_FEATURE ("remove bare name from table of size 1");
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			len = 0;
+			env = nih_str_array_new (NULL);
+
+			ret = environ_add (&env, NULL, &len, TRUE, "FOO=BAR");
+			TEST_NE_P (ret, NULL);
+
+			TEST_EQ (len, 1);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+			TEST_EQ_P (env[1], NULL);
+		}
+
+		ret = environ_remove (&env, NULL, &len, "FOO");
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (ret, NULL);
+
+			TEST_EQ (len, 1);
+
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+
+			TEST_NE_P (env[0], NULL);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+
+			TEST_EQ_P (env[1], NULL);
+
+			nih_free (env);
+			continue;
+		}
+
+		TEST_NE_P (ret, NULL);
+		TEST_EQ (len, 0);
+		TEST_EQ_P (env[0], NULL);
+
+		nih_free (env);
+	}
+
+	TEST_FEATURE ("remove first name=value entry from table of size 2");
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			len = 0;
+			env = nih_str_array_new (NULL);
+
+			ret = environ_add (&env, NULL, &len, TRUE, "FOO=BAR");
+			TEST_NE_P (ret, NULL);
+
+			TEST_EQ (len, 1);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+			TEST_EQ_P (env[1], NULL);
+
+			ret = environ_add (&env, NULL, &len, TRUE, "BAZ=QUX");
+			TEST_NE_P (ret, NULL);
+
+			TEST_EQ (len, 2);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+
+			TEST_ALLOC_PARENT (env[1], env);
+			TEST_ALLOC_SIZE (env[1], 8);
+			TEST_EQ_STR (env[1], "BAZ=QUX");
+
+			TEST_EQ_P (env[2], NULL);
+		}
+
+		/* Remove first entry added */
+		ret = environ_remove (&env, NULL, &len, "FOO=BAR");
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (ret, NULL);
+
+			TEST_EQ (len, 2);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+
+			TEST_ALLOC_PARENT (env[1], env);
+			TEST_ALLOC_SIZE (env[1], 8);
+			TEST_EQ_STR (env[1], "BAZ=QUX");
+
+			TEST_EQ_P (env[2], NULL);
+
+			nih_free (env);
+			continue;
+		}
+
+		TEST_NE_P (ret, NULL);
+		TEST_EQ (len, 1);
+
+		TEST_ALLOC_PARENT (env[0], env);
+		TEST_ALLOC_SIZE (env[0], 8);
+		TEST_EQ_STR (env[0], "BAZ=QUX");
+
+		TEST_EQ_P (env[1], NULL);
+
+		nih_free (env);
+	}
+
+	TEST_FEATURE ("remove first bare name entry from table of size 2");
+
+	/* Set a variable to allow the bare name to be expanded */
+	assert0 (setenv ("UPSTART_TEST_VARIABLE", "foo", 1));
+
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			len = 0;
+			env = nih_str_array_new (NULL);
+
+			ret = environ_add (&env, NULL, &len, TRUE, "UPSTART_TEST_VARIABLE");
+			TEST_NE_P (ret, NULL);
+
+			TEST_EQ (len, 1);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+
+			/* Should have been expanded */
+			TEST_EQ_STR (env[0], "UPSTART_TEST_VARIABLE=foo");
+
+			TEST_EQ_P (env[1], NULL);
+
+			ret = environ_add (&env, NULL, &len, TRUE, "BAZ=QUX");
+			TEST_NE_P (ret, NULL);
+
+			TEST_EQ (len, 2);
+
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "UPSTART_TEST_VARIABLE=foo");
+
+			TEST_ALLOC_PARENT (env[1], env);
+			TEST_ALLOC_SIZE (env[1], 8);
+			TEST_EQ_STR (env[1], "BAZ=QUX");
+
+			TEST_EQ_P (env[2], NULL);
+		}
+
+		/* Remove first entry added */
+		ret = environ_remove (&env, NULL, &len, "UPSTART_TEST_VARIABLE");
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (ret, NULL);
+
+			TEST_EQ (len, 2);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "UPSTART_TEST_VARIABLE=foo");
+
+			TEST_ALLOC_PARENT (env[1], env);
+			TEST_ALLOC_SIZE (env[1], 8);
+			TEST_EQ_STR (env[1], "BAZ=QUX");
+
+			TEST_EQ_P (env[2], NULL);
+
+			nih_free (env);
+			continue;
+		}
+
+		TEST_NE_P (ret, NULL);
+		TEST_EQ (len, 1);
+
+		TEST_ALLOC_PARENT (env[0], env);
+		TEST_ALLOC_SIZE (env[0], 8);
+		TEST_EQ_STR (env[0], "BAZ=QUX");
+
+		TEST_EQ_P (env[1], NULL);
+
+		nih_free (env);
+	}
+
+	assert0 (unsetenv ("UPSTART_TEST_VARIABLE"));
+
+	TEST_FEATURE ("remove last name=value entry from table of size 2");
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			len = 0;
+			env = nih_str_array_new (NULL);
+
+			ret = environ_add (&env, NULL, &len, TRUE, "FOO=BAR");
+			TEST_NE_P (ret, NULL);
+
+			TEST_EQ (len, 1);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+			TEST_EQ_P (env[1], NULL);
+
+			ret = environ_add (&env, NULL, &len, TRUE, "BAZ=QUX");
+			TEST_NE_P (ret, NULL);
+
+			TEST_EQ (len, 2);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+
+			TEST_ALLOC_PARENT (env[1], env);
+			TEST_ALLOC_SIZE (env[1], 8);
+			TEST_EQ_STR (env[1], "BAZ=QUX");
+
+			TEST_EQ_P (env[2], NULL);
+		}
+
+		/* Remove last entry added */
+		ret = environ_remove (&env, NULL, &len, "BAZ=QUX");
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (ret, NULL);
+
+			TEST_EQ (len, 2);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+
+			TEST_ALLOC_PARENT (env[1], env);
+			TEST_ALLOC_SIZE (env[1], 8);
+			TEST_EQ_STR (env[1], "BAZ=QUX");
+
+			TEST_EQ_P (env[2], NULL);
+
+			nih_free (env);
+			continue;
+		}
+
+		TEST_NE_P (ret, NULL);
+		TEST_EQ (len, 1);
+
+		TEST_ALLOC_PARENT (env[0], env);
+		TEST_ALLOC_SIZE (env[0], 8);
+		TEST_EQ_STR (env[0], "FOO=BAR");
+
+		TEST_EQ_P (env[1], NULL);
+
+		nih_free (env);
+	}
+
+	TEST_FEATURE ("remove last bare name entry from table of size 2");
+
+	/* Set a variable to allow the bare name to be expanded */
+	assert0 (setenv ("UPSTART_TEST_VARIABLE", "foo", 1));
+
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			len = 0;
+			env = nih_str_array_new (NULL);
+
+			ret = environ_add (&env, NULL, &len, TRUE, "FOO=BAR");
+			TEST_NE_P (ret, NULL);
+
+			TEST_EQ (len, 1);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+			TEST_EQ_P (env[1], NULL);
+
+			ret = environ_add (&env, NULL, &len, TRUE, "UPSTART_TEST_VARIABLE");
+			TEST_NE_P (ret, NULL);
+
+			TEST_EQ (len, 2);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+
+			TEST_ALLOC_PARENT (env[1], env);
+			TEST_ALLOC_SIZE (env[1], 8);
+
+			/* Should have been expanded */
+			TEST_EQ_STR (env[1], "UPSTART_TEST_VARIABLE=foo");
+
+			TEST_EQ_P (env[2], NULL);
+		}
+
+		/* Remove last entry added */
+		ret = environ_remove (&env, NULL, &len, "UPSTART_TEST_VARIABLE");
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (ret, NULL);
+
+			TEST_EQ (len, 2);
+			TEST_ALLOC_PARENT (env[0], env);
+			TEST_ALLOC_SIZE (env[0], 8);
+			TEST_EQ_STR (env[0], "FOO=BAR");
+
+			TEST_ALLOC_PARENT (env[1], env);
+			TEST_ALLOC_SIZE (env[1], 8);
+			TEST_EQ_STR (env[1], "UPSTART_TEST_VARIABLE=foo");
+
+			TEST_EQ_P (env[2], NULL);
+
+			nih_free (env);
+			continue;
+		}
+
+		TEST_NE_P (ret, NULL);
+		TEST_EQ (len, 1);
+
+		TEST_ALLOC_PARENT (env[0], env);
+		TEST_ALLOC_SIZE (env[0], 8);
+		TEST_EQ_STR (env[0], "FOO=BAR");
+
+		TEST_EQ_P (env[1], NULL);
+
+		nih_free (env);
+	}
+
+	assert0 (unsetenv ("UPSTART_TEST_VARIABLE"));
 }
 
 void
@@ -1590,6 +2046,7 @@
 	setenv ("UPSTART_NO_SESSIONS", "1", 1);
 
 	test_add ();
+	test_remove ();
 	test_append ();
 	test_set ();
 	test_lookup ();
 
=== modified file 'test/test_util_common.c'
--- test/test_util_common.c	2013-10-02 08:59:20 +0000
+++ test/test_util_common.c	2013-10-22 10:15:27 +0000
@@ -120,7 +120,7 @@
 }
 
 /* TRUE to denote that Upstart is running in user session mode
- * (FALSE to denote it's using the users D-Bus session bus).
+ * (FALSE to denote it's using a D-Bus session bus only).
  */
 int test_user_mode = FALSE;
 
@@ -349,6 +349,16 @@
 {
 	static char path[PATH_MAX + 1024] = { 0 };
 	int         ret;
+	int         env_valid;
+
+	/* Sanity check calling environment */
+	if (test_user_mode) {
+		env_valid = getenv ("UPSTART_SESSION") ? TRUE : FALSE;
+	} else {
+		env_valid = getenv ("DBUS_SESSION_BUS_ADDRESS") ? TRUE : FALSE;
+	}
+
+	nih_assert (env_valid);
 
 	ret = sprintf (path, "%s %s",
 			get_initctl_binary (),
 
=== modified file 'util/initctl.c'
--- util/initctl.c	2013-07-21 23:54:16 +0000
+++ util/initctl.c	2013-10-22 10:15:27 +0000
@@ -342,8 +342,7 @@
 			use_dbus = getuid () ? TRUE : FALSE;
 		if (use_dbus >= 0 && dbus_bus_type < 0)
 			dbus_bus_type = DBUS_BUS_SYSTEM;
-	}
-	else {
+	} else {
 		if (! user_addr) {
 			nih_error ("UPSTART_SESSION isn't set in the environment. "
 				       "Unable to locate the Upstart instance.");
@@ -2827,7 +2826,7 @@
  * Command-line options accepted for all arguments.
  **/
 static NihOption options[] = {
-	{ 0, "session", N_("use D-Bus session bus to connect to init daemon (for testing)"),
+	{ 0, "session", N_("use D-Bus session bus to connect to init daemon"),
 	  NULL, NULL, NULL, dbus_bus_type_setter },
 	{ 0, "system", N_("use D-Bus system bus to connect to init daemon"),
 	  NULL, NULL, NULL, dbus_bus_type_setter },
 
=== modified file 'util/man/initctl.8'
--- util/man/initctl.8	2013-05-31 15:41:20 +0000
+++ util/man/initctl.8	2013-10-22 10:15:27 +0000
@@ -47,9 +47,9 @@
 .\"
 .TP
 .B \-\-session
-Connect to
+Connect to the
 .BR init (8)
-daemon using the D\-Bus session bus (for testing purposes only).
+daemon using the D\-Bus session bus.
 .\"
 .TP
 .B \-\-system
 
=== modified file 'util/tests/test_initctl.c'
--- util/tests/test_initctl.c	2013-09-26 16:33:07 +0000
+++ util/tests/test_initctl.c	2013-10-22 10:15:27 +0000
@@ -16698,6 +16698,89 @@
         TEST_EQ (rmdir (logdir), 0);
 }
 
+void
+test_session_init (void)
+{
+	size_t           lines;
+	pid_t            dbus_pid = 0;
+	pid_t            upstart_pid = 0;
+	nih_local char  *cmd = NULL;
+	char           **output;
+	nih_local char  *dbus_session_address = NULL;
+	nih_local char  *upstart_session = NULL;
+	char            *address;
+
+	TEST_GROUP ("User Mode");
+
+	TEST_FEATURE ("ensure session init connects to D-Bus session bus");
+
+	/* Start a dbus-daemon */
+	TEST_DBUS (dbus_pid);
+
+	/* Not required */
+	assert0 (unsetenv ("DBUS_SYSTEM_BUS_ADDRESS"));
+
+	address = getenv ("DBUS_SESSION_BUS_ADDRESS");
+	TEST_NE_P (address, NULL);
+
+	dbus_session_address = nih_strdup (NULL, address);
+	TEST_NE_P (dbus_session_address, NULL);
+
+	/* Stop Upstart connectng to the D-Bus session bus at
+	 * startup (it will attempt to do so, but fail and try again on
+	 * SIGUSR1.
+	 */
+	assert0 (unsetenv ("DBUS_SESSION_BUS_ADDRESS"));
+
+	START_UPSTART (upstart_pid, TRUE);
+
+	/* Save the Upstart session socket details and unset to stop
+	 * initctl finding upstart via this route.
+	 */
+	address = getenv ("UPSTART_SESSION");
+	TEST_NE_P (address, NULL);
+	upstart_session = nih_strdup (NULL, address);
+	TEST_NE_P (upstart_session, NULL);
+	assert0 (unsetenv ("UPSTART_SESSION"));
+
+	/* Ensure it is not possible to query the running version via
+	 * the D-Bus session bus.
+	 */
+	cmd = nih_sprintf (NULL, "%s --session version 2>&1", get_initctl_binary ());
+	TEST_NE_P (cmd, NULL);
+	RUN_COMMAND (NULL, cmd, &output, &lines);
+	TEST_EQ (lines, 1);
+	TEST_STR_MATCH (output[0], "initctl: Name \"com.ubuntu.Upstart\" does not exist*");
+
+	/* Re-apply in the test environment */
+	assert0 (setenv ("DBUS_SESSION_BUS_ADDRESS", dbus_session_address, 1));
+	assert0 (setenv ("UPSTART_SESSION", upstart_session, 1));
+
+	/* Set the variable in Upstarts environment too */
+	cmd = nih_sprintf (NULL, "%s --user set-env -g %s=%s",
+			get_initctl_binary (),
+			"DBUS_SESSION_BUS_ADDRESS",
+			dbus_session_address);
+	TEST_NE_P (cmd, NULL);
+	RUN_COMMAND (NULL, cmd, &output, &lines);
+	TEST_EQ (lines, 0);
+
+	/* Send signal to upstart requesting it reconnect to D-Bus */
+	assert0 (kill (upstart_pid, SIGUSR1));
+
+	/* It should now be possible to query the running version via
+	 * the D-Bus session bus.
+	 */
+	cmd = nih_sprintf (NULL, "%s --session version 2>&1", get_initctl_binary ());
+	TEST_NE_P (cmd, NULL);
+	RUN_COMMAND (NULL, cmd, &output, &lines);
+	TEST_EQ (lines, 1);
+	TEST_STR_MATCH (output[0], "init (upstart [0-9.][0-9.]*");
+
+	STOP_UPSTART (upstart_pid);
+	TEST_DBUS_END (dbus_pid);
+}
+
 int
 main (int   argc,
       char *argv[])
@@ -16741,5 +16824,7 @@
 		test_notify_disk_writeable ();
 	}
 
+	test_session_init ();
+
 	return 0;
 }