[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.LNX.2.20.1603200708060.10468@monopod.intra.ispras.ru>
Date: Sun, 20 Mar 2016 07:15:52 +0300 (MSK)
From: Alexander Monakov <amonakov@...ras.ru>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 1/3] overhaul environment functions
On Sat, 19 Mar 2016, Rich Felker wrote:
> > > > + 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.
>
> for (;;) { clearenv(); putenv("FOO=BAR"); }
>
> Since __environ!=oldenv at the time of every putenv call, each call
> allocates a new environment array and the old one is leaked.
But the old one is not leaked: like I said, 'free(oldenv)' frees it.
> > > > 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.
>
> Why? s points to memory allocated by malloc, and I don't see anywhere
> it's freed if __putenv fails.
In old code there's explicit 'free(s)' on fallthrough path if __putenv
returned -1; in new code __putenv frees its last argument on oom itself
(at 'oom:' label).
Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
