[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.LNX.2.20.1603050844370.31711@monopod.intra.ispras.ru>
Date: Sat, 5 Mar 2016 09:01:54 +0300 (MSK)
From: Alexander Monakov <amonakov@...ras.ru>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] slim down and avoid undefined behavior in
unsetenv
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.
> 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.
> > 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 :)
> > + 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?..
> > + __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.
> 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 :)
> 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.
> 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.
> 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.
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.
