[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160320195655.GA21636@brightrain.aerifal.cx>
Date: Sun, 20 Mar 2016 15:56:55 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 1/3] overhaul environment functions
On Sun, Mar 20, 2016 at 07:15:52AM +0300, Alexander Monakov wrote:
> 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.
You're right; I missed this. So it looks to me like you're doing
exactly the right thing.
> > > > > 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).
I missed that. Should be OK, then.
After debunking most of my concerns, are there any things left to
change in patch 1, or should I go ahead and apply it?
Rich
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.
