Review: Approve
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.
Diff comments:
> === modified file 'ChangeLog'
> --- ChangeLog 2014-07-11 21:32:38 +0000
> +++ ChangeLog 2014-07-16 12:59:21 +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 12:59:21 +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 (char *) * (*len + 1)))
I would be inclined to say "sizeof (**env)" rather than "sizeof (char *)", on a
don't-repeat-yourself principle, and to make correctness clearer without having
to look up the type of env.
> + 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 12:59:21 +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);
> }
>
--
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
