Re: [musl] Re: musl bugs found through gnulib

Re: [musl] Re: musl bugs found through gnulib

[idunham]

> [CCing the musl list]
> Isaac Dunham wrote in
> <http://lists.gnu.org/archive/html/bug-gnulib/2012-06/msg00101.html>:
>> musl is designed for standards conformance,
>
> There is a recipe, in <http://sourceware.org/glibc/wiki/Testing/Gnulib>,
> that explains how to use gnulib to check a libc against bugs.
Be warned: a bad test can cause failures as well.
It's been one of the musl developers' complaints about gnulib that the
tests are buggy and frequently check for glibc behavior instead of
standard behavior.
> When I apply this to musl-0.9.1, I get this list of problems:
>
> Replacements of *printf, because of
>   checking whether printf supports infinite 'long double' arguments... no
>   checking whether printf supports the 'ls' directive... no
>   checking whether printf survives out-of-memory conditions... no
At least one of these (infinite long double, IIRC) is invalid or a test
for a GNU-ism. This was previously discussed on the musl ML. OOM behavior
is undefined AFAICT (feel free to point out a standard), and the scenario
is a lot less likely with musl than glibc for several reasons.

> Replacement of duplocale, because of
>   checking whether duplocale(LC_GLOBAL_LOCALE) works... no
Need to check this one
> Replacement of fdopen, because of
>   checking whether fdopen sets errno... no
I presume this is nonconformance to POSIX ("otherwise, a null pointer
shall be returned and errno set...")?

> Replacement of futimens, because of
>   checking whether futimens works... no
Could be a bug.
> Replacement of getcwd, because of
>   checking whether getcwd handles long file names properly... no, but it
> is partly working
Is this a test for ERANGE handling (error on name >= size)? Other than
that, I see no specification covering this.
>   checking whether getcwd aborts when 4k < cwd_length < 16k... no
AFAICT, only required to error when size =< cwd_length. If size !<
(cwd_length + 1), that is conformant behavior. (See man 3posix getcwd)

> Replacement of getopt, because of
>   checking whether getopt is POSIX compatible... no
We'd need to see this test...(will look later).
> Replacement of glob, because of
>   checking for GNU glob interface version 1... no
> (not sure this is a bug or just an incompatibility compared to glibc)
Looks like an incompatability, since it specifies "GNU interface"...
> Replacement of iconv and iconv_open, because of
>   checking whether iconv supports conversion between UTF-8 and
> UTF-{16,32}{BE,LE}... no
Not "nonconformant" from the standpoint of POSIX, AFAICT, but it is
incomplete. musl is UTF8 native, but I don't think it supports UTF16/UTF32
yet.
> Replacement of mktime, because of
>   checking for working mktime... no
> Replacement of perror, because of
>   checking whether perror matches strerror... no
> Replacement of popen, because of
>   checking whether popen works with closed stdin... no
 Look like bugs, if the description is correct.

> Replacement of regex, because of
>   checking for working re_compile_pattern... no
This is #ifdef __USE_GNU
I'm not aware of any standard covering GNU APIs...

> Replacement of strtod, because of
>   checking whether strtod obeys C99... no
>
> For each of the replacements, first look at the test program's results
> (in config.log), then look at the test program's source code (in m4/*.m4).
>
Thanks,
Isaac Dunham

Re: Re: musl bugs found through gnulib

[Rich Felker]

On Sun, Jun 17, 2012 at 05:05:23PM -0700, idunham@lavabit.com wrote:
> > Replacement of fdopen, because of
> >   checking whether fdopen sets errno... no
> I presume this is nonconformance to POSIX ("otherwise, a null pointer
> shall be returned and errno set...")?

The EBADF error is a "may fail", not a "shall fail", per POSIX. Since
it requires an extra syscall in the general case (although in some
special cases, we're already making a syscall that could detect
EBADF), I find it's probably just an undesirable performance hit.
Applications should interpret the "may fail" as meaning it's not
valid/portable to call fdopen with an invalid file descriptor and
assume you can get a FILE* that will generate EBADF when it's used
later, not as meaning that the implementation will do their
error-checking for them.

> > Replacement of futimens, because of
> >   checking whether futimens works... no
> Could be a bug.

See the #ifdef __linux__ in the test file. Due to old buggy kernels,
they ALWAYS consider futimens broken on Linux.

> > Replacement of getcwd, because of
> >   checking whether getcwd handles long file names properly... no, but it
> > is partly working
> Is this a test for ERANGE handling (error on name >= size)? Other than
> that, I see no specification covering this.
> >   checking whether getcwd aborts when 4k < cwd_length < 16k... no
> AFAICT, only required to error when size =< cwd_length. If size !<
> (cwd_length + 1), that is conformant behavior. (See man 3posix getcwd)

I think musl is correct on everything for getcwd, but I'd like to
double-check.

> > Replacement of getopt, because of
> >   checking whether getopt is POSIX compatible... no
> We'd need to see this test...(will look later).

It's testing for GNU behavior not POSIX behavior.

> > Replacement of glob, because of
> >   checking for GNU glob interface version 1... no
> > (not sure this is a bug or just an incompatibility compared to glibc)
> Looks like an incompatability, since it specifies "GNU interface"...

Same.

> > Replacement of iconv and iconv_open, because of
> >   checking whether iconv supports conversion between UTF-8 and
> > UTF-{16,32}{BE,LE}... no
> Not "nonconformant" from the standpoint of POSIX, AFAICT, but it is
> incomplete. musl is UTF8 native, but I don't think it supports UTF16/UTF32
> yet.

It does. musl's iconv supports all UTFs (but no endian auto-detection;
explicit endianness is required in the argument to iconv_open),
all ISO-8859s, lots of other legacy 8bit charsets, and some legacy
Chinese and Japanese charsets as source charsets only (not
destination).

> > Replacement of mktime, because of
> >   checking for working mktime... no
> > Replacement of perror, because of
> >   checking whether perror matches strerror... no
> > Replacement of popen, because of
> >   checking whether popen works with closed stdin... no
>  Look like bugs, if the description is correct.

Yes, I'm looking into these as possible bugs, but I don't think
anything's wrong with perror...

> > Replacement of regex, because of
> >   checking for working re_compile_pattern... no
> This is #ifdef __USE_GNU
> I'm not aware of any standard covering GNU APIs...

Well gnulib wants to offer this API, so if it doesn't exist, gnulib
must provide it..

Rich

Re: Re: musl bugs found through gnulib

[Rich Felker]

On Wed, Jun 20, 2012 at 03:28:02PM -0400, Rich Felker wrote:
> > Replacement of getcwd, because of
> >   checking whether getcwd handles long file names properly... no, but it is partly working

This test is failing because musl uses the kernel to resolve the
current directory name, and the kernel does not support pathnames
longer than PATH_MAX. For some reason, the test only considers this an
error if AT_FDCWD is defined.

Does gnulib aim to provide a getcwd that always works regardless of
path depth? If so, replacing getcwd is the right action for gnulib on
musl.

> >   checking whether getcwd aborts when 4k < cwd_length < 16k... no

No is the correct result here. This test is looking for a bug that
only exists on some archs with large page sizes (>4k), and no means it
did not find the bug.

> > Replacement of mktime, because of
> >   checking for working mktime... no

This test is buggy; it goes into an infinite loop due to integer
overflow UB, because the condition to break out of the loop is only
checked when the test does not fail:

      for (j = 1; ; j <<= 1)
        if (! bigtime_test (j))
          result |= 4;
        else if (INT_MAX / 2 < j)
          break;

However this does indicate a bug in musl. The relevant code is very
old and I suspect it's not checking for integer overflows at all, just
generating huge time_t values that get truncated rather than mapped to
(time_t)-1.

Both need to be fixed.

> > test-fcntl.c:382: assertion failed
> > FAIL: test-fcntl
> 
> Pending; intend to fix.

Fixed.

> > test-fma2.h:116: assertion failed
> > FAIL: test-fma2
> 
> Unknown. Asking nsz..

Fixed by nsz. :-)

