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

[James Hunt]

* 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.

[Dimitri John Ledkov (xnox)]

lgtm.

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2013-10-04 21:34:25 +0000
+++ ChangeLog	2013-10-18 09:17:07 +0000
@@ -1,4 +1,24 @@
-2013-01-04  Steve Langasek  <[e-mail]
[phone]  James Hunt  <[e-mail]>
+
+	* 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'"
+
[phone]  Steve Langasek  <[e-mail]
 
 	* extra/upstart-local-bridge.c: use SOCKET_PATH in our event
 	  environment, instead of clobbering PATH.  (LP: #1235480)
 
=== modified file 'init/job_class.c'
--- init/job_class.c	2013-07-22 00:15:43 +0000
+++ init/job_class.c	2013-10-18 09:17:07 +0000
@@ -97,6 +97,13 @@
 static char **job_environ = NULL;
 
 /**
+ * initial_umask:
+ *
+ * Value of umask at startup.
+ **/
+mode_t initial_umask;
+
+/**
  * job_class_init:
  *
  * Initialise the job classes hash table.
@@ -353,7 +360,7 @@
 
 	class->console = default_console >= 0 ? default_console : CONSOLE_LOG;
 
-	class->umask = JOB_DEFAULT_UMASK;
+	class->umask = (user_mode && ! no_inherit_env) ? initial_umask : JOB_DEFAULT_UMASK;
 	class->nice = JOB_NICE_INVALID;
 	class->oom_score_adj = JOB_DEFAULT_OOM_SCORE_ADJ;
 
 
=== modified file 'init/job_process.c'
--- init/job_process.c	2013-10-03 14:43:24 +0000
+++ init/job_process.c	2013-10-18 09:17:07 +0000
@@ -126,11 +126,11 @@
 /**
  * no_inherit_env:
  *
- * If TRUE, do not copy the Session Inits environment to that provided to jobs.
+ * If TRUE, do not copy the Session Inits environment variables or umask
+ * to that provided to jobs.
  **/
 int no_inherit_env = FALSE;
 
-
 /* Prototypes for static functions */
 static void job_process_kill_timer      (Job *job, NihTimer *timer);
 static void job_process_terminated      (Job *job, ProcessType process,
 
=== modified file 'init/main.c'
--- init/main.c	2013-07-31 09:28:48 +0000
+++ init/main.c	2013-10-18 09:17:07 +0000
@@ -128,7 +128,7 @@
 extern int          default_console;
 extern int          write_state_file;
 extern char        *log_dir;
-
+extern mode_t       initial_umask;
 
 /**
  * options:
@@ -143,7 +143,7 @@
 		NULL, "VALUE", NULL, console_type_setter },
 
 	{ 0, "no-inherit-env", N_("jobs will not inherit environment of init"),
-		NULL, NULL, &no_inherit_env ,NULL },
+		NULL, NULL, &no_inherit_env , NULL },
 
 	{ 0, "logdir", N_("specify alternative directory to store job output logs in"),
 		NULL, "DIR", &log_dir, NULL },
@@ -270,7 +270,7 @@
 		/* Allow devices to be created with the actual perms
 		 * specified.
 		 */
-		(void)umask (0);
+		initial_umask = umask (0);
 
 		/* Check if key /dev entries already exist; if they do,
 		 * we should assume we don't need to mount /dev.
@@ -385,6 +385,11 @@
 		(int)getuid (), (int)getpid (), (int)getppid ());
 #endif /* DEBUG */
 
+	if (user_mode) {
+		/* Save initial value */
+		initial_umask = umask (0);
+		(void)umask (initial_umask);
+	}
 
 	/* Reset the signal state and install the signal handler for those
 	 * signals we actually want to catch; this also sets those that
 
=== modified file 'init/tests/test_main.c'
--- init/tests/test_main.c	2013-06-25 09:19:05 +0000
+++ init/tests/test_main.c	2013-10-18 09:17:07 +0000
@@ -107,7 +107,7 @@
 	CREATE_FILE (xdg_conf_dir, "bar.conf", "exec true");
 	CREATE_FILE (xdg_conf_dir, "baz.conf", "exec true");
 
-	start_upstart_common (&upstart_pid, TRUE, NULL, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, NULL, logdir, NULL);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -139,7 +139,7 @@
 	CREATE_FILE (xdg_conf_dir, "xdg_dir_job.conf", "exec true");
 	CREATE_FILE (confdir_a, "conf_dir_job.conf", "exec true");
 
-	start_upstart_common (&upstart_pid, TRUE, confdir_a, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir_a, logdir, NULL);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -177,7 +177,7 @@
 	extra[4] = NULL;
 
 	/* pass 2 confdir directories */
-	start_upstart_common (&upstart_pid, TRUE, NULL, logdir, extra);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, NULL, logdir, extra);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -217,7 +217,7 @@
 	extra[4] = NULL;
 
 	/* pass 2 confdir directories */
-	start_upstart_common (&upstart_pid, TRUE, NULL, logdir, extra);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, NULL, logdir, extra);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -266,7 +266,7 @@
 	/* Disable user mode */
 	test_user_mode = FALSE;
 
