Review: Needs Information 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.
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 <[email protected]> > + > + * 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 <[email protected]> > > * 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); > + Actually cjwatson & manpages cleared this up, that it's the memcpy that's unsafe for overlapping memory and memmove is the correct call to make. However, I am now question whether it is safe to in-place molest ***env (pointer to char**). Given that ***env array may be nih_alloc'ed and thus its total size is not just the size of it's type, but memory alligned NihAllocCtx structure after it as well. Which would mean memmoves are not safe against NIH objects. > + (*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 [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/upstart-devel