Rich

Re: Re: musl bugs found through gnulib

[Rich Felker]

On Mon, Jun 18, 2012 at 12:49:44AM +0200, Bruno Haible wrote:
> [CCing the musl list]
> Isaac Dunham wrote in
> <http://lists.gnu.org/archive/html/bug-gnulib/2012-06/msg00101.html>:
> > musl is designed for standards conformance,
> 
> There is a recipe, in <http://sourceware.org/glibc/wiki/Testing/Gnulib>,
> that explains how to use gnulib to check a libc against bugs. When I apply
> this to musl-0.9.1, I get this list of problems:
> 
> Replacements of *printf, because of
>   checking whether printf supports infinite 'long double' arguments... no

Fixed. (Not really a bug, but fixed anyway.)

>   checking whether printf supports the 'ls' directive... no

Previously fixed.

>   checking whether printf survives out-of-memory conditions... no

Fixed.

> Replacement of duplocale, because of
>   checking whether duplocale(LC_GLOBAL_LOCALE) works... no

Fixed.

> Replacement of fdopen, because of
>   checking whether fdopen sets errno... no

Not a bug. I believe this was fixed in gnulib.

> Replacement of futimens, because of
>   checking whether futimens works... no

Not a bug; just confusing message.

> Replacement of getcwd, because of
>   checking whether getcwd handles long file names properly... no, but it is partly working
>   checking whether getcwd aborts when 4k < cwd_length < 16k... no

Still open; probably not a bug.

> Replacement of getopt, because of
>   checking whether getopt is POSIX compatible... no

Not a bug.

> Replacement of glob, because of
>   checking for GNU glob interface version 1... no
> (not sure this is a bug or just an incompatibility compared to glibc)

Not supported.

> Replacement of iconv and iconv_open, because of
>   checking whether iconv supports conversion between UTF-8 and UTF-{16,32}{BE,LE}... no

Fixed.

> 
> Replacement of mktime, because of
>   checking for working mktime... no

Still open.

> Replacement of perror, because of
>   checking whether perror matches strerror... no

Fixed.

> Replacement of popen, because of
>   checking whether popen works with closed stdin... no

