Merge lp:~jamesodhunt/upstart/bug-1269731 into lp:upstart

[James Hunt]

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

1592. By James Hunt on 2014-01-17

* scripts/pyupstart.py:
  - JobInstance:destroy(): Whitespace.
  - SessionInit::reexec(): Add a log message.
* scripts/tests/test_pyupstart_session_init.py:
  - TestSessionUpstart: Add pid to ps output.
  - TestSessionInitReExec::test_session_init_reexec():
    - Create an invalid job to ensure re-exec can handle it.
    - Use 'with' for handling re-exec exception.
* scripts/tests/test_pyupstart_system_init.py:
  - TestSystemInitReExec::test_pid1_reexec():
    - Create an invalid job to ensure re-exec can handle it.

[James Hunt (jamesodhunt)]

Note that the Python scenario tests will fail unless /sbin/init on the test system actually has the fix for bug 1269731. This will be much easier to test once lp:~jamesodhunt/upstart/fix-python-tests gets merged into lp:upstart since that allows the tests to run against different Upstart binaries.

[Colin Watson (cjwatson)]

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2014-01-15 11:51:18 +0000
+++ ChangeLog	2014-01-17 15:40:28 +0000
@@ -1,3 +1,26 @@
[phone]  James Hunt  <[e-mail]>
+
+	* init/conf.c:
+	  - conf_file_serialise(): Handle ConfFile's without an
+	    associated JobClass, resulting from an unparseable job configuration
+	    file (LP: #1269731).
+	  - conf_file_deserialise(): Comments.
+	* init/job_class.c: job_class_deserialise_all(): Comments.
+	* init/tests/test_conf.c: test_source_reload_file(): Add new tests:
+	  - "Invalid .conf file does not stop ConfFile being serialised".
+	  - "ConfFile with no JobClass can be deserialised".
+	* scripts/pyupstart.py:
+	  - JobInstance:destroy(): Whitespace.
+	  - SessionInit::reexec(): Add a log message.
+	* scripts/tests/test_pyupstart_session_init.py:
+	  - TestSessionUpstart: Add pid to ps output.
+	  - TestSessionInitReExec::test_session_init_reexec():
+	    - Create an invalid job to ensure re-exec can handle it.
+	    - Use 'with' for handling re-exec exception.
+	* scripts/tests/test_pyupstart_system_init.py:
+	  - TestSystemInitReExec::test_pid1_reexec():
+	    - Create an invalid job to ensure re-exec can handle it.
+
 2014-01-15  James Hunt  <[e-mail]>
 
 	* util/telinit.c: upstart_open(): Connect using private socket

=== modified file 'init/conf.c'
--- init/conf.c	2013-11-16 12:42:57 +0000
+++ init/conf.c	2014-01-17 15:40:28 +0000
@@ -1636,6 +1636,19 @@
 	if (! state_set_json_int_var_from_obj (json, file, flag))
 		goto error;
 
+	if (! file->job) {
+		/* File exists on disk but contains invalid
+		 * (unparseable) syntax, and hence no associated JobClass.
+		 * Thus, simply encode the ConfFile without a class.
+		 *
+		 * Deserialisation is handled automatically since
+		 * JobClasses are deserialised by directly iterating
+		 * through all JobClass'es found in the JSON. Here,
+		 * there simply won't be a JobClass to deserialise.
+		 */
+		goto out;
+	}
+
 	/*
 	 * Ignore the "best" JobClass associated with this ConfFile
 	 * (file->job) since it won't be serialised.
@@ -1681,6 +1694,7 @@
 
 	json_object_object_add (json, "job_class", json_job_class);
 
+out:
 	return json;
 
 error:
@@ -1717,8 +1731,8 @@
 		goto error;
 
 	/* Note that the associated JobClass is not handled at this
-	 * stage: it can't be the JobClasses haven't been deserialised
-	 * yet. As such, the ConfFile->JobClass link is dealt with by
+	 * stage: it can't be since the JobClasses haven't been deserialised
+	 * yet. As such, the ConfFile->JobClass link is dealt with in
 	 * job_class_deserialise_all().
 	 */
 	file->job = NULL;

=== modified file 'init/job_class.c'
--- init/job_class.c	2013-11-04 09:34:51 +0000
+++ init/job_class.c	2014-01-17 15:40:28 +0000
@@ -2429,6 +2429,9 @@
 		if (! state_check_json_type (json_class, object))
 			goto error;
 
+		/* Responsible for associating a JobClass with its
+		 * parent ConfFile.
+		 */
 		class = job_class_deserialise (json_class);
 
 		/* Either memory is low or -- more likely -- a JobClass

=== modified file 'init/tests/test_conf.c'
--- init/tests/test_conf.c	2013-06-25 10:13:12 +0000
+++ init/tests/test_conf.c	2014-01-17 15:40:28 +0000
@@ -3724,14 +3724,15 @@
 void
 test_source_reload_file (void)
 {
-	ConfSource *source;
-	ConfFile   *file, *old_file;
-	FILE       *f;
-	int         ret, fd, nfds;
-	char        dirname[PATH_MAX];
-	char        tmpname[PATH_MAX], filename[PATH_MAX];
-	fd_set      readfds, writefds, exceptfds;
-	NihError   *err;
+	ConfSource  *source;
+	ConfFile    *file, *old_file;
+	FILE        *f;
+	int          ret, fd, nfds;
+	char         dirname[PATH_MAX];
+	char         tmpname[PATH_MAX], filename[PATH_MAX];
+	fd_set       readfds, writefds, exceptfds;
+	NihError    *err;
+	json_object *json;
 
 	TEST_FUNCTION_FEATURE ("conf_source_reload",
 			       "with configuration file");
@@ -4500,12 +4501,76 @@
 	strcat (filename, "/bar.conf");
 	unlink (filename);
 
+	/* Re-enable inotify */
+	assert0 (unsetenv ("INOTIFY_DISABLE"));
+
+	TEST_FEATURE ("Invalid .conf file does not stop ConfFile being serialised");
+	conf_init ();
+	TEST_LIST_EMPTY (conf_sources);
+
+	f = fopen (filename, "w");
+	/* Create an invalid job by adding an invalid stanza */
+	fprintf (f, "invalid\n");
+	fclose (f);
+
+	source = conf_source_new (NULL, filename, CONF_FILE);
+	TEST_NE_P (source, NULL);
+	TEST_LIST_NOT_EMPTY (conf_sources);
+	TEST_HASH_EMPTY (source->files);
+
+	file = conf_file_new (source, "/path/to/file");
+	TEST_NE_P (file, NULL);
+	TEST_HASH_NOT_EMPTY (source->files);
+
+	/* Initially, a ConfFile has no associated JobClass */
+	TEST_EQ_P (file->job, NULL);
+
+	/* Normally, this would create a JobClass and associate it with
+	 * its parent ConfFile, but that doesn't happen when the on-disk
+	 * job configuration file is invalid.
+	 */
+	ret = conf_source_reload (source);
+
+	/* Although the on-disk file is invalid, there is no error here
+	 * since it's already been handled by conf_reload_path() (which
+	 * displays an error message with details of how the job
+	 * configuration file is invalid).
+	 */
+	TEST_EQ (ret, 0);
+
+	/* We know the job was invalid * by the fact that the ConfFile
+	 * still has no associated JobClass.
+	 */
+	TEST_EQ (file->job, NULL);
+
+	/* See if we can serialise the ConfFile without an associated
+	 * JobClass.
+	 */
+	json = conf_file_serialise (file);
+	TEST_NE_P (json, NULL);
+
+	/* Test there is no JobClass in the JSON */
+	TEST_EQ_P (json_object_object_get (json, "job_class"), NULL);
+
+	TEST_FEATURE ("ConfFile with no JobClass can be deserialised");
+
+	nih_free (source);
+
+	source = conf_source_new (NULL, filename, CONF_FILE);
+	TEST_NE_P (source, NULL);
+	TEST_LIST_NOT_EMPTY (conf_sources);
+	TEST_HASH_EMPTY (source->files);
+
+	file = conf_file_deserialise (source, json);
+	TEST_NE_P (file, NULL);
+
+	nih_free (source);
+
+	json_object_put (json);
+
+	unlink (filename);
 	rmdir (dirname);
-
 	nih_log_set_priority (NIH_LOG_MESSAGE);
-
-	/* Re-enable inotify */
-	assert0 (unsetenv ("INOTIFY_DISABLE"));
 }
 
 

=== modified file 'scripts/pyupstart.py'
--- scripts/pyupstart.py	2013-09-12 10:56:22 +0000
+++ scripts/pyupstart.py	2014-01-17 15:40:28 +0000
@@ -925,8 +925,8 @@
         """
         Stop the instance and cleanup.
 
-	Note: If the instance specified retain when created, this will
-	be a NOP.
+        Note: If the instance specified retain when created, this will
+        be a NOP.
         """
         if not self.job.retain:
             self.stop()
@@ -1119,4 +1119,5 @@
         if not self.proxy:
             raise UpstartException('Not yet connected')
 
+        self.logger.debug("Restarting Session Init")
         self.proxy.Restart()

=== modified file 'scripts/tests/test_pyupstart_session_init.py'
--- scripts/tests/test_pyupstart_session_init.py	2013-11-03 00:28:07 +0000
+++ scripts/tests/test_pyupstart_session_init.py	2014-01-17 15:40:28 +0000
@@ -51,7 +51,7 @@
     FILE_BRIDGE_CONF = 'upstart-file-bridge.conf'
     REEXEC_CONF = 're-exec.conf'
 
-    PSCMD_FMT = 'ps --no-headers -p %d -o comm,args'
+    PSCMD_FMT = 'ps --no-headers -p %d -o pid,comm,args'
 
     def setUp(self):
 
@@ -343,6 +343,16 @@
         version = self.upstart.version()
         self.assertTrue(version)
 
+        # Create an invalid job to ensure this causes no problems for
+        # the re-exec. Note that we cannot use job_create() since
+        # that validates the syntax of the .conf file).
+        #
+        # We create this file before any other to allow time for Upstart
+        # to _attempt to parse it_ by the time the re-exec is initiated.
+        invalid_conf = "{}/invalid.conf".format(self.upstart.test_dir)
+        with open(invalid_conf, 'w', encoding='utf-8') as fh:
+            print("invalid", file=fh)
+
         # create a job and start it, marking it such that the .conf file
         # will be retained when object becomes unusable (after re-exec).
         job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)
@@ -360,7 +370,8 @@
         # Trigger re-exec and catch the D-Bus exception resulting
         # from disconnection from Session Init when it severs client
         # connections.
-        self.assertRaises(dbus.exceptions.DBusException, self.upstart.reexec)
+        with self.assertRaises(dbus.exceptions.DBusException):
+            self.upstart.reexec()
 
         os.kill(self.upstart.pid, 0)
 
@@ -411,6 +422,8 @@
         with self.assertRaises(ProcessLookupError):
             os.kill(pid, 0)
 
+        os.remove(invalid_conf)
+
         self.stop_session_init()
 
     def test_session_init_reexec_when_pid1_does(self):

=== modified file 'scripts/tests/test_pyupstart_system_init.py'
--- scripts/tests/test_pyupstart_system_init.py	2013-09-12 10:56:22 +0000
+++ scripts/tests/test_pyupstart_system_init.py	2014-01-17 15:40:28 +0000
@@ -61,6 +61,16 @@
       version = self.upstart.version()
       self.assertTrue(version)
 
+      # Create an invalid job to ensure this causes no problems for
+      # the re-exec. Note that we cannot use job_create() since
+      # that validates the syntax of the .conf file).
+      #
+      # We create this file before any other to allow time for Upstart
+      # to _attempt to parse it_ by the time the re-exec is initiated.
+      invalid_conf = "{}/invalid.conf".format(self.upstart.test_dir)
+      with open(invalid_conf, 'w', encoding='utf-8') as fh:
+          print("invalid", file=fh)
+
       # create a job and start it, marking it such that the .conf file
       # will be retained when object becomes unusable (after re-exec).
       job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)
@@ -118,6 +128,8 @@
       with self.assertRaises(ProcessLookupError):
           os.kill(pid, 0)
 
+      os.remove(invalid_conf)
+
       # Clean up
       self.upstart.destroy()