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

[Alexander Monakov <amonakov@ispras.ru>]

On Sat, 5 Mar 2016, Rich Felker wrote:
> > -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?

I don't mind; I did consider this point and went with this style because
indent increase is not too bad and it's a bit easier to see that it just
guards the for loop. But I can change it when resubmitting the patch.

> 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?

Indeed, I haven't noticed that. Personally I'd prefer to make the rename a
separate patch, though.

> >  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.

That was your idea from the previous discussion :)

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

I guess not, without additional code tracking current size?..

> > +			__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.

In that case I'd like to make that change while swapping the if/else branches
around.

> 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.

There were a few instances of 'for (int i=0; ...)' already so I felt I have a
license to do this :)

> 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?

Well, the whole thing started with removing benign undefined behavior, so I
felt it's in line to remove int-indexing (not ssize_t) on an unbounded array.
Apart from that, I like more how the 'for' statement reads with this change.

> 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.

Yeah, that's a bit of a loss, but I hope it's alright and not too obfuscated.

> 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?

Nope, sorry; I'm not dogfooding musl.

Alexander