From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9709 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: Sat, 19 Mar 2016 21:11:16 +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> 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 1458411092 19373 80.91.229.3 (19 Mar 2016 18:11:32 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 19 Mar 2016 18:11:32 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9722-gllmg-musl=m.gmane.org@lists.openwall.com Sat Mar 19 19:11:30 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 1ahLLW-0003W4-1O for gllmg-musl@m.gmane.org; Sat, 19 Mar 2016 19:11:30 +0100 Original-Received: (qmail 7358 invoked by uid 550); 19 Mar 2016 18:11:28 -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 7339 invoked from network); 19 Mar 2016 18:11:27 -0000 In-Reply-To: <20160319164624.GW21636@brightrain.aerifal.cx> User-Agent: Alpine 2.20 (LNX 67 2015-01-07) Xref: news.gmane.org gmane.linux.lib.musl.general:9709 Archived-At: On Sat, 19 Mar 2016, Rich Felker wrote: > > putenv: > > * handle "=value" input via unsetenv too (will return -1/EINVAL); > > What happens now? It adds an env var with zero-length name? Returns -1 without modifying errno. > > Not changed: > > Failure to extend allocation tracking array (previously __env_map, now > > env_alloced) is ignored rather than causing to report -1/ENOMEM to the > > caller; the worst-case consequence is leaking this allocation when it > > is removed or replaced in a subsequent environment access. > > I'd like to fix this too but it can wait. I think it should be trivial > to do as a small patch on top. I thought about that, but decided to avoid doing that in the first cut (__env_change return type changes to 'int', dummy definitions need to be adjusted accordingly, and oom handling in __env_change is not beautiful either) > > --- a/src/env/putenv.c > > +++ b/src/env/putenv.c > > @@ -1,58 +1,47 @@ > > +#include > > +#include > > +#include "libc.h" > > + > > +char *__strchrnul(const char *, int); > > + > > +static void dummy(char *p, char *r) {} > > +weak_alias(dummy, __env_change); > > + > > +int __putenv(char *s, size_t l, char *r) > > +{ > > + size_t i=0; > > + if (__environ) > > + for (; __environ[i]; i++) > > + if (!strncmp(__environ[i], s, l+1)) { > > + char *tmp = __environ[i]; > > + __environ[i] = s; > > + __env_change(tmp, r); > > + return 0; > > + } > > As far as I can tell, this leaves multiple definitions in place. Am I > missing something? Maybe it's only unset and not replacement that's > safe against multiple-definition madness? Removing multiple definitions is what patch 2/3 does. This patch just keeps the status quo (only unsetenv looks at duplicate definitions). > > > + 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. I think realloc can be used if the program does not modify environ, but if it does something funky like 'environ++[2] = 0;' then memcpy'ing after realloc is not safe (unlike doing malloc-memcpy-free as above). > Perhaps we should also store the allocated size and grow > it exponentially rather than assuming it's only the filled size and > calling realloc every time, but maybe it's actually better to resize > up/down. Do you have an opinion? Some fraction of times realloc will keep it in place due to binning, right? I think taking 'simplicity' in efficiency-simplicity tradeoff here is justified, on the basis that majority of software does not repeatedly change environment, and for shells this libc facility is of little help anyway. > > int setenv(const char *var, const char *value, int overwrite) > > { > > char *s; > > - int l1, l2; > > - > > - if (!var || !*var || strchr(var, '=')) { > > + size_t l1 = __strchrnul(var, '=') - var, l2; > > + if (!l1 || var[l1]) { > > errno = EINVAL; > > return -1; > > } > > As mentioned above we should probably keep the POSIX-old behavior of > accepting a null pointer and treating it as a diagnosed error. OK; in an old revision I had 'if (!var || !(l1 = __strchrnul(var, '=')) ...' > > 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. > > +#include > > +#include > > +#include > > +#include "libc.h" > > + > > +char *__strchrnul(const char *, int); > > + > > +static void dummy(char *p, char *r) {} > > +weak_alias(dummy, __env_change); > > + > > +int unsetenv(const char *name) > > +{ > > + size_t l = __strchrnul(name, '=') - name; > > + if (!l || name[l]) { > > + errno = EINVAL; > > + return -1; > > + } > > + if (!__environ) return 0; > > + for (char **e = __environ; *e; e++) > > + while (*e && !strncmp(name, *e, l) && l[*e] == '=') { > > + char **ee = e, *tmp = *e; > > + do *ee = *(ee+1); > > + while (*++ee); > > + __env_change(tmp, 0); > > + } > > + return 0; > > +} > > I think this looks ok. I'd like to add a minor tweak here: instead of retesting '*e' in 'while' loop header, do 'if (!*e) return 0;' after __env_change; this helps the compiler to generate slightly cleaner code. Thanks for the review! Alexander