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

[James Hunt]

= Behaviour Change =

Previously running 'initctl list-env' in a Session Init environment would only list those minimal variables added by init (PATH and TERM) along with any variables explicitly set via 'initctl set-env'. However, it now lists that set plus all variables set in inits environment at startup time.

* init/job_class.c: job_class_environment_init(): Copy inits environment
  to the default job class environment for user mode where appropriate.
* init/job_process.c: job_process_run(): Don't copy inits environment
  into the job instances environment table as those values are now
  already in the table.
* util/tests/test_initctl.c: Updates for new behaviour (where
  'list-env' will now contain the entire environment of the init
  process, not just those variables explicitly set via set-env).
* util/man/initctl.8: Update on behaviour.

[Stéphane Graber (stgraber)]

I think this looks good.

Do we have a test that runs upstart with no_inherit_env set and checks that the environment in that case is sane (exact match to what we'd expect)?
If we don't, we definitely should add that as your branch relaxes some of the existing tests in that regard.

[lp:~jamesodhunt/upstart/bug-1159895]

1479. By James Hunt on 2013-06-03

* util/tests/test_initctl.c:
  - test_no_inherit_job_env(): New function to test --no-inherit-env.
  - test_job_env():
    - Move code that sets HOME+PATH if not set from
      test_default_job_env() so that test_no_inherit_job_env() can make
      use of it.
    - Session file cleanup tweaks to work with test_no_inherit_job_env().

[James Hunt (jamesodhunt)]

Good call - added some basic '--no-inherit-env' tests.

review: Needs Resubmitting

[Stéphane Graber (stgraber)]

Perfect, thanks.

Note that your changelog changes are currently failing to merge so you'll need to resolve that conflict on merge.

review: Approve

[lp:~jamesodhunt/upstart/bug-1159895]

1480. By James Hunt on 2013-06-20

Sync with lp:upstart.

=== modified file 'ChangeLog'
--- ChangeLog	2013-06-05 16:18:42 +0000
+++ ChangeLog	2013-06-20 09:37:27 +0000
@@ -33,8 +33,29 @@
 	  approach of detecting the format of the JSON is safer.
 	* init/state.h: Remove STATE_VERSION.
 
[phone]  James Hunt  <[e-mail]>
+
+	* util/tests/test_initctl.c:
+	  - test_no_inherit_job_env(): New function to test --no-inherit-env.
+	  - test_job_env():
+	    - Move code that sets HOME+PATH if not set from
+	      test_default_job_env() so that test_no_inherit_job_env() can make
+	      use of it.
+	    - Session file cleanup tweaks to work with test_no_inherit_job_env().
+
 2013-05-31  James Hunt  <[e-mail]>
 
+	* init/job_class.c: job_class_environment_init(): Copy inits environment
+	  to the default job class environment for user mode where appropriate.
+	  (LP: #1159895).
+	* init/job_process.c: job_process_run(): Don't copy inits environment
+	  into the job instances environment table as those values are now
+	  already in the table.
+	* util/tests/test_initctl.c: Updates for new behaviour (where
+	  'list-env' will now contain the entire environment of the init
+	  process, not just those variables explicitly set via set-env).
+	* util/man/initctl.8: Update on behaviour.
+
 	[ Eric S. Raymond <[e-mail]> ]
 	* init/man/init.5: Fix unliftable markup (LP: #1185108).
 

=== modified file 'init/job_class.c'
--- init/job_class.c	2013-05-29 10:36:36 +0000
+++ init/job_class.c	2013-06-20 09:37:27 +0000
@@ -62,6 +62,9 @@
 #include <json.h>
 
 extern json_object *json_classes;
+extern int user_mode;
+extern int no_inherit_env;
+extern char **environ;
 
 /* Prototypes for static functions */
 static void  job_class_add (JobClass *class);
@@ -115,10 +118,14 @@
 {
 	char * const default_environ[] = { JOB_DEFAULT_ENVIRONMENT, NULL };
 
-	if (! job_environ) {
-		job_environ = NIH_MUST (nih_str_array_new (NULL));
-		NIH_MUST (environ_append (&job_environ, NULL, 0, TRUE, default_environ));
-	}
+	if (job_environ)
+		return;
+
+	job_environ = NIH_MUST (nih_str_array_new (NULL));
+	NIH_MUST (environ_append (&job_environ, NULL, 0, TRUE, default_environ));
+
+	if (user_mode && ! no_inherit_env)
+		NIH_MUST(environ_append (&job_environ, NULL, 0, TRUE, environ));
 }
 
 /**

=== modified file 'init/job_process.c'
--- init/job_process.c	2013-05-24 17:21:47 +0000
+++ init/job_process.c	2013-06-20 09:37:27 +0000
@@ -283,9 +283,6 @@
 	envc = 0;
 	env = NIH_MUST (nih_str_array_new (NULL));
 
-	if (user_mode && ! no_inherit_env)
-		NIH_MUST(environ_append (&env, NULL, &envc, TRUE, environ));
-
 	if (job->env)
 		NIH_MUST(environ_append (&env, NULL, &envc, TRUE, job->env));
 

=== modified file 'util/man/initctl.8'
--- util/man/initctl.8	2013-02-15 14:07:05 +0000
+++ util/man/initctl.8	2013-06-20 09:37:27 +0000
@@ -565,10 +565,13 @@
 job-specific environment table; otherwise the global environment table
 that is applied to all jobs when they first start is queried.
 
-Note that the global job environment table only includes the minimal set
-of standard system variables added by the
-.BR init (8)
-daemon along with any variables set using
+Note that the global job environment table comprises those variables
+already set in the
+.BR init (8)
+daemons environment at startup, the minimal set of standard system
+variables added by the
+.BR init (8)
+daemon, and any variables set using
 .BR set\-env "."
 See
 .BR init (5)

=== modified file 'util/tests/test_initctl.c'
--- util/tests/test_initctl.c	2013-02-27 11:46:04 +0000
+++ util/tests/test_initctl.c	2013-06-20 09:37:27 +0000
@@ -16436,21 +16436,6 @@
 	assert (upstart_pid);
 	assert (dbus_pid);
 
-	/*******************************************************************/
-	/* Ensure basic variables are set in the current environment */
-
-	if (! getenv ("TERM")) {
-		fprintf (stderr, "WARNING: setting TERM to '%s' as not set\n",
-				TEST_INITCTL_DEFAULT_TERM);
-		assert0 (setenv ("TERM", TEST_INITCTL_DEFAULT_TERM, 1));
-	}
-
-	if (! getenv ("PATH")) {
-		fprintf (stderr, "WARNING: setting PATH to '%s' as not set\n",
-				TEST_INITCTL_DEFAULT_PATH);
-		assert0 (setenv ("PATH", TEST_INITCTL_DEFAULT_PATH, 1));
-	}
-
 	cmd = nih_sprintf (NULL, "%s reset-env 2>&1", get_initctl ());
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
@@ -16463,9 +16448,9 @@
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
 
-	TEST_EQ (line_count, 2);
-	TEST_STR_MATCH (output[0], "PATH=*");
-	TEST_STR_MATCH (output[1], "TERM=*");
+	TEST_GE (line_count, 2);
+	TEST_STR_ARRAY_CONTAINS (output, "PATH=*");
+	TEST_STR_ARRAY_CONTAINS (output, "TERM=*");
 	nih_free (output);
 
 	/*******************************************************************/
@@ -16475,9 +16460,9 @@
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
 
-	TEST_EQ (line_count, 2);
-	TEST_STR_MATCH (output[0], "PATH=*");
-	TEST_STR_MATCH (output[1], "TERM=*");
+	TEST_GE (line_count, 2);
+	TEST_STR_ARRAY_CONTAINS (output, "PATH=*");
+	TEST_STR_ARRAY_CONTAINS (output, "TERM=*");
 	nih_free (output);
 
 	/*******************************************************************/
@@ -16542,17 +16527,15 @@
 
 	fi = fopen (logfile, "r");
 	TEST_NE_P (fi, NULL);
-	TEST_FILE_MATCH (fi, "PATH=*");
-	TEST_FILE_MATCH (fi, "TERM=*");
+	TEST_FILE_CONTAINS (fi, "PATH=*");
+	TEST_FILE_CONTAINS (fi, "TERM=*");
 
 	/* asterisk required to match '\r\n' */
-	TEST_FILE_MATCH (fi, "UPSTART_JOB=foo*");
-	TEST_FILE_MATCH (fi, "UPSTART_INSTANCE=*");
-	TEST_FILE_MATCH (fi, "UPSTART_SESSION=*");
-	TEST_FILE_END (fi);
+	TEST_FILE_CONTAINS (fi, "UPSTART_JOB=foo*");
+	TEST_FILE_CONTAINS (fi, "UPSTART_INSTANCE=*");
+	TEST_FILE_CONTAINS (fi, "UPSTART_SESSION=*");
 	fclose (fi);
 
-
 	DELETE_FILE (confdir, "foo.conf");
 	TEST_EQ (unlink (logfile), 0);
 
@@ -16729,6 +16712,7 @@
 	cmd = nih_sprintf (NULL, "%s reset-env 2>&1", get_initctl ());
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
+	TEST_EQ (line_count, 0);
 	nih_free (output);
 
 	/* Ensure nothing changed */
@@ -16746,7 +16730,7 @@
 			name, value);
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	cmd = nih_sprintf (NULL, "%s get-env %s 2>&1", get_initctl (),
 			name);
@@ -16767,6 +16751,7 @@
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
 	TEST_EQ (line_count, 1);
 	TEST_EQ_STR (output[0], "initctl: No such variable: foo");
+	nih_free (output);
 
 	/*******************************************************************/
 	TEST_FEATURE ("set-env in 'name=' form");
@@ -16777,6 +16762,7 @@
 			name);
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
+	TEST_EQ (line_count, 0);
 	nih_free (output);
 
 	cmd = nih_sprintf (NULL, "%s get-env %s 2>&1", get_initctl (),
@@ -16802,6 +16788,7 @@
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
 	TEST_EQ (line_count, 1);
 	TEST_EQ_STR (output[0], "initctl: No such variable: foo");
+	nih_free (output);
 
 	/*******************************************************************/
 	TEST_FEATURE ("set-env in 'name' form");
@@ -16837,6 +16824,7 @@
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
 	TEST_EQ (line_count, 1);
 	TEST_EQ_STR (output[0], "initctl: No such variable: foo");
+	nih_free (output);
 
 	/*******************************************************************/
 	TEST_FEATURE ("set-env for already set variable");
@@ -16849,7 +16837,7 @@
 			name, value);
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	/* check it */
 	cmd = nih_sprintf (NULL, "%s get-env %s 2>&1", get_initctl (),
@@ -16866,7 +16854,7 @@
 			name, value);
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	/* check it again */
 	cmd = nih_sprintf (NULL, "%s get-env %s 2>&1", get_initctl (),
@@ -16889,6 +16877,7 @@
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
 	TEST_EQ (line_count, 1);
 	TEST_EQ_STR (output[0], "initctl: No such variable: foo");
+	nih_free (output);
 
 	/*******************************************************************/
 	TEST_FEATURE ("set-env --retain");
@@ -16901,7 +16890,7 @@
 			name, value);
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	/* check it */
 	cmd = nih_sprintf (NULL, "%s get-env %s 2>&1", get_initctl (),
@@ -16917,7 +16906,7 @@
 			get_initctl (), name, "HELLO");
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	/* check that value did *NOT* change */
 	cmd = nih_sprintf (NULL, "%s get-env %s 2>&1", get_initctl (),
@@ -16939,6 +16928,7 @@
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
 	TEST_EQ (line_count, 1);
 	TEST_EQ_STR (output[0], "initctl: No such variable: foo");
+	nih_free (output);
 
 	/*******************************************************************/
 	TEST_FEATURE ("set-env with space within value and trailing tab");
@@ -16950,7 +16940,7 @@
 			name, value);
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	cmd = nih_sprintf (NULL, "%s get-env %s 2>&1", get_initctl (),
 			name);
@@ -16971,6 +16961,7 @@
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
 	TEST_EQ (line_count, 1);
 	TEST_EQ_STR (output[0], "initctl: No such variable: foo");
+	nih_free (output);
 
 	/*******************************************************************/
 	TEST_FEATURE ("list-env output order");
@@ -16981,19 +16972,19 @@
 			"zygote", "cell");
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	cmd = nih_sprintf (NULL, "%s set-env %s='%s' 2>&1", get_initctl (),
 			"median", "middle");
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	cmd = nih_sprintf (NULL, "%s set-env %s='%s' 2>&1", get_initctl (),
 			"aardvark", "mammal");
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	cmd = nih_sprintf (NULL, "%s list-env 2>&1", get_initctl ());
 	TEST_NE_P (cmd, NULL);
@@ -17037,13 +17028,13 @@
 			"aardvark", "mammal");
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	cmd = nih_sprintf (NULL, "%s set-env %s='%s' 2>&1", get_initctl (),
 			"zygote", "cell");
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	cmd = nih_sprintf (NULL, "%s list-env 2>&1", get_initctl ());
 	TEST_NE_P (cmd, NULL);
@@ -17067,25 +17058,26 @@
 			"aardvark", "mammal");
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	cmd = nih_sprintf (NULL, "%s set-env %s='%s' 2>&1", get_initctl (),
 			"FOO", "BAR");
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	cmd = nih_sprintf (NULL, "%s set-env %s='%s' 2>&1", get_initctl (),
 			"_________", "_________");
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
-	nih_free (output);
+	TEST_EQ (line_count, 0);
 
 	CREATE_FILE (confdir, "modified-env.conf", "exec env");
 
 	cmd = nih_sprintf (NULL, "%s start modified-env 2>&1", get_initctl ());
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
+	TEST_EQ (line_count, 1);
 	nih_free (output);
 
 	logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
@@ -17211,6 +17203,7 @@
 	TEST_NE_P (cmd, NULL);
 
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
+	TEST_EQ (line_count, 1);
 	nih_free (output);
 
 	logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
@@ -17254,6 +17247,7 @@
 	cmd = nih_sprintf (NULL, "%s start foo 2>&1", get_initctl ());
 	TEST_NE_P (cmd, NULL);
 	RUN_COMMAND (NULL, cmd, &output, &line_count);
