[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160305051838.GC9349@brightrain.aerifal.cx>
Date: Sat, 5 Mar 2016 00:18:38 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] slim down and avoid undefined behavior in unsetenv
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