-	start_upstart_common (&upstart_pid, FALSE, NULL, logdir, NULL);
+	start_upstart_common (&upstart_pid, FALSE, FALSE, NULL, logdir, NULL);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -296,7 +296,7 @@
 	CREATE_FILE (confdir_a, "bar.conf", "exec true");
 	CREATE_FILE (confdir_b, "baz.conf", "exec true");
 
-	start_upstart_common (&upstart_pid, FALSE, confdir_b, logdir, NULL);
+	start_upstart_common (&upstart_pid, FALSE, FALSE, confdir_b, logdir, NULL);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -333,7 +333,7 @@
 	extra[3] = confdir_b;
 	extra[4] = NULL;
 
-	start_upstart_common (&upstart_pid, FALSE, NULL, logdir, extra);
+	start_upstart_common (&upstart_pid, FALSE, FALSE, NULL, logdir, extra);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -376,7 +376,7 @@
 	extra[3] = confdir_b;
 	extra[4] = NULL;
 
-	start_upstart_common (&upstart_pid, FALSE, NULL, logdir, extra);
+	start_upstart_common (&upstart_pid, FALSE, FALSE, NULL, logdir, extra);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
 
=== modified file 'test/test_util_common.c'
--- test/test_util_common.c	2013-10-02 08:59:20 +0000
+++ test/test_util_common.c	2013-10-18 09:17:07 +0000
@@ -420,6 +420,7 @@
  * @pid: PID of running instance,
  * @user: TRUE if upstart should run in User Session mode (FALSE to
  * use the users D-Bus session bus),
+ * @inherit_env: if TRUE, inherit parent environment,
  * @confdir: full path to configuration directory,
  * @logdir: full path to log directory,
  * @extra: optional extra arguments.
@@ -427,8 +428,9 @@
  * Wrapper round _start_upstart() which specifies common options.
  **/
 void
-start_upstart_common (pid_t *pid, int user, const char *confdir,
-		      const char *logdir, char * const *extra)
+start_upstart_common (pid_t *pid, int user, int inherit_env,
+		      const char *confdir, const char *logdir,
+		      char * const *extra)
 {
 	nih_local char  **args = NULL;
 
@@ -449,8 +451,10 @@
 	NIH_MUST (nih_str_array_add (&args, NULL, NULL,
 				"--no-sessions"));
 
-	NIH_MUST (nih_str_array_add (&args, NULL, NULL,
-				"--no-inherit-env"));
+	if (! inherit_env) {
+		NIH_MUST (nih_str_array_add (&args, NULL, NULL,
+					"--no-inherit-env"));
+	}
 
 	if (confdir) {
 		NIH_MUST (nih_str_array_add (&args, NULL, NULL,
@@ -483,7 +487,7 @@
 void
 start_upstart (pid_t *pid)
 {
-	start_upstart_common (pid, FALSE, NULL, NULL, NULL);
+	start_upstart_common (pid, FALSE, FALSE, NULL, NULL, NULL);
 }
 
 /**
 
=== modified file 'test/test_util_common.h'
--- test/test_util_common.h	2013-09-26 16:33:07 +0000
+++ test/test_util_common.h	2013-10-18 09:17:07 +0000
@@ -341,7 +341,7 @@
  * Start an instance of Upstart and return PID in @pid.
  **/
 #define START_UPSTART(pid, user_mode)                                \
-	start_upstart_common (&(pid), user_mode, NULL, NULL, NULL)
+	start_upstart_common (&(pid), user_mode, FALSE, NULL, NULL, NULL)
 
 /**
  * KILL_UPSTART:
@@ -700,8 +700,9 @@
 
 void _start_upstart (pid_t *pid, int user, char * const *args);
 
-void start_upstart_common (pid_t *pid, int user, const char *confdir,
-		      const char *logdir, char * const *extra);
+void start_upstart_common (pid_t *pid, int user, int inherit_env,
+		      const char *confdir, const char *logdir,
+		      char * const *extra);
 
 void start_upstart (pid_t *pid);
 
 
=== modified file 'util/tests/test_initctl.c'
--- util/tests/test_initctl.c	2013-09-26 16:33:07 +0000
+++ util/tests/test_initctl.c	2013-10-18 09:17:07 +0000
@@ -10839,7 +10839,7 @@
 	/*******************************************************************/
 	TEST_FEATURE ("single job producing output across a re-exec");
 
-	start_upstart_common (&upstart_pid, FALSE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, FALSE, FALSE, confdir, logdir, NULL);
 
 	contents = nih_sprintf (NULL, 
 			"pre-start exec echo pre-start\n"
@@ -11059,7 +11059,7 @@
 	/* Reset initctl global from previous tests */
 	dest_name = NULL;
 
-	start_upstart_common (&upstart_pid, TRUE, NULL, NULL, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, NULL, NULL, NULL);
 
 	session_file = get_session_file (dirname, upstart_pid);
 
@@ -11168,7 +11168,7 @@
 	/*******************************************************************/
 	TEST_FEATURE ("system shutdown: no jobs");
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -11194,7 +11194,7 @@
 	CREATE_FILE (confdir, "long-running.conf",
 			"exec sleep 999");
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -11276,7 +11276,7 @@
 		        "  sleep 999\n"
 			"end script");
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -11329,7 +11329,7 @@
 
 	CREATE_FILE (confdir, "session-end.conf", job);
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -11392,7 +11392,7 @@
 
 	CREATE_FILE (confdir, "session-end-term.conf", job);
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -11460,7 +11460,7 @@
 
 	CREATE_FILE (confdir, "session-end-term.conf", job);
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	/* Should be running */
 	assert0 (kill (upstart_pid, 0));
@@ -11512,7 +11512,7 @@
 	/*******************************************************************/
 	TEST_FEATURE ("session shutdown: no jobs");
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	/* Further required initctl global resets. Shudder. */
 	user_mode = TRUE;
@@ -11548,7 +11548,7 @@
 	CREATE_FILE (confdir, "long-running.conf",
 			"exec sleep 999");
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	cmd = nih_sprintf (NULL, "%s start %s 2>&1",
 			get_initctl (), "long-running");
@@ -11596,7 +11596,7 @@
 			"start on startup\n"
 			"exec sleep 999");
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	upstart = upstart_open (NULL);
 	TEST_NE_P (upstart, NULL);
@@ -11641,7 +11641,7 @@
 		        "  sleep 999\n"
 			"end script");
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	cmd = nih_sprintf (NULL, "%s start %s 2>&1",
 			get_initctl (), "long-running-term");
@@ -11697,7 +11697,7 @@
 
 	CREATE_FILE (confdir, "session-end.conf", job);
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	upstart = upstart_open (NULL);
 	TEST_NE_P (upstart, NULL);
@@ -11760,7 +11760,7 @@
 
 	CREATE_FILE (confdir, "session-end-term.conf", job);
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	upstart = upstart_open (NULL);
 	TEST_NE_P (upstart, NULL);
@@ -11830,7 +11830,7 @@
 
 	CREATE_FILE (confdir, "session-end-term.conf", job);
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	cmd = nih_sprintf (NULL, "%s start %s 2>&1",
 			get_initctl (), "long-running-term");
@@ -11906,6 +11906,98 @@
 }
 
 void
+test_umask (void)
+{
+	char             confdir[PATH_MAX];
+	char             logdir[PATH_MAX];
+	pid_t            upstart_pid = 0;
+	nih_local char  *logfile = NULL;
+	mode_t           job_umask;
+	nih_local char  *job_umask_str = NULL;
+	size_t           length;
+	int              ret;
+	mode_t           original_umask;
+	mode_t           test_umask = 0111;
+	mode_t           default_umask = 022;
+
+        TEST_FILENAME (confdir);
+        TEST_EQ (mkdir (confdir, 0755), 0);
+
+        TEST_FILENAME (logdir);
+        TEST_EQ (mkdir (logdir, 0755), 0);
+
+	original_umask = umask (test_umask);
+
+	TEST_GROUP ("Session Init umask value");
+
+	/**********************************************************************/
+	TEST_FEATURE ("ensure Session Init inherits umask by default");
+
+	/* Has to be a script since umask is a shell built-in */
+	CREATE_FILE (confdir, "umask.conf",
+			"start on startup\n"
+			"script\n"
+			"umask\n"
+			"end script");
+
+	start_upstart_common (&upstart_pid, TRUE, TRUE, confdir, logdir, NULL);
+
+	logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
+				logdir,
+				"umask.log"));
+
+	WAIT_FOR_FILE (logfile);
+
+	job_umask_str = nih_file_read (NULL, logfile, &length);
+
+	ret = sscanf (job_umask_str, "%o", (unsigned int *)&job_umask);
+	TEST_EQ (ret, 1);
+	TEST_EQ (job_umask, test_umask);
+
+	DELETE_FILE (confdir, "umask.conf");
+	assert0 (unlink (logfile));
+
+	STOP_UPSTART (upstart_pid);
+
+	/**********************************************************************/
+	TEST_FEATURE ("ensure Session Init defaults umask with '--no-inherit-env'");
+
+	/* Has to be a script since umask is a shell built-in */
+	CREATE_FILE (confdir, "umask.conf",
+			"start on startup\n"
+			"script\n"
+			"umask\n"
+			"end script");
+
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
+
+	logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
+				logdir,
+				"umask.log"));
+
+	WAIT_FOR_FILE (logfile);
+
+	job_umask_str = nih_file_read (NULL, logfile, &length);
+
+	ret = sscanf (job_umask_str, "%o", (unsigned int *)&job_umask);
+	TEST_EQ (ret, 1);
+	TEST_EQ (job_umask, default_umask);
+
+	DELETE_FILE (confdir, "umask.conf");
+	assert0 (unlink (logfile));
+
+	STOP_UPSTART (upstart_pid);
+
+	/**********************************************************************/
+
+	/* Restore */
+	(void)umask (original_umask);
+
+        assert0 (rmdir (confdir));
+        assert0 (rmdir (logdir));
+}
+
+void
 test_show_config (void)
 {
 	char             dirname[PATH_MAX];
@@ -16519,7 +16611,7 @@
 	nih_local char  *session_file = NULL;
 	FILE            *fi;
 
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, extra);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, extra);
 
 	/*******************************************************************/
 	TEST_FEATURE ("ensure list-env in '--user --no-inherit-env' environment gives expected output");
@@ -16633,7 +16725,7 @@
 	}
 
 	TEST_DBUS (dbus_pid);
-	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
+	start_upstart_common (&upstart_pid, TRUE, FALSE, confdir, logdir, NULL);
 
 	cmd = nih_sprintf (NULL, "%s list-sessions 2>&1", get_initctl_binary ());
 	TEST_NE_P (cmd, NULL);
@@ -16727,6 +16819,7 @@
 	test_reexec ();
 	test_list_sessions ();
 	test_quiesce ();
+	test_umask ();
 
 	if (in_chroot () && !dbus_configured ()) {
 		fprintf(stderr, "\n\n"