From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9054 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] fix use of pointer after free in unsetenv Date: Mon, 4 Jan 2016 16:05:29 -0500 Message-ID: <20160104210528.GX238@brightrain.aerifal.cx> References: <5689AA38.60108@openwall.com> <20160104030558.GT238@brightrain.aerifal.cx> <568A4ED2.9020609@openwall.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1451941546 11522 80.91.229.3 (4 Jan 2016 21:05:46 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 4 Jan 2016 21:05:46 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9067-gllmg-musl=m.gmane.org@lists.openwall.com Mon Jan 04 22:05:46 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1aGCK0-0007N7-4n for gllmg-musl@m.gmane.org; Mon, 04 Jan 2016 22:05:44 +0100 Original-Received: (qmail 19683 invoked by uid 550); 4 Jan 2016 21:05:42 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 19661 invoked from network); 4 Jan 2016 21:05:41 -0000 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:9054 Archived-At: 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