Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 4 Jan 2016 16:05:29 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] fix use of pointer after free in unsetenv

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.

> > for (i=0; __environ[i]; i++) {
> > 	char *e = __environ[i];
> > 	if (!memcmp(name, e, l) && e[l] == '=') {
> > 		for (j=i--; __environ[j]; j++)
> > 			__environ[j] = __environ[j+1];
> > 		if (__env_map) {
> > 			for (j=0; __env_map[j] && __env_map[j] != e; j++);
> > 			if (__env_map[j]) {
> > 				free(__env_map[j]);
> > 				do __env_map[j] = __env_map[j+1];
> > 				while (__env_map[++j]);
> > 			}
> > 		}
> > 	}
> > }
> 
> 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". However the order in __env_map is irrelevant. Its only purpose is
to track which slots are allocated so we can free them.

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.