From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9712 Path: news.gmane.org!not-for-mail From: Alexander Monakov Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH 1/3] overhaul environment functions Date: Sun, 20 Mar 2016 07:15:52 +0300 (MSK) Message-ID: References: <1457895230-13602-1-git-send-email-amonakov@ispras.ru> <1457895230-13602-2-git-send-email-amonakov@ispras.ru> <20160319164624.GW21636@brightrain.aerifal.cx> <20160320002356.GX21636@brightrain.aerifal.cx> 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 1458447367 28034 80.91.229.3 (20 Mar 2016 04:16:07 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 20 Mar 2016 04:16:07 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9725-gllmg-musl=m.gmane.org@lists.openwall.com Sun Mar 20 05:16:07 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 1ahUmd-0006ob-5g for gllmg-musl@m.gmane.org; Sun, 20 Mar 2016 05:16:07 +0100 Original-Received: (qmail 28068 invoked by uid 550); 20 Mar 2016 04:16:04 -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 28050 invoked from network); 20 Mar 2016 04:16:03 -0000 In-Reply-To: <20160320002356.GX21636@brightrain.aerifal.cx> User-Agent: Alpine 2.20 (LNX 67 2015-01-07) Xref: news.gmane.org gmane.linux.lib.musl.general:9712 Archived-At: On Sat, 19 Mar 2016, Rich Felker wrote: > > > > + static char **oldenv; > > > > + char **newenv; > > > > + if (__environ == oldenv) { > > > > + newenv = realloc(oldenv, sizeof *newenv * (i+2)); > > > > + if (!newenv) goto oom; > > > > + } else { > > > > + newenv = malloc(sizeof *newenv * (i+2)); > > > > + if (!newenv) goto oom; > > > > + if (i) memcpy(newenv, __environ, sizeof *newenv * i); > > > > + free(oldenv); > > > > + } > > > > > > Rather than using malloc when __environ != oldenv, I think we should > > > use realloc on oldenv, so that we don't leak internally-allocated > > > environ arrays if the program repeatedly does environ=0 or calls your > > > new clearenv. > > > > How can we leak internally allocated environ here? If there's one, oldenv > > points to it, and we free it right after memcpy. > > for (;;) { clearenv(); putenv("FOO=BAR"); } > > Since __environ!=oldenv at the time of every putenv call, each call > allocates a new environment array and the old one is leaked. But the old one is not leaked: like I said, 'free(oldenv)' frees it. > > > > if (!overwrite && getenv(var)) return 0; > > > > > > > > - l1 = strlen(var); > > > > l2 = strlen(value); > > > > s = malloc(l1+l2+2); > > > > - if (s) { > > > > - memcpy(s, var, l1); > > > > - s[l1] = '='; > > > > - memcpy(s+l1+1, value, l2); > > > > - s[l1+l2+1] = 0; > > > > - if (!__putenv(s, 1)) return 0; > > > > - } > > > > - free(s); > > > > - return -1; > > > > + if (!s) return -1; > > > > + memcpy(s, var, l1); > > > > + s[l1] = '='; > > > > + memcpy(s+l1+1, value, l2+1); > > > > + return __putenv(s, l1, s); > > > > } > > > > > > This leaks when __putenv fails. It's no worse than before but should > > > probably be fixed. > > > > Only when __env_change, specifically, "fails"; not when __putenv OOMs. > > Why? s points to memory allocated by malloc, and I don't see anywhere > it's freed if __putenv fails. In old code there's explicit 'free(s)' on fallthrough path if __putenv returned -1; in new code __putenv frees its last argument on oom itself (at 'oom:' label). Alexander