upstart

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

  1. bug-1240686
  2. Merge into trunk
Proposed by James Hunt
Status: Merged
Merged at revision: 1552
Proposed branch: lp:~jamesodhunt/upstart/bug-1240686
Merge into: lp:upstart
Diff against target: 528 lines (+170/-40)
8 files modified
ChangeLog (+21/-1)
init/job_class.c (+8/-1)
init/job_process.c (+2/-2)
init/main.c (+8/-3)
init/tests/test_main.c (+8/-8)
test/test_util_common.c (+9/-5)
test/test_util_common.h (+4/-3)
util/tests/test_initctl.c (+110/-17)
To merge this branch: bzr merge lp:~jamesodhunt/upstart/bug-1240686
Reviewer Review Type Date Requested Status
Dimitri John Ledkov Approve
Review via email: mp+191771@code.launchpad.net

Description of the change

* init/job_class.c:
  - job_class_new(): Set umask for job to current value for Session
    Init by default (LP: #1240686).
* init/job_process.c: Comments.
* init/main.c: main(): Save current umask.
* test/test_util_common.c:
  - start_upstart_common(): Add extra param for inheriting environment
    rather than hard-coding '--no-inherit-env'.
  - start_upstart(: Updated call to start_upstart_common().
* init/tests/test_main.c: Updated calls to start_upstart_common().
* test/test_util_common.h: START_UPSTART(): Updated call to
  start_upstart_common().
* util/tests/test_initctl.c:
  - Updated calls to start_upstart_common().
  - test_umask(): New tests:
    - "ensure Session Init inherits umask by default"
    - "ensure Session Init defaults umask with '--no-inherit-env'"

No impact on re-exec since umask is retained across an exec.

To post a comment you must log in.
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

lgtm.

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 2013-10-04 21:34:25 +0000
3+++ ChangeLog 2013-10-18 09:17:07 +0000
4@@ -1,4 +1,24 @@
5-2013-01-04 Steve Langasek <steve.langasek@ubuntu.com
6+2013-10-18 James Hunt <james.hunt@ubuntu.com>
7+
8+ * init/job_class.c:
9+ - job_class_new(): Set umask for job to current value for Session
10+ Init by default (LP: #1240686).
11+ * init/job_process.c: Comments.
12+ * init/main.c: main(): Save current umask.
13+ * test/test_util_common.c:
14+ - start_upstart_common(): Add extra param for inheriting environment
15+ rather than hard-coding '--no-inherit-env'.
16+ - start_upstart(: Updated call to start_upstart_common().
17+ * init/tests/test_main.c: Updated calls to start_upstart_common().
18+ * test/test_util_common.h: START_UPSTART(): Updated call to
19+ start_upstart_common().
20+ * util/tests/test_initctl.c:
21+ - Updated calls to start_upstart_common().
22+ - test_umask(): New tests:
23+ - "ensure Session Init inherits umask by default"
24+ - "ensure Session Init defaults umask with '--no-inherit-env'"
25+
26+2013-10-04 Steve Langasek <steve.langasek@ubuntu.com
27
28 * extra/upstart-local-bridge.c: use SOCKET_PATH in our event
29 environment, instead of clobbering PATH. (LP: #1235480)
30
31=== modified file 'init/job_class.c'
32--- init/job_class.c 2013-07-22 00:15:43 +0000
33+++ init/job_class.c 2013-10-18 09:17:07 +0000
34@@ -97,6 +97,13 @@
35 static char **job_environ = NULL;
36
37 /**
38+ * initial_umask:
39+ *
40+ * Value of umask at startup.
41+ **/
42+mode_t initial_umask;
43+
44+/**
45 * job_class_init:
46 *
47 * Initialise the job classes hash table.
48@@ -353,7 +360,7 @@
49
50 class->console = default_console >= 0 ? default_console : CONSOLE_LOG;
51
52- class->umask = JOB_DEFAULT_UMASK;
53+ class->umask = (user_mode && ! no_inherit_env) ? initial_umask : JOB_DEFAULT_UMASK;
54 class->nice = JOB_NICE_INVALID;
55 class->oom_score_adj = JOB_DEFAULT_OOM_SCORE_ADJ;
56
57
58=== modified file 'init/job_process.c'
59--- init/job_process.c 2013-10-03 14:43:24 +0000
60+++ init/job_process.c 2013-10-18 09:17:07 +0000
61@@ -126,11 +126,11 @@
62 /**
63 * no_inherit_env:
64 *
65- * If TRUE, do not copy the Session Inits environment to that provided to jobs.
66+ * If TRUE, do not copy the Session Inits environment variables or umask
67+ * to that provided to jobs.
68 **/
69 int no_inherit_env = FALSE;
70
71-
72 /* Prototypes for static functions */
73 static void job_process_kill_timer (Job *job, NihTimer *timer);
74 static void job_process_terminated (Job *job, ProcessType process,
75
76=== modified file 'init/main.c'
77--- init/main.c 2013-07-31 09:28:48 +0000
78+++ init/main.c 2013-10-18 09:17:07 +0000
79@@ -128,7 +128,7 @@
80 extern int default_console;
81 extern int write_state_file;
82 extern char *log_dir;
83-
84+extern mode_t initial_umask;
85
86 /**
87 * options:
88@@ -143,7 +143,7 @@
89 NULL, "VALUE", NULL, console_type_setter },
90
91 { 0, "no-inherit-env", N_("jobs will not inherit environment of init"),
92- NULL, NULL, &no_inherit_env ,NULL },
93+ NULL, NULL, &no_inherit_env , NULL },
94
95 { 0, "logdir", N_("specify alternative directory to store job output logs in"),
96 NULL, "DIR", &log_dir, NULL },
97@@ -270,7 +270,7 @@
98 /* Allow devices to be created with the actual perms
99 * specified.
100 */
101- (void)umask (0);
102+ initial_umask = umask (0);
103
104 /* Check if key /dev entries already exist; if they do,
105 * we should assume we don't need to mount /dev.
106@@ -385,6 +385,11 @@
107 (int)getuid (), (int)getpid (), (int)getppid ());
108 #endif /* DEBUG */
109
110+ if (user_mode) {
111+ /* Save initial value */
112+ initial_umask = umask (0);
113+ (void)umask (initial_umask);
114+ }
115
116 /* Reset the signal state and install the signal handler for those
117 * signals we actually want to catch; this also sets those that
118
119=== modified file 'init/tests/test_main.c'
120--- init/tests/test_main.c 2013-06-25 09:19:05 +0000
121+++ init/tests/test_main.c 2013-10-18 09:17:07 +0000
122@@ -107,7 +107,7 @@
123 CREATE_FILE (xdg_conf_dir, "bar.conf", "exec true");
124 CREATE_FILE (xdg_conf_dir, "baz.conf", "exec true");
125
126- start_upstart_common (&upstart_pid, TRUE, NULL, logdir, NULL);
127+ start_upstart_common (&upstart_pid, TRUE, FALSE, NULL, logdir, NULL);
128
129 /* Should be running */
130 assert0 (kill (upstart_pid, 0));
131@@ -139,7 +139,7 @@
132 CREATE_FILE (xdg_conf_dir, "xdg_dir_job.conf", "exec true");
133 CREATE_FILE (confdir_a, "conf_dir_job.conf", "exec true");
134
135- start_upstart_common (&upstart_pid, TRUE, confdir_a, logdir, NULL);
136+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir_a, logdir, NULL);
137
138 /* Should be running */
139 assert0 (kill (upstart_pid, 0));
140@@ -177,7 +177,7 @@
141 extra[4] = NULL;
142
143 /* pass 2 confdir directories */
144- start_upstart_common (&upstart_pid, TRUE, NULL, logdir, extra);
145+ start_upstart_common (&upstart_pid, TRUE, FALSE, NULL, logdir, extra);
146
147 /* Should be running */
148 assert0 (kill (upstart_pid, 0));
149@@ -217,7 +217,7 @@
150 extra[4] = NULL;
151
152 /* pass 2 confdir directories */
153- start_upstart_common (&upstart_pid, TRUE, NULL, logdir, extra);
154+ start_upstart_common (&upstart_pid, TRUE, FALSE, NULL, logdir, extra);
155
156 /* Should be running */
157 assert0 (kill (upstart_pid, 0));
158@@ -266,7 +266,7 @@
159 /* Disable user mode */
160 test_user_mode = FALSE;
161
162- start_upstart_common (&upstart_pid, FALSE, NULL, logdir, NULL);
163+ start_upstart_common (&upstart_pid, FALSE, FALSE, NULL, logdir, NULL);
164
165 /* Should be running */
166 assert0 (kill (upstart_pid, 0));
167@@ -296,7 +296,7 @@
168 CREATE_FILE (confdir_a, "bar.conf", "exec true");
169 CREATE_FILE (confdir_b, "baz.conf", "exec true");
170
171- start_upstart_common (&upstart_pid, FALSE, confdir_b, logdir, NULL);
172+ start_upstart_common (&upstart_pid, FALSE, FALSE, confdir_b, logdir, NULL);
173
174 /* Should be running */
175 assert0 (kill (upstart_pid, 0));
176@@ -333,7 +333,7 @@
177 extra[3] = confdir_b;
178 extra[4] = NULL;
179
180- start_upstart_common (&upstart_pid, FALSE, NULL, logdir, extra);
181+ start_upstart_common (&upstart_pid, FALSE, FALSE, NULL, logdir, extra);
182
183 /* Should be running */
184 assert0 (kill (upstart_pid, 0));
185@@ -376,7 +376,7 @@
186 extra[3] = confdir_b;
187 extra[4] = NULL;
188
189- start_upstart_common (&upstart_pid, FALSE, NULL, logdir, extra);
190+ start_upstart_common (&upstart_pid, FALSE, FALSE, NULL, logdir, extra);
191
192 /* Should be running */
193 assert0 (kill (upstart_pid, 0));
194
195=== modified file 'test/test_util_common.c'
196--- test/test_util_common.c 2013-10-02 08:59:20 +0000
197+++ test/test_util_common.c 2013-10-18 09:17:07 +0000
198@@ -420,6 +420,7 @@
199 * @pid: PID of running instance,
200 * @user: TRUE if upstart should run in User Session mode (FALSE to
201 * use the users D-Bus session bus),
202+ * @inherit_env: if TRUE, inherit parent environment,
203 * @confdir: full path to configuration directory,
204 * @logdir: full path to log directory,
205 * @extra: optional extra arguments.
206@@ -427,8 +428,9 @@
207 * Wrapper round _start_upstart() which specifies common options.
208 **/
209 void
210-start_upstart_common (pid_t *pid, int user, const char *confdir,
211- const char *logdir, char * const *extra)
212+start_upstart_common (pid_t *pid, int user, int inherit_env,
213+ const char *confdir, const char *logdir,
214+ char * const *extra)
215 {
216 nih_local char **args = NULL;
217
218@@ -449,8 +451,10 @@
219 NIH_MUST (nih_str_array_add (&args, NULL, NULL,
220 "--no-sessions"));
221
222- NIH_MUST (nih_str_array_add (&args, NULL, NULL,
223- "--no-inherit-env"));
224+ if (! inherit_env) {
225+ NIH_MUST (nih_str_array_add (&args, NULL, NULL,
226+ "--no-inherit-env"));
227+ }
228
229 if (confdir) {
230 NIH_MUST (nih_str_array_add (&args, NULL, NULL,
231@@ -483,7 +487,7 @@
232 void
233 start_upstart (pid_t *pid)
234 {
235- start_upstart_common (pid, FALSE, NULL, NULL, NULL);
236+ start_upstart_common (pid, FALSE, FALSE, NULL, NULL, NULL);
237 }
238
239 /**
240
241=== modified file 'test/test_util_common.h'
242--- test/test_util_common.h 2013-09-26 16:33:07 +0000
243+++ test/test_util_common.h 2013-10-18 09:17:07 +0000
244@@ -341,7 +341,7 @@
245 * Start an instance of Upstart and return PID in @pid.
246 **/
247 #define START_UPSTART(pid, user_mode) \
248- start_upstart_common (&(pid), user_mode, NULL, NULL, NULL)
249+ start_upstart_common (&(pid), user_mode, FALSE, NULL, NULL, NULL)
250
251 /**
252 * KILL_UPSTART:
253@@ -700,8 +700,9 @@
254
255 void _start_upstart (pid_t *pid, int user, char * const *args);
256
257-void start_upstart_common (pid_t *pid, int user, const char *confdir,
258- const char *logdir, char * const *extra);
259+void start_upstart_common (pid_t *pid, int user, int inherit_env,
260+ const char *confdir, const char *logdir,
261+ char * const *extra);
262
263 void start_upstart (pid_t *pid);
264
265
266=== modified file 'util/tests/test_initctl.c'
267--- util/tests/test_initctl.c 2013-09-26 16:33:07 +0000
268+++ util/tests/test_initctl.c 2013-10-18 09:17:07 +0000
269@@ -10839,7 +10839,7 @@
270 /*******************************************************************/
271 TEST_FEATURE ("single job producing output across a re-exec");
272
273- start_upstart_common (&upstart_pid, FALSE, confdir, logdir, NULL);
274+ start_upstart_common (&upstart_pid, FALSE, FALSE, confdir, logdir, NULL);
275
276 contents = nih_sprintf (NULL,
277 "pre-start exec echo pre-start\n"
278@@ -11059,7 +11059,7 @@
279 /* Reset initctl global from previous tests */
280 dest_name = NULL;
281
282- start_upstart_common (&upstart_pid, TRUE, NULL, NULL, NULL);
283+ start_upstart_common (&upstart_pid, TRUE, FALSE, NULL, NULL, NULL);
284
285 session_file = get_session_file (dirname, upstart_pid);
286
287@@ -11168,7 +11168,7 @@
288 /*******************************************************************/
289 TEST_FEATURE ("system shutdown: no jobs");
290
291- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
292+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
293
294 /* Should be running */
295 assert0 (kill (upstart_pid, 0));
296@@ -11194,7 +11194,7 @@
297 CREATE_FILE (confdir, "long-running.conf",
298 "exec sleep 999");
299
300- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
301+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
302
303 /* Should be running */
304 assert0 (kill (upstart_pid, 0));
305@@ -11276,7 +11276,7 @@
306 " sleep 999\n"
307 "end script");
308
309- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
310+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
311
312 /* Should be running */
313 assert0 (kill (upstart_pid, 0));
314@@ -11329,7 +11329,7 @@
315
316 CREATE_FILE (confdir, "session-end.conf", job);
317
318- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
319+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
320
321 /* Should be running */
322 assert0 (kill (upstart_pid, 0));
323@@ -11392,7 +11392,7 @@
324
325 CREATE_FILE (confdir, "session-end-term.conf", job);
326
327- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
328+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
329
330 /* Should be running */
331 assert0 (kill (upstart_pid, 0));
332@@ -11460,7 +11460,7 @@
333
334 CREATE_FILE (confdir, "session-end-term.conf", job);
335
336- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
337+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
338
339 /* Should be running */
340 assert0 (kill (upstart_pid, 0));
341@@ -11512,7 +11512,7 @@
342 /*******************************************************************/
343 TEST_FEATURE ("session shutdown: no jobs");
344
345- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
346+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
347
348 /* Further required initctl global resets. Shudder. */
349 user_mode = TRUE;
350@@ -11548,7 +11548,7 @@
351 CREATE_FILE (confdir, "long-running.conf",
352 "exec sleep 999");
353
354- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
355+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
356
357 cmd = nih_sprintf (NULL, "%s start %s 2>&1",
358 get_initctl (), "long-running");
359@@ -11596,7 +11596,7 @@
360 "start on startup\n"
361 "exec sleep 999");
362
363- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
364+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
365
366 upstart = upstart_open (NULL);
367 TEST_NE_P (upstart, NULL);
368@@ -11641,7 +11641,7 @@
369 " sleep 999\n"
370 "end script");
371
372- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
373+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
374
375 cmd = nih_sprintf (NULL, "%s start %s 2>&1",
376 get_initctl (), "long-running-term");
377@@ -11697,7 +11697,7 @@
378
379 CREATE_FILE (confdir, "session-end.conf", job);
380
381- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
382+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
383
384 upstart = upstart_open (NULL);
385 TEST_NE_P (upstart, NULL);
386@@ -11760,7 +11760,7 @@
387
388 CREATE_FILE (confdir, "session-end-term.conf", job);
389
390- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
391+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
392
393 upstart = upstart_open (NULL);
394 TEST_NE_P (upstart, NULL);
395@@ -11830,7 +11830,7 @@
396
397 CREATE_FILE (confdir, "session-end-term.conf", job);
398
399- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
400+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
401
402 cmd = nih_sprintf (NULL, "%s start %s 2>&1",
403 get_initctl (), "long-running-term");
404@@ -11906,6 +11906,98 @@
405 }
406
407 void
408+test_umask (void)
409+{
410+ char confdir[PATH_MAX];
411+ char logdir[PATH_MAX];
412+ pid_t upstart_pid = 0;
413+ nih_local char *logfile = NULL;
414+ mode_t job_umask;
415+ nih_local char *job_umask_str = NULL;
416+ size_t length;
417+ int ret;
418+ mode_t original_umask;
419+ mode_t test_umask = 0111;
420+ mode_t default_umask = 022;
421+
422+ TEST_FILENAME (confdir);
423+ TEST_EQ (mkdir (confdir, 0755), 0);
424+
425+ TEST_FILENAME (logdir);
426+ TEST_EQ (mkdir (logdir, 0755), 0);
427+
428+ original_umask = umask (test_umask);
429+
430+ TEST_GROUP ("Session Init umask value");
431+
432+ /**********************************************************************/
433+ TEST_FEATURE ("ensure Session Init inherits umask by default");
434+
435+ /* Has to be a script since umask is a shell built-in */
436+ CREATE_FILE (confdir, "umask.conf",
437+ "start on startup\n"
438+ "script\n"
439+ "umask\n"
440+ "end script");
441+
442+ start_upstart_common (&upstart_pid, TRUE, TRUE, confdir, logdir, NULL);
443+
444+ logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
445+ logdir,
446+ "umask.log"));
447+
448+ WAIT_FOR_FILE (logfile);
449+
450+ job_umask_str = nih_file_read (NULL, logfile, &length);
451+
452+ ret = sscanf (job_umask_str, "%o", (unsigned int *)&job_umask);
453+ TEST_EQ (ret, 1);
454+ TEST_EQ (job_umask, test_umask);
455+
456+ DELETE_FILE (confdir, "umask.conf");
457+ assert0 (unlink (logfile));
458+
459+ STOP_UPSTART (upstart_pid);
460+
461+ /**********************************************************************/
462+ TEST_FEATURE ("ensure Session Init defaults umask with '--no-inherit-env'");
463+
464+ /* Has to be a script since umask is a shell built-in */
465+ CREATE_FILE (confdir, "umask.conf",
466+ "start on startup\n"
467+ "script\n"
468+ "umask\n"
469+ "end script");
470+
471+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
472+
473+ logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
474+ logdir,
475+ "umask.log"));
476+
477+ WAIT_FOR_FILE (logfile);
478+
479+ job_umask_str = nih_file_read (NULL, logfile, &length);
480+
481+ ret = sscanf (job_umask_str, "%o", (unsigned int *)&job_umask);
482+ TEST_EQ (ret, 1);
483+ TEST_EQ (job_umask, default_umask);
484+
485+ DELETE_FILE (confdir, "umask.conf");
486+ assert0 (unlink (logfile));
487+
488+ STOP_UPSTART (upstart_pid);
489+
490+ /**********************************************************************/
491+
492+ /* Restore */
493+ (void)umask (original_umask);
494+
495+ assert0 (rmdir (confdir));
496+ assert0 (rmdir (logdir));
497+}
498+
499+void
500 test_show_config (void)
501 {
502 char dirname[PATH_MAX];
503@@ -16519,7 +16611,7 @@
504 nih_local char *session_file = NULL;
505 FILE *fi;
506
507- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, extra);
508+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, extra);
509
510 /*******************************************************************/
511 TEST_FEATURE ("ensure list-env in '--user --no-inherit-env' environment gives expected output");
512@@ -16633,7 +16725,7 @@
513 }
514
515 TEST_DBUS (dbus_pid);
516- start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
517+ start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
518
519 cmd = nih_sprintf (NULL, "%s list-sessions 2>&1", get_initctl_binary ());
520 TEST_NE_P (cmd, NULL);
521@@ -16727,6 +16819,7 @@
522 test_reexec ();
523 test_list_sessions ();
524 test_quiesce ();
525+ test_umask ();
526
527 if (in_chroot () && !dbus_configured ()) {
528 fprintf(stderr, "\n\n"

Subscribers

People subscribed via source and target branches