From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9472 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] slim down and avoid undefined behavior in unsetenv Date: Sat, 5 Mar 2016 01:14:45 -0500 Message-ID: <20160305061444.GG9349@brightrain.aerifal.cx> 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 1457158502 22794 80.91.229.3 (5 Mar 2016 06:15:02 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 5 Mar 2016 06:15:02 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9485-gllmg-musl=m.gmane.org@lists.openwall.com Sat Mar 05 07:15:02 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 1ac5US-0005fA-Sb for gllmg-musl@m.gmane.org; Sat, 05 Mar 2016 07:15:00 +0100 Original-Received: (qmail 17948 invoked by uid 550); 5 Mar 2016 06:14:59 -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 17927 invoked from network); 5 Mar 2016 06:14:58 -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:9472 Archived-At: On Sat, Mar 05, 2016 at 09:01:54AM +0300, Alexander Monakov wrote: > 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. OK. > > 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. Either way is fine with me. > > > 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 :) Ah. :) > > > + 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?.. You're right; I missed that. Perhaps tracking the current size would be nice, but I think it would have complexity cost we might not like. On the other hand tracking both the number of slots currently use and the allocated size of __env_map might be a good idea to avoid pathological realloc behavior. But I think this should be done separately if at all. > > > > + __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. OK. > > 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 :) Oh, I didn't remember there being any. OK. > > 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. Uhg, I missed that the types were wrong too. For the initial environment it's safe in practice to assume the indices fit in int, I think, but there's no reason it couldn't grow beyond that size via setenv/putenv. > > 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. Yeah, I think it's okay. > > 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. Then we should probably come up with a few good sanity-check and corner-case tests for the env functions to add to libc-test. The changes are probably all fine but in the 1.1.13 release cycle we had a bunch of stupid regressions in stuff that looked trivial so I'd like to get in a habit of adding testing when making changes in places that could cause widespready breakage, especially when the changes are not fixing presently-observable bugs. Rich