Re: [PATCH] slim down and avoid undefined behavior in unsetenv

[Rich Felker <dalias@libc.org>]

On Sat, Feb 20, 2016 at 09:09:57PM +0300, Alexander Monakov wrote:
> Implementation of unsetenv used to invoke (a benign) undefined
> behavior by using pointer value that was passed to free() in
> comparisons against NULL in two loops that would remove that pointer
> from __env_map and __environ arrays.
> 
> Factor out handling of __env_map into a separate function __env_free
> and move it to putenv.c, allowing to make __env_map private to that
> file. Do not attempt to preserve order in __env_map when removing the
> entry (it is not important).
> ---
> On Mon, 4 Jan 2016, Rich Felker wrote:
> > Oh. In that case I guess it's unnecessary to rewind, yes.
> > 
> > BTW what might be best to do is something like:
> > 
> > 	char *tmp = __environ[i];
> > 	for (j=i ; __environ[j]; j++)
> > 		__environ[j] = __environ[j+1];
> > 	__env_free(tmp);
> > 
> > where __env_free has a weak def as a nop and gets its strong def from
> > setenv.c or putenv.c. This refactoring would make it possible for
> > unsetenv not to pull in free, and the reordering might make it less
> > likely for dangerous things to happen in a broken program that reads
> > the environment concurrently with unsetenv.
> 
> Thanks -- here's a patch.
> 
>  src/env/putenv.c   | 15 ++++++++++++++-
>  src/env/unsetenv.c | 29 +++++++++++++----------------
>  2 files changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/src/env/putenv.c b/src/env/putenv.c
> index 4042869..86b2024 100644
> --- a/src/env/putenv.c
> +++ b/src/env/putenv.c
> @@ -2,7 +2,20 @@
>  #include <string.h>
>  
>  extern char **__environ;
> -char **__env_map;
> +static char **__env_map;
> +
> +void __env_free(char *p)
> +{
> +	if (__env_map)

Perhaps if (!__env_map) return; to avoid gratuitous indention of the
whole rest of the function?

> +		for (char **e = __env_map; *e; e++)
> +			if (*e == p) {
> +				char **ee = e;
> +				while (*(ee+1)) ee++;
> +				*e = *ee; *ee = 0;
> +				free(p);
> +				return;
> +			}
> +}

Aside from that, I really like this, especially making __env_map
private. But perhaps we should rename it not to use __ prefix now that
it's static?

>  int __putenv(char *s, int a)
>  {
> diff --git a/src/env/unsetenv.c b/src/env/unsetenv.c
> index 3569335..f0f369f 100644
> --- a/src/env/unsetenv.c
> +++ b/src/env/unsetenv.c
> @@ -1,31 +1,28 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <errno.h>
> +#include "libc.h"
>  
>  extern char **__environ;
> -extern char **__env_map;
> +
> +static void dummy(char *p) {}
> +weak_alias(dummy, __env_free);

This makes it so unsetenv no longer requires full malloc, I think,
right? Nice.

>  int unsetenv(const char *name)
>  {
> -	int i, j;
>  	size_t l = strlen(name);
>  
> -	if (!*name || strchr(name, '=')) {
> +	if (!*name || memchr(name, '=', l)) {
>  		errno = EINVAL;
>  		return -1;
>  	}
> -again:
> -	for (i=0; __environ[i] && (memcmp(name, __environ[i], l) || __environ[i][l] != '='); i++);
> -	if (__environ[i]) {
> -		if (__env_map) {
> -			for (j=0; __env_map[j] && __env_map[j] != __environ[i]; j++);
> -			free (__env_map[j]);
> -			for (; __env_map[j]; j++)
> -				__env_map[j] = __env_map[j+1];
> -		}
> -		for (; __environ[i]; i++)
> -			__environ[i] = __environ[i+1];
> -		goto again;
> -	}
> +	for (char **e = __environ; *e; )
> +		if (!memcmp(name, *e, l) && l[*e] == '=') {
> +			char **ee = e, *tmp = *e;
> +			do *ee = *(ee+1);
> +			while (*++ee);

We could use memmove here but I'm not sure if it's nicer or not.

> +			__env_free(tmp);
> +		} else
> +			e++;

As a matter of style, if the 'if' body is a block I generally try to
do the same for the else.

Also we're not using clause-1 declarations in for statements elsewhere
in musl afaik, but I'm not opposed to adopting their use where it
makes sense.

I think the loop logic might be clearer with indices instead of
pointers, but I'm not sure. Is there a reason you preferred switching
to pointers? One nice (I think; others may disagree) aspect of indices
is that instead of the if/else we could just have an unconditional i++
in the for's expression-3 and an i-- inside the if block.

These are all minor comments, and the patch looks like it could be
okay as-is. I didn't see any bugs. Have you done any testing?

Rich