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

[Dimitri John Ledkov]

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  <james.hunt at 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.hunt at 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);
> +

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.