Merge lp:~stgraber/upstart/upstart-fix-respawn into lp:upstart

[Stéphane Graber]

This fixes a regression recently found when working on Ubuntu Touch.

The problem is that if a job has the respawn flag and doesn't contain any of the standard shell characters in its exec line, upstart will still set job->script to TRUE once the initial run has succeeded. This mean that on first respawn of the job, it'll end up forking one extra time and confuse upstart.

The change that introduced this was done to fix a case where tests would fail when running from a directory containing a shell character in its name (~test for example). The assumption at the time being that ->script wasn't persistent (clearly wrong) and that it wouldn't hurt to set it after the job has been started in order to simplify testing.

The fix is to revert the change to upstart itself and instead include the same shell characters list in the testsuite and have the testsuite check against it just like init itself does.

[Steve Langasek (vorlon)]

As discussed on IRC, this change should include an affirmative test case to ensure the correct values of ->script on respawn, to avoid this bug being reintroduced later.

review: Needs Fixing

[lp:~stgraber/upstart/upstart-fix-respawn]

1499. By Stéphane Graber on 2013-07-01

Add some tests

[Steve Langasek (vorlon)]

Locally applying the changes to init/tests/test_job_process.c without the changes to the code, the test suite still passes; so the added test case clearly doesn't actually check for the bug in question and avoid regressing this particular code path.

Needs a test case involving a job with both an exec line and shell chars.

review: Needs Fixing

[lp:~stgraber/upstart/upstart-fix-respawn]

1500. By Stéphane Graber on 2013-07-01

Add check for ->script value when the exec line contains shell characters in one of the command arguments.

=== modified file 'init/job_process.c'
--- init/job_process.c	2013-05-31 15:34:00 +0000
+++ init/job_process.c	2013-07-01 23:24:24 +0000
@@ -261,12 +261,6 @@
 			NIH_MUST (nih_str_array_addp (&argv, NULL,
 						      &argc, cmd));
 		}
-
-		/* At the end, always set proc->script to TRUE, even if the user didn't
-		 * explicitly set it (when using shell variables). That way tests
-		 * can reliably check for shell-specific behaviour.
-		 */
-		proc->script = TRUE;
 	} else {
 		/* Split the command on whitespace to produce a list of
 		 * arguments that we can exec directly.
 
=== modified file 'init/tests/test_job_process.c'
--- init/tests/test_job_process.c	2013-06-25 09:19:05 +0000
+++ init/tests/test_job_process.c	2013-07-01 23:24:24 +0000
@@ -83,6 +83,14 @@
 #define MAX_ITERATIONS            5
 
 /**
+ * SHELL_CHARS:
+ *
+ * This is the list of characters that, if encountered in a process, cause
+ * it to always be run with a shell.
+ **/
+#define SHELL_CHARS "~`!$^&*()=|\\{}[];\"'<>?"
+
+/**
  * CHECK_FILE_EQ:
  *
  * @_file: FILE to read from,
@@ -501,6 +509,7 @@
 
 		waitpid (job->pid[PROCESS_MAIN], NULL, 0);
 		TEST_EQ (stat (filename, &statbuf), 0);
+		TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
 
 		/* Filename should contain the pid */
 		output = fopen (filename, "r");
@@ -677,7 +686,7 @@
 		output = fopen (filename, "r");
 		TEST_FILE_EQ (output, "BAR=BAZ\n");
 		TEST_FILE_EQ (output, "FOO=BAR\n");
-		if (job->class->process[PROCESS_MAIN]->script)
+		if (job->class->process[PROCESS_MAIN]->script || strpbrk (job->class->process[PROCESS_MAIN]->command, SHELL_CHARS))
 			TEST_FILE_EQ (output, "PWD=/\n");
 		TEST_FILE_EQ (output, "UPSTART_INSTANCE=\n");
 		TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
@@ -733,7 +742,7 @@
 		output = fopen (filename, "r");
 		TEST_FILE_EQ (output, "BAR=BAZ\n");
 		TEST_FILE_EQ (output, "FOO=BAR\n");
-		if (job->class->process[PROCESS_MAIN]->script)
+		if (job->class->process[PROCESS_MAIN]->script || strpbrk (job->class->process[PROCESS_MAIN]->command, SHELL_CHARS))
 			TEST_FILE_EQ (output, "PWD=/\n");
 		TEST_FILE_EQ (output, "UPSTART_INSTANCE=foo\n");
 		TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
@@ -791,7 +800,7 @@
 		TEST_FILE_EQ (output, "BAR=BAZ\n");
 		TEST_FILE_EQ (output, "CRACKLE=FIZZ\n");
 		TEST_FILE_EQ (output, "FOO=SMACK\n");
-		if (job->class->process[PROCESS_PRE_STOP]->script)
+		if (job->class->process[PROCESS_PRE_STOP]->script || strpbrk (job->class->process[PROCESS_PRE_STOP]->command, SHELL_CHARS))
 			TEST_FILE_EQ (output, "PWD=/\n");
 		TEST_FILE_EQ (output, "UPSTART_INSTANCE=\n");
 		TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
@@ -849,7 +858,7 @@
 		TEST_FILE_EQ (output, "BAR=BAZ\n");
 		TEST_FILE_EQ (output, "CRACKLE=FIZZ\n");
 		TEST_FILE_EQ (output, "FOO=SMACK\n");
-		if (job->class->process[PROCESS_POST_STOP]->script)
+		if (job->class->process[PROCESS_POST_STOP]->script || strpbrk (job->class->process[PROCESS_POST_STOP]->command, SHELL_CHARS))
 			TEST_FILE_EQ (output, "PWD=/\n");
 		TEST_FILE_EQ (output, "UPSTART_INSTANCE=\n");
 		TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
@@ -6069,6 +6078,7 @@
 		TEST_EQ (job->failed, FALSE);
 		TEST_EQ (job->failed_process, PROCESS_INVALID);
 		TEST_EQ (job->exit_status, 0);
+		TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
 
 		TEST_FILE_EQ (output, ("test: test main process (1) "
 				       "terminated with status 1\n"));
@@ -6156,6 +6166,7 @@
 		TEST_EQ (job->failed, FALSE);
 		TEST_EQ (job->failed_process, PROCESS_INVALID);
 		TEST_EQ (job->exit_status, 0);
+		TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
 
 		TEST_FILE_EQ (output, ("test: test main process (1) "
 				       "terminated with status 1\n"));
@@ -6240,6 +6251,7 @@
 		TEST_EQ (job->failed, TRUE);
 		TEST_EQ (job->failed_process, PROCESS_INVALID);
 		TEST_EQ (job->exit_status, 0);
+		TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
 
 		TEST_FILE_EQ (output, ("test: test respawning too fast, "
 				       "stopped\n"));
@@ -6313,6 +6325,7 @@
 		TEST_EQ (job->failed, FALSE);
 		TEST_EQ (job->failed_process, PROCESS_INVALID);
 		TEST_EQ (job->exit_status, 0);
+		TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
 
 		nih_free (job);
 	}
@@ -6390,6 +6403,7 @@
 		TEST_EQ (job->failed, FALSE);
 		TEST_EQ (job->failed_process, PROCESS_INVALID);
 		TEST_EQ (job->exit_status, 0);
+		TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
 
 		TEST_FILE_EQ (output, ("test: test main process ended, "
 				       "respawning\n"));
@@ -6462,6 +6476,7 @@
 		TEST_EQ (job->failed, FALSE);
 		TEST_EQ (job->failed_process, PROCESS_INVALID);
 		TEST_EQ (job->exit_status, 0);
+		TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
 
 		nih_free (job);
 	}
@@ -7423,6 +7438,7 @@
 		TEST_EQ (job->failed, FALSE);
 		TEST_EQ (job->failed_process, PROCESS_INVALID);
 		TEST_EQ (job->exit_status, 0);
+		TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
 
 		TEST_FILE_EQ (output, ("test: test main process ended, "
 				       "respawning\n"));
@@ -7739,6 +7755,7 @@
 		TEST_EQ (job->failed, FALSE);
 		TEST_EQ (job->failed_process, PROCESS_INVALID);
 		TEST_EQ (job->exit_status, 0);
+		TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
 
 		TEST_FILE_EQ (output, ("test: test main process ended, "
 				       "respawning\n"));