Fixed.

> Replacement of regex, because of
>   checking for working re_compile_pattern... no

Not supported.

> Replacement of strtod, because of
>   checking whether strtod obeys C99... no

Previously fixed.

> test-duplocale.c:70: assertion failed
> FAIL: test-duplocale

Fixed.

> test-fcntl.c:382: assertion failed
> FAIL: test-fcntl

Pending; intend to fix.

> test-fdatasync.c:50: assertion failed
> FAIL: test-fdatasync

Fixed.

> test-fma2.h:116: assertion failed
> FAIL: test-fma2

Unknown. Asking nsz..

> test-fsync.c:50: assertion failed
> FAIL: test-fsync

Fixed.

> test-fwrite.c:53: assertion failed
> FAIL: test-fwrite

Fixed.

> test-getlogin_r.c:88: assertion failed
> FAIL: test-getlogin_r

Fixed.

> test-grantpt.c:34: assertion failed
> FAIL: test-grantpt

Buggy/useless test.

> test-localeconv.c:41: assertion failed
> FAIL: test-localeconv

Fixed.

> Segmentation fault
> FAIL: test-localename

Still open.

> test-ptsname_r.c:118: assertion failed
> FAIL: test-ptsname_r

Fixed.

> test-strerror_r.c:118: assertion failed
> FAIL: test-strerror_r

Fixed.

> test-wcwidth.c:71: assertion failed
> FAIL: test-wcwidth

Fixed.

> When I compile all of gnulib, I also get a compilation error
> (may be a musl or a gnulib problem, haven't investigated):
> fsusage.c: In function 'get_fs_usage':
> fsusage.c:222:17: error: storage size of 'fsd' isn't known
> fsusage.c:224:3: warning: implicit declaration of function 'statfs' [-Wimplicit-function-declaration]
> fsusage.c:222:17: warning: unused variable 'fsd' [-Wunused-variable]
> make[4]: *** [fsusage.o] Error 1

OK, this is valid fallback code for when statvfs fails, but the
headers required for it have not been included.

Basically the only still-open issues are getcwd, mktime, fma,
localename, so I'll avoid future spam by just addressing them. I've
kept all the Cc's so far, but if this is getting OT for gnulib folks,
I'll be happy to drop the Cc. Just let me know.

Rich

Re: Re: musl bugs found through gnulib

[Rich Felker]

On Tue, Jun 19, 2012 at 10:10:11PM -0600, Eric Blake wrote:
> Unfortunately, you are out of date.  POSIX _does_ require
> duplocale(LC_GLOBAL_LOCALE) to work:
> 
> http://austingroupbugs.net/view.php?id=301

OK. I'll add support. For now all it requires is avoiding
dereferencing the pointer, anyway.

> Yes, Linux 2.6.32 introduced F_GETOWN_EX for precisely this reason, and
> you should be using it.

When I wrote that code, 2.6.32 was new enough, and the issue seemed
minor enough, that I didn't bother. Now I agree it would be nice
though.

> >> test-grantpt.c:34: assertion failed
> >> FAIL: test-grantpt
> > 
> > This is an invalid test. POSIX specifies this function "may fail", not
> > "shall fail", and since the function is inherently a no-op, it would
> > be idiotic to make it perform a syscall to check the validity of the
> > file descriptor...
> 
> This is one of the cases where gnulib prefers to emulate the shall fail
> semantics of glibc, as they are more useful to program around.

I don't see how it's nicer. All it does it make pty acquisition
slightly slower (one extra useless syscall). The only time you would
call grantpt without knowing that the fd is valid is right after
calling posix_openpt without checking the return value, and in that
case, it seems unlikely that you'd check the return value of grantpt.
And last time I asked, I remember being told that gnulib does not
intend to facilitate this sort of lazy programming anyway.

In any case, if you are relying on lazy error checking like that,
unlockpt will already report the error...

> >> test-ptsname_r.c:118: assertion failed
> >> FAIL: test-ptsname_r
> > 
> > It's testing that ptsname_r both sets errno and returns the error
> > code, and that they're the same. Since this function is nonstandard,
> > there's no spec for it, so perhaps this is desirable; I was assuming
> > it should return -1 on failure.
> 
> There _is_ a proposed standard for it now:
> 
> http://austingroupbugs.net/view.php?id=508
> 
> which requires only the return value to be 0 or an errno value, and not
> that errno be set.  gnulib should only be checking for a valid return value.

Okay, I'll update it to match this.

I wish they'd just standardized the superior BSD openpty function
instead...

> >> test-strerror_r.c:118: assertion failed
> >> FAIL: test-strerror_r
> > 
> > This test is looking for a null terminator at the n-1 position of the
> > buffer if strerror_r fails with ERANGE (buffer too small). I don't see
> > anywhere the function is specified to write to the buffer AT ALL on
> > failure, so this test seems invalid.
> 
> This is a case where POSIX is rather weak, but where quality of
> implementation demands that the most useful interface is one that
> provides the most information back to the user.  glibc had a number of
> bugs that were fixed in this area to improve QoI, and gnulib now prefers
> to rely on those improvements.

I don't see anything which forbids it from writing in this case, so I
suppose I could change it.

Rich

Re: Re: musl bugs found through gnulib

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2012-06-19 23:04:45 -0400]:
> Some more updates..
> 
> On Mon, Jun 18, 2012 at 12:49:44AM +0200, Bruno Haible wrote:
> > Replacement of perror, because of
> >   checking whether perror matches strerror... no
> > 

