From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9717 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: Mon, 21 Mar 2016 15:45:23 +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> <20160320195655.GA21636@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 1458564339 10762 80.91.229.3 (21 Mar 2016 12:45:39 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 21 Mar 2016 12:45:39 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9730-gllmg-musl=m.gmane.org@lists.openwall.com Mon Mar 21 13:45:39 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 1ahzDG-0004Eo-41 for gllmg-musl@m.gmane.org; Mon, 21 Mar 2016 13:45:38 +0100 Original-Received: (qmail 5876 invoked by uid 550); 21 Mar 2016 12:45:34 -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 5856 invoked from network); 21 Mar 2016 12:45:34 -0000 In-Reply-To: User-Agent: Alpine 2.20 (LNX 67 2015-01-07) Xref: news.gmane.org gmane.linux.lib.musl.general:9717 Archived-At: On Sun, 20 Mar 2016, Alexander Monakov wrote: > On Sun, 20 Mar 2016, Rich Felker wrote: > > After debunking most of my concerns, are there any things left to > > change in patch 1, or should I go ahead and apply it? > > Let me send a v2 with two changes: reinstating setenv EINVAL on null arg, > and tweaking unsetenv like I mentioned. Should be good with those changes, > I think. I've attempted another go at testing, and it turns out the new code still has a memory leak, although a subtler and smaller one. It is caused by failure to use existing null slots in __env_change when it is called with p != 0, but no slot in env_alloced matches p (so the early-out by replacing p by r and freeing p does not happen). I'm quite surprised I didn't catch this sooner, because the testcase for the existing leak (alternating setenv/putenv calls) also exposes this one, albeit at a slower rate. I've probably made some mistake like misinterpreting the exit status; not sure. The following incremental diff shows how I have addressed this issue. I'm leaving it for comments, and will send an updated patch with two changes mentioned above, plus __env_change renamed to __env_rm_add, some time later. Alexander diff --git a/src/env/setenv.c b/src/env/setenv.c index db9d72a..051c8d0 100644 --- a/src/env/setenv.c +++ b/src/env/setenv.c @@ -9,17 +9,23 @@ void __env_change(char *p, char *r) { static char **env_alloced; static size_t env_alloced_n; + char **nullslot = 0; for (size_t i=0; i < env_alloced_n; i++) if (env_alloced[i] == p) { env_alloced[i] = r; free(p); return; + } else if (!env_alloced[i]) { + nullslot = env_alloced + i; } if (!r) return; - char **new_ea = realloc(env_alloced, sizeof *new_ea * (env_alloced_n+1)); - if (!new_ea) return; - new_ea[env_alloced_n++] = r; - env_alloced = new_ea; + if (!nullslot) { + char **t = realloc(env_alloced, sizeof *t * (env_alloced_n+1)); + if (!t) return; + env_alloced = t; + nullslot = env_alloced + env_alloced_n++; + } + *nullslot = r; } int setenv(const char *var, const char *value, int overwrite)