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

[Rich Felker <dalias@libc.org>]

On Sat, Mar 05, 2016 at 09:01:54AM +0300, Alexander Monakov wrote:
> 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.

OK.

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

Either way is fine with me.

> > >  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 :)

Ah. :)

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

You're right; I missed that. Perhaps tracking the current size would
be nice, but I think it would have complexity cost we might not like.

On the other hand tracking both the number of slots currently use and
the allocated size of __env_map might be a good idea to avoid
pathological realloc behavior. But I think this should be done
separately if at all.

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

OK.

> > 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 :)

Oh, I didn't remember there being any. OK.

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

Uhg, I missed that the types were wrong too. For the initial
environment it's safe in practice to assume the indices fit in int, I
think, but there's no reason it couldn't grow beyond that size via
setenv/putenv.

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

Yeah, I think it's okay.

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

Then we should probably come up with a few good sanity-check and
corner-case tests for the env functions to add to libc-test. The
changes are probably all fine but in the 1.1.13 release cycle we had a
bunch of stupid regressions in stuff that looked trivial so I'd like
to get in a habit of adding testing when making changes in places that
could cause widespready breakage, especially when the changes are not
fixing presently-observable bugs.

Rich