From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9713 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH 1/3] overhaul environment functions Date: Sun, 20 Mar 2016 15:56:55 -0400 Message-ID: <20160320195655.GA21636@brightrain.aerifal.cx> 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 1458503837 11278 80.91.229.3 (20 Mar 2016 19:57:17 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 20 Mar 2016 19:57:17 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9726-gllmg-musl=m.gmane.org@lists.openwall.com Sun Mar 20 20:57:16 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 1ahjTO-0004di-7S for gllmg-musl@m.gmane.org; Sun, 20 Mar 2016 20:57:14 +0100 Original-Received: (qmail 7213 invoked by uid 550); 20 Mar 2016 19:57:10 -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 7185 invoked from network); 20 Mar 2016 19:57:09 -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:9713 Archived-At: On Sun, Mar 20, 2016 at 07:15:52AM +0300, Alexander Monakov wrote: > 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. You're right; I missed this. So it looks to me like you're doing exactly the right thing. > > > > > 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). I missed that. Should be OK, then. After debunking most of my concerns, are there any things left to change in patch 1, or should I go ahead and apply it? Rich