Merge lp:~jamesodhunt/upstart/bug-1222705-reprise into lp:upstart

[James Hunt]

[Dimitri John Ledkov (xnox)]

See inline comment about memove call.

review: Needs Information

[Colin Watson (cjwatson)]

memcpy is unsafe for that, but the entire point of memmove is to be safe in this situation.

[Dimitri John Ledkov (xnox)]

memmove's are all good. Not sure if it's safe to use them against nih_alloced objects, given they have NihAllocCtx after them. Someone else should review this bit. See also the inline comment.

review: Needs Information

[lp:~jamesodhunt/upstart/bug-1222705-reprise]

1657. By James Hunt on 2014-07-16

* init/environ.c: environ_remove(): Shrink memory to fit what we're
  actually using.
* init/tests/test_environ.c: test_remove(): Remove now-invalid ENOMEM
  checks.

[James Hunt (jamesodhunt)]

It's safe because all the environ_*() calls operate on the result of nih_str_array_new(). That function will return a block of memory big enough to hold a single 'char *' pointer, and set it to NULL. The NihAllocCtx is situated _below_ the return value of nih_str_array_new() in memory and that single NihAllocCtx will be used to keep track of the size and child allocations of the entire array (should it grow bigger than its current size of zero (the terminator is not counted)).

Subsequent to nih_str_array_new() being called, that zero-sized array will be increased in size by calls to environ_add(), which in turn calls nih_str_array_addp(). That latter function then calls nih_realloc() to increase the size of the single NihAllocCtx previously mentioned.

So, if the array contains pointers to 10 env vars, it has size 11 (10 + 1 for the NULL terminator), and has a single NihAllocCtx just before the zeroth element. So it's actual byte size is:

  sizeof (NihAllocCtx) + (sizeof (char *) * (number_of_elements + 1))

As such, it is safe to call memmove to shuffle those pointers since they are the user-accessible way to reference the strings the array points to - Nih keeps track itself using the NihAllocCtx object.

I've added a call to nih_relloc() to compact the size of the array (from NIH's perspective) as it is wasteful (but not actually incorrect) not to call nih_realloc() and leave 'sizeof (char *)' bytes lying around until the array eventually gets deallocated.

[Colin Watson (cjwatson)]

I agree with your analysis re NihAllocCtx. This looks right, and the form of the new code matches a similar section from environ_add, so that's good. Just one style comment.

review: Approve

[James Hunt (jamesodhunt)]

To be convinced this fix works:

$ mkdir /tmp/conf /tmp/log
$ cat >>/tmp/conf/bug-1222705.conf<<EOT
start on startup

exec sleep 999
post-stop exec initctl unset-env wibble
EOT
$ gdb --args init/init --confdir /tmp/conf --logdir /tmp/log --debug
(gdb) run

In separate window:

$ initctl list-sessions
$ UPSTART_SESSION=unix:abstract=/com/ubuntu/upstart-session/$uid/$pid restart bug-1222705

[lp:~jamesodhunt/upstart/bug-1222705-reprise]

1658. By James Hunt on 2014-07-16

* init/environ.c: environ_remove(): Style tweak courtesy of cjwatson's
  review.

[James Hunt (jamesodhunt)]

Thanks - style tweak applied.

=== modified file 'ChangeLog'
--- ChangeLog	2014-07-11 21:32:38 +0000
+++ ChangeLog	2014-07-16 15:55:55 +0000
@@ -1,3 +1,11 @@
[phone]  James Hunt  <[e-mail]>
+
+	* init/environ.c: environ_remove():
+	  - Ensure removed entry is deref'd (LP: #1222705).
+	  - Avoid recreating array.
+	* init/tests/test_environ.c: test_remove(): Add checks to ensure
+	  removed entry freed as expected.
+
 2014-07-11  James Hunt  <[e-mail]>
 
 	* NEWS: Release 1.13

=== modified file 'init/environ.c'
--- init/environ.c	2013-10-24 13:33:05 +0000
+++ init/environ.c	2014-07-16 15:55:55 +0000
@@ -176,9 +176,7 @@
 {
 	size_t    _len;
 	size_t    keylen;
-	size_t    new_len = 0;
 	char    **e;
-	char    **new_env;
 
 	nih_assert (env);
 	nih_assert (str);
@@ -195,10 +193,6 @@
 	if (*len < 1)
 		return NULL;
 
-	new_env = nih_str_array_new (NULL);
-	if (! new_env)
-		return NULL;
-
 	for (e = *env; e && *e; e++) {
 		keylen = strcspn (*e, "=");
 
@@ -206,17 +200,21 @@
 		 * name=value pair, or a bare name), so don't copy it to
 		 * the new environment.
 		 */
-		if (! strncmp (str, *e, keylen))
-			continue;
-
-		if (! environ_add (&new_env, parent, &new_len, TRUE, *e))
-			return NULL;
+		if (! strncmp (str, *e, keylen)) {
+			nih_unref (*e, *env);
+
+			/* shuffle up the remaining entries */
+			memmove (e, e + 1, (char *)(*env + *len) - (char *)e);
+
+			(*len)--;
+		}
 	}
 
