Merge lp:~jamesodhunt/upstart/bug-1302117-remerged into lp:upstart

Proposed by James Hunt on 2014-07-01
Status: Merged
Merged at revision: 1642
Proposed branch: lp:~jamesodhunt/upstart/bug-1302117-remerged
Merge into: lp:upstart
Diff against target: 242 lines (+119/-4)
4 files modified
ChangeLog (+14/-0)
init/log.c (+12/-0)
init/tests/test_job_process.c (+17/-4)
util/tests/test_initctl.c (+76/-0)
To merge this branch: bzr merge lp:~jamesodhunt/upstart/bug-1302117-remerged
Reviewer Review Type Date Requested Status
Dimitri John Ledkov 2014-07-01 Approve on 2014-07-07
Review via email: mp+225177@code.launchpad.net

Description of the change

Just prior to re-exec Upstart serialises all the internal objects. As a result, log_serialise() gets called for reach job process. If a job process has unflushed data, Upstart attempts to persist it prior to the re-exec. However, the entails calling log_file_open() *in the parent*, which was setting the umask to ensure the log file is created with a known permission.

The fix was to save the umask, set it, write the log file, then restore the umask.

Also, added a new test to avoid regression.

To post a comment you must log in.
Dimitri John Ledkov (xnox) wrote :

This merge proposal is exactly the same as the previous one that was reverted.
The revert reason was failing test_job_process test below:

not ok 139 - with exec call by process after fork
 wrong value for rmdir (dirname), expected 0 got -1
 at tests/test_job_process.c:8767 (test_handler).

It still fails with this merge proposal.

review: Needs Fixing
1642. By James Hunt on 2014-07-03

* Sync with lp:upstart.

1643. By James Hunt on 2014-07-07

* init/tests/test_job_process.c: test_handler():
  - Remove logs generated by PROCESS_MAIN. This wasn't necessary before
    due to the manifestation of LP: #1302117 not restoring the correct
    umask.

Dimitri John Ledkov (xnox) wrote :

Looks good to me, with below mentioned included cleanups.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ChangeLog'
2--- ChangeLog 2014-07-03 08:59:30 +0000
3+++ ChangeLog 2014-07-07 10:15:49 +0000
4@@ -1,3 +1,10 @@
5+2014-07-07 James Hunt <james.hunt@ubuntu.com>
6+
7+ * init/tests/test_job_process.c: test_handler():
8+ - Remove logs generated by PROCESS_MAIN. This wasn't necessary before
9+ due to the manifestation of LP: #1302117 not restoring the correct
10+ umask.
11+
12 2014-07-03 James Hunt <james.hunt@ubuntu.com>
13
14 * init/job_process.c:
15@@ -97,6 +104,13 @@
16 - Clarify default behaviour of 'respawn' stanza.
17 - Add missing 'respawn limit unlimited' details.
18
19+2014-04-10 James Hunt <james.hunt@ubuntu.com>
20+
21+ * init/log.c: log_file_open(): Restore umask after opening log file
22+ (LP: #1302117).
23+ * util/tests/test_initctl.c: test_reexec(): New test:
24+ - "ensure re-exec does not disrupt umask".
25+
26 2014-03-11 James Hunt <james.hunt@ubuntu.com>
27
28 * NEWS: Release 1.12.1
29
30=== modified file 'init/log.c'
31--- init/log.c 2014-06-04 17:37:59 +0000
32+++ init/log.c 2014-07-07 10:15:49 +0000
33@@ -405,6 +405,9 @@
34 struct stat statbuf;
35 int ret = -1;
36 int mode = LOG_DEFAULT_MODE;
37+#if 1
38+ mode_t old;
39+#endif
40 int flags = (O_CREAT | O_APPEND | O_WRONLY |
41 O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK);
42
43@@ -439,7 +442,11 @@
44 nih_assert (log->fd == -1);
45
46 /* Impose some sane defaults. */
47+#if 1
48+ old = umask (LOG_DEFAULT_UMASK);
49+#else
50 umask (LOG_DEFAULT_UMASK);
51+#endif
52
53 /* Non-blocking to avoid holding up the main loop. Without
54 * this, we'd probably need to spawn a thread to handle
55@@ -447,6 +454,11 @@
56 */
57 log->fd = open (log->path, flags, mode);
58
59+ /* Restore */
60+#if 1
61+ umask (old);
62+#endif
63+
64 log->open_errno = errno;
65
66 /* Open may have failed due to path being unaccessible
67
68=== modified file 'init/tests/test_job_process.c'
69--- init/tests/test_job_process.c 2014-06-05 17:57:29 +0000
70+++ init/tests/test_job_process.c 2014-07-07 10:15:49 +0000
71@@ -1,11 +1,8 @@
72-/*
73- * FIXME: test_job_process has been left behind
74- */
75 /* upstart
76 *
77 * test_job_process.c - test suite for init/job_process.c
78 *
79- * Copyright 2011 Canonical Ltd.
80+ * Copyright © 2011-2014 Canonical Ltd.
81 * Author: Scott James Remnant <scott@netsplit.com>.
82 *
83 * This program is free software; you can redistribute it and/or modify
84@@ -5489,11 +5486,16 @@
85 unsigned long data;
86 struct timespec now;
87 char dirname[PATH_MAX];
88+ nih_local char *logfile = NULL;
89+ int fds[2] = { -1, -1};
90
91 TEST_FILENAME (dirname);
92 TEST_EQ (mkdir (dirname, 0755), 0);
93 TEST_EQ (setenv ("UPSTART_LOGDIR", dirname, 1), 0);
94
95+ logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s.log",
96+ dirname, "test"));
97+
98 TEST_FUNCTION ("job_process_handler");
99 program_name = "test";
100 output = tmpfile ();
101@@ -5763,6 +5765,9 @@
102 class->process[PROCESS_PRE_START] = process_new (class);
103 class->process[PROCESS_PRE_START]->command = "echo";
104
105+ assert0 (pipe (fds));
106+ close (fds[1]);
107+
108 TEST_ALLOC_FAIL {
109 TEST_ALLOC_SAFE {
110 job = job_new (class, "");
111@@ -5770,6 +5775,9 @@
112 blocked = blocked_new (job, BLOCKED_EVENT, event);
113 event_block (event);
114 nih_list_add (&job->blocking, &blocked->entry);
115+
116+ job->process_data[PROCESS_PRE_START] = NIH_MUST (
117+ job_process_data_new (job->process_data, job, PROCESS_PRE_START, fds[0]));
118 }
119
120 job->goal = JOB_START;
121@@ -5812,9 +5820,12 @@
122 nih_free (job);
123 }
124
125+ close (fds[0]);
126+
127 nih_free (class->process[PROCESS_PRE_START]);
128 class->process[PROCESS_PRE_START] = NULL;
129
130+ unlink (logfile);
131
132 /* Check that we can handle a failing pre-start process of the job,
133 * which changes the goal to stop and transitions a state change in
134@@ -5890,6 +5901,7 @@
135 nih_free (class->process[PROCESS_PRE_START]);
136 class->process[PROCESS_PRE_START] = NULL;
137
138+ unlink (logfile);
139
140 /* Check that we can handle a killed starting task, which should
141 * act as if it failed. A different error should be output and
142@@ -5964,6 +5976,7 @@
143 nih_free (class->process[PROCESS_PRE_START]);
144 class->process[PROCESS_PRE_START] = NULL;
145
146+ unlink (logfile);
147
148 /* Check that we can catch the running task of a service stopping
149 * with an error, and if the job is to be respawned, go into
150
151=== modified file 'util/tests/test_initctl.c'
152--- util/tests/test_initctl.c 2014-06-05 10:13:51 +0000
153+++ util/tests/test_initctl.c 2014-07-07 10:15:49 +0000
154@@ -11052,6 +11052,8 @@
155 FILE *file;
156 int ok;
157 int ret;
158+ mode_t expected_umask;
159+ size_t len;
160
161 TEST_GROUP ("re-exec support");
162
163@@ -11317,6 +11319,80 @@
164 STOP_UPSTART (upstart_pid);
165
166 /*******************************************************************/
167+ TEST_FEATURE ("ensure re-exec does not disrupt umask");
168+
169+ contents = nih_sprintf (NULL, "exec sh -c umask");
170+ TEST_NE_P (contents, NULL);
171+
172+ CREATE_FILE (confdir, "umask.conf", contents);
173+ nih_free (contents);
174+
175+ start_upstart_common (&upstart_pid, TRUE, TRUE, confdir, logdir, NULL);
176+
177+ cmd = nih_sprintf (NULL, "%s start umask 2>&1", get_initctl ());
178+ TEST_NE_P (cmd, NULL);
179+ RUN_COMMAND (NULL, cmd, &output, &lines);
180+ nih_free (output);
181+
182+ logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
183+ logdir,
184+ "umask.log"));
185+
186+ /* Wait for log to be created */
187+ ok = FALSE;
188+ for (int i = 0; i < 5; i++) {
189+ sleep (1);
190+ if (! stat (logfile, &statbuf)) {
191+ ok = TRUE;
192+ break;
193+ }
194+ }
195+ TEST_EQ (ok, TRUE);
196+
197+ contents = nih_file_read (NULL, logfile, &len);
198+ TEST_NE_P (contents, NULL);
199+ TEST_TRUE (len);
200+
201+ /* overwrite '\n' */
202+ contents[len-1] = '\0';
203+
204+ expected_umask = (mode_t)atoi (contents);
205+ assert0 (unlink (logfile));
206+ nih_free (contents);
207+
208+ /* Restart */
209+ REEXEC_UPSTART (upstart_pid, TRUE);
210+
211+ /* Re-run job */
212+ RUN_COMMAND (NULL, cmd, &output, &lines);
213+ nih_free (output);
214+
215+ /* Wait for log to be recreated */
216+ ok = FALSE;
217+ for (int i = 0; i < 5; i++) {
218+ sleep (1);
219+ if (! stat (logfile, &statbuf)) {
220+ ok = TRUE;
221+ break;
222+ }
223+ }
224+ TEST_EQ (ok, TRUE);
225+
226+ contents = nih_file_read (NULL, logfile, &len);
227+ TEST_NE_P (contents, NULL);
228+ TEST_TRUE (len);
229+
230+ /* overwrite '\n' */
231+ contents[len-1] = '\0';
232+
233+ TEST_EQ (expected_umask, (mode_t)atoi (contents));
234+
235+ STOP_UPSTART (upstart_pid);
236+
237+ DELETE_FILE (confdir, "umask.conf");
238+ assert0 (unlink (logfile));
239+
240+ /*******************************************************************/
241
242 TEST_DBUS_END (dbus_pid);
243

Subscribers

People subscribed via source and target branches