Review: Needs Information
See inline comment about memove call.
Diff comments:
> === modified file 'ChangeLog'
> --- ChangeLog 2014-07-11 21:32:38 +0000
> +++ ChangeLog 2014-07-16 09:47:35 +0000
> @@ -1,3 +1,11 @@
> +2014-07-16 James Hunt <james.h...@ubuntu.com>
> +
> + * 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 <james.h...@ubuntu.com>
>
> * 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 09:47:35 +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,17 @@
> * 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);
> +
This memmove call worries me. I thought that with recent glibc memmove/memcpy
are unsafe for dst < src, because the copy/move is done "backwards" and thus
overrides src data before it moves.
> + (*len)--;
> + }
> }
>
> - *env = new_env;
> - *len = new_len;
> -
> - 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 09:47:35 +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);
> }
>
> @@ -713,6 +718,7 @@
> TEST_NE_P (ret, NULL);
> TEST_EQ (len, 0);
> TEST_EQ_P (env[0], NULL);
> + TEST_FREE (removed);
>
> nih_free (env);
> }
> @@ -730,6 +736,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);
> }
>
> @@ -755,6 +764,7 @@
> TEST_NE_P (ret, NULL);
> TEST_EQ (len, 0);
> TEST_EQ_P (env[0], NULL);
> + TEST_FREE (removed);
>
> nih_free (env);
> }
> @@ -781,6 +791,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);
> @@ -816,6 +828,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 +865,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);
> @@ -887,6 +902,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 +937,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);
> }
> @@ -954,6 +972,7 @@
> TEST_EQ_STR (env[0], "FOO=BAR");
>
> TEST_EQ_P (env[1], NULL);
> + TEST_FREE (removed);
>
> nih_free (env);
> }
> @@ -991,6 +1010,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);
> }
>
> @@ -1023,6 +1045,7 @@
> TEST_EQ_STR (env[0], "FOO=BAR");
>
> TEST_EQ_P (env[1], NULL);
> + TEST_FREE (removed);
>
> nih_free (env);
> }
>
--
https://code.launchpad.net/~jamesodhunt/upstart/bug-1222705-reprise/+merge/226983
Your team Upstart Reviewers is subscribed to branch lp:upstart.
--
upstart-devel mailing list
upstart-devel@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/upstart-devel