-	*env = new_env;
-	*len = new_len;
+	/* shrink amount of memory used */
+	if (! nih_realloc (*env, parent, sizeof (**env) * (*len + 1)))
+		return NULL;
 
-	return new_env;
+	return *env;
 }
 
 /**

=== modified file 'init/tests/test_environ.c'
--- init/tests/test_environ.c	2013-10-24 13:33:05 +0000
+++ init/tests/test_environ.c	2014-07-16 15:55:55 +0000
@@ -618,8 +618,10 @@
 void
 test_remove (void)
 {
-	char   **env = NULL, **ret;
-	size_t   len = 0;
+	char    **env = NULL;
+	char     *removed = NULL;
+	char    **ret = NULL;
+	size_t    len = 0;
 
 	TEST_FUNCTION ("environ_remove");
 
@@ -688,6 +690,9 @@
 			TEST_ALLOC_PARENT (env[0], env);
 			TEST_ALLOC_SIZE (env[0], 8);
 			TEST_EQ_STR (env[0], "FOO=BAR");
+			removed = env[0];
+			TEST_FREE_TAG (removed);
+
 			TEST_EQ_P (env[1], NULL);
 		}
 
@@ -695,17 +700,6 @@
 
 		if (test_alloc_failed) {
 			TEST_EQ_P (ret, NULL);
-
-			TEST_EQ (len, 1);
-
-			TEST_ALLOC_PARENT (env[0], env);
-			TEST_ALLOC_SIZE (env[0], 8);
-
-			TEST_NE_P (env[0], NULL);
-			TEST_EQ_STR (env[0], "FOO=BAR");
-
-			TEST_EQ_P (env[1], NULL);
-
 			nih_free (env);
 			continue;
 		}
@@ -713,6 +707,7 @@
 		TEST_NE_P (ret, NULL);
 		TEST_EQ (len, 0);
 		TEST_EQ_P (env[0], NULL);
+		TEST_FREE (removed);
 
 		nih_free (env);
 	}
@@ -730,6 +725,9 @@
 			TEST_ALLOC_PARENT (env[0], env);
 			TEST_ALLOC_SIZE (env[0], 8);
 			TEST_EQ_STR (env[0], "FOO=BAR");
+			removed = env[0];
+			TEST_FREE_TAG (removed);
+
 			TEST_EQ_P (env[1], NULL);
 		}
 
@@ -737,17 +735,6 @@
 
 		if (test_alloc_failed) {
 			TEST_EQ_P (ret, NULL);
-
-			TEST_EQ (len, 1);
-
-			TEST_ALLOC_PARENT (env[0], env);
-			TEST_ALLOC_SIZE (env[0], 8);
-
-			TEST_NE_P (env[0], NULL);
-			TEST_EQ_STR (env[0], "FOO=BAR");
-
-			TEST_EQ_P (env[1], NULL);
-
 			nih_free (env);
 			continue;
 		}
@@ -755,6 +742,7 @@
 		TEST_NE_P (ret, NULL);
 		TEST_EQ (len, 0);
 		TEST_EQ_P (env[0], NULL);
+		TEST_FREE (removed);
 
 		nih_free (env);
 	}
@@ -781,6 +769,8 @@
 			TEST_ALLOC_PARENT (env[0], env);
 			TEST_ALLOC_SIZE (env[0], 8);
 			TEST_EQ_STR (env[0], "FOO=BAR");
+			removed = env[0];
+			TEST_FREE_TAG (removed);
 
 			TEST_ALLOC_PARENT (env[1], env);
 			TEST_ALLOC_SIZE (env[1], 8);
@@ -794,18 +784,6 @@
 
 		if (test_alloc_failed) {
 			TEST_EQ_P (ret, NULL);
-
-			TEST_EQ (len, 2);
-			TEST_ALLOC_PARENT (env[0], env);
-			TEST_ALLOC_SIZE (env[0], 8);
-			TEST_EQ_STR (env[0], "FOO=BAR");
-
-			TEST_ALLOC_PARENT (env[1], env);
-			TEST_ALLOC_SIZE (env[1], 8);
-			TEST_EQ_STR (env[1], "BAZ=QUX");
-
-			TEST_EQ_P (env[2], NULL);
-
 			nih_free (env);
 			continue;
 		}
@@ -816,6 +794,7 @@
 		TEST_ALLOC_PARENT (env[0], env);
 		TEST_ALLOC_SIZE (env[0], 8);
 		TEST_EQ_STR (env[0], "BAZ=QUX");
+		TEST_FREE (removed);
 
 		TEST_EQ_P (env[1], NULL);
 
@@ -852,6 +831,8 @@
 			TEST_ALLOC_PARENT (env[0], env);
 			TEST_ALLOC_SIZE (env[0], 8);
 			TEST_EQ_STR (env[0], "UPSTART_TEST_VARIABLE=foo");
+			removed = env[0];
+			TEST_FREE_TAG (removed);
 
 			TEST_ALLOC_PARENT (env[1], env);
 			TEST_ALLOC_SIZE (env[1], 8);
@@ -865,18 +846,6 @@
 
 		if (test_alloc_failed) {
 			TEST_EQ_P (ret, NULL);
-
-			TEST_EQ (len, 2);
-			TEST_ALLOC_PARENT (env[0], env);
-			TEST_ALLOC_SIZE (env[0], 8);
-			TEST_EQ_STR (env[0], "UPSTART_TEST_VARIABLE=foo");
-
-			TEST_ALLOC_PARENT (env[1], env);
-			TEST_ALLOC_SIZE (env[1], 8);
-			TEST_EQ_STR (env[1], "BAZ=QUX");
-
-			TEST_EQ_P (env[2], NULL);
-
 			nih_free (env);
 			continue;
 		}
@@ -887,6 +856,7 @@
 		TEST_ALLOC_PARENT (env[0], env);
 		TEST_ALLOC_SIZE (env[0], 8);
 		TEST_EQ_STR (env[0], "BAZ=QUX");
+		TEST_FREE (removed);
 
 		TEST_EQ_P (env[1], NULL);
 
@@ -921,6 +891,8 @@
 			TEST_ALLOC_PARENT (env[1], env);
 			TEST_ALLOC_SIZE (env[1], 8);
 			TEST_EQ_STR (env[1], "BAZ=QUX");
+			removed = env[1];
+			TEST_FREE_TAG (removed);
 
 			TEST_EQ_P (env[2], NULL);
 		}
@@ -930,18 +902,6 @@
 
 		if (test_alloc_failed) {
 			TEST_EQ_P (ret, NULL);
-
-			TEST_EQ (len, 2);
-			TEST_ALLOC_PARENT (env[0], env);
-			TEST_ALLOC_SIZE (env[0], 8);
-			TEST_EQ_STR (env[0], "FOO=BAR");
-
-			TEST_ALLOC_PARENT (env[1], env);
-			TEST_ALLOC_SIZE (env[1], 8);
-			TEST_EQ_STR (env[1], "BAZ=QUX");
-
-			TEST_EQ_P (env[2], NULL);
-
 			nih_free (env);
 			continue;
 		}
@@ -954,6 +914,7 @@
 		TEST_EQ_STR (env[0], "FOO=BAR");
 
 		TEST_EQ_P (env[1], NULL);
+		TEST_FREE (removed);
 
 		nih_free (env);
 	}
@@ -991,6 +952,9 @@
 			/* Should have been expanded */
 			TEST_EQ_STR (env[1], "UPSTART_TEST_VARIABLE=foo");
 
+			removed = env[1];
+			TEST_FREE_TAG (removed);
+
 			TEST_EQ_P (env[2], NULL);
 		}
 
@@ -999,18 +963,6 @@
 
 		if (test_alloc_failed) {
 			TEST_EQ_P (ret, NULL);
-
-			TEST_EQ (len, 2);
-			TEST_ALLOC_PARENT (env[0], env);
-			TEST_ALLOC_SIZE (env[0], 8);
-			TEST_EQ_STR (env[0], "FOO=BAR");
-
-			TEST_ALLOC_PARENT (env[1], env);
-			TEST_ALLOC_SIZE (env[1], 8);
-			TEST_EQ_STR (env[1], "UPSTART_TEST_VARIABLE=foo");
-
-			TEST_EQ_P (env[2], NULL);
-
 			nih_free (env);
 			continue;
 		}
@@ -1023,6 +975,7 @@
 		TEST_EQ_STR (env[0], "FOO=BAR");
 
 		TEST_EQ_P (env[1], NULL);
+		TEST_FREE (removed);
 
 		nih_free (env);
 	}