From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9360 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: Sun, 21 Feb 2016 12:51:30 +0300 (MSK) Message-ID: References: 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 1456048313 16059 80.91.229.3 (21 Feb 2016 09:51:53 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 21 Feb 2016 09:51:53 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9373-gllmg-musl=m.gmane.org@lists.openwall.com Sun Feb 21 10:51:48 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 1aXQg5-0002JW-IR for gllmg-musl@m.gmane.org; Sun, 21 Feb 2016 10:51:45 +0100 Original-Received: (qmail 18272 invoked by uid 550); 21 Feb 2016 09:51:42 -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 18254 invoked from network); 21 Feb 2016 09:51:41 -0000 In-Reply-To: User-Agent: Alpine 2.20 (LNX 67 2015-01-07) Xref: news.gmane.org gmane.linux.lib.musl.general:9360 Archived-At: ... aaand the OCD kicks in :) On Sat, 20 Feb 2016, Alexander Monakov wrote: > diff --git a/src/env/unsetenv.c b/src/env/unsetenv.c > index 3569335..f0f369f 100644 > --- a/src/env/unsetenv.c > +++ b/src/env/unsetenv.c [snip] > size_t l = strlen(name); > > - if (!*name || strchr(name, '=')) { > + if (!*name || memchr(name, '=', l)) { Here I could have changed '!*name' to '!l' for a small cleanup as well. > errno = EINVAL; > return -1; > } Some places in musl tail-call to __syscall_ret(-Exxx) to set errno. I wonder if it's accidental or there's a guideline for using one style or the other? The only place I imagine the tailcall style might be undesired is sem_trywait, where returning failure is not expected to be rare. What do you think about a change that introduces __set_errno that accepts positive errno and returns -1L? With that change __syscall_ret can become return r < -4095UL ? r : __set_errno(-r); > -again: [snip] > + for (char **e = __environ; *e; ) > + if (!memcmp(name, *e, l) && l[*e] == '=') { Here the usage of memcmp requires that it scans buffers left-to-right and stops on first mismatch. As I understand the standards do not guarantee that, but musl's current implementation does, and is not interposable. Still, a gotcha. Alexander