Date: Mon, 4 Jan 2016 16:53:15 -0500 From: Rich Felker <dalias@...c.org> To: musl@...ts.openwall.com Subject: Re: [PATCH] fix use of pointer after free in unsetenv 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
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.