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

[James Hunt]

[Steve Langasek (vorlon)]

+ /* Get the relevant session */
+ session = session_from_dbus (NULL, message);
+
+ /* We don't want chroot sessions snooping outside their domain.
+ *
+ * Ideally, we'd allow them to query their own session, but the
+ * current implementation doesn't lend itself to that.
+ */
+ if (session && session->chroot) {
+ nih_warn (_("Ignoring state query from chroot session"));
+ return 0;
+ }
+
+ /* Disallow users from obtaining state details, 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) {

Is there ever any possibility of a non-null session, where session->chroot is null and session->user is 0? If such a session exists, all users will be able to read it.

It's probably better to just do:

+ if (session && session->user != uid) {

this way, we always block access unless the uid matches - much more future-proof.

+ control_bus_flush ();

I'm not sure why this is needed in the GetState case. Is this because we can't serialize dbus state? Why would that be important in this context? Since we don't need to be able to deserialize the resulting state, I think this should be omitted unless not flushing somehow causes state_to_string() to fail.

+ while (TRUE) {
+ bytes = write (fd, buffer->buf, buffer->len);
+
+ if (! bytes)
+ break;
+ else if (bytes > 0)
+ nih_io_buffer_shrink (buffer, (size_t)bytes);
+ else if (bytes < 0 && (errno != EAGAIN && errno != EWOULDBLOCK)) {
+ break;
+ }
+ }
+

nih_io_buffer_shrink() seems an awfully inefficient way to do this, but I guess it's consistent with the rest of the code.

Shouldn't EINTR be handled here? Also, EWOULDBLOCK is AIUI only a specified behavior in the case of a socket, which we know this is not. So I think you want (errno != EAGAIN && errno != EINTR).

Otherwise, this looks good. I hope you didn't have to construct that json test data by hand ("Christmas"? :).

review: Needs Fixing

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

1395. By James Hunt on 2012-12-06

* init/control.c: control_get_state():
  - Simplified session check.
  - Changed error message to refer to 'state', not 'restart'.
  - Don't call control_bus_flush() since it's not technically required
    in this non-re-exec scenario.
* init/state.c: state_write_file():
  - Added comment.
  - Check write for EINTR, not EAGAIN/EWOULDBLOCK.

[James Hunt (jamesodhunt)]

> + /* Get the relevant session */
> + session = session_from_dbus (NULL, message);
> +
> + /* We don't want chroot sessions snooping outside their domain.
> + *
> + * Ideally, we'd allow them to query their own session, but the
> + * current implementation doesn't lend itself to that.
> + */
> + if (session && session->chroot) {
> + nih_warn (_("Ignoring state query from chroot session"));
> + return 0;
> + }
> +
> + /* Disallow users from obtaining state details, 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) {
>
> Is there ever any possibility of a non-null session, where session->chroot is null and session->user is 0? If such a session exists, all users will be able to read it.
>
> It's probably better to just do:
>
> + if (session && session->user != uid) {
>
> this way, we always block access unless the uid matches - much more future-proof.

Agreed - fixed.

>
> + control_bus_flush ();
>
> I'm not sure why this is needed in the GetState case. Is this because we can't serialize dbus state? Why would that be important in this context? Since we don't need to be able to deserialize the resulting state, I think this should be omitted unless not flushing somehow causes state_to_string() to fail.

This call is consistent with the re-exec behaviour, but you are correct in that it's not technically required (and a facility to query state shouldn't potentially be changing it), so I've removed the call for this scenario.

>
> + while (TRUE) {
> + bytes = write (fd, buffer->buf, buffer->len);
> +
> + if (! bytes)
> + break;
> + else if (bytes > 0)
> + nih_io_buffer_shrink (buffer, (size_t)bytes);
> + else if (bytes < 0 && (errno != EAGAIN && errno != EWOULDBLOCK)) {
> + break;
> + }
> + }
> +
>
> nih_io_buffer_shrink() seems an awfully inefficient way to do this, but I guess it's consistent with the rest of the code.

This could indeed be written in a more efficient manner, but it's using the standard NIH idiom and is simple and clear I think. Also recall that this code only gets exercised in what is hopefully an impossible failure scenario anyway :-)

>
> Shouldn't EINTR be handled here? Also, EWOULDBLOCK is AIUI only a specified behavior in the case of a socket, which we know this is not. So I think you want (errno != EAGAIN && errno != EINTR).
>

Good catch! Had my socket cap on. Fixed :)

> Otherwise, this looks good. I hope you didn't have to construct that json test data by hand ("Christmas"? :).

Thankfully not ;-)

=== 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 09:42:22 +0000
@@ -34,6 +34,10 @@
       <arg name="jobs" type="ao" direction="out" />
     </method>
 
+    <method name="GetState">
+      <arg name="state" type="s" direction="out" />
+    </method>
+
     <!-- Signals for changes to the job list -->
     <signal name="JobAdded">
       <arg name="job" type="o" />

=== modified file 'init/Makefile.am'
--- init/Makefile.am	2012-11-18 05:57:58 +0000
+++ init/Makefile.am	2012-12-06 09:42:22 +0000
@@ -126,8 +126,15 @@
 	$(com_ubuntu_Upstart_Job_OUTPUTS) \
 	$(com_ubuntu_Upstart_Instance_OUTPUTS)
 
-
-EXTRA_DIST = init.supp
+TEST_DATA_DIR = $(PWD)/tests/data
+
+AM_CPPFLAGS += -DTEST_DATA_DIR="\"$(TEST_DATA_DIR)\""
+
+TEST_DATA_FILES = \
+	$(TEST_DATA_DIR)/upstart-1.6-blocked.json
+
+EXTRA_DIST = init.supp $(TEST_DATA_FILES)
+	
 
 test_util_SOURCES = \
 	tests/test_util.c tests/test_util.h

=== modified file 'init/control.c'
--- init/control.c	2012-09-20 15:16:52 +0000
+++ init/control.c	2012-12-06 09:42:22 +0000
@@ -949,3 +949,65 @@
 
 	return 0;
 }
+
+/**
+ * control_get_state:
+ *
+ * @data: not used,
+ * @message: D-Bus connection and message received,
+ * @state: output string returned to client.
+ *
+ * Convert internal state to JSON string.
+ *
+ * Returns: zero on success, negative value on raised error.
+ **/
+int
+control_get_state (void           *data,
+		   NihDBusMessage  *message,
+		   char           **state)
+{
+	Session  *session;
+	uid_t     uid;
+	size_t    len;
+
+	nih_assert (message);
+	nih_assert (state);
+
+	uid = getuid ();
+
+	/* Get the relevant session */
+	session = session_from_dbus (NULL, message);
+
+	/* We don't want chroot sessions snooping outside their domain.
+	 *
+	 * Ideally, we'd allow them to query their own session, but the
+	 * current implementation doesn't lend itself to that.
+	 */
+	if (session && session->chroot) {
+		nih_warn (_("Ignoring state query from chroot session"));
+		return 0;
+	}
+
+	/* Disallow users from obtaining state details, 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 state"));
+		return -1;
+	}
+
+	if (state_to_string (state, &len) < 0)
+		goto error;
+
+	nih_ref (*state, message);
+
+	return 0;
+
+error:
+	nih_dbus_error_raise_printf (DBUS_ERROR_NO_MEMORY,
+			_("Out of Memory"));
+	return -1;
+}

=== modified file 'init/control.h'
--- init/control.h	2012-09-11 14:59:40 +0000
+++ init/control.h	2012-12-06 09:42:22 +0000
@@ -105,6 +105,11 @@
 int control_bus_release_name (void)
 	__attribute__ ((warn_unused_result));
 
+int control_get_state (void           *data,
+		   NihDBusMessage  *message,
+		   char           **state)
+	__attribute__ ((warn_unused_result));
+
 NIH_END_EXTERN
 
 #endif /* INIT_CONTROL_H */

=== modified file 'init/job_class.c'
--- init/job_class.c	2012-11-14 14:47:19 +0000
+++ init/job_class.c	2012-12-06 09:42:22 +0000
@@ -271,9 +271,7 @@
 
 	/* If we found an entry, ensure we only consider the appropriate session */
 	while (registered && registered->session != class->session)
-	{
 		registered = (JobClass *)nih_hash_search (job_classes, class->name, &registered->entry);
-	}
 
 	if (registered != best) {
 		if (registered)
@@ -314,9 +312,7 @@
 
 	/* If we found an entry, ensure we only consider the appropriate session */
 	while (registered && registered->session != class->session)
-	{
 		registered = (JobClass *)nih_hash_search (job_classes, class->name, &registered->entry);
-	}
 
 	if (registered == class) {
 		if (class != best) {
@@ -364,7 +360,7 @@
  * @class: new class to select.
  *
  * Adds @class to the hash table iff no existing entry of the
- * same name exists.
+ * same name exists for the same session.
  **/
 void
 job_class_add_safe (JobClass *class)
@@ -376,7 +372,11 @@
 
 	control_init ();
 
-	existing = (JobClass *)nih_hash_search (job_classes, class->name, NULL);
+	/* Ensure no existing class exists for the same session */
+	do {
+		existing = (JobClass *)nih_hash_search (job_classes,
+				class->name, existing ? &existing->entry : NULL);
+	} while (existing && existing->session != class->session);
 
 	nih_assert (! existing);
 
@@ -1592,6 +1592,15 @@
 	json = json_object_new_object ();
 	if (! json)
 		return NULL;
+	
+	/* XXX: user and chroot jobs are not currently supported
+	 * due to ConfSources not currently being serialised.
+	 */
+	if (class->session) {
+		nih_info ("WARNING: serialisation of user jobs and "
+			"chroot sessions not currently supported");
+		goto error;
+	}
 
 	session_index = session_get_index (class->session);
 	if (session_index < 0)
@@ -1797,6 +1806,7 @@
 {
 	json_object    *json_normalexit;
 	JobClass       *class = NULL;
+	Session        *session;
 	int             session_index = -1;
 	int             ret;
 	nih_local char *name = NULL;
@@ -1814,21 +1824,24 @@
 	if (session_index < 0)
 		goto error;
 
+	session = session_from_index (session_index);
+
+	/* XXX: user and chroot jobs are not currently supported
+	 * due to ConfSources not currently being serialised.
+	 */
+	if (session) {
+		nih_info ("WARNING: deserialisation of user jobs and "
+			"chroot sessions not currently supported");
+		goto error;
+	}
+
 	if (! state_get_json_string_var_strict (json, "name", NULL, name))
 		goto error;
 
-	class = NIH_MUST (job_class_new (NULL, name,
-	                                 session_from_index (session_index)));
+	class = job_class_new (NULL, name, session);
 	if (! class)
 		goto error;
 
-	if (class->session != NULL) {
-		nih_warn ("XXX: WARNING (%s:%d): deserialisation of "
-				"user jobs and chroot sessions not currently supported",
-				__func__, __LINE__);
-		goto error;
-	}
-
 	/* job_class_new() sets path */
 	if (! state_get_json_string_var_strict (json, "path", NULL, path))
 		goto error;
@@ -2002,7 +2015,9 @@
 	return class;
 
 error:
-	nih_free (class);
+	if (class)
+		nih_free (class);
+
 	return NULL;
 }
 
@@ -2043,19 +2058,12 @@
 			goto error;
 
 		class = job_class_deserialise (json_class);
+
+		/* For parity with the serialisation code, don't treat
+		 * errors as fatal for the entire deserialisation.
+		 */
 		if (! class)
-			goto error;
-
-		/* FIXME:
-		 *
-		 * If user sessions exist (ie 'initctl --session list'
-		 * has been run), we get this failure:
-		 *
-		 *             serialised path='/com/ubuntu/Upstart/jobs/1000/bang'
-		 * path set by job_class_new()='/com/ubuntu/Upstart/jobs/_/1000/bang'
-		 *
-		 */
-
+			continue;
 	}
 
 	return 0;

=== modified file 'init/session.h'
--- init/session.h	2012-06-29 16:19:33 +0000
+++ init/session.h	2012-12-06 09:42:22 +0000
@@ -39,7 +39,8 @@
  * with a chroot path)).
  *
  * This structure is used to identify collections of jobs
- * that share either a common @chroot and/or common @user.
+ * that share either a common @chroot and/or common @user. Note that
+ * @conf_path is unique across all sessions.
  *
  * Summary of Session values for different environments:
  *

=== modified file 'init/state.c'
--- init/state.c	2012-11-23 11:36:47 +0000
+++ init/state.c	2012-12-06 09:42:22 +0000
@@ -2,7 +2,7 @@
  *
  * state.c - serialisation and deserialisation support.
  *
- * Copyright © 2012 Canonical Ltd.
+ * Copyright  2012 Canonical Ltd.
  * Author: James Hunt <[e-mail]>.
  *
  * This program is free software; you can redistribute it and/or modify
@@ -48,7 +48,8 @@
 json_object *json_events = NULL;
 json_object *json_classes = NULL;
 
-extern int use_session_bus;
+extern int   use_session_bus;
+extern char *log_dir;
 
 /**
  * args_copy:
@@ -68,22 +69,17 @@
 int restart = FALSE;
 
 /* Prototypes for static functions */
-static json_object *
-state_serialise_blocked (const Blocked *blocked)
-	__attribute__ ((malloc, warn_unused_result));
-
-static Blocked *
-state_deserialise_blocked (void *parent, json_object *json, NihList *list)
-	__attribute__ ((malloc, warn_unused_result));
-
 static JobClass *
 state_index_to_job_class (int job_class_index)
 	__attribute__ ((warn_unused_result));
 
 static Job *
-state_get_job (const char *job_class, const char *job_name)
+state_get_job (const Session *session, const char *job_class,
+	       const char *job_name)
 	__attribute__ ((warn_unused_result));
 
+static void state_write_file (NihIoBuffer *buffer);
+
 /**
  * state_read:
  *
@@ -246,10 +242,59 @@
 	return 0;
 
 error:
+	/* Failed to reconstruct internal state so attempt to write
+	 * the JSON state data to a file to allow for manual post
+	 * re-exec analysis.
+	 */
+	if (buffer->len && log_dir)
+		state_write_file (buffer);
+
 	return -1;
 }
 
 /**
+ * state_write_file:
+ *
+ * @buffer: NihIoBuffer containing JSON data.
+ *
+ * Write JSON data contained in @buffer to STATE_FILE below log_dir.
+ *
+ * Failures are ignored since this is designed to be called in an error
+ * scenario anyway.
+ **/
+void
+state_write_file (NihIoBuffer *buffer)
+{
+	int              fd;
+	ssize_t          bytes;
+	nih_local char  *state_file = NULL;
+
+	nih_assert (buffer);
+
+	state_file = nih_sprintf (NULL, "%s/%s", log_dir, STATE_FILE);
+	if (! state_file)
+		return;
+
+	/* Note the very restrictive permissions */
+	fd = open (state_file, (O_CREAT|O_WRONLY|O_TRUNC), S_IRUSR);
+	if (fd < 0)
+		return;
+
+	while (TRUE) {
+		bytes = write (fd, buffer->buf, buffer->len);
+
+		if (! bytes)
+			break;
+		else if (bytes > 0)
+			nih_io_buffer_shrink (buffer, (size_t)bytes);
+		else if (bytes < 0 && errno != EINTR)
+			break;
+	}
+
+	close (fd);
+}
+
+/**
  * state_write_objects:
  *
  * @fd: file descriptor to write serialisation data on,
@@ -1139,6 +1184,12 @@
 		if (! class)
 			goto error;
 
+		/* XXX: user and chroot jobs are not currently supported
+		 * due to ConfSources not currently being serialised.
+		 */
+		if (class->session)
+			continue;
+
 		if (! state_get_json_var_full (json_class, "jobs", array, json_jobs))
 			goto error;
 
@@ -1164,7 +1215,7 @@
 				goto error;
 
 			/* lookup job */
-			job = state_get_job (class->name, job_name);
+			job = state_get_job (class->session, class->name, job_name);
 			if (! job)
 				goto error;
 
@@ -1200,11 +1251,12 @@
  *
  * Returns: JSON-serialised Blocked object, or NULL on error.
  **/
-static json_object *
+json_object *
 state_serialise_blocked (const Blocked *blocked)
 {
 	json_object  *json;
 	json_object  *json_blocked_data;
+	int           session_index;
 
 	nih_assert (blocked);
 
@@ -1228,6 +1280,18 @@
 						blocked->job->class->name))
 				goto error;
 
+			session_index = session_get_index (blocked->job->class->session);
+			if (session_index < 0)
+				goto error;
+
+			/* Encode parent classes session index to aid in
+			 * finding the correct job on deserialisation.
+			 */
+			if (! state_set_json_int_var (json_blocked_data,
+						"session",
+						session_index))
+				goto error;
+
 			if (! state_set_json_string_var (json_blocked_data,
 						"name",
 						blocked->job->name))
@@ -1389,7 +1453,7 @@
  *
  * Returns: new Blocked object, or NULL on error.
  **/
-static Blocked *
+Blocked *
 state_deserialise_blocked (void *parent, json_object *json,
 		NihList *list)
 {
@@ -1397,7 +1461,7 @@
 	Blocked         *blocked = NULL;
 	nih_local char  *blocked_type_str = NULL;
 	BlockedType      blocked_type;
-	int             ret;
+	int              ret;
 
 	nih_assert (parent);
 	nih_assert (json);
@@ -1421,6 +1485,8 @@
 			nih_local char  *job_name = NULL;
 			nih_local char  *job_class_name = NULL;
 			Job             *job;
+			Session         *session;
+			int              session_index;
 
 			if (! state_get_json_string_var_strict (json_blocked_data,
 						"name", NULL, job_name))
@@ -1430,7 +1496,19 @@
 						"class", NULL, job_class_name))
 				goto error;
 
-			job = state_get_job (job_class_name, job_name);
+			/* On error, assume NULL session since the likelihood
+			 * is we're upgrading from Upstart 1.6 that did not set
+			 * the 'session' JSON object.
+			 */
+			if (! state_get_json_int_var (json_blocked_data, "session", session_index))
+				session_index = 0;
+
+			if (session_index < 0)
+				goto error;
+
+			session = session_from_index (session_index);
+
+			job = state_get_job (session, job_class_name, job_name);
 			if (! job)
 				goto error;
 
@@ -1583,8 +1661,14 @@
 		if (! json_blocked)
 			goto error;
 
+
+		/* Don't error in this scenario to allow for possibility
+		 * that version of Upstart that performed the
+		 * serialisation did not correctly handle user and
+		 * chroot jobs.
+		 */
 		if (! state_deserialise_blocked (parent, json_blocked, list))
-			goto error;
+			;
 	}
 
 	return 0;
@@ -1625,6 +1709,7 @@
 /**
  * state_get_job:
  *
+ * @session: session of job class,
  * @job_class: name of job class,
  * @job_name: name of job instance.
  *
@@ -1635,15 +1720,21 @@
  * job not found.
  **/
 static Job *
-state_get_job (const char *job_class, const char *job_name)
+state_get_job (const Session *session,
+	       const char *job_class,
+	       const char *job_name)
 {
-	JobClass  *class;
+	JobClass  *class = NULL;
 	Job       *job;
 
 	nih_assert (job_class);
 	nih_assert (job_classes);
 
-	class = (JobClass *)nih_hash_lookup (job_classes, job_class);
+	do {
+		class = (JobClass *)nih_hash_search (job_classes,
+				job_class, class ? &class->entry : NULL);
+	} while (class && class->session != session);
+
 	if (! class)
 		goto error;
 

=== modified file 'init/state.h'
--- init/state.h	2012-11-14 14:47:19 +0000
+++ init/state.h	2012-12-06 09:42:22 +0000
@@ -367,6 +367,18 @@
 #define STATE_WAIT_SECS_ENV "UPSTART_STATE_WAIT_SECS"
 
 /**
+ * STATE_FILE:
+ *
+ * Name of file that is written below the job log directory if the
+ * newly re-exec'ed init instance failed to understand the JSON sent to
+ * it by the old instance.
+ *
+ * This could happen for example if the old instance generated invalid
+ * JSON, or JSON in an unexected format.
+ **/
+#define STATE_FILE "upstart.state"
+
+/**
  * state_get_timeout:
  *
  * @var: name of long integer var to set to timeout value.

=== added directory 'init/tests/data'
=== added file 'init/tests/data/upstart-1.6.json'
--- init/tests/data/upstart-1.6.json	1970-01-01 00:00:00 +0000
+++ init/tests/data/upstart-1.6.json	2012-12-06 09:42:22 +0000
@@ -0,0 +1,1 @@
+{ "sessions": [ ], "events": [ { "session": 0, "name": "Christmas", "fd": -1, "progress": "EVENT_PENDING", "failed": 0, "blockers": 0, "blocking": [ { "data": { "class": "bar", "name": "" }, "type": "BLOCKED_JOB" } ] } ], "job_classes": [ { "session": 0, "name": "bar", "path": "\/com\/ubuntu\/Upstart\/jobs\/bar", "instance": "", "jobs": [ { "name": "", "path": "\/com\/ubuntu\/Upstart\/jobs\/bar\/_", "goal": "JOB_STOP", "state": "JOB_WAITING", "env": [ ], "start_env": [ ], "stop_env": [ ], "fds": [ ], "pid": [ 0, 0, 0, 0, 0 ], "blocker": 0, "kill_process": "PROCESS_INVALID", "failed": 0, "failed_process": "PROCESS_INVALID", "exit_status": 0, "respawn_time": 0, "respawn_count": 0, "trace_forks": 0, "trace_state": "TRACE_NONE", "log": [ { "path": null }, { "path": null }, { "path": null }, { "path": null }, { "path": null } ] } ], "description": null, "author": null, "version": null, "env": [ ], "export": [ ], "emits": [ ], "process": [ { "script": 0, "command": null }, { "script": 0, "command": null }, { "script": 0, "command": null }, { "script": 0, "command": null }, { "script": 0, "command": null } ], "expect": "EXPECT_NONE", "task": 0, "kill_timeout": 5, "kill_signal": 15, "respawn": 0, "respawn_limit": 10, "respawn_interval": 5, "normalexit": [ ], "console": "CONSOLE_LOG", "umask": 18, "nice": 0, "oom_score_adj": 0, "limits": [ { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 }, { "rlim_cur": 0, "rlim_max": 0 } ], "chroot": null, "chdir": null, "setuid": null, "setgid": null, "deleted": 0, "debug": 0, "usage": null } ] }

=== modified file 'init/tests/test_job_process.c'
=== modified file 'init/tests/test_state.c'
--- init/tests/test_state.c	2012-11-14 14:47:19 +0000
+++ init/tests/test_state.c	2012-12-06 09:42:22 +0000
@@ -50,11 +50,27 @@
 #include "control.h"
 #include "test_util.h"
 
+#ifndef TEST_DATA_DIR
+#error ERROR: TEST_DATA_DIR not defined
+#endif
+
+/* These functions are 'protected'.
+ *
+ * The test code needs access, but they cannot be public due to
+ * header-file complications.
+ */
+json_object *
+state_serialise_blocked (const Blocked *blocked)
+	__attribute__ ((malloc, warn_unused_result));
+
+Blocked *
+state_deserialise_blocked (void *parent, json_object *json, NihList *list)
+	__attribute__ ((malloc, warn_unused_result));
 
 /**
  * AlreadySeen:
  *
- * Used to allow objects that directly or indirectly reference on
+ * Used to allow objects that directly or indirectly reference
  * another to be inspected and compared without causing infinite
  * recursion.
  *
@@ -127,6 +143,43 @@
 int blocked_diff (const Blocked *a, const Blocked *b, AlreadySeen seen)
 	__attribute__ ((warn_unused_result));
 
+void test_upstart1_6_upgrade (const char *conf_file, const char *path);
+
+/**
+ * TestDataFile:
+ *
+ * @conf_file: Name of ConfFile that must be created prior to
+ *  deserialising JSON data in @filename.
+ * @filename: basename of data file,
+ * @func: function to run to test @filename.
+ *
+ * Representation of a JSON data file used for ensuring that the current
+ * version of Upstart is able to deserialise all previous JSON data file
+ * format versions.
+ *
+ * @conf_file is required since we do not currently serialise ConfFile
+ * and ConfSource objects so these entities must be created immediately
+ * prior to attempting deserialisation.
+ *
+ * @func returns nothing so is expected to assert on any error.
+ **/
+typedef struct test_data_file {
+	char		*conf_file;
+	char		*filename;
+		  void (*func) (const char *conf_file, const char *path);
+} TestDataFile;
+
+/**
+ * test_data_files:
+ *
+ * Array of data files to test.
+ **/
+TestDataFile test_data_files[] = {
+	{ "bar", "upstart-1.6.json", test_upstart1_6_upgrade },
+
+	{ NULL, NULL, NULL }
+};
+
 /* Data with some embedded nulls */
 const    char test_data[] = {
 	'h', 'e', 'l', 'l', 'o', 0x0, 0x0, 0x0, ' ', 'w', 'o', 'r', 'l', 'd', '\n', '\r', '\0'
@@ -139,7 +192,7 @@
 int64_t  values64[] = {INT64_MIN, -1, 0, 1, INT64_MAX};
 const Process test_procs[] = {
 	{ 0, "echo hello" },
-	{ 1, "echo hello" },
+	{ 1, "echo hello" }
 };
 rlim_t rlimit_values[] = { 0, 1, 2, 3, 7, RLIM_INFINITY };
 
@@ -993,17 +1046,23 @@
 void
 test_blocking (void)
 {
-	nih_local char        *json_string = NULL;
-	ConfSource            *source = NULL;
-	ConfFile              *file;
-	JobClass              *class;
-	JobClass              *new_class;
-	Job                   *job;
-	Job                   *new_job;
-	Event                 *event;
-	Event                 *new_event;
-	Blocked               *blocked;
-	size_t                 len;
+	nih_local char          *json_string = NULL;
+	nih_local char          *parent_str = NULL;
+	ConfSource              *source = NULL;
+	ConfFile                *file;
+	JobClass                *class;
+	JobClass                *new_class;
+	Job                     *job;
+	Job                     *new_job;
+	Event                   *event;
+	Event                   *new_event;
+	Blocked                 *blocked;
+	Blocked                 *new_blocked;
+	NihList                  blocked_list;
+	size_t                   len;
+	json_object             *json_blocked;
+	Session                 *session;
+	Session                 *new_session;
 
 	conf_init ();
 	session_init ();
@@ -1014,6 +1073,278 @@
 	TEST_GROUP ("Blocked serialisation and deserialisation");
 
 	/*******************************/
+	TEST_FEATURE ("BLOCKED_JOB serialisation and deserialisation");
+
+	nih_list_init (&blocked_list);
+	TEST_LIST_EMPTY (&blocked_list);
+
+	source = conf_source_new (NULL, "/tmp/foo", CONF_JOB_DIR);
+	TEST_NE_P (source, NULL);
+
+	file = conf_file_new (source, "/tmp/foo/bar");
+	TEST_NE_P (file, NULL);
+	class = file->job = job_class_new (file, "bar", NULL);
+
+	TEST_NE_P (class, NULL);
+	TEST_HASH_EMPTY (job_classes);
+	TEST_TRUE (job_class_consider (class));
+	TEST_HASH_NOT_EMPTY (job_classes);
+
+	job = job_new (class, "");
+	TEST_NE_P (job, NULL);
+
+	parent_str = nih_strdup (NULL, "parent");
+	TEST_NE_P (parent_str, NULL);
+
+	blocked = blocked_new (NULL, BLOCKED_JOB, job);
+	TEST_NE_P (blocked, NULL);
+
+	json_blocked = state_serialise_blocked (blocked);
+	TEST_NE_P (json_blocked, NULL);
+
+	new_blocked = state_deserialise_blocked (parent_str,
+			json_blocked, &blocked_list);
+	TEST_NE_P (new_blocked, NULL);
+	TEST_LIST_NOT_EMPTY (&blocked_list);
+
+	assert0 (blocked_diff (blocked, new_blocked, ALREADY_SEEN_SET));
+
+	json_object_put (json_blocked);
+	nih_free (source);
+
+	/*******************************/
+	TEST_FEATURE ("BLOCKED_EVENT serialisation and deserialisation");
+
+	event = event_new (NULL, "event", NULL);
+	TEST_NE_P (event, NULL);
+
+	nih_list_init (&blocked_list);
+
+	source = conf_source_new (NULL, "/tmp/foo", CONF_JOB_DIR);
+	TEST_NE_P (source, NULL);
+
+	file = conf_file_new (source, "/tmp/foo/bar");
+	TEST_NE_P (file, NULL);
+	class = file->job = job_class_new (file, "bar", NULL);
+
+	TEST_NE_P (class, NULL);
+	TEST_HASH_EMPTY (job_classes);
+	TEST_TRUE (job_class_consider (class));
+	TEST_HASH_NOT_EMPTY (job_classes);
+
+	job = job_new (class, "");
+	TEST_NE_P (job, NULL);
+
+	TEST_LIST_EMPTY (&job->blocking);
+
+	blocked = blocked_new (NULL, BLOCKED_EVENT, event);
+	TEST_NE_P (blocked, NULL);
+
+	nih_list_add (&job->blocking, &blocked->entry);
+	TEST_LIST_NOT_EMPTY (&job->blocking);
+
+	event->blockers = 1;
+
+	parent_str = nih_strdup (NULL, "parent");
+	TEST_NE_P (parent_str, NULL);
+
+	json_blocked = state_serialise_blocked (blocked);
+	TEST_NE_P (json_blocked, NULL);
+
+	TEST_LIST_EMPTY (&blocked_list);
+	new_blocked = state_deserialise_blocked (parent_str,
+			json_blocked, &blocked_list);
+	TEST_NE_P (new_blocked, NULL);
+	TEST_LIST_NOT_EMPTY (&blocked_list);
+
+	assert0 (blocked_diff (blocked, new_blocked, ALREADY_SEEN_SET));
+
+	json_object_put (json_blocked);
+	nih_free (source);
+	nih_free (event);
+
+	/*******************************/
+	/* Test Upstart 1.6+ behaviour
+	 *
+	 * The data serialisation format for this version now includes
+	 * a "session" entry in the JSON for the blocked job.
+	 *
+	 * Note that this test is NOT testing whether a JobClass with an
+	 * associated Upstart session is handled correctly, it is merely
+	 * testing that a JobClass with the NULL session is handled
+	 * correctly!
+	 */
+	TEST_FEATURE ("BLOCKED_JOB with JSON session object");
+
+	TEST_LIST_EMPTY (sessions);
+	TEST_LIST_EMPTY (events);
+	TEST_LIST_EMPTY (conf_sources);
+	TEST_HASH_EMPTY (job_classes);
+
+	event = event_new (NULL, "Christmas", NULL);
+	TEST_NE_P (event, NULL);
+	TEST_LIST_EMPTY (&event->blocking);
+
+	source = conf_source_new (NULL, "/tmp/foo", CONF_JOB_DIR);
+	TEST_NE_P (source, NULL);
+
+	file = conf_file_new (source, "/tmp/foo/bar");
+	TEST_NE_P (file, NULL);
+	/* Create class with NULL session */
+	class = file->job = job_class_new (NULL, "bar", NULL);
+
+	TEST_NE_P (class, NULL);
+	TEST_HASH_EMPTY (job_classes);
+	TEST_TRUE (job_class_consider (class));
+	TEST_HASH_NOT_EMPTY (job_classes);
+
+	job = job_new (class, "");
+	TEST_NE_P (job, NULL);
+	TEST_HASH_NOT_EMPTY (class->instances);
+
+	blocked = blocked_new (event, BLOCKED_JOB, job);
+	TEST_NE_P (blocked, NULL);
+
+	nih_list_add (&event->blocking, &blocked->entry);
+	job->blocker = event;
+
+	TEST_LIST_NOT_EMPTY (&event->blocking);
+
+	assert0 (state_to_string (&json_string, &len));
+	TEST_GT (len, 0);
+
+	/* XXX: We don't remove the source as these are not
+	 * recreated on re-exec, so we'll re-use the existing one.
+	 */
+	nih_list_remove (&class->entry);
+	nih_list_remove (&event->entry);
+
+	TEST_LIST_EMPTY (events);
+	TEST_LIST_NOT_EMPTY (conf_sources);
+	TEST_HASH_EMPTY (job_classes);
+
+	assert0 (state_from_string (json_string));
+
+	TEST_LIST_NOT_EMPTY (conf_sources);
+	TEST_LIST_NOT_EMPTY (events);
+	TEST_HASH_NOT_EMPTY (job_classes);
+	TEST_LIST_EMPTY (sessions);
+
+	new_class = (JobClass *)nih_hash_lookup (job_classes, "bar");
+	TEST_NE_P (new_class, NULL);
+	nih_list_remove (&new_class->entry);
+
+	/* Upstart 1.6 can only deserialise the NULL session */
+	TEST_EQ_P (class->session, NULL);
+
+	new_event = (Event *)nih_list_remove (events->next);
+	TEST_LIST_EMPTY (events);
+	TEST_LIST_NOT_EMPTY (&new_event->blocking);
+	assert0 (event_diff (event, new_event, ALREADY_SEEN_SET));
+
+	nih_free (event);
+	nih_free (new_event);
+	nih_free (source);
+	nih_free (new_class);
+
+	TEST_HASH_EMPTY (job_classes);
+
+	TEST_LIST_EMPTY (sessions);
+	TEST_LIST_EMPTY (events);
+	TEST_LIST_EMPTY (conf_sources);
+	TEST_HASH_EMPTY (job_classes);
+
+	/*******************************/
+	/* We don't currently handle user+chroot jobs, so let's assert
+	 * that behaviour.
+	 */
+	TEST_FEATURE ("ensure BLOCKED_JOB with non-NULL session is ignored");
+
+	TEST_LIST_EMPTY (sessions);
+	TEST_LIST_EMPTY (events);
+	TEST_LIST_EMPTY (conf_sources);
+	TEST_HASH_EMPTY (job_classes);
+
+	session = session_new (NULL, "/my/session", getuid ());
+	TEST_NE_P (session, NULL);
+	session->conf_path = NIH_MUST (nih_strdup (session, "/lives/here"));
+	TEST_LIST_NOT_EMPTY (sessions);
+
+	/* We simulate a user job being blocked by a system event, hence
+	 * the session is not associated with the event.
+	 */
+	event = event_new (NULL, "Christmas", NULL);
+	TEST_NE_P (event, NULL);
+	TEST_LIST_EMPTY (&event->blocking);
+
+	source = conf_source_new (NULL, "/tmp/foo", CONF_JOB_DIR);
+	source->session = session;
+	TEST_NE_P (source, NULL);
+
+	file = conf_file_new (source, "/tmp/foo/bar");
+	TEST_NE_P (file, NULL);
+
+	/* Create class with non-NULL session, simulating a user job */
+	class = file->job = job_class_new (NULL, "bar", session);
+	TEST_NE_P (class, NULL);
+
+	TEST_HASH_EMPTY (job_classes);
+	TEST_TRUE (job_class_consider (class));
+	TEST_HASH_NOT_EMPTY (job_classes);
+
+	job = job_new (class, "");
+	TEST_NE_P (job, NULL);
+	TEST_HASH_NOT_EMPTY (class->instances);
+
+	blocked = blocked_new (event, BLOCKED_JOB, job);
+	TEST_NE_P (blocked, NULL);
+
+	nih_list_add (&event->blocking, &blocked->entry);
+	job->blocker = event;
+
+	TEST_LIST_NOT_EMPTY (&event->blocking);
+
+	assert0 (state_to_string (&json_string, &len));
+	TEST_GT (len, 0);
+
+	/* XXX: We don't remove the source as these are not
+	 * recreated on re-exec, so we'll re-use the existing one.
+	 */
+	nih_list_remove (&class->entry);
+	nih_free (event);
+	nih_list_remove (&session->entry);
+
+	TEST_LIST_EMPTY (events);
+	TEST_LIST_NOT_EMPTY (conf_sources);
+	TEST_HASH_EMPTY (job_classes);
+
+	assert0 (state_from_string (json_string));
+
+	TEST_LIST_NOT_EMPTY (conf_sources);
+	TEST_LIST_NOT_EMPTY (events);
+
+	/* We don't expect any job_classes since the serialised one
+	 * related to a user session.
+	 */
+	TEST_HASH_EMPTY (job_classes);
+
+	/* However, the session itself will exist */
+	TEST_LIST_NOT_EMPTY (sessions);
+
+	new_session = (Session *)nih_list_remove (sessions->next);
+
+	nih_free (session);
+	nih_free (new_session);
+	event = (Event *)nih_list_remove (events->next);
+	nih_free (event);
+	nih_free (source);
+
+	TEST_LIST_EMPTY (sessions);
+	TEST_LIST_EMPTY (events);
+	TEST_LIST_EMPTY (conf_sources);
+	TEST_HASH_EMPTY (job_classes);
+
+	/*******************************/
 	TEST_FEATURE ("event blocking a job");
 
 	TEST_LIST_EMPTY (sessions);
@@ -2608,6 +2939,141 @@
 	/*******************************/
 }
 
+/**
+ * test_upgrade:
+ * 
+ * Run tests that simulate an upgrade by attempting to deserialise an
+ * older version of the JSON data format than is currently used.
+ **/
+void
+test_upgrade (void)
+{
+	TestDataFile  *datafile;
+
+	TEST_GROUP ("upgrade tests");
+
+	for (datafile = test_data_files; datafile && datafile->filename; datafile++) {
+		nih_local char  *path = NULL;
+		nih_local char  *name = NULL;
+
+		nih_assert (datafile->func != NULL);
+
+		name = NIH_MUST (nih_sprintf (NULL, "with data file '%s'",
+					datafile->filename));
+		TEST_FEATURE (name);
+
+		path = NIH_MUST (nih_sprintf (NULL, "%s/%s",
+					TEST_DATA_DIR, datafile->filename));
+
+		datafile->func (datafile->conf_file, path);
+	}
+}
+
+/**
+ * test_upstart1_6_upgrade:
+ *
+ * @conf_file: name of ConfFile to create prior to running test,
+ * @path: full path to JSON data file to deserialise.
+ *
+ * Test for original Upstart 1.6 serialisation data format containing
+ * a blocked object that does not contain a 'session' element.
+ *
+ * Note that this test is NOT testing whether a JobClass with an
+ * associated Upstart session is handled correctly, it is merely
+ * testing that a JobClass with the NULL session encoded in the JSON
+ * is handled correctly.
+ **/
+void
+test_upstart1_6_upgrade (const char *conf_file, const char *path)
+{
+	nih_local char  *json_string = NULL;
+	Event           *event;
+	ConfSource      *source;
+	ConfFile        *file;
+	nih_local char  *conf_file_path = NULL;
+	struct stat      statbuf;
+	size_t           len;
+
+	nih_assert (conf_file);
+	nih_assert (path);
+
+	conf_init ();
+	session_init ();
+	event_init ();
+	control_init ();
+	job_class_init ();
+
+	TEST_LIST_EMPTY (sessions);
+	TEST_LIST_EMPTY (events);
+	TEST_LIST_EMPTY (conf_sources);
+	TEST_HASH_EMPTY (job_classes);
+
+	/* Check data file exists */
+	TEST_EQ (stat (path, &statbuf), 0);
+
+	json_string = nih_file_read (NULL, path, &len);
+	TEST_NE_P (json_string, NULL);
+
+	/* Create the ConfSource and ConfFile objects to simulate
+	 * Upstart reading /etc/init on startup. Required since we
+	 * don't currently serialise these objects.
+	 */
+	source = conf_source_new (NULL, "/tmp/foo", CONF_JOB_DIR);
+	TEST_NE_P (source, NULL);
+
+	conf_file_path = NIH_MUST (nih_sprintf (NULL, "%s/%s",
+				"/tmp/foo", conf_file));
+
+	file = conf_file_new (source, conf_file_path);
+	TEST_NE_P (file, NULL);
+
+	/* Recreate state from JSON data file */
+	assert0 (state_from_string (json_string));
+
+	TEST_LIST_NOT_EMPTY (conf_sources);
+	TEST_LIST_NOT_EMPTY (events);
+	TEST_HASH_NOT_EMPTY (job_classes);
+	TEST_LIST_EMPTY (sessions);
+
+	event = (Event *)nih_list_remove (events->next);
+	TEST_NE_P (event, NULL);
+	TEST_EQ_STR (event->name, "Christmas");
+
+	NIH_HASH_FOREACH (job_classes, iter) {
+		JobClass *class = (JobClass *)iter;
+
+		TEST_EQ_STR (class->name, "bar");
+		TEST_EQ_STR (class->path, "/com/ubuntu/Upstart/jobs/bar");
+		TEST_HASH_NOT_EMPTY (class->instances);
+
+		NIH_HASH_FOREACH (class->instances, iter2) {
+			Blocked        *blocked;
+			Job            *job = (Job *)iter2;
+			nih_local char *instance_path = NULL;
+
+			/* instance name */
+			TEST_EQ_STR (job->name, "");
+
+			instance_path = NIH_MUST (nih_sprintf (NULL, "%s/_", class->path));
+			TEST_EQ_STR (job->path, instance_path);
+
+			/* job is blocked by the event */
+			TEST_EQ (job->blocker, event);
+
+			/* First entry in list should be a Blocked
+			 * object pointing to the job.
+			 */
+			TEST_LIST_NOT_EMPTY (&event->blocking);
+			blocked = (Blocked *)(&event->blocking)->next;
+			TEST_EQ (blocked->type, BLOCKED_JOB);
+			TEST_EQ (blocked->job, job);
+		}
+	}
+
+	nih_free (event);
+	nih_free (conf_sources);
+}
+
 int
 main (int   argc,
       char *argv[])
@@ -2632,6 +3098,7 @@
 	test_log_serialise ();
 	test_job_serialise ();
 	test_job_class_serialise ();
+	test_upgrade ();
 
 	return 0;
 }