Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 20 Mar 2016 15:56:55 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 1/3] overhaul environment functions

On Sun, Mar 20, 2016 at 07:15:52AM +0300, Alexander Monakov wrote:
> 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.

You're right; I missed this. So it looks to me like you're doing
exactly the right thing.

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

I missed that. Should be OK, then.

After debunking most of my concerns, are there any things left to
change in patch 1, or should I go ahead and apply it?

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.