Re: [PATCH] fix use of pointer after free in unsetenv

[Rich Felker <dalias@libc.org>]

On Tue, Jan 05, 2016 at 12:28:12AM +0300, Alexander Monakov wrote:
> On Mon, 4 Jan 2016, Rich Felker wrote:
> > On Mon, Jan 04, 2016 at 06:47:36PM +0300, Alexander Monakov wrote:
> > > On Mon, 4 Jan 2016, Alexander Monakov wrote:
> > > > To me the implementation looks weird due to how it restarts scanning __environ
> > > > with 'goto again' from position 0 instead of current position. I can propose
> > > > the following rewrite (untested):
> > 
> > The "goto again" is for the rare (generally malicious) case of
> > duplicate definitions, to ensure that unsetenv removes them all.
> 
> Yes, but my point was that rewinding all the way back to i=0 looks odd -- I
> understood the need to scan all entries.

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.

> > > Hm, there's no need to preserve relative order of env entries, is there?
> > 
> > Yes, there is. If FOO=x and FOO=y both appear in environ[],
> > unsetenv("BAR") must not cause getenv("FOO") to change from "x" to
> > "y".
> 
> Thanks, I did not consider that. I'm curious, is that just from QoI
> perspective, or also required somewhere?

It is a requirement, and it's simply a consequence of the fact that
functions cannot have random side effects they're not specified to
have. unsetenv can't change the value of FOO from x to y any more than
calling strlen can. The fact that the issue arises at all when
environ[] was not setup manually by the program is a consequence of
the kernel not enforcing uniqueness of env var names, which (AFAIK) is
an implementation detail anyway.

Rich