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

[Dimitri John Ledkov]

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

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.