Merge lp:~jamesodhunt/upstart/bug-1083723 into lp:~upstart-devel/upstart/1.6

[James Hunt]

[Steve Langasek (vorlon)]

+ /* Disallow users from restarting Upstart, unless they happen to
+ * own this process (which they may do in the test scenario and
+ * when running Upstart as a non-privileged user).
+ */
+ if (session && session->user && session->user != uid) {

Same comment as for https://code.launchpad.net/~jamesodhunt/upstart/bug-1079715/+merge/135388: this should be 'session && session->user != uid', and not assume that all users should be allowed to call this method for a non-null session with user==0.

+ if (init_is_upstart ()) {
+ ret = restart_upstart ();
+ }

Please get rid of the init_is_upstart() check (and function); now that restarting is done over dbus, we shouldn't open a separate connection first to check that init is upstart. I think it's reasonable and appropriate that if someone calls 'telinit u' against a non-upstart init, and as a result init does not re-exec itself, telinit should exit non-zero instead of silently failing to take the requested action. Note that this is not an issue for the library upgrade case, because the library packages sensibly do:

 telinit u 2>/dev/null || true

If there is some reason I'm missing that it's important to return 0 in the case of a non-upstart init, please at least don't call upstart_open() twice, but instead save the result so we're not setting up and tearing down the dbus connection twice.

Otherwise, this looks good.

review: Needs Fixing

[lp:~jamesodhunt/upstart/bug-1083723]

1396. By James Hunt on 2012-12-06

* init/control.c: control_restart():
  - Simplified session check.
* util/telinit.c:
  - init_is_upstart(): Removed as redundant.
  - upstart_open(): Removed erroneous comment.
  - restart_upstart(): Changed return values for consistency with other
    calls in main() where an error is expected to set ret to < 0.
  - main(): Just call restart_upstart() in re-exec scenario to avoid the
    creation of a second D-Bus connection.

[James Hunt (jamesodhunt)]

> + /* Disallow users from restarting Upstart, unless they happen to
> + * own this process (which they may do in the test scenario and
> + * when running Upstart as a non-privileged user).
> + */
> + if (session && session->user && session->user != uid) {
>
> Same comment as for https://code.launchpad.net/~jamesodhunt/upstart/bug-1079715/+merge/135388: this should be 'session && session->user != uid', and not assume that all users should be allowed to call this method for a non-null session with user==0.

Fixed.

>
> + if (init_is_upstart ()) {
> + ret = restart_upstart ();
> + }
>
> Please get rid of the init_is_upstart() check (and function); now that restarting is done over dbus, we shouldn't open a separate connection first to check that init is upstart. I think it's reasonable and appropriate that if someone calls 'telinit u' against a non-upstart init, and as a result init does not re-exec itself, telinit should exit non-zero instead of silently failing to take the requested action. Note that this is not an issue for the library upgrade case, because the library packages sensibly do:
>
> telinit u 2>/dev/null || true
>
> If there is some reason I'm missing that it's important to return 0 in the case of a non-upstart init, please at least don't call upstart_open() twice, but instead save the result so we're not setting up and tearing down the dbus connection twice.

The initial init_is_upstart() check would have given us the opportunity to display a cleaner error message in the non-Upstart init scenario without more invasive code changes (something along the lines of "telinit: ERROR - init is not Upstart"). What currently happens if /sbin/init is not Upstart is this:

    # telinit u
    telinit: Failed to connect to socket /var/run/dbus/system_bus_socket: No such file or directory
    # echo $?
    1

It's not particularly pretty, but we don't anticipate this scenario happening much and of course it can be quietened down by redirecting stderr if required.

=== modified file 'dbus/com.ubuntu.Upstart.xml'
--- dbus/com.ubuntu.Upstart.xml	2012-09-09 21:22:06 +0000
+++ dbus/com.ubuntu.Upstart.xml	2012-12-06 11:40:27 +0000
@@ -34,6 +34,8 @@
       <arg name="jobs" type="ao" direction="out" />
     </method>
 
+    <method name="Restart"/>
+
     <!-- Signals for changes to the job list -->
     <signal name="JobAdded">
       <arg name="job" type="o" />
 
=== modified file 'init/control.c'
--- init/control.c	2012-09-20 15:16:52 +0000
+++ init/control.c	2012-12-06 11:40:27 +0000
@@ -949,3 +949,58 @@
 
 	return 0;
 }
+
+/**
+ * control_restart:
+ * @data: not used,
+ * @message: D-Bus connection and message received.
+ *
+ * Implements the Restart method of the com.ubuntu.Upstart
+ * interface.
+ *
+ * Called to request that Upstart performs a stateful re-exec.
+ *
+ * Returns: zero on success, negative value on raised error.
+ **/
+int
+control_restart (void           *data,
+		 NihDBusMessage *message)
+{
+	Session  *session;
+	uid_t     uid;
+
+	nih_assert (message != NULL);
+
+	uid = getuid ();
+
+	/* Get the relevant session */
+	session = session_from_dbus (NULL, message);
+
+	/* Chroot sessions must not be able to influence
+	 * the outside system.
+	 *
+	 * Making this a NOP is safe since it is the Upstart outside the
+	 * chroot which manages all chroot jobs.
+	 */
+	if (session && session->chroot) {
+		nih_warn (_("Ignoring restart request from chroot session"));
+		return 0;
+	}
+
+	/* Disallow users from restarting Upstart, unless they happen to
+	 * own this process (which they may do in the test scenario and
+	 * when running Upstart as a non-privileged user).
+	 */
+	if (session && session->user != uid) {
+		nih_dbus_error_raise_printf (
+			DBUS_INTERFACE_UPSTART ".Error.PermissionDenied",
+			_("You do not have permission to request restart"));
+		return -1;
+	}
+
+	nih_info (_("Restarting"));
+
+	stateful_reexec ();
+
+	return 0;
+}
 
=== modified file 'init/control.h'
--- init/control.h	2012-09-11 14:59:40 +0000
+++ init/control.h	2012-12-06 11:40:27 +0000
@@ -105,6 +105,9 @@
 int control_bus_release_name (void)
 	__attribute__ ((warn_unused_result));
 
+int  control_restart (void *data, NihDBusMessage *message)
+	__attribute__ ((warn_unused_result));
+
 NIH_END_EXTERN
 
 #endif /* INIT_CONTROL_H */
 
=== modified file 'util/Makefile.am'
--- util/Makefile.am	2011-06-01 11:13:42 +0000
+++ util/Makefile.am	2012-12-06 11:40:27 +0000
@@ -71,7 +71,8 @@
 	utmp.c utmp.h \
 	sysv.c sysv.h
 nodist_telinit_SOURCES = \
-	$(com_ubuntu_Upstart_OUTPUTS)
+	$(com_ubuntu_Upstart_OUTPUTS) \
+	$(com_ubuntu_Upstart_Instance_OUTPUTS)
 telinit_LDADD = \
 	$(LTLIBINTL) \
 	$(NIH_LIBS) \
 
=== modified file 'util/man/telinit.8'
--- util/man/telinit.8	2012-11-07 14:28:34 +0000
+++ util/man/telinit.8	2012-12-06 11:40:27 +0000
@@ -71,10 +71,16 @@
 .BR U " or " u
 to request that the
 .BR init (8)
-daemon re-execute itself.  This is necessary when upgrading the
+daemon re-execute itself.  This is necessary when upgrading the Upstart
 .BR init (8)
 daemon itself or any of its dependent system libraries
 to ensure disks can be unmounted cleanly on shutdown.
+
+Note that if the init daemon is
+.I not
+Upstart, this option will have no effect on the running
+.BR init (8)
+daemon.
 .\"
 .SH OPTIONS
 .TP
 
=== modified file 'util/telinit.c'
--- util/telinit.c	2009-07-09 11:50:19 +0000
+++ util/telinit.c	2012-12-06 11:40:27 +0000
@@ -38,12 +38,27 @@
 #include <nih/logging.h>
 #include <nih/error.h>
 
+#include <nih-dbus/dbus_error.h>
+#include <nih-dbus/dbus_proxy.h>
+#include <nih-dbus/errors.h>
+#include <nih-dbus/dbus_connection.h>
+
+#include "dbus/upstart.h"
+
+#include "com.ubuntu.Upstart.h"
+
 #include "sysv.h"
 
 
 /* Prototypes for option functions */
 int env_option (NihOption *option, const char *arg);
 
+NihDBusProxy * upstart_open (const void *parent)
+	__attribute__ ((warn_unused_result));
+
+int restart_upstart (void)
+	__attribute__ ((warn_unused_result));
+
 
 /**
  * extra_env:
@@ -83,6 +98,79 @@
 	return 0;
 }
 
+/**
+ * upstart_open:
+ * @parent: parent object for new proxy.
+ *
+ * Opens a connection to the Upstart init daemon and returns a proxy
+ * to the manager object. If @dest_name is not NULL, a connection is
+ * instead opened to the system bus and the proxy linked to the
+ * well-known name given.
+ *
+ * If @parent is not NULL, it should be a pointer to another object
+ * which will be used as a parent for the returned proxy.  When all
+ * parents of the returned proxy are freed, the returned proxy will
+ * also be freed.
+ *
+ * Returns: newly allocated D-Bus proxy or NULL on raised error.
+ **/
+NihDBusProxy *
+upstart_open (const void *parent)
+{
+	DBusError       dbus_error;
+	DBusConnection *connection;
+	NihDBusProxy *  upstart;
+
+	dbus_error_init (&dbus_error);
+
+	connection = dbus_bus_get (DBUS_BUS_SYSTEM, &dbus_error);
+	if (! connection) {
+		nih_dbus_error_raise (dbus_error.name, dbus_error.message);
+		dbus_error_free (&dbus_error);
+		return NULL;
+	}
+
+	dbus_connection_set_exit_on_disconnect (connection, FALSE);
+	dbus_error_free (&dbus_error);
+
+	upstart = nih_dbus_proxy_new (parent, connection,
+				      DBUS_SERVICE_UPSTART,
+				      DBUS_PATH_UPSTART,
+				      NULL, NULL);
+	if (! upstart) {
+		dbus_connection_unref (connection);
+		return NULL;
+	}
+
+	upstart->auto_start = FALSE;
+
+	/* Drop initial reference now the proxy holds one */
+	dbus_connection_unref (connection);
+
+	return upstart;
+}
+
+/**
+ * restart_upstart:
+ *
+ * Request Upstart restart itself.
+ *
+ * Returns: 0 on SUCCESS, -1 on raised error.
+ **/
+int
+restart_upstart (void)
+{
+	nih_local NihDBusProxy *upstart = NULL;
+
+	upstart = upstart_open (NULL);
+	if (! upstart)
+		return -1;
+
+	if (upstart_restart_sync (NULL, upstart) < 0)
+		return -1;
+
+	return 0;
+}
 
 #ifndef TEST
 /**
@@ -107,7 +195,7 @@
 {
 	char **args;
 	int    runlevel;
-	int    ret;
+	int    ret = 0;
 
 	nih_main_init (argv[0]);
 
@@ -174,9 +262,8 @@
 		break;
 	case 'U':
 	case 'u':
-		ret = kill (1, SIGTERM);
-		if (ret < 0)
-			nih_error_raise_system ();
+		/* If /sbin/init is not Upstart, just exit non-zero */
+		ret = restart_upstart ();
 		break;
 	default:
 		nih_assert_not_reached ();