upstart

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

  1. upstart-fix-respawn
  2. Merge into trunk
Proposed by Stéphane Graber
Status: Merged
Merged at revision: 1498
Proposed branch: lp:~stgraber/upstart/upstart-fix-respawn
Merge into: lp:upstart
Diff against target: 143 lines (+21/-10)
2 files modified
init/job_process.c (+0/-6)
init/tests/test_job_process.c (+21/-4)
To merge this branch: bzr merge lp:~stgraber/upstart/upstart-fix-respawn
Reviewer Review Type Date Requested Status
Steve Langasek Needs Fixing
Review via email: mp+172420@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Steve Langasek (vorlon) wrote :

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 updated
1499. By Stéphane Graber

Add some tests

Revision history for this message
Steve Langasek (vorlon) wrote :

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 updated
1500. By Stéphane Graber

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'init/job_process.c'
2--- init/job_process.c 2013-05-31 15:34:00 +0000
3+++ init/job_process.c 2013-07-01 23:24:24 +0000
4@@ -261,12 +261,6 @@
5 NIH_MUST (nih_str_array_addp (&argv, NULL,
6 &argc, cmd));
7 }
8-
9- /* At the end, always set proc->script to TRUE, even if the user didn't
10- * explicitly set it (when using shell variables). That way tests
11- * can reliably check for shell-specific behaviour.
12- */
13- proc->script = TRUE;
14 } else {
15 /* Split the command on whitespace to produce a list of
16 * arguments that we can exec directly.
17
18=== modified file 'init/tests/test_job_process.c'
19--- init/tests/test_job_process.c 2013-06-25 09:19:05 +0000
20+++ init/tests/test_job_process.c 2013-07-01 23:24:24 +0000
21@@ -83,6 +83,14 @@
22 #define MAX_ITERATIONS 5
23
24 /**
25+ * SHELL_CHARS:
26+ *
27+ * This is the list of characters that, if encountered in a process, cause
28+ * it to always be run with a shell.
29+ **/
30+#define SHELL_CHARS "~`!$^&*()=|\\{}[];\"'<>?"
31+
32+/**
33 * CHECK_FILE_EQ:
34 *
35 * @_file: FILE to read from,
36@@ -501,6 +509,7 @@
37
38 waitpid (job->pid[PROCESS_MAIN], NULL, 0);
39 TEST_EQ (stat (filename, &statbuf), 0);
40+ TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
41
42 /* Filename should contain the pid */
43 output = fopen (filename, "r");
44@@ -677,7 +686,7 @@
45 output = fopen (filename, "r");
46 TEST_FILE_EQ (output, "BAR=BAZ\n");
47 TEST_FILE_EQ (output, "FOO=BAR\n");
48- if (job->class->process[PROCESS_MAIN]->script)
49+ if (job->class->process[PROCESS_MAIN]->script || strpbrk (job->class->process[PROCESS_MAIN]->command, SHELL_CHARS))
50 TEST_FILE_EQ (output, "PWD=/\n");
51 TEST_FILE_EQ (output, "UPSTART_INSTANCE=\n");
52 TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
53@@ -733,7 +742,7 @@
54 output = fopen (filename, "r");
55 TEST_FILE_EQ (output, "BAR=BAZ\n");
56 TEST_FILE_EQ (output, "FOO=BAR\n");
57- if (job->class->process[PROCESS_MAIN]->script)
58+ if (job->class->process[PROCESS_MAIN]->script || strpbrk (job->class->process[PROCESS_MAIN]->command, SHELL_CHARS))
59 TEST_FILE_EQ (output, "PWD=/\n");
60 TEST_FILE_EQ (output, "UPSTART_INSTANCE=foo\n");
61 TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
62@@ -791,7 +800,7 @@
63 TEST_FILE_EQ (output, "BAR=BAZ\n");
64 TEST_FILE_EQ (output, "CRACKLE=FIZZ\n");
65 TEST_FILE_EQ (output, "FOO=SMACK\n");
66- if (job->class->process[PROCESS_PRE_STOP]->script)
67+ if (job->class->process[PROCESS_PRE_STOP]->script || strpbrk (job->class->process[PROCESS_PRE_STOP]->command, SHELL_CHARS))
68 TEST_FILE_EQ (output, "PWD=/\n");
69 TEST_FILE_EQ (output, "UPSTART_INSTANCE=\n");
70 TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
71@@ -849,7 +858,7 @@
72 TEST_FILE_EQ (output, "BAR=BAZ\n");
73 TEST_FILE_EQ (output, "CRACKLE=FIZZ\n");
74 TEST_FILE_EQ (output, "FOO=SMACK\n");
75- if (job->class->process[PROCESS_POST_STOP]->script)
76+ if (job->class->process[PROCESS_POST_STOP]->script || strpbrk (job->class->process[PROCESS_POST_STOP]->command, SHELL_CHARS))
77 TEST_FILE_EQ (output, "PWD=/\n");
78 TEST_FILE_EQ (output, "UPSTART_INSTANCE=\n");
79 TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
80@@ -6069,6 +6078,7 @@
81 TEST_EQ (job->failed, FALSE);
82 TEST_EQ (job->failed_process, PROCESS_INVALID);
83 TEST_EQ (job->exit_status, 0);
84+ TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
85
86 TEST_FILE_EQ (output, ("test: test main process (1) "
87 "terminated with status 1\n"));
88@@ -6156,6 +6166,7 @@
89 TEST_EQ (job->failed, FALSE);
90 TEST_EQ (job->failed_process, PROCESS_INVALID);
91 TEST_EQ (job->exit_status, 0);
92+ TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
93
94 TEST_FILE_EQ (output, ("test: test main process (1) "
95 "terminated with status 1\n"));
96@@ -6240,6 +6251,7 @@
97 TEST_EQ (job->failed, TRUE);
98 TEST_EQ (job->failed_process, PROCESS_INVALID);
99 TEST_EQ (job->exit_status, 0);
100+ TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
101
102 TEST_FILE_EQ (output, ("test: test respawning too fast, "
103 "stopped\n"));
104@@ -6313,6 +6325,7 @@
105 TEST_EQ (job->failed, FALSE);
106 TEST_EQ (job->failed_process, PROCESS_INVALID);
107 TEST_EQ (job->exit_status, 0);
108+ TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
109
110 nih_free (job);
111 }
112@@ -6390,6 +6403,7 @@
113 TEST_EQ (job->failed, FALSE);
114 TEST_EQ (job->failed_process, PROCESS_INVALID);
115 TEST_EQ (job->exit_status, 0);
116+ TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
117
118 TEST_FILE_EQ (output, ("test: test main process ended, "
119 "respawning\n"));
120@@ -6462,6 +6476,7 @@
121 TEST_EQ (job->failed, FALSE);
122 TEST_EQ (job->failed_process, PROCESS_INVALID);
123 TEST_EQ (job->exit_status, 0);
124+ TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
125
126 nih_free (job);
127 }
128@@ -7423,6 +7438,7 @@
129 TEST_EQ (job->failed, FALSE);
130 TEST_EQ (job->failed_process, PROCESS_INVALID);
131 TEST_EQ (job->exit_status, 0);
132+ TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
133
134 TEST_FILE_EQ (output, ("test: test main process ended, "
135 "respawning\n"));
136@@ -7739,6 +7755,7 @@
137 TEST_EQ (job->failed, FALSE);
138 TEST_EQ (job->failed_process, PROCESS_INVALID);
139 TEST_EQ (job->exit_status, 0);
140+ TEST_EQ (job->class->process[PROCESS_MAIN]->script, FALSE);
141
142 TEST_FILE_EQ (output, ("test: test main process ended, "
143 "respawning\n"));

Subscribers

People subscribed via source and target branches