James Hunt has proposed merging lp:~jamesodhunt/upstart/bug-1302117 into lp:upstart.
Requested reviews: Upstart Reviewers (upstart-reviewers) Related bugs: Bug #1302117 in upstart : "Session Init changes umask on re-exec" https://bugs.launchpad.net/upstart/+bug/1302117 For more details, see: https://code.launchpad.net/~jamesodhunt/upstart/bug-1302117/+merge/215173 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. -- https://code.launchpad.net/~jamesodhunt/upstart/bug-1302117/+merge/215173 Your team Upstart Reviewers is requested to review the proposed merge of lp:~jamesodhunt/upstart/bug-1302117 into lp:upstart.
=== modified file 'ChangeLog' --- ChangeLog 2014-03-11 14:44:56 +0000 +++ ChangeLog 2014-04-10 13:17:21 +0000 @@ -1,3 +1,10 @@ +2014-04-10 James Hunt <[email protected]> + + * init/log.c: log_file_open(): Restore umask after opening log file + (LP: #1302117). + * util/tests/test_initctl.c: test_reexec(): New test: + - "ensure re-exec does not disrupt umask". + 2014-03-11 James Hunt <[email protected]> * NEWS: Release 1.12.1 === modified file 'init/log.c' --- init/log.c 2013-06-26 12:10:23 +0000 +++ init/log.c 2014-04-10 13:17:21 +0000 @@ -405,6 +405,7 @@ struct stat statbuf; int ret = -1; int mode = LOG_DEFAULT_MODE; + mode_t old; int flags = (O_CREAT | O_APPEND | O_WRONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); @@ -439,7 +440,7 @@ nih_assert (log->fd == -1); /* Impose some sane defaults. */ - umask (LOG_DEFAULT_UMASK); + old = umask (LOG_DEFAULT_UMASK); /* Non-blocking to avoid holding up the main loop. Without * this, we'd probably need to spawn a thread to handle @@ -447,6 +448,9 @@ */ log->fd = open (log->path, flags, mode); + /* Restore */ + umask (old); + log->open_errno = errno; /* Open may have failed due to path being unaccessible === modified file 'util/tests/test_initctl.c' --- util/tests/test_initctl.c 2014-04-09 12:58:58 +0000 +++ util/tests/test_initctl.c 2014-04-10 13:17:21 +0000 @@ -11050,6 +11050,8 @@ FILE *file; int ok; int ret; + mode_t expected_umask; + size_t len; TEST_GROUP ("re-exec support"); @@ -11315,6 +11317,80 @@ STOP_UPSTART (upstart_pid); /*******************************************************************/ + TEST_FEATURE ("ensure re-exec does not disrupt umask"); + + contents = nih_sprintf (NULL, "exec sh -c umask"); + TEST_NE_P (contents, NULL); + + CREATE_FILE (confdir, "umask.conf", contents); + nih_free (contents); + + start_upstart_common (&upstart_pid, TRUE, TRUE, confdir, logdir, NULL); + + cmd = nih_sprintf (NULL, "%s start umask 2>&1", get_initctl ()); + TEST_NE_P (cmd, NULL); + RUN_COMMAND (NULL, cmd, &output, &lines); + nih_free (output); + + logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s", + logdir, + "umask.log")); + + /* Wait for log to be created */ + ok = FALSE; + for (int i = 0; i < 5; i++) { + sleep (1); + if (! stat (logfile, &statbuf)) { + ok = TRUE; + break; + } + } + TEST_EQ (ok, TRUE); + + contents = nih_file_read (NULL, logfile, &len); + TEST_NE_P (contents, NULL); + TEST_TRUE (len); + + /* overwrite '\n' */ + contents[len-1] = '\0'; + + expected_umask = (mode_t)atoi (contents); + assert0 (unlink (logfile)); + nih_free (contents); + + /* Restart */ + REEXEC_UPSTART (upstart_pid, TRUE); + + /* Re-run job */ + RUN_COMMAND (NULL, cmd, &output, &lines); + nih_free (output); + + /* Wait for log to be recreated */ + ok = FALSE; + for (int i = 0; i < 5; i++) { + sleep (1); + if (! stat (logfile, &statbuf)) { + ok = TRUE; + break; + } + } + TEST_EQ (ok, TRUE); + + contents = nih_file_read (NULL, logfile, &len); + TEST_NE_P (contents, NULL); + TEST_TRUE (len); + + /* overwrite '\n' */ + contents[len-1] = '\0'; + + TEST_EQ (expected_umask, (mode_t)atoi (contents)); + + STOP_UPSTART (upstart_pid); + + DELETE_FILE (confdir, "umask.conf"); + assert0 (unlink (logfile)); + + /*******************************************************************/ TEST_DBUS_END (dbus_pid);
-- upstart-devel mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/upstart-devel
