From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9471 Path: news.gmane.org!not-for-mail From: Alexander Monakov Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] slim down and avoid undefined behavior in unsetenv Date: Sat, 5 Mar 2016 09:01:54 +0300 (MSK) Message-ID: References: <20160305051838.GC9349@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 1457157729 12853 80.91.229.3 (5 Mar 2016 06:02:09 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 5 Mar 2016 06:02:09 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9484-gllmg-musl=m.gmane.org@lists.openwall.com Sat Mar 05 07:02:09 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 1ac5Hz-0005iH-Rm for gllmg-musl@m.gmane.org; Sat, 05 Mar 2016 07:02:07 +0100 Original-Received: (qmail 10214 invoked by uid 550); 5 Mar 2016 06:02:06 -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 10194 invoked from network); 5 Mar 2016 06:02:05 -0000 In-Reply-To: <20160305051838.GC9349@brightrain.aerifal.cx> User-Agent: Alpine 2.20 (LNX 67 2015-01-07) Xref: news.gmane.org gmane.linux.lib.musl.general:9471 Archived-At: On Sat, 5 Mar 2016, Rich Felker wrote: > > -char **__env_map; > > +static char **__env_map; > > + > > +void __env_free(char *p) > > +{ > > + if (__env_map) > > Perhaps if (!__env_map) return; to avoid gratuitous indention of the > whole rest of the function? I don't mind; I did consider this point and went with this style because indent increase is not too bad and it's a bit easier to see that it just guards the for loop. But I can change it when resubmitting the patch. > Aside from that, I really like this, especially making __env_map > private. But perhaps we should rename it not to use __ prefix now that > it's static? Indeed, I haven't noticed that. Personally I'd prefer to make the rename a separate patch, though. > > extern char **__environ; > > -extern char **__env_map; > > + > > +static void dummy(char *p) {} > > +weak_alias(dummy, __env_free); > > This makes it so unsetenv no longer requires full malloc, I think, > right? Nice. That was your idea from the previous discussion :) > > + for (char **e = __environ; *e; ) > > + if (!memcmp(name, *e, l) && l[*e] == '=') { > > + char **ee = e, *tmp = *e; > > + do *ee = *(ee+1); > > + while (*++ee); > > We could use memmove here but I'm not sure if it's nicer or not. I guess not, without additional code tracking current size?.. > > + __env_free(tmp); > > + } else > > + e++; > > As a matter of style, if the 'if' body is a block I generally try to > do the same for the else. In that case I'd like to make that change while swapping the if/else branches around. > Also we're not using clause-1 declarations in for statements elsewhere > in musl afaik, but I'm not opposed to adopting their use where it > makes sense. There were a few instances of 'for (int i=0; ...)' already so I felt I have a license to do this :) > I think the loop logic might be clearer with indices instead of > pointers, but I'm not sure. Is there a reason you preferred switching > to pointers? Well, the whole thing started with removing benign undefined behavior, so I felt it's in line to remove int-indexing (not ssize_t) on an unbounded array. Apart from that, I like more how the 'for' statement reads with this change. > One nice (I think; others may disagree) aspect of indices > is that instead of the if/else we could just have an unconditional i++ > in the for's expression-3 and an i-- inside the if block. Yeah, that's a bit of a loss, but I hope it's alright and not too obfuscated. > These are all minor comments, and the patch looks like it could be > okay as-is. I didn't see any bugs. Have you done any testing? Nope, sorry; I'm not dogfooding musl. Alexander