this is a musl issue:

diff --git a/src/stdio/perror.c b/src/stdio/perror.c
index 4349ac5..fdcb4d7 100644
--- a/src/stdio/perror.c
+++ b/src/stdio/perror.c
@@ -10,7 +10,7 @@ void perror(const char *msg)
 
        FLOCK(f);
        
-       if (msg) {
+       if (msg && *msg) {
                fwrite(msg, strlen(msg), 1, f);
                fputc(':', f);
                fputc(' ', f);

Re: Re: musl bugs found through gnulib

[Rich Felker]

Some more updates..

On Mon, Jun 18, 2012 at 12:49:44AM +0200, Bruno Haible wrote:
> Replacements of *printf, because of
> [...]
>   checking whether printf survives out-of-memory conditions... no

This was caused by the pointer-arithmetic overflow bug I just fixed in
git. It should no longer fail, and never failed before except in i386
binaries running on x86_64 kernels.

> Replacement of duplocale, because of
>   checking whether duplocale(LC_GLOBAL_LOCALE) works... no

POSIX does not specify any use of LC_GLOBAL_LOCALE except as an
argument to uselocale. Is there a reason it's needed? Perhaps more
importantly, is the replacement when libc doesn't provide this
functionality bloated/painful?

> Replacement of fdopen, because of
>   checking whether fdopen sets errno... no

Seems to have been fixed in gnulib.

> Replacement of getcwd, because of
>   checking whether getcwd handles long file names properly... no, but it is partly working
>   checking whether getcwd aborts when 4k < cwd_length < 16k... no

Still unclear what the cause of these failures is. Anyone else looked
into them, or do I still need to?

> Replacement of iconv and iconv_open, because of
>   checking whether iconv supports conversion between UTF-8 and UTF-{16,32}{BE,LE}... no

I fixed all the UTF-16-related bugs that were breaking this test. It
should pass now.

> Replacement of mktime, because of
>   checking for working mktime... no
> 
> Replacement of perror, because of
>   checking whether perror matches strerror... no
> 
> Replacement of popen, because of
>   checking whether popen works with closed stdin... no
> 
> Replacement of regex, because of
>   checking for working re_compile_pattern... no
> 
> Replacement of strtod, because of
>   checking whether strtod obeys C99... no
> 
> For each of the replacements, first look at the test program's results
> (in config.log), then look at the test program's source code (in m4/*.m4).
> 
> Furthermore we have test failures:
> 
> test-duplocale.c:70: assertion failed
> FAIL: test-duplocale
> 
> test-fcntl.c:382: assertion failed
> FAIL: test-fcntl

This is caused by the fact that the F_GETOWN fcntl on Linux is broken;
there's no way to distinguish error returns from non-error negative
return values. So we never set errno when calling F_GETOWN and assume
the return value is not an error. There's a new-ish Linux-specific
F_GETOWN_EX we could use when it's available, but the fallback code
would still fail just like it does now, because it's a fundamental
limitation in the API.

> test-fdatasync.c:50: assertion failed
> FAIL: test-fdatasync

This function was dummied-out for some reason. Fixed.

> test-fsync.c:50: assertion failed
> FAIL: test-fsync

Same.

> test-fwrite.c:53: assertion failed
> FAIL: test-fwrite

This seems like it might be a real bug. On musl, unbuffered files
actually have a one-byte buffer, but on writing, the buffer is
supposed to be flushed as soon as it fills, rather than waiting for
another write when it's full. I'll have to run some tests...

> test-getlogin_r.c:88: assertion failed
> FAIL: test-getlogin_r

This was broken; it should be fixed now.

> test-grantpt.c:34: assertion failed
> FAIL: test-grantpt

This is an invalid test. POSIX specifies this function "may fail", not
"shall fail", and since the function is inherently a no-op, it would
be idiotic to make it perform a syscall to check the validity of the
file descriptor...

> test-localeconv.c:41: assertion failed
> FAIL: test-localeconv

Fixed lots of issues; not sure if it works now.

> Segmentation fault
> FAIL: test-localename

This might be due to our incomplete locale implementation, or because
the test uses locale names that don't exist. I doubt it should
segfault though. I'll look into this one later.

> test-ptsname_r.c:118: assertion failed
> FAIL: test-ptsname_r

It's testing that ptsname_r both sets errno and returns the error
code, and that they're the same. Since this function is nonstandard,
there's no spec for it, so perhaps this is desirable; I was assuming
it should return -1 on failure.

> test-strerror_r.c:118: assertion failed
> FAIL: test-strerror_r

This test is looking for a null terminator at the n-1 position of the
buffer if strerror_r fails with ERANGE (buffer too small). I don't see
anywhere the function is specified to write to the buffer AT ALL on
failure, so this test seems invalid.

> test-wcwidth.c:71: assertion failed
> FAIL: test-wcwidth

It's checking for wcwidth(0x3000)==2. This definitely used to work,
but it might have been broken when I overhauled wcwidth. I'll look
into it..

> When I compile all of gnulib, I also get a compilation error
> (may be a musl or a gnulib problem, haven't investigated):
> fsusage.c: In function 'get_fs_usage':
> fsusage.c:222:17: error: storage size of 'fsd' isn't known
> fsusage.c:224:3: warning: implicit declaration of function 'statfs' [-Wimplicit-function-declaration]
> fsusage.c:222:17: warning: unused variable 'fsd' [-Wunused-variable]
> make[4]: *** [fsusage.o] Error 1

This looks like a gnulib problem. On musl, statvfs should get used,
and this code should not even be compiled... Judging from the
comments, it looks like a hard-coded workaround for broken glibc
and/or Linux versions, but the header include seems to be missing in
the workaround case...

Rich

Re: Re: musl bugs found through gnulib

[John Spencer]

On 06/18/2012 04:55 PM, Rich Felker wrote:
>
> If you'd like to post a script to do this that actually works with all
> gnulib programs, I'll consider that as an alternative to working with
> the gnulib developers... ;-)
>

http://sprunge.us/dKaK

it works by passing configure cache variables that will skip over the 
bogus conformance tests.
unfortunately the compilation of fseterr/freadahead is unconditonal, 
thus the 2 C files must be "fixed" to not produce the well known compile 
errors.

the only test that isn't turned off is the one for "POSIX getopt", 
because apparently the meaning is "GNU getopt", so using musl's getopt 
instead of the replacement one might cause problems.

Re: Re: musl bugs found through gnulib

[Rich Felker]

On Mon, Jun 18, 2012 at 08:07:40PM -0600, Eric Blake wrote:
> On 06/18/2012 06:11 PM, Rich Felker wrote:
> > Some updates...
> > 
> > On Mon, Jun 18, 2012 at 12:49:44AM +0200, Bruno Haible wrote:
> >> There is a recipe, in <http://sourceware.org/glibc/wiki/Testing/Gnulib>,
> >> that explains how to use gnulib to check a libc against bugs. When I apply
> >> this to musl-0.9.1, I get this list of problems:
> >>
> >> Replacements of *printf, because of
> >> [...]
> >>   checking whether printf survives out-of-memory conditions... no
> > 
> > No idea. Copying out the test and running it directly, it passes just
> > fine for me. Maybe gnulib has already replaced printf with its own
> > malloc-using version by the time it gets to this test??
> 
> No; configure-time tests are relatively independent, and all done prior
> to any replacements at compile-time.  You should be able to inspect
> config.log to see the actual test that configure ran.

OK, then no idea what's causing this. I was going to try running the
test but I didn't have autotools installed on the system I wanted to
test on, so I put it off..

> >> Replacement of fdopen, because of
> >>   checking whether fdopen sets errno... no
> > 
> > There was one bug here (failure to set errno when mode string was
> > invalid) but I don't think that's the case gnulib was testing for. It
> > seems gnulib wants an error for the "may fail" when the fd is invalid.
> 
> The 'EBADF may fail' condition is rather weak.  And since glibc
> guarantees a definite failure, it is nicer to program to the glibc
> interface that guarantees immediate failure on a bad fd at fdopen() time
> than it is to deal with the surprises that result when fdopen() succeeds
> but later attempts to use the stream fail.  Perhaps it might be worth

The only real-world situation I can think of where you'd care that
fdopen detect EBADF is when you've just called a function that
allocates the file descriptor and directly passed it to fdopen without
first checking the return value. For instance:

FILE *f = fdopen(open(pathname, O_RDWR|O_CLOEXEC), "rb+");

instead of:

int fd = open(pathname, O_RDWR|O_CLOEXEC);
if (fd<0) goto error;
FILE *f = fdopen(fd, "rb+");

The former is rather lazy programming, but maybe gnulib intends to
support this kind of programming.

> splitting this into two gnulib modules, one for the strict POSIX
> compliance and one for the glibc interface, but that depends on how
> likely people are to want to program to the weaker POSIX interface when
> it is just as easy to make fdopen() guarantee failure on bad fds and
> save efforts up front.

My thought in having musl skip the test is to maximize performance of
fdopen, assuming you might be using it in a situation like on a newly
accept()ed network connection where every syscall counts (think
multi-threaded httpd). For read-only fdopen, no syscalls are needed,
but if writing is possible, fdopen has to make a syscall to check
whether the fd is a tty in order to decide whether to enable line
buffering or full buffering, and in principle it could detect EBADF at
the same time for no cost.

> >> Replacement of futimens, because of
> >>   checking whether futimens works... no
> > 
> > gnulib always forces this test to fail if __linux__ is defined.
> 
> That's because the Linux kernel got it wrong for quite some time, and
> worse, it was file-system dependent - even if it worked on one machine
> and file system, compiling in support for futimens and then running on
> another file system would experience random compliance failures due to
> the poor file system implementation.
> 
> It's been a while, so maybe we can finally graduate this module into
> assuming that the Linux kernel is better behaved by default, and make
> the user specifically request the fallbacks if they are worried about
> using the binary on a file system that still has bugs.  But don't take
> it personally - this is not a case of musl getting it wrong, but of the
> kernel getting it wrong.

Yes, it might be nice if the test output made it clear that it was
hard-coded to fail so nobody goes looking for nonexistant bugs..

Rich

Re: Re: musl bugs found through gnulib

[Rich Felker]

Some updates...

On Mon, Jun 18, 2012 at 12:49:44AM +0200, Bruno Haible wrote:
> There is a recipe, in <http://sourceware.org/glibc/wiki/Testing/Gnulib>,
> that explains how to use gnulib to check a libc against bugs. When I apply
> this to musl-0.9.1, I get this list of problems:
> 
> Replacements of *printf, because of
> [...]
>   checking whether printf survives out-of-memory conditions... no

No idea. Copying out the test and running it directly, it passes just
fine for me. Maybe gnulib has already replaced printf with its own
malloc-using version by the time it gets to this test??

> Replacement of fdopen, because of
>   checking whether fdopen sets errno... no

There was one bug here (failure to set errno when mode string was
invalid) but I don't think that's the case gnulib was testing for. It
seems gnulib wants an error for the "may fail" when the fd is invalid.

> Replacement of futimens, because of
>   checking whether futimens works... no

gnulib always forces this test to fail if __linux__ is defined.

Rich

Re: Re: musl bugs found through gnulib

[Rich Felker]

On Mon, Jun 18, 2012 at 05:26:32PM +0200, Szabolcs Nagy wrote:
> * Rich Felker <dalias@aerifal.cx> [2012-06-18 10:55:45 -0400]:
> > > there's only one broken program out there, which is gnu coreutils' "od".
> > > if someone wants to use coreutils and cares about this uncommon
> > > issue, he/she can simply use a patch for this specific program.
> > 
> > The problem is that gnulib is potentially replacing printf in many
> > programs (I haven't checked this; would you care to check?) over this
> > stupid issue that only affects one broken program, resulting in
> > massive bloat for users, and potentially breaking things.
> > 
> 
> hm, what's the issue with od?
> is it about printing random binary data as long double?

Yes.

> if od invokes undefined behaviour then it's a coreutils bug

Yes.

> it sounds bad that based on a broken test gnulib may replace
> a correct printf implementation with its own broken one
> just to make a broken coreutils tool to work so now we
> need a libc workaround for this issue..

Coreutils should at least be changed to call isnanl() in the data
before calling printf. Then they can get by with just replacing isnanl
(ugly but relatively cheap and non-intrusive) instead of replacing all
of printf.

Rich

Re: Re: musl bugs found through gnulib

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2012-06-18 10:55:45 -0400]:
> > there's only one broken program out there, which is gnu coreutils' "od".
> > if someone wants to use coreutils and cares about this uncommon
> > issue, he/she can simply use a patch for this specific program.
> 
> The problem is that gnulib is potentially replacing printf in many
> programs (I haven't checked this; would you care to check?) over this
> stupid issue that only affects one broken program, resulting in
> massive bloat for users, and potentially breaking things.
> 

hm, what's the issue with od?
is it about printing random binary data as long double?

if od invokes undefined behaviour then it's a coreutils bug

it sounds bad that based on a broken test gnulib may replace
a correct printf implementation with its own broken one
just to make a broken coreutils tool to work so now we
need a libc workaround for this issue..

Re: Re: musl bugs found through gnulib

[Rich Felker]

On Mon, Jun 18, 2012 at 03:02:16PM +0200, John Spencer wrote:
> On 06/18/2012 10:21 AM, Szabolcs Nagy wrote:
> >* Rich Felker<dalias@aerifal.cx>  [2012-06-17 19:54:26 -0400]:
> >
> >>On Mon, Jun 18, 2012 at 12:49:44AM +0200, Bruno Haible wrote:
> >>>Replacements of *printf, because of
> >>>   checking whether printf supports infinite 'long double' arguments... no
> >>I'm just going to change isnanl to handle the invalid bit patterns
> >>because I'm sick of this issue...
> >>
> please don't do this. this would be a win of Evil against Good.
> 
> do you see were this goes ?
> first they wanted you to implement __freadahead, now they want 4
> such functions

Actually at first they were trying to do it in gnulib, and I told them
rather than trying to put ugly musl-specific hacks in gnulib that will
break in the future, I'd prefer to just provide the function, as long
as it can be detected by configure and not...

> plus a broken __MUSL__ macro.

This is rejected.

Bruno has criticized my rejection of it as purely theoretical,
ignoring practical needs, etc. But you've seen it on the list and irc:
how many times have new users/contributors shown up trying to get a
package working with musl, asked if there's __MUSL__ or similar, and
then LEARNED SOMETHING and FIXED THE PACKAGE RIGHT when told that
there's no such thing and that the package's configure script should
be fixed to detect things rather than hard-coding assumptions?

I hate to think how many packages would have broken #ifdef __MUSL__
patches in them by now if the people submitting patches had that
available to work with...and then the package maintainers would get
frustrated with musl in a hurry when they had to fix things again
later because the assumption they hard-coded was bad.

> this is usually refered to using the proverb "give them the small
> finger and they take the entire hand".

I do feel a bit like my interaction with Eric versus later interaction
with Bruce has been something of a bait-and-switch. But you know the
old saying: never attribute to malice what can be adequately
explained....

> the tactics of the gnulib folks is to wear you down, until they have
> no work to do at all.

I don't follow your reasoning here. If anything Bruno has been
pushing to make more work for himself by adding more special cases
that have to be maintained, instead of adopting a general solution..

> also note how Bruno changed the subjects in "gnulib portability
> issues" thread on the gnulib ML into something that suggests the
> opposite, i.e. that musl is broken.
> 
> there's only one broken program out there, which is gnu coreutils' "od".
> if someone wants to use coreutils and cares about this uncommon
> issue, he/she can simply use a patch for this specific program.

The problem is that gnulib is potentially replacing printf in many
programs (I haven't checked this; would you care to check?) over this
stupid issue that only affects one broken program, resulting in
massive bloat for users, and potentially breaking things.

> >i think we should not invest too much time
> >into gnulib but work on a proper libc
> >testsuit instead..
> 
> full ACK. now that we know which tests are responsible for detecting

I think nsz was talking about testing, not about app compatibility.
The gnulib tests have turned out to be 50-50 or worse for detecting
actual bugs versus broken tests. Off-hand, that's slightly worse than
Open POSIX Test Suite's rate...

In any case, I think we've gotten almost all we can out of the gnulib
tests in terms of fixing bugs.

> many musl functions (wrongly) as broken, we can simply create some
> "gnulib-fix" program that makes these tests always succeed, thus
> keeping their bloat outside.

If you'd like to post a script to do this that actually works with all
gnulib programs, I'll consider that as an alternative to working with
the gnulib developers... ;-)

> PLUS add a warning at a prominent spot that programs using GNULIB
> should be built statically to avoid future incompatibilities due to
> their illegal poking at internals.

I don't think there's any poking at internals yet, and the proposal is
NOT to include any of that stuff.

> sabotage currently uses the following approach to beat gnulib into
> functioning:
> 
> empty_file() {
>     rm -f "$1"
>     touch "$1"
> }
> 
> ../configure -C --prefix=/ || exit 1
> 
> #fixing gnulib
> 
> cd lib
> for i in printf.c fprintf.c freadahead.c closein.c fseterr.c vfprintf.c ; do
>     empty_file "$i"
> done
> 
> echo "#define __printf__ printf" >> config.h
> echo "#define rpl_fprintf fprintf" >> config.h
> 
> cd ..

This looks like ugly hackery that's poking at gnulib internals rather
than operating at the configure level like it should. I think it's a
bit hypocritical to criticize them for poking at libc internals and
then turn around and poke at their internals...

> sed -i 's,atexit (close_stdin),,' gzip.c
> # probably better to just echo "void close_stdin(void) {}" > close_in.c

This will make some of the GNU coreutils non-conformant to POSIX.

I've already discussed on this list why using an atexit function for
closing stdin or stdout is the heart of the problem and the source of
all the complexity. If the main program just closed the stream at the
natural point in the normal program flow, it would be trivial to do
correctly and portably.

Rich

Re: Re: musl bugs found through gnulib

[John Spencer]

On 06/18/2012 10:21 AM, Szabolcs Nagy wrote:
> * Rich Felker<dalias@aerifal.cx>  [2012-06-17 19:54:26 -0400]:
>
>> On Mon, Jun 18, 2012 at 12:49:44AM +0200, Bruno Haible wrote:
>>> Replacements of *printf, because of
>>>    checking whether printf supports infinite 'long double' arguments... no
>> I'm just going to change isnanl to handle the invalid bit patterns
>> because I'm sick of this issue...
>>
please don't do this. this would be a win of Evil against Good.

do you see were this goes ?
first they wanted you to implement __freadahead, now they want 4 such 
functions plus a broken __MUSL__ macro.
this is usually refered to using the proverb "give them the small finger 
and they take the entire hand".

the tactics of the gnulib folks is to wear you down, until they have no 
work to do at all.

also note how Bruno changed the subjects in "gnulib portability issues" 
thread on the gnulib ML into something that suggests the opposite, i.e. 
that musl is broken.

there's only one broken program out there, which is gnu coreutils' "od".
if someone wants to use coreutils and cares about this uncommon issue, 
he/she can simply use a patch for this specific program.

> i wonder if these gnulib bugs will get fixed..
>
> gnulib should go over an audit
> (otherwise it's not credible considering all
> the false tests so far)
>
> some of the gnulib tests will find musl bugs
> but many tests are wrong
>
> i think we should not invest too much time
> into gnulib but work on a proper libc
> testsuit instead..

full ACK. now that we know which tests are responsible for detecting 
many musl functions (wrongly) as broken, we can simply create some 
"gnulib-fix" program that makes these tests always succeed, thus keeping 
their bloat outside.
PLUS add a warning at a prominent spot that programs using GNULIB should 
be built statically to avoid future incompatibilities due to their 
illegal poking at internals.

sabotage currently uses the following approach to beat gnulib into 
functioning:

empty_file() {
     rm -f "$1"
     touch "$1"
}

./configure -C --prefix=/ || exit 1

#fixing gnulib

cd lib
for i in printf.c fprintf.c freadahead.c closein.c fseterr.c vfprintf.c ; do
     empty_file "$i"
done

echo "#define __printf__ printf" >> config.h
echo "#define rpl_fprintf fprintf" >> config.h

cd ..

sed -i 's,atexit (close_stdin),,' gzip.c
# probably better to just echo "void close_stdin(void) {}" > close_in.c

--JS

Re: Re: musl bugs found through gnulib

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2012-06-17 19:54:26 -0400]:

> On Mon, Jun 18, 2012 at 12:49:44AM +0200, Bruno Haible wrote:
> > Replacements of *printf, because of
> >   checking whether printf supports infinite 'long double' arguments... no
> 
> I'm just going to change isnanl to handle the invalid bit patterns
> because I'm sick of this issue...
> 

i wonder if these gnulib bugs will get fixed..

gnulib should go over an audit
(otherwise it's not credible considering all
the false tests so far)

some of the gnulib tests will find musl bugs
but many tests are wrong

i think we should not invest too much time
into gnulib but work on a proper libc
testsuit instead..

> > Replacement of getcwd, because of
> >   checking whether getcwd handles long file names properly... no, but it is partly working
> >   checking whether getcwd aborts when 4k < cwd_length < 16k... no
> 
> I'm not sure if it's related, but musl defines PATH_MAX and does not
> attempt to support explicit pathnames longer than PATH_MAX. I'll have
> to look at what the test is doing.
> 

heh it sounds as if gnulib wants getcwd to abort
that's wrong

> > Replacement of perror, because of
> >   checking whether perror matches strerror... no
> 
> Odd, it uses strerror..
> 

this one is musl, in perror.c:
- if (msg) {
+ if (msg && *msg) {

but the gnulib replacement looks dangerous/wrong
(why would you want perror to abort?)

Re: Re: musl bugs found through gnulib

[Rich Felker]

On Mon, Jun 18, 2012 at 12:49:44AM +0200, Bruno Haible wrote:
> Replacements of *printf, because of
>   checking whether printf supports infinite 'long double' arguments... no

I'm just going to change isnanl to handle the invalid bit patterns
because I'm sick of this issue...

>   checking whether printf supports the 'ls' directive... no

Fixed in git on Jun 8.

>   checking whether printf survives out-of-memory conditions... no

This test must be doing something wrong. musl's printf does not use
any dynamic memory.

> Replacement of duplocale, because of
>   checking whether duplocale(LC_GLOBAL_LOCALE) works... no

I'll look into this. Locales are not presently supported, but the
interfaces are intended to work and behave as they should for an
implementation that has exactly one possible locale.

> Replacement of fdopen, because of
>   checking whether fdopen sets errno... no

I'll look into it.

> Replacement of futimens, because of
>   checking whether futimens works... no

Odd, this is just a syscall wrapper, no?

> Replacement of getcwd, because of
>   checking whether getcwd handles long file names properly... no, but it is partly working
>   checking whether getcwd aborts when 4k < cwd_length < 16k... no

I'm not sure if it's related, but musl defines PATH_MAX and does not
attempt to support explicit pathnames longer than PATH_MAX. I'll have
to look at what the test is doing.

> Replacement of getopt, because of
>   checking whether getopt is POSIX compatible... no

This was a bug I reported to gnulib. The test is checking for GNU
semantics which are incompatible with POSIX, i.e. the test is
misnamed.

> Replacement of glob, because of
>   checking for GNU glob interface version 1... no
> (not sure this is a bug or just an incompatibility compared to glibc)

I think gnulib just wants GNU glob functionality. This is legitimate
for it to replace since we (at present, at least) don't attempt to
offer any GNU functionality in glob.

> Replacement of iconv and iconv_open, because of
>   checking whether iconv supports conversion between UTF-8 and UTF-{16,32}{BE,LE}... no

These conversions are definitely supported. The only related thing we
don't support is BOMs and stateful/endian-detected UTF-16/32.

> Replacement of mktime, because of
>   checking for working mktime... no

I need to check this. I've suspected it might have a bug.

> Replacement of perror, because of
>   checking whether perror matches strerror... no

Odd, it uses strerror..

> Replacement of popen, because of
>   checking whether popen works with closed stdin... no

Sounds like a bug; I'll check it out.

> Replacement of regex, because of
>   checking for working re_compile_pattern... no

This is a check for the GNU regex interfaces as opposed to POSIX
regcomp/regexec.

> Replacement of strtod, because of
>   checking whether strtod obeys C99... no

This was caused by a missing negative zero case for "-0x" followed by
non-hex chars. It's fixed in git.

> For each of the replacements, first look at the test program's results
> (in config.log), then look at the test program's source code (in m4/*.m4).

OK, looking...

Rich