Re: [PATCH 1/3] overhaul environment functions

[Alexander Monakov <amonakov@ispras.ru>]

On Sat, 19 Mar 2016, Rich Felker wrote:
> > > > +	static char **oldenv;
> > > > +	char **newenv;
> > > > +	if (__environ == oldenv) {
> > > > +		newenv = realloc(oldenv, sizeof *newenv * (i+2));
> > > > +		if (!newenv) goto oom;
> > > > +	} else {
> > > > +		newenv = malloc(sizeof *newenv * (i+2));
> > > > +		if (!newenv) goto oom;
> > > > +		if (i) memcpy(newenv, __environ, sizeof *newenv * i);
> > > > +		free(oldenv);
> > > > +	}
> > > 
> > > Rather than using malloc when __environ != oldenv, I think we should
> > > use realloc on oldenv, so that we don't leak internally-allocated
> > > environ arrays if the program repeatedly does environ=0 or calls your
> > > new clearenv.
> > 
> > How can we leak internally allocated environ here? If there's one, oldenv
> > points to it, and we free it right after memcpy.
> 
> for (;;) { clearenv(); putenv("FOO=BAR"); }
> 
> Since __environ!=oldenv at the time of every putenv call, each call
> allocates a new environment array and the old one is leaked.

But the old one is not leaked: like I said, 'free(oldenv)' frees it.

> > > >  	if (!overwrite && getenv(var)) return 0;
> > > >  
> > > > -	l1 = strlen(var);
> > > >  	l2 = strlen(value);
> > > >  	s = malloc(l1+l2+2);
> > > > -	if (s) {
> > > > -		memcpy(s, var, l1);
> > > > -		s[l1] = '=';
> > > > -		memcpy(s+l1+1, value, l2);
> > > > -		s[l1+l2+1] = 0;
> > > > -		if (!__putenv(s, 1)) return 0;
> > > > -	}
> > > > -	free(s);
> > > > -	return -1;
> > > > +	if (!s) return -1;
> > > > +	memcpy(s, var, l1);
> > > > +	s[l1] = '=';
> > > > +	memcpy(s+l1+1, value, l2+1);
> > > > +	return __putenv(s, l1, s);
> > > >  }
> > > 
> > > This leaks when __putenv fails. It's no worse than before but should
> > > probably be fixed.
> > 
> > Only when __env_change, specifically, "fails"; not when __putenv OOMs.
> 
> Why? s points to memory allocated by malloc, and I don't see anywhere
> it's freed if __putenv fails.

In old code there's explicit 'free(s)' on fallthrough path if __putenv
returned -1; in new code __putenv frees its last argument on oom itself
(at 'oom:' label).

Alexander