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

[James Hunt]

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.

[Dimitri John Ledkov (xnox)]

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

[lp:~jamesodhunt/upstart/bug-1302117-remerged]

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)]

Looks good to me, with below mentioned included cleanups.

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2014-07-03 08:59:30 +0000
+++ ChangeLog	2014-07-07 10:15:49 +0000
@@ -1,3 +1,10 @@
[phone]  James Hunt  <[e-mail]>
+
+	* 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.
+
 2014-07-03  James Hunt  <[e-mail]>
 
 	* init/job_process.c:
@@ -97,6 +104,13 @@
 	  - Clarify default behaviour of 'respawn' stanza.
 	  - Add missing 'respawn limit unlimited' details.
 
[phone]  James Hunt  <[e-mail]>
+
+	* 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  <[e-mail]>
 
 	* NEWS: Release 1.12.1

=== modified file 'init/log.c'
--- init/log.c	2014-06-04 17:37:59 +0000
+++ init/log.c	2014-07-07 10:15:49 +0000
@@ -405,6 +405,9 @@
 	struct stat  statbuf;
 	int          ret = -1;
 	int          mode = LOG_DEFAULT_MODE;
+#if 1
+	mode_t       old;
+#endif
 	int          flags = (O_CREAT | O_APPEND | O_WRONLY |
 			      O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK);
 
@@ -439,7 +442,11 @@
 	nih_assert (log->fd == -1);
 
 	/* Impose some sane defaults. */
+#if 1
+	old = umask (LOG_DEFAULT_UMASK);
+#else
 	umask (LOG_DEFAULT_UMASK);
+#endif
 
 	/* Non-blocking to avoid holding up the main loop. Without
 	 * this, we'd probably need to spawn a thread to handle
@@ -447,6 +454,11 @@
 	 */
 	log->fd = open (log->path, flags, mode);
 
+	/* Restore */
+#if 1
+	umask (old);
+#endif
+
 	log->open_errno = errno;
 
 	/* Open may have failed due to path being unaccessible

=== modified file 'init/tests/test_job_process.c'
--- init/tests/test_job_process.c	2014-06-05 17:57:29 +0000
+++ init/tests/test_job_process.c	2014-07-07 10:15:49 +0000
@@ -1,11 +1,8 @@
-/*
- * FIXME: test_job_process has been left behind
- */
 /* upstart
  *
  * test_job_process.c - test suite for init/job_process.c
  *
- * Copyright  2011 Canonical Ltd.
+ * Copyright © 2011-2014 Canonical Ltd.
  * Author: Scott James Remnant <[e-mail]>.
  *
  * This program is free software; you can redistribute it and/or modify
@@ -5489,11 +5486,16 @@
 	unsigned long   data;
 	struct timespec now;
 	char            dirname[PATH_MAX];
+	nih_local char *logfile = NULL;
+	int             fds[2] = { -1, -1};
 
 	TEST_FILENAME (dirname);       
 	TEST_EQ (mkdir (dirname, 0755), 0);
 	TEST_EQ (setenv ("UPSTART_LOGDIR", dirname, 1), 0);
 
+	logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s.log",
+				dirname, "test"));
+
 	TEST_FUNCTION ("job_process_handler");
 	program_name = "test";
 	output = tmpfile ();
@@ -5763,6 +5765,9 @@
 	class->process[PROCESS_PRE_START] = process_new (class);
 	class->process[PROCESS_PRE_START]->command = "echo";
 
+	assert0 (pipe (fds));
+	close (fds[1]);
+
 	TEST_ALLOC_FAIL {
 		TEST_ALLOC_SAFE {
 			job = job_new (class, "");
@@ -5770,6 +5775,9 @@
 			blocked = blocked_new (job, BLOCKED_EVENT, event);
 			event_block (event);
 			nih_list_add (&job->blocking, &blocked->entry);
+
+			job->process_data[PROCESS_PRE_START] = NIH_MUST (
+					job_process_data_new (job->process_data, job, PROCESS_PRE_START, fds[0]));
 		}
 
 		job->goal = JOB_START;
@@ -5812,9 +5820,12 @@
 		nih_free (job);
 	}
 
+	close (fds[0]);
+
 	nih_free (class->process[PROCESS_PRE_START]);
 	class->process[PROCESS_PRE_START] = NULL;
 
+	unlink (logfile);
 
 	/* Check that we can handle a failing pre-start process of the job,
 	 * which changes the goal to stop and transitions a state change in
@@ -5890,6 +5901,7 @@
 	nih_free (class->process[PROCESS_PRE_START]);
 	class->process[PROCESS_PRE_START] = NULL;
 
+	unlink (logfile);
 
 	/* Check that we can handle a killed starting task, which should
 	 * act as if it failed.  A different error should be output and
@@ -5964,6 +5976,7 @@
 	nih_free (class->process[PROCESS_PRE_START]);
 	class->process[PROCESS_PRE_START] = NULL;
 
+	unlink (logfile);
 
 	/* Check that we can catch the running task of a service stopping
 	 * with an error, and if the job is to be respawned, go into

=== modified file 'util/tests/test_initctl.c'
--- util/tests/test_initctl.c	2014-06-05 10:13:51 +0000
+++ util/tests/test_initctl.c	2014-07-07 10:15:49 +0000
@@ -11052,6 +11052,8 @@
 	FILE            *file;
 	int              ok;
 	int              ret;
+	mode_t           expected_umask;
+	size_t           len;
 
 	TEST_GROUP ("re-exec support");
 
@@ -11317,6 +11319,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);