From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9358 Path: news.gmane.org!not-for-mail From: Alexander Monakov Newsgroups: gmane.linux.lib.musl.general Subject: [PATCH] slim down and avoid undefined behavior in unsetenv Date: Sat, 20 Feb 2016 21:09:57 +0300 (MSK) Message-ID: 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 1455991818 25328 80.91.229.3 (20 Feb 2016 18:10:18 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 20 Feb 2016 18:10:18 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9371-gllmg-musl=m.gmane.org@lists.openwall.com Sat Feb 20 19:10:18 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 1aXByz-00019z-PY for gllmg-musl@m.gmane.org; Sat, 20 Feb 2016 19:10:17 +0100 Original-Received: (qmail 30406 invoked by uid 550); 20 Feb 2016 18:10:14 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 30350 invoked from network); 20 Feb 2016 18:10:09 -0000 User-Agent: Alpine 2.20 (LNX 67 2015-01-07) Xref: news.gmane.org gmane.linux.lib.musl.general:9358 Archived-At: Implementation of unsetenv used to invoke (a benign) undefined behavior by using pointer value that was passed to free() in comparisons against NULL in two loops that would remove that pointer from __env_map and __environ arrays. Factor out handling of __env_map into a separate function __env_free and move it to putenv.c, allowing to make __env_map private to that file. Do not attempt to preserve order in __env_map when removing the entry (it is not important). --- On Mon, 4 Jan 2016, Rich Felker wrote: > 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. Thanks -- here's a patch. src/env/putenv.c | 15 ++++++++++++++- src/env/unsetenv.c | 29 +++++++++++++---------------- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/src/env/putenv.c b/src/env/putenv.c index 4042869..86b2024 100644 --- a/src/env/putenv.c +++ b/src/env/putenv.c @@ -2,7 +2,20 @@ #include extern char **__environ; -char **__env_map; +static char **__env_map; + +void __env_free(char *p) +{ + if (__env_map) + for (char **e = __env_map; *e; e++) + if (*e == p) { + char **ee = e; + while (*(ee+1)) ee++; + *e = *ee; *ee = 0; + free(p); + return; + } +} int __putenv(char *s, int a) { diff --git a/src/env/unsetenv.c b/src/env/unsetenv.c index 3569335..f0f369f 100644 --- a/src/env/unsetenv.c +++ b/src/env/unsetenv.c @@ -1,31 +1,28 @@ #include #include #include +#include "libc.h" extern char **__environ; -extern char **__env_map; + +static void dummy(char *p) {} +weak_alias(dummy, __env_free); int unsetenv(const char *name) { - int i, j; size_t l = strlen(name); - if (!*name || strchr(name, '=')) { + if (!*name || memchr(name, '=', l)) { errno = EINVAL; return -1; } -again: - for (i=0; __environ[i] && (memcmp(name, __environ[i], l) || __environ[i][l] != '='); i++); - if (__environ[i]) { - if (__env_map) { - for (j=0; __env_map[j] && __env_map[j] != __environ[i]; j++); - free (__env_map[j]); - for (; __env_map[j]; j++) - __env_map[j] = __env_map[j+1]; - } - for (; __environ[i]; i++) - __environ[i] = __environ[i+1]; - goto again; - } + for (char **e = __environ; *e; ) + if (!memcmp(name, *e, l) && l[*e] == '=') { + char **ee = e, *tmp = *e; + do *ee = *(ee+1); + while (*++ee); + __env_free(tmp); + } else + e++; return 0; }