Re: cluts review

[Solar Designer <solar@openwall.com>]

Rich,

Thank you for your comments.  Would you please start similarly reviewing
and commenting on Luka's code in here without me having to do it? ;-)

On Wed, Jul 13, 2011 at 09:38:38AM -0400, Rich Felker wrote:
> On Wed, Jul 13, 2011 at 03:07:23PM +0400, Solar Designer wrote:
> > The uses of SA_NODEFER appeared to be a bug anyway, so I am simply
> > removing them.
> 
> It's not a bug.

I meant that I saw no valid reason in the surrounding code to set that
flag.  But you appear to mention that there was a reason in one of the
instances.  Maybe there was.

> > +#define _BSD_SOURCE /* for scandir() and alphasort() */
> 
> These are POSIX 2008 functions. _BSD_SOURCE should not be needed for
> anything.

Is it acceptable to require POSIX 2008?  Don't we want cluts to build
and run on significantly older systems, which wouldn't know to define
these functions on -D_POSIX_C_SOURCE=200809L?

> > +#include <sys/param.h> /* for PATH_MAX */
> 
> This is a classic error I fought with all the time early in musl's
> life cycle - it's absolutely the wrong fix. sys/param.h is completely
> nonstandard. PATH_MAX comes from limits.h, as long as you have the
> proper feature test macros defined, but it might not be defined, in
> which case you have to use sysconf/pathconf. That could still come
> back as "no limit" though, in which case security for functions which
> need a PATH_MAX-sized buffer is broken...

What specific feature test macros would you recommend for getting
PATH_MAX defined?

> > +#define _XOPEN_SOURCE /* for sigaction() */
> 
> Needs a value, not a blank definition. Current version is 700.

My understanding is that blank means requesting an older version of the
spec, but I admit I am not sure which one - and as I pointed out in
another posting a while ago, it does appear to differ across systems.

So, your proposal is to always request the latest spec version.
Wouldn't it possibly be better to request the oldest sufficient for us?

> > -    act.sa_flags   = SA_NODEFER;
> > +    act.sa_flags   = 0;
> 
> This was being used as part of the longjmp trick.

Why, how?  Did we seriously want to keep the signal blocked?

> By the way, there are a lot of warnings about local vars potentially
> clobbered by longjmp. Those are worth checking out.

Right.

Thanks,

Alexander