+	TEST_EQ (line_count, 1);
 	nih_free (output);
 
 	logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
@@ -17333,6 +17327,73 @@
 	/*******************************************************************/
 }
 
+void
+test_no_inherit_job_env (const char *runtimedir, const char *confdir, const char *logdir)
+{
+	nih_local char  *cmd = NULL;
+	char           **output;
+	size_t           lines;
+	pid_t            upstart_pid = 0;
+	char            *extra[] = { "--no-inherit-env", NULL };
+	nih_local char  *logfile = NULL;
+	nih_local char  *session_file = NULL;
+	FILE            *fi;
+
+	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, extra);
+
+	/*******************************************************************/
+	TEST_FEATURE ("ensure list-env in '--user --no-inherit-env' environment gives expected output");
+
+	cmd = nih_sprintf (NULL, "%s list-env 2>&1", get_initctl ());
+	TEST_NE_P (cmd, NULL);
+	RUN_COMMAND (NULL, cmd, &output, &lines);
+
+	/* environment should comprise the default environment only */
+	TEST_EQ (lines, 2);
+	TEST_STR_MATCH (output[0], "PATH=*");
+	TEST_STR_MATCH (output[1], "TERM=*");
+	nih_free (output);
+
+	/*******************************************************************/
+	TEST_FEATURE ("ensure '--user --no-inherit-env' provides expected job environment");
+
+	CREATE_FILE (confdir, "foo.conf", "exec env");
+
+	cmd = nih_sprintf (NULL, "%s start foo 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,
+				"foo.log"));
+
+	WAIT_FOR_FILE (logfile);
+
+	fi = fopen (logfile, "r");
+	TEST_NE_P (fi, NULL);
+	TEST_FILE_CONTAINS (fi, "PATH=*");
+	TEST_FILE_CONTAINS (fi, "TERM=*");
+
+	/* asterisk required to match '\r\n' */
+	TEST_FILE_CONTAINS (fi, "UPSTART_JOB=foo*");
+	TEST_FILE_CONTAINS (fi, "UPSTART_INSTANCE=*");
+	TEST_FILE_CONTAINS (fi, "UPSTART_SESSION=*");
+	fclose (fi);
+
+	DELETE_FILE (confdir, "foo.conf");
+	TEST_EQ (unlink (logfile), 0);
+
+	/*******************************************************************/
+
+	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions/%d.session",
+				runtimedir, (int)upstart_pid));
+
+	STOP_UPSTART (upstart_pid);
+
+	unlink (session_file);
+}
+
 /*
  * Test all the commands which affect the job environment table together
  * as they are so closely related.
@@ -17376,6 +17437,21 @@
 
 	TEST_EQ (setenv ("XDG_RUNTIME_DIR", runtimedir, 1), 0);
 
+	/*******************************************************************/
+	/* Ensure basic variables are set in the current environment */
+
+	if (! getenv ("TERM")) {
+		fprintf (stderr, "WARNING: setting TERM to '%s' as not set\n",
+				TEST_INITCTL_DEFAULT_TERM);
+		assert0 (setenv ("TERM", TEST_INITCTL_DEFAULT_TERM, 1));
+	}
+
+	if (! getenv ("PATH")) {
+		fprintf (stderr, "WARNING: setting PATH to '%s' as not set\n",
+				TEST_INITCTL_DEFAULT_PATH);
+		assert0 (setenv ("PATH", TEST_INITCTL_DEFAULT_PATH, 1));
+	}
+
 	TEST_DBUS (dbus_pid);
 	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
 
@@ -17410,14 +17486,21 @@
 	/*******************************************************************/
 
 	STOP_UPSTART (upstart_pid);
+	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions/%d.session",
+				runtimedir, (int)upstart_pid));
+	unlink (session_file);
+
+	/*******************************************************************/
+
+	test_no_inherit_job_env (runtimedir, confdir, logdir);
+
+	/*******************************************************************/
+
 	TEST_DBUS_END (dbus_pid);
 	assert0 (unsetenv ("UPSTART_CONFDIR"));
 	assert0 (unsetenv ("UPSTART_LOGDIR"));
 	assert0 (unsetenv ("UPSTART_SESSION"));
 
-	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions/%d.session",
-				runtimedir, (int)upstart_pid));
-	unlink (session_file);
 	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions", runtimedir));
         TEST_EQ (rmdir (session_file), 0);
 	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart", runtimedir));