cluts review

cluts review

[Solar Designer]

Hi Luka, Rich -

I've just experimented with cluts a bit.  I'll start by posting a patch
containing only changes that I had to make for cluts to build (without
fixing any warnings, etc. - there are still lots of them).

The uses of SA_NODEFER appeared to be a bug anyway, so I am simply
removing them.

-std=c99 appears to be needed to get LLONG_MAX, etc. defined by glibc's
header files when compiling with older gcc (I used 3.4.5).

As it is, this patch is a hack.  I think that extensive cleanups to the
code being patched are needed.

I have other observations as well (such as from actually running the
tests), which I hope to post separately.

diff -urp cluts.orig/cluts.c cluts/cluts.c
--- cluts.orig/cluts.c	2011-07-09 11:26:23 +0000
+++ cluts/cluts.c	2011-07-13 10:13:29 +0000
@@ -1,3 +1,4 @@
+#define _BSD_SOURCE /* for scandir() and alphasort() */
 #include <dirent.h>
 #include <stdio.h>
 #include <unistd.h>
@@ -5,6 +6,7 @@
 #include <sys/stat.h>
 #include <errno.h>
 #include <sys/wait.h>
+#include <sys/param.h> /* for PATH_MAX */
 
 /*
  * Copyright (c) 2011 Luka Marд█etiд┤<paxcoder@gmail.com>
diff -urp cluts.orig/makefile cluts/makefile
--- cluts.orig/makefile	2011-07-09 11:26:23 +0000
+++ cluts/makefile	2011-07-13 10:08:29 +0000
@@ -7,7 +7,7 @@
 # There's ABSOLUTELY NO WARRANTY, express or implied.
 #
 
-CFLAGS = -O2 -Wall -Wextra
+CFLAGS = -std=c99 -O2 -Wall -Wextra
 LIBS = -lpthread -lrt -lm
 SRC = $(wildcard tests/*.c) $(wildcard *.c)
 BIN = $(SRC:.c=)
diff -urp cluts.orig/tests/alloc.c cluts/tests/alloc.c
--- cluts.orig/tests/alloc.c	2011-07-09 11:26:23 +0000
+++ cluts/tests/alloc.c	2011-07-13 10:09:50 +0000
@@ -1,3 +1,4 @@
+#define _XOPEN_SOURCE /* for sigaction() */
 #include <stdio.h>
 #include <stdlib.h>
 #include <pthread.h>
@@ -57,7 +58,7 @@ int main()
     sem_t sem; ///< semaphore is used for thread mutex
 
     act.sa_handler = bridge_sig_jmp;
-    act.sa_flags   = SA_NODEFER;
+    act.sa_flags   = 0;
     sigaction(SIGSEGV, &act, &oldact[0]);
     
     //make a head element for the ring-queue which will hold info about blocks:
@@ -230,7 +231,7 @@ int safe_free(void *vp, char *err_msg)
 {
         struct sigaction oldact, act;
         act.sa_handler = bridge_sig_jmp;
-        act.sa_flags   = SA_NODEFER;
+        act.sa_flags   = 0;
         
         sigaction(SIGABRT, &act, &oldact);
         if (!setjmp(env[1]))
diff -urp cluts.orig/tests/string.c cluts/tests/string.c
--- cluts.orig/tests/string.c	2011-07-09 11:26:23 +0000
+++ cluts/tests/string.c	2011-07-13 10:10:25 +0000
@@ -1,3 +1,4 @@
+#define _XOPEN_SOURCE
 #include <signal.h>
 #include <stdio.h>
 #include <sys/mman.h>
@@ -54,7 +55,7 @@ int main()
 
     //call bridge_sig_jmp on segmentation fault:
     act.sa_handler=bridge_sig_jmp;
-    act.sa_flags=SA_NODEFER;
+    act.sa_flags=0;
     sigaction(SIGSEGV, &act, &oldact);
     for (i=0; i<sizeof(buf_size)/sizeof(buf_size[0]); ++i) {
         //stretch to a page size:

Alexander

Re: cluts review

[Luka Marčetić]

On 07/13/2011 01:07 PM, Solar Designer wrote [...]

Hmm, hope you won't mind me saying: Diff output for multiple files 
replacing 1-2 non-subsequent lines is not very readable. Anyway, I'll 
add _POSIX_SOURCE for sigaction and replace SA_NODEFER with 0 (I'm sure 
it served its purpose *somewhere* :-/). Not sure why _BSD_SOURCE is 
needed for alphasort, but I see from the man it is (I usually look up 
functions from SUSv4), so I'll add that too. Instead of <sys/param.h> 
for PATH_MAX, will limits.h do (that's what i usually include)?

> As it is, this patch is a hack.  I think that extensive cleanups to the
> code being patched are needed.

As I've announced, alloc.c is going to be rewritten: As Rich suggested 
to me, it'll use arrays instead of lists which will speed up things and 
make them prettier.. actually I think it's in alloc.c's TODO. String.c 
will also be rewritten, because, as I've said, it's impractical to write 
so much code for a few tests (not sure yet how exactly). If there are 
any other critiques, I would like to hear them.

> I have other observations as well (such as from actually running the
> tests), which I hope to post separately.

Ok. I only hope it's understood that I've made the tests report failures 
alone. It would serve no purpose to output success messages (eg. 
numeric.c would report a *lot* of things). If that's not what you meant, 
shoot, I'm listening.
Thanks
Luka.

Re: cluts review

[Solar Designer]

Luka,

On Wed, Jul 13, 2011 at 02:02:44PM +0200, Luka Mar??eti?? wrote:
> Hmm, hope you won't mind me saying: Diff output for multiple files 
> replacing 1-2 non-subsequent lines is not very readable.

I don't mind, but I also don't understand what you're trying to say.

It is a patch that you can directly apply to your code, with patch(1) -
or you can indeed just read it and see what has changed where, then
decide which of the changes to merge or what else to do about them.

What would be more readable than that, yet just as specific?

> Instead of <sys/param.h> 
> for PATH_MAX, will limits.h do (that's what i usually include)?

No, it doesn't get PATH_MAX defined for me.

What you could actually want to do is get rid of the dependency on
PATH_MAX and FILENAME_MAX.  The system does not guarantee that actual
pathnames fit in PATH_MAX anyway.  So you may want to replace those
strcat()'s with dynamic memory allocation or add a check for potential
buffer overflow there (then report the error and skip the test).

For dynamic memory allocation, you may use asprintf(3), but it is not
very portable, or you may use my concat() function:

http://openwall.info/wiki/people/solar/software/public-domain-source-code/concat

(put it in a common source file).

> Ok. I only hope it's understood that I've made the tests report failures 
> alone. It would serve no purpose to output success messages (eg. 
> numeric.c would report a *lot* of things). If that's not what you meant, 
> shoot, I'm listening.

I definitely did not intend to complain about the lack of micro-test
success reports.  I do intend to post more stuff.

Thanks,

Alexander

Re: cluts review

[Luka Marčetić]

On 07/13/2011 02:21 PM, Solar Designer wrote:
> Luka,
>
> On Wed, Jul 13, 2011 at 02:02:44PM +0200, Luka Mar??eti?? wrote:
>> Hmm, hope you won't mind me saying: Diff output for multiple files
>> replacing 1-2 non-subsequent lines is not very readable.
> I don't mind, but I also don't understand what you're trying to say.

I guess I like doing things manually. But thanks for the patch. Anyway, 
here:
https://github.com/lmarcetic/cluts/commit/7c836ff779c1f9ffecdae9f7d469772e88d3bc68
(note also that cluts.c should now return correct nr. of failed tests, 
that would've been a valid critique)

>> Instead of<sys/param.h>
>> for PATH_MAX, will limits.h do (that's what i usually include)?
> No, it doesn't get PATH_MAX defined for me.

Strange, SUSv4 (which is really the std I'm testing for compliance 
against) says it should be there. I'm surprised you don't have any 
issues with SUS-specific functions. So, do you want me to replace limits 
with param.h?

> What you could actually want to do is get rid of the dependency on
> PATH_MAX and FILENAME_MAX.  The system does not guarantee that actual
> pathnames fit in PATH_MAX anyway.  So you may want to replace those
> strcat()'s with dynamic memory allocation or add a check for potential
> buffer overflow there (then report the error and skip the test).

Ah, wherever I do this, I think the path isn't big anyway (cluts.c and 
buf.c). I'll keep it in mind if there's ever a chance they might be. Agreed?

> For dynamic memory allocation, you may use asprintf(3), but it is not
> very portable, or you may use my concat() function:

Thanks, I didn't even know about that one!

> http://openwall.info/wiki/people/solar/software/public-domain-source-code/concat
>
> (put it in a common source file).

I already have my sreturnf function for such purposes (right now it uses 
vsnprintf). Why do you think *snprintf and *asprintf aren't portable?

Luka.

Re: cluts review

[Rich Felker]

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. The bug is just that Luka is relying on glibc's
practice of defining everything by default. Since the tests are
testing a POSIX environment, -D_POSIX_C_SOURCE=200809L (or even
-D_XOPEN_SOURCE=700) should be in the CFLAGS.

> -std=c99 appears to be needed to get LLONG_MAX, etc. defined by glibc's
> header files when compiling with older gcc (I used 3.4.5).

Defining the right feature test macros would also fix this, but it's
good to have -std=c99 anyway.

> +#define _BSD_SOURCE /* for scandir() and alphasort() */

These are POSIX 2008 functions. _BSD_SOURCE should not be needed for
anything.

> +#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...

> +#define _XOPEN_SOURCE /* for sigaction() */

Needs a value, not a blank definition. Current version is 700.

> -    act.sa_flags   = SA_NODEFER;
> +    act.sa_flags   = 0;

This was being used as part of the longjmp trick.

By the way, there are a lot of warnings about local vars potentially
clobbered by longjmp. Those are worth checking out. I found gcc was
pretty strict about breaking my code in the dynamic linker when I
broke the rules for longjmp...

Rich

Re: cluts review

[Rich Felker]

On Wed, Jul 13, 2011 at 02:57:21PM +0200, Luka Marčetić wrote:
> >>Instead of<sys/param.h>
> >>for PATH_MAX, will limits.h do (that's what i usually include)?
> >No, it doesn't get PATH_MAX defined for me.
> 
> Strange, SUSv4 (which is really the std I'm testing for compliance
> against) says it should be there. I'm surprised you don't have any
> issues with SUS-specific functions. So, do you want me to replace
> limits with param.h?

Don't use sys/param.h unless you make it conditional on some check for
broken platforms (HURD?) that lack PATH_MAX.

> I already have my sreturnf function for such purposes (right now it
> uses vsnprintf). Why do you think *snprintf and *asprintf aren't
> portable?

*asprintf is not portable because it's a GNU extension, but it's
trivial to implement it as a wrapper for vsnprintf which is standard.
See the code in musl for an example of how to do it.

Rich

Re: cluts review

[Solar Designer]

On Wed, Jul 13, 2011 at 02:57:21PM +0200, Luka Mar??eti?? wrote:
> I guess I like doing things manually.

Sure.  I also don't blindly apply patches that people send me for my
code.  But I don't think there's a better way to represent the changes
being proposed.

> But thanks for the patch. Anyway, here:
> https://github.com/lmarcetic/cluts/commit/7c836ff779c1f9ffecdae9f7d469772e88d3bc68

OK.  Why the "#define _POSIX_C_SOURCE 200809L //sigaction" vs. "#define
_XOPEN_SOURCE //sigaction" inconsistency, though?  I think _XOPEN_SOURCE
is a safer bet here.  And I generally avoid C++ comments in C source
files, even though this became legal in C99 (and many C compilers
recognized C++ comments before then).  If we enable/require C99 anyway,
this is a non-issue, and perhaps I am too old-fashioned. ;-)  Testing
cluts on some older systems could make sense, though - not so much to
test those systems' libc's, but rather to better test cluts itself.

> (note also that cluts.c should now return correct nr. of failed tests, 
> that would've been a valid critique)

Yes, I noticed weird output there.  Thanks for fixing it.

> >What you could actually want to do is get rid of the dependency on
> >PATH_MAX and FILENAME_MAX.  The system does not guarantee that actual
> >pathnames fit in PATH_MAX anyway.  So you may want to replace those
> >strcat()'s with dynamic memory allocation or add a check for potential
> >buffer overflow there (then report the error and skip the test).
> 
> Ah, wherever I do this, I think the path isn't big anyway (cluts.c and 
> buf.c). I'll keep it in mind if there's ever a chance they might be. Agreed?

I don't strictly object to this, but I am trying to help you improve
your code quality overall.  It may be preferable to consistently apply
best practices to cluts source code rather than to take shortcuts in
places where this is currently deemed acceptable.

That said, I agree that you have other priorities right now than dealing
with those existing pathname length issues.

> I already have my sreturnf function for such purposes (right now it uses 
> vsnprintf).

Oh, I didn't notice this one yet.  Thanks for pointing it out.

> Why do you think *snprintf and *asprintf aren't portable?

They just are less portable than older functions such as sprintf().
In practice, I think *snprintf() are portable enough these days and for
this specific application, so your use of vsnprintf() is fine (although
calling it with "NULL, 0" is a stress-test).  asprintf() may or may not
be portable enough depending on what systems cluts is meant to run on.

Alexander

Re: cluts review

[Rich Felker]

On Wed, Jul 13, 2011 at 05:54:23PM +0400, Solar Designer wrote:
> > But thanks for the patch. Anyway, here:
> > https://github.com/lmarcetic/cluts/commit/7c836ff779c1f9ffecdae9f7d469772e88d3bc68
> 
> OK.  Why the "#define _POSIX_C_SOURCE 200809L //sigaction" vs. "#define
> _XOPEN_SOURCE //sigaction" inconsistency, though?  I think _XOPEN_SOURCE
> is a safer bet here.

I would really put this in the makefile for consistency. In principle,
the values of feature test macros could lead to different versions of
certain functions being used, possibly even with different
interfaces/ABI, and cause problems with linking together object files
compiled with different settings.

The default with no feature test macro defined is not "_GNU_SOURCE" or
"variety bag". In principle it's supposed to be pure C, and in reality
it varies a lot and you shouldn't rely on it. Especially proprietary
unices, but also GNU, like to expose their traditional non-conformant
versions of certain functions (e.g. strerror_r) when no feature test
macro is present, which is a major issue for testing.

> And I generally avoid C++ comments in C source
> files, even though this became legal in C99 (and many C compilers
> recognized C++ comments before then).  If we enable/require C99 anyway,
> this is a non-issue, and perhaps I am too old-fashioned. ;-)  Testing
> cluts on some older systems could make sense, though - not so much to
> test those systems' libc's, but rather to better test cluts itself.

I think it's pretty hard to test functions that are part of a standard
that depends on and includes C99, while not requiring a (mostly)
C99-supporting compiler. I do tend to think // comments look "lazy",
but it's not a big deal either way.

> > Why do you think *snprintf and *asprintf aren't portable?
> 
> They just are less portable than older functions such as sprintf().
> In practice, I think *snprintf() are portable enough these days and for
> this specific application, so your use of vsnprintf() is fine (although
> calling it with "NULL, 0" is a stress-test).  asprintf() may or may not
> be portable enough depending on what systems cluts is meant to run on.

snprintf is C99 and POSIX and I think it's pretty reasonable to rely
on it. Legacy systems can implement it as a hideous wrapper for
tmpfile(), fprintf(), and fread() if they're not willing to fix their
libcs. :-)

Rich

Re: cluts review

[Rich Felker]

On Wed, Jul 13, 2011 at 04:21:28PM +0400, Solar Designer wrote:
> > Instead of <sys/param.h> 
> > for PATH_MAX, will limits.h do (that's what i usually include)?
> 
> No, it doesn't get PATH_MAX defined for me.

Even with _POSIX_C_SOURCE=200809L? Did glibc really go and break that
too? :( The standard doesn't require it to be defined, but this is
just like their issue of removing PAGE_SIZE. All it does is create
more work and bloat for the application which has to go retrieve the
value via sysconf/pathconf. And if these still return "unlimited",
then many interfaces have *inherent* security bugs that cannot be
fixed due to writing out a string that's assumed to be able to
accommodate at least PATH_MAX bytes...

> What you could actually want to do is get rid of the dependency on
> PATH_MAX and FILENAME_MAX.  The system does not guarantee that actual
> pathnames fit in PATH_MAX anyway.  So you may want to replace those
> strcat()'s with dynamic memory allocation or add a check for potential
> buffer overflow there (then report the error and skip the test).

Indeed, I would use snprintf for this and just error out if the string
gets truncated, but you could also perform dynamic allocation.

Rich

Re: cluts review

[Solar Designer]

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

Re: cluts review

[Solar Designer]

On Wed, Jul 13, 2011 at 09:42:38AM -0400, Rich Felker wrote:
> *asprintf is not portable because it's a GNU extension, but it's
> trivial to implement it as a wrapper for vsnprintf which is standard.

Right.  FWIW, non-ancient *BSD's also have asprintf(), and with slightly
safer semantics than glibc: on error, they not only return -1, but also
reset the pointer to NULL.  Dmitry V. Levin of ALT Linux tried to push
this semantics change to glibc, but Ulrich rejected it.  So it remains
in a patch in ALT Linux and Owl.

It would be nice for cluts to test non-portable functions like that as
well, when they're available, and to point out semantics differences
like this (not as errors, but as properties of the given platform).

Alexander

Re: cluts review

[Rich Felker]

On Wed, Jul 13, 2011 at 06:12:49PM +0400, Solar Designer wrote:
> > > +#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?

Hmm. In this case also defining _BSD_SOURCE might be good, but I'm a
bit worried about glibc behavior, which seems to be to *prefer* BSD or
GNU variants over the standard version of some functions when the
corresponding BSD/GNU feature test macros are defined... Not sure what
the best solution is.

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

This works for me (on glibc):

cat <<EOF >test.c
#define _XOPEN_SOURCE 700
#include <limits.h>
PATH_MAX
EOF
gcc -E test.c | tail -n 1

What system are you using that lacks it? One (mildly ugly) solution,
if needed, would be:

#ifndef PATH_MAX
#include <sys/param.h>
#endif

> > > +#define _XOPEN_SOURCE /* for sigaction() */
> > 
> > Needs a value, not a blank definition. Current version is 700.
> 
> So, your proposal is to always request the latest spec version.
> Wouldn't it possibly be better to request the oldest sufficient for us?

My theory is that implementations will silently drop back to giving
you an older version (and informing you in _XOPEN_VERSION) if they
can't provide the version you request. I believe this is true in
practice but I'm not 100% sure. I just prefer doing things that way to
trying to remember exactly what changed in each version of the
standard and which version is actually the "oldest sufficient".

> > > -    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?

It's to leave the signal unblocked so that longjmp can return without
having to restore the signal mask. It might be slightly more portable
to just manually unblock it or use sigsetjmp/siglongjmp to do so, but
unless I'm mistaken the last system with broken SA_NODEFER was Linux
1.3 or so...

Rich

Re: cluts review

[Solar Designer]

On Wed, Jul 13, 2011 at 10:00:02AM -0400, Rich Felker wrote:
> On Wed, Jul 13, 2011 at 05:54:23PM +0400, Solar Designer wrote:
> > OK.  Why the "#define _POSIX_C_SOURCE 200809L //sigaction" vs. "#define
> > _XOPEN_SOURCE //sigaction" inconsistency, though?  I think _XOPEN_SOURCE
> > is a safer bet here.
> 
> I would really put this in the makefile for consistency. In principle,
> the values of feature test macros could lead to different versions of
> certain functions being used, possibly even with different
> interfaces/ABI, and cause problems with linking together object files
> compiled with different settings.

This makes sense to me.  Historically, I tend to use the #define thing,
but you have a valid point for doing it in the Makefile instead.

> I think it's pretty hard to test functions that are part of a standard
> that depends on and includes C99, while not requiring a (mostly)
> C99-supporting compiler.

Of course, those newer functions would need to be skipped when cluts is
built and run on an older system.

> snprintf is C99 and POSIX and I think it's pretty reasonable to rely
> on it.

Agreed.

> Legacy systems can implement it as a hideous wrapper for
> tmpfile(), fprintf(), and fread() if they're not willing to fix their
> libcs. :-)

Yes, this is hideous.

Alexander

Re: cluts review

[Solar Designer]

On Wed, Jul 13, 2011 at 10:03:04AM -0400, Rich Felker wrote:
> On Wed, Jul 13, 2011 at 04:21:28PM +0400, Solar Designer wrote:
> > > Instead of <sys/param.h> 
> > > for PATH_MAX, will limits.h do (that's what i usually include)?
> > 
> > No, it doesn't get PATH_MAX defined for me.
> 
> Even with _POSIX_C_SOURCE=200809L?

I didn't try this one.  Now that I did, PATH_MAX gets defined.  In fact,
my addition of _BSD_SOURCE for scandir() and alphasort() also got
PATH_MAX defined with <limits.h>.  I didn't notice because I was trying
one change at a time.

Alexander

Re: cluts review

[Solar Designer]

On Wed, Jul 13, 2011 at 10:26:03AM -0400, Rich Felker wrote:
> cat <<EOF >test.c
> #define _XOPEN_SOURCE 700
> #include <limits.h>
> PATH_MAX
> EOF
> gcc -E test.c | tail -n 1

Yeah, it works for me too.  It's just that in cluts.c, after all those
other #include's and with no feature test macros defined at all,
<limits.h> wouldn't provide PATH_MAX.  Somehow <limits.h> on its
own, also with no feature test macros at all, does provide PATH_MAX.
Anyhow, I am not going to investigate further.  It is clear that when we
do request at least _BSD_SOURCE or _XOPEN_SOURCE, then <limits.h> does
give us PATH_MAX.

> > > > -    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?
> 
> It's to leave the signal unblocked so that longjmp can return without
> having to restore the signal mask.

Oh, you're right, and my comment above re: "keep the signal blocked"
didn't make sense (it's the other way around).

> It might be slightly more portable
> to just manually unblock it or use sigsetjmp/siglongjmp to do so, but
> unless I'm mistaken the last system with broken SA_NODEFER was Linux
> 1.3 or so...

I think that we need to switch to using sigsetjmp/siglongjmp as I had
suggested before.

Alexander

Re: cluts review

[Solar Designer]

Luka, Rich -

The below is not criticism, but just some feedback on my test run of
cluts earlier today.

So, with the changes that I described before, I built cluts on a glibc
2.3.6'ish linux-threads system (yes, pre-NPTL).

Then I ran it under an account with RLIMIT_AS set to 400 MB:

$ ulimit -v
409600
$ ./cluts
Executing 'alloc' test collection...
<multithreaded>
</multithreaded>
malloc 'allocated' unwritable memory
malloc 'allocated' unwritable memory
malloc 'allocated' unwritable memory

It ran for a while, then went to sleep.

$ ps axu
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
sandbox  21173  0.0  0.2   2400  1504 pts/5    S    14:05   0:00 -bash
sandbox  21480  0.0  0.0   1932   512 pts/5    S+   14:13   0:00 ./cluts
sandbox  21481 29.7 73.9 402768 381328 pts/5   S+   14:13   1:02 ./cluts
sandbox  21484  0.0  0.2   2396  1464 pts/6    S    14:13   0:00 -bash
sandbox  21500  0.0 73.9 402768 381328 pts/5   S+   14:14   0:00 ./cluts
sandbox  21504  0.0 73.9 402768 381328 pts/5   S+   14:14   0:00 ./cluts
sandbox  21522  0.0  0.2   2400  1500 pts/7    S    14:15   0:00 -bash
sandbox  21542  0.0  0.1   1564   548 pts/7    S+   14:16   0:00 cat
sandbox  21545  0.0  0.1   2432   916 pts/6    R+   14:17   0:00 ps axu

top:

  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
21556 sandbox   18   0   956  956  780 R  2.0  0.2   0:00.35 top
21173 sandbox    9   0  1504 1504 1140 S  0.0  0.3   0:00.15 bash
21480 sandbox    9   0   512  512  420 S  0.0  0.1   0:00.00 cluts
21481 sandbox    9   0  372M 372M  464 S  0.0 74.0   1:02.43 alloc
21484 sandbox    9   0  1464 1464 1112 S  0.0  0.3   0:00.05 bash
21500 sandbox    8   0  372M 372M  464 S  0.0 74.0   0:00.09 alloc
21504 sandbox    9   0  372M 372M  464 S  0.0 74.0   0:00.10 alloc
21522 sandbox   10   0  1504 1504 1144 S  0.0  0.3   0:00.06 bash

The "unwritable memory" allocation is probably a glibc issue, albeit a
minor one because malloc() may return such allocations anyway unless
overcommit is fully disabled in the kernel.  The only issue is that for
hitting RLIMIT_AS, it could actually detect the inability to allocate
more address space, so it'd return NULL, which apparently it didn't.

As to cluts going to sleep, this is probably a bug in alloc.c.  If this
file is going to be rewritten, then it might not make sense to chase the
bug down now.  But it is useful to re-test the new version with low
RLIMIT_AS as well.

After a while, I killed the alloc thread that consumed the most CPU time.

(BTW, all of them are just ./cluts in "ps" because cluts.c passes its
own argv[0] to exec.  Perhaps it'd be nicer to fix that.)

The 'alloc' test collection crashed!
Executing 'buf' test collection...
ttyname_r(STDERR_FILENO, s, sizeof(s)-1) failed to set errno=ERANGE (it is E2BIG)
The 'buf' test collection failed 256 test(s).
Executing 'numeric' test collection...
strtoumax("0x", &endptr0) offsets endptr by 1 instead of by 0
strtoumax("0x 1", &endptr0) offsets endptr by 1 instead of by 0
strtoumax("0x+1", &endptr16) offsets endptr by 1 instead of by 0
strtoumax("0x-1", &endptr16) offsets endptr by 1 instead of by 0
strtoumax("0Xx", &endptr16) offsets endptr by 1 instead of by 0
strtoumax("0xX", &endptr16) offsets endptr by 1 instead of by 0
strtof("1e", &endptr, 16) should return 0.000000(0x0p+0), errno=<any>
        instead, it returns 1.000000(0x1p+0), errno=0(Success)

I got lots of errors from "numeric" - I didn't bother recording most.
The above are just a few of them.

The 'numeric' test collection failed 16384 test(s).
Executing 'string' test collection...
The 'string' test collection passed.

Test collections passed: 1/4

This is the known issue with number of failed test collections, right?
Should have been 2, I guess.

Thanks,

Alexander

Re: cluts review

[Luka Marčetić]

On 07/13/2011 03:38 PM, Rich Felker wrote:



> Since the tests are
> testing a POSIX environment, -D_POSIX_C_SOURCE=200809L (or even
> -D_XOPEN_SOURCE=700) should be in the CFLAGS.
>

How about I just put define POSIX_C_SOURCE 200809L everywhere in the 
header, that should do the trick, right? If I put it as a flag, it might 
not be clear to someone who just yanks a test collection/source file out 
of the suite. *shrugs*

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

Can you remind me what this does exactly? I can't remember anymore, 
seemed to me it really was not needed. man says so as to not prevent the 
signal handler from (paraphrasing:) calling a signal itself. I don't 
need this, but you probably suggested it for some other reason then. 
Please do remind me. Thanks.

> By the way, there are a lot of warnings about local vars potentially
> clobbered by longjmp. Those are worth checking out. I found gcc was
> pretty strict about breaking my code in the dynamic linker when I
> broke the rules for longjmp...

These result from the -02 parameter. -Wall and -Wextra should report 
nothing. I checked buf.c for clobbering, and corrected what was 
necessary. There might be some other files where the same is needed, 
will look into then.

> *asprintf is not portable because it's a GNU extension, but it's
> trivial to implement it as a wrapper for vsnprintf which is standard.
> See the code in musl for an example of how to do it.

Oh. That might be why I haven't heard of it. Well, I am using the other 
one for sreturnf...
Thanks
Luka.

Re: cluts review

[Luka Marčetić]

On 07/13/2011 06:03 PM, Solar Designer wrote:
> Luka, Rich -
>
> The below is not criticism, but just some feedback on my test run of
> cluts earlier today.
>
> So, with the changes that I described before, I built cluts on a glibc
> 2.3.6'ish linux-threads system (yes, pre-NPTL).
> [...]
>
> As to cluts going to sleep, this is probably a bug in alloc.c.  If this
> file is going to be rewritten, then it might not make sense to chase the
> bug down now.  But it is useful to re-test the new version with low
> RLIMIT_AS as well.

Thanks for the tip.

> After a while, I killed the alloc thread that consumed the most CPU time.
>
> (BTW, all of them are just ./cluts in "ps" because cluts.c passes its
> own argv[0] to exec.  Perhaps it'd be nicer to fix that.)

Wasn't aware of this. Will fix.

> The 'alloc' test collection crashed!
> Executing 'buf' test collection...
>
> I got lots of errors from "numeric" - I didn't bother recording most.
> The above are just a few of them.

In my opinion, the errors are warranted. But I asked Rich for review on 
that one. In fact, the collection blocks a lot of error messages. Each 
string that gets passed to multiple functions is printed only with the 
first function that fails running with it (in your, strtoumax appears a 
lot of times, because that's the first function that gets executed with 
each string). The huge long long integer printout is I guess what makes 
it look as if there are more messages than there are. Other than that, 
it's probably the implementations. If it still turns out to be a test 
data issue, let me know.

> The 'numeric' test collection failed 16384 test(s).

Hmm, is it an up-to-date version of cluts.c? The number is overblown.

> Executing 'string' test collection...
> The 'string' test collection passed.

String.c always does (it still returns 0 no matter what) ;-P

> Test collections passed: 1/4
>
> This is the known issue with number of failed test collections, right?
> Should have been 2, I guess.

This is the number of passed collections. I don't know why it says "out 
of four" here, if you still have the buf.c-less version :-/

> Thanks,
>
> Alexander

Thank you for the review.
I'll offer my responses to feedback in form of commits :-)
Luka.

Re: cluts review

[Solar Designer]

Luka,

On Wed, Jul 13, 2011 at 06:25:51PM +0200, Luka Mar??eti?? wrote:
> >>-    act.sa_flags   = SA_NODEFER;
> >>+    act.sa_flags   = 0;
> >This was being used as part of the longjmp trick.
> 
> Can you remind me what this does exactly? I can't remember anymore, 
> seemed to me it really was not needed. man says so as to not prevent the 
> signal handler from (paraphrasing:) calling a signal itself. I don't 
> need this, but you probably suggested it for some other reason then. 
> Please do remind me. Thanks.

As Rich reminded me (in here), this was needed to keep the signal
unblocked even after longjmp() back into your main program.  Otherwise,
the kernel blocks the signal when calling the signal handler, the signal
handler longjmp()s (doesn't return), and the signal remains blocked.

My recommended fix is to keep sa_flags at 0 (as changed above), but to
switch to using sigjmp_buf/sigsetjmp/siglongjmp.

> >By the way, there are a lot of warnings about local vars potentially
> >clobbered by longjmp. Those are worth checking out. I found gcc was
> >pretty strict about breaking my code in the dynamic linker when I
> >broke the rules for longjmp...
> 
> These result from the -02 parameter.

This doesn't mean that they are harmless.  It only means that gcc
doesn't always see the problems.

Do you understand what clobbering by longjmp means and why it occurs?

Alexander

Re: cluts review

[Solar Designer]

On Wed, Jul 13, 2011 at 06:55:01PM +0200, Luka Mar??eti?? wrote:
> On 07/13/2011 06:03 PM, Solar Designer wrote:
> >I got lots of errors from "numeric" - I didn't bother recording most.
> >The above are just a few of them.
> 
> In my opinion, the errors are warranted.

Quite possibly they are.  I did not review them at all.  I am leaving
that for you and Rich.

> >The 'numeric' test collection failed 16384 test(s).
> 
> Hmm, is it an up-to-date version of cluts.c? The number is overblown.

cluts as of 10 hours ago or so.

> >Test collections passed: 1/4
> >
> >This is the known issue with number of failed test collections, right?
> >Should have been 2, I guess.
> 
> This is the number of passed collections. I don't know why it says "out 
> of four" here, if you still have the buf.c-less version :-/

This version did have buf.c.

Alexander

Re: cluts review

[Luka Marčetić]

On 07/13/2011 07:05 PM, Solar Designer wrote:
[...]
Maybe my Thunderbird is impotent, but this list gets swamped pretty 
fast. Anyway, so I need SA_NODEFER because the handler doesn't return, 
understood. As for the sigsetjmp and stuff, that's what we had before. 
And yet again, I forgot why it was deprecated (I think there was signal 
structs being different across platforms, though I'm not sure where the 
problem was), but anyway Rich suggested sigaction.
This reminds me, the code is distinctly C99, and it tests SUSv4 
functions, so if you don't mind, for cluts, I'll use those two standards 
and go back to SA_NODEFER. If I decide to pay more attention to that 
distributed computer project of yours in the future (I thought there 
would be some smart CS people applying for that one), then the situation 
will probably be different ;-)
Oh, and I do believe I know aht "clobbered" means (overwriting the new 
value of the variable with the old one, from when the context was saved, 
right?). That's what I've said I've checked with buf.c. I'll check it 
for other relevant collections now too... Will push new minor fixes in a 
few minutes.
Luka.

P.S. Perhaps I should start thinking about how the final cluts.c will 
look like, otherwise it might become hard to change all the test 
collections later...

Re: cluts review

[Solar Designer]

Luka, Rich -

On Wed, Jul 13, 2011 at 07:25:07PM +0200, Luka Mar??eti?? wrote:
> Anyway, so I need SA_NODEFER because the handler doesn't return, 
> understood. As for the sigsetjmp and stuff, that's what we had before. 
> And yet again, I forgot why it was deprecated (I think there was signal 
> structs being different across platforms, though I'm not sure where the 
> problem was), but anyway Rich suggested sigaction.

I think you're confusing things.  Maybe Rich suggested that you use
sigaction() instead of signal()?  That's fine, but it has nothing to do
with the choice of setjmp() vs. sigsetjmp().

> This reminds me, the code is distinctly C99, and it tests SUSv4 
> functions, so if you don't mind, for cluts, I'll use those two standards 

I am fine with limiting cluts to newer systems if Rich is fine with that.

What I am saying here about sig* has little to do with newer vs. older
systems.  It's just that sigsetjmp() and friends appears to be a cleaner
way to deal with the problem.  Quite in line with what you're advocating.

> and go back to SA_NODEFER.

OK, but there's a cleaner way to do it.

> Oh, and I do believe I know aht "clobbered" means (overwriting the new 
> value of the variable with the old one, from when the context was saved, 
> right?).

Yes.  Do you know in what cases this happens, and how to prevent it?

> That's what I've said I've checked with buf.c.

What exactly did you check/change?

> P.S. Perhaps I should start thinking about how the final cluts.c will 
> look like, otherwise it might become hard to change all the test 
> collections later...

Speaking of overall structure of cluts, I think it's not cluts.c but the
building/linking of the individual test collections that you should
decide on first.  Right now, you have one top-level makefile only (BTW,
the name "Makefile" is more standard on Unix-like systems), which builds
all *.c files into their corresponding binary executables.  And you
include your common code right into each C source.  A cleaner way
would be to build the individual C files into *.o files and to get them
linked together as appropriate - so your common code is only compiled
once, and only some of its symbols are exported.  Also, you could have a
separate Makefile under tests/, which you'd invoke with a sub-make, or
you could get rid of those tests/ and common/ subdirectories in order to
simplify the build process (cluts.c would then need to learn of the
tests to run by other means - e.g., by a filename prefix).  Just some
thoughts.

Thanks,

Alexander

Re: cluts review

[Luka Marčetić]

On 07/13/2011 07:52 PM, Solar Designer wrote:
>> and go back to SA_NODEFER.
> OK, but there's a cleaner way to do it.

If you mean SA_NODEFER is new, my comment is: Yes, but note that for 
sigsetjmp to do the same job, I'd need yet another global variable. So, 
given that SA_NODEFER is SUSv4 which I'm using anyway, I still regard it 
as cleaner than sigsetjmp.

>> Oh, and I do believe I know aht "clobbered" means (overwriting the new
>> value of the variable with the old one, from when the context was saved,
>> right?).
> Yes.  Do you know in what cases this happens, and how to prevent it?

If you don't mind i'll skip this one :-(

>> That's what I've said I've checked with buf.c.
> What exactly did you check/change?

I don't remember if I had to change anything, but I can comment on 
-Wclobbered messages, that might convince you:
     'function' is changed inside a switch loop only if no test is run 
(function>n, where n is the number of tests). If no tests are run, there 
should be no SIGABRT/SIGSEGV signals. If there is, it should crash the 
test collection so I can fix it.
     'failed' indeed gets clobbered (we get a non-incremented value) if 
the above signals have been caught, but then is incremented after 
longjmp (actually, I think that's the one I did fix)
     'err_expected' may be clobbered. it doesn't matter, as printing the 
error message that includes it won't happen.
     'stream' - to be honest, I don't know why it reports that stream 
could be clobbered. I did check it, and it's set outside the sigset 
blocks, and shouldn't be changed

>    Right now, you have one top-level makefile only (BTW,
> the name "Makefile" is more standard on Unix-like systems),

Didn't know that, will rename, thanks.

>   which builds
> all *.c files into their corresponding binary executables.  And you
> include your common code right into each C source.  A cleaner way
> would be to build the individual C files into *.o files and to get them
> linked together as appropriate - so your common code is only compiled
> once, and only some of its symbols are exported.

I could make a rule for common files (that much I can manage), but make 
would then have to know what to link together. Being a make noob, I'm 
not sure what's the proper/expected form for .h files either. It's even 
a question whether it'd be of any use to me, given that my code doesn't 
compile into a single binary, it is multiple independent binaries, all 
including only some .h's. But if you know there's a better way, I'm open 
to it, and will cooperate.


>    Also, you could have a
> separate Makefile under tests/, which you'd invoke with a sub-make, or
> you could get rid of those tests/ and common/ subdirectories in order to
> simplify the build process (cluts.c would then need to learn of the
> tests to run by other means - e.g., by a filename prefix).  Just some
> thoughts.

Hmm.

> Thanks,
>
> Alexander

Sorry for the late reply, I was afk. And I apologize if I'm touchy about 
my code (don't let that stop you from commenting, please).
Thanks,
-Luka

Re: cluts review

[Rich Felker]

On Wed, Jul 13, 2011 at 09:52:01PM +0400, Solar Designer wrote:
> I think you're confusing things.  Maybe Rich suggested that you use
> sigaction() instead of signal()?  That's fine, but it has nothing to do
> with the choice of setjmp() vs. sigsetjmp().

You should always use sigaction instead of signal because the latter
leaves too much implementation-defined to be useful. With that said,
let's just drop this bikeshed. sigsetjmp/siglongjmp are just as easy
to use and it's more certain that they'll work in the presence of
buggy implementations, so let's go with them.

> > This reminds me, the code is distinctly C99, and it tests SUSv4 
> > functions, so if you don't mind, for cluts, I'll use those two standards 
> 
> I am fine with limiting cluts to newer systems if Rich is fine with that.

I think it's reasonable for now, but at some point it might be worth
evaluating the level of dependency and reducing it if anyone wants to
test older libraries. I don't think there's any use in avoiding C99
features that are purely at the compiler level (like compound
literals) because any current or future compiler capable of being used
on unix-like systems will support these.

> > Oh, and I do believe I know aht "clobbered" means (overwriting the new 
> > value of the variable with the old one, from when the context was saved, 
> > right?).
> 
> Yes.  Do you know in what cases this happens, and how to prevent it?

Actually saying it's "clobbered" is an understatement. Breaking the
rules invokes undefined behavior, and in at least one real-world case,
gcc generated code that performed a memory read from something like
0x60 (absolute address) instead of 0x60(%esp). Don't ask me why;
that's UB. Proper code should not invoke UB.

Rich

Re: cluts review

[Rich Felker]

On Wed, Jul 13, 2011 at 09:29:06PM +0200, Luka Marčetić wrote:
> On 07/13/2011 07:52 PM, Solar Designer wrote:
> >>and go back to SA_NODEFER.
> >OK, but there's a cleaner way to do it.
> 
> If you mean SA_NODEFER is new, my comment is: Yes, but note that for
> sigsetjmp to do the same job, I'd need yet another global variable.
> So, given that SA_NODEFER is SUSv4 which I'm using anyway, I still
> regard it as cleaner than sigsetjmp.

You don't need another global var to use sigsetjmp. You just change
the existing jump buffer type from jmp_buf to sigjmp_buf and change
the setjmp/longjmp calls to sigsetjmp/siglongjmp.

Rich

Re: cluts review

[Rich Felker]

On Wed, Jul 13, 2011 at 06:55:01PM +0200, Luka Marčetić wrote:
> that gets executed with each string). The huge long long integer
> printout is I guess what makes it look as if there are more messages
> than there are. Other than that, it's probably the implementations.

musl definitely has a lot of bugs in this area, and may rewrite the
code (for floating point I will definitely rewrite it because the
current approach is inexact and fundamentally can't be right). But I
think at least the huge long double test is wrong:

strtold(....  offsets endptr by 4940 instead of by 0

As far as I can tell, the string you pass is of the expected form for
the subject sequence. It may overflow to infinity or lose precision,
but it's still valid. strtold is only supposed to store the initial
pointer in endptr if the subject sequence is empty or does not have
the expected form; otherwise it stores a pointer to the byte just past
the end of the subject sequence.

Does this make sense?

Admittedly I need to review the rest of the tests more thoroughly too.

Rich

Re: cluts review

[Solar Designer]

On Wed, Jul 13, 2011 at 09:29:06PM +0200, Luka Mar??eti?? wrote:
> On 07/13/2011 07:52 PM, Solar Designer wrote:
> >>and go back to SA_NODEFER.
> >OK, but there's a cleaner way to do it.
> 
> If you mean SA_NODEFER is new,

No, I don't.

> my comment is: Yes, but note that for 
> sigsetjmp to do the same job, I'd need yet another global variable.

No, you wouldn't.  You already have a jmp_buf variable, which you'd
replace with a sigjmp_buf one.  It's a trivial change, really.

(I wrote the above before I saw Rich's response about the same.)

> >>Oh, and I do believe I know aht "clobbered" means (overwriting the new
> >>value of the variable with the old one, from when the context was saved,
> >>right?).
> >Yes.  Do you know in what cases this happens, and how to prevent it?
> 
> If you don't mind i'll skip this one :-(

Rich might be able to provide a better / more correct answer (I'd be
interested), but here's my understanding:

The clobbering happens when those variables are kept in registers rather
than in memory (on stack).  To prevent it from happening, you may force
the compiler not to place the variables in registers.  One way to do it
is to take the variable's address:

(void) &var;

(I am not sure what guarantees this provides.  IIRC, it was documented
to provide the needed safety under GNU C.)

Another is to declare it "volatile", but this does a bit more than is
needed (so has extra performance impact).

Better yet, structure your function such that there are no variables to
clobber.  If you put your sigsetjmp() at the very beginning of the
function, before any local variable is assigned a value, there's nothing
to clobber yet.

> >>That's what I've said I've checked with buf.c.
> >What exactly did you check/change?
> 
> I don't remember if I had to change anything, but I can comment on 
> -Wclobbered messages, that might convince you:
...

So basically you review the code and then knowingly ignore the warnings?
That's not great.

Alexander

Re: cluts review

[Solar Designer]

On Thu, Jul 14, 2011 at 12:39:45AM +0400, Solar Designer wrote:
> Rich might be able to provide a better / more correct answer (I'd be
> interested), but here's my understanding:
> 
> The clobbering happens when those variables are kept in registers rather
> than in memory (on stack).  To prevent it from happening, you may force
> the compiler not to place the variables in registers.  One way to do it
> is to take the variable's address:
> 
> (void) &var;
> 
> (I am not sure what guarantees this provides.  IIRC, it was documented
> to provide the needed safety under GNU C.)

I can't find where I think I previously saw this documented for gcc.

> Another is to declare it "volatile", but this does a bit more than is
> needed (so has extra performance impact).

This is documented in lots of places.  For example:

https://www.securecoding.cert.org/confluence/display/seccode/MSC22-C.+Use+the+setjmp(),+longjmp()+facility+securely

and indeed we're taking a risk by potentially interrupting and
re-entering non-async-signal safe functions:

https://www.securecoding.cert.org/confluence/display/seccode/SIG32-C.+Do+not+call+longjmp%28%29+from+inside+a+signal+handler

IIRC, I brought this up before, and Rich correctly pointed out that we
were doing it for low-risk functions only, and this is only a test suite.

> Better yet, structure your function such that there are no variables to
> clobber.  If you put your sigsetjmp() at the very beginning of the
> function, before any local variable is assigned a value, there's nothing
> to clobber yet.

Here's an example:

http://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/popa3d/popa3d/protocol.c?rev=HEAD

int pop_handle_state(struct pop_command *commands)
{
	char line[POP_BUFFER_SIZE];
	char *params;
	struct pop_command *command;
	int response;

	if (sigsetjmp(pop_timed_out, 1)) return POP_CRASH_NETTIME;

	while (pop_get_line(line, sizeof(line))) {
...

It also invokes siglongjmp() from a signal handler, but it only installs
the handler for the duration of a read() syscall, which is async signal
safe, so there's no risk.

Alexander

Re: cluts review

[Rich Felker]

On Wed, Jul 13, 2011 at 06:55:01PM +0200, Luka Marčetić wrote:
> >I got lots of errors from "numeric" - I didn't bother recording most.
> >The above are just a few of them.
> 
> In my opinion, the errors are warranted. But I asked Rich for review

I'm a bit confused by this one:

strtoumax("0xX", &endptr) should return 0, errno=0(Invalid error number)
        instead, it returns 0, errno=EINVAL

Why do you expect it to return with errno=0? Since the subject
sequence does not have a correct form, I believe the implementation
may (and should, from a quality of implementation standpoint) set
errno to EINVAL. Of course a portable application will need to examine
endptr instead of errno to check this case.

The remaining errors all seem correct. It's expected that glibc fails
lots of these tests. See:
http://sourceware.org/bugzilla/show_bug.cgi?id=12701

Rich