[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160305061444.GG9349@brightrain.aerifal.cx>
Date: Sat, 5 Mar 2016 01:14:45 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] slim down and avoid undefined behavior in unsetenv
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
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.
