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: [musl] Re: musl bugs found through gnulib

[Pádraig Brady]

On 06/20/2012 05:04 AM, Rich Felker wrote:
> Some more updates..
> 
> On Mon, Jun 18, 2012 at 12:49:44AM +0200, Bruno Haible wrote:
>> 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...

This is addressed in different ways in these two commits:

http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commit;h=defe5737
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commit;h=2ab2617e

cheers,
Pádraig.

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

[Paul Eggert]

On 06/20/2012 07:21 PM, Rich Felker wrote:

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

Thanks, I pushed the following patch into gnulib:

---
 ChangeLog    |    9 +++++++++
 m4/mktime.m4 |   25 ++++++++++++++-----------
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 199b06c..1661a62 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2012-06-21  Paul Eggert  <eggert@cs.ucla.edu>
+
+	mktime: fix integer overflow in 'configure'-time test
+	* m4/mktime.m4 (gl_FUNC_MKTIME): Do not rely on undefined behavior
+	after integer overflow.  Problem reported by Rich Felker in
+	<http://lists.gnu.org/archive/html/bug-gnulib/2012-06/msg00257.html>.
+	Also, don't look for further instances of a bug if we've already
+	found one instance; this helps 'configure' run faster.
+
 2012-06-20  John Darrington  <john@darrington.wattle.id.au>  (tiny change)
 
 	tmpfile, clean-temp: Fix invocation of GetVersionEx.
diff --git a/m4/mktime.m4 b/m4/mktime.m4
index 5e05dfa..14fcf7f 100644
--- a/m4/mktime.m4
+++ b/m4/mktime.m4
@@ -1,4 +1,4 @@
-# serial 21
+# serial 22
 dnl Copyright (C) 2002-2003, 2005-2007, 2009-2012 Free Software Foundation,
 dnl Inc.
 dnl This file is free software; the Free Software Foundation
@@ -192,20 +192,23 @@ main ()
       if (tz_strings[i])
         putenv (tz_strings[i]);
 
-      for (t = 0; t <= time_t_max - delta; t += delta)
+      for (t = 0; t <= time_t_max - delta && (result & 1) == 0; t += delta)
         if (! mktime_test (t))
           result |= 1;
-      if (! (mktime_test ((time_t) 1)
-             && mktime_test ((time_t) (60 * 60))
-             && mktime_test ((time_t) (60 * 60 * 24))))
+      if ((result & 2) == 0
+          && ! (mktime_test ((time_t) 1)
+                && mktime_test ((time_t) (60 * 60))
+                && mktime_test ((time_t) (60 * 60 * 24))))
         result |= 2;
 
-      for (j = 1; ; j <<= 1)
-        if (! bigtime_test (j))
-          result |= 4;
-        else if (INT_MAX / 2 < j)
-          break;
-      if (! bigtime_test (INT_MAX))
+      for (j = 1; (result & 4) == 0; j <<= 1)
+        {
+          if (! bigtime_test (j))
+            result |= 4;
+          if (INT_MAX / 2 < j)
+            break;
+        }
+      if ((result & 8) == 0 && ! bigtime_test (INT_MAX))
         result |= 8;
     }
   if (! irix_6_4_bug ())
-- 
1.7.6.5

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3678 bytes --]

On 06/19/2012 09:04 PM, Rich Felker wrote:

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

Unfortunately, you are out of date.  POSIX _does_ require
duplocale(LC_GLOBAL_LOCALE) to work:

http://austingroupbugs.net/view.php?id=301


    If the locobj argument is LC_GLOBAL_LOCALE, duplocale() shall
    create a new locale object containing a copy of the global locale
    determined by the setlocale() function.

    The behavior is undefined if the locobj argument is not a valid
    locale object handle.

  After line 24978 add a new paragraph to APPLICATION USAGE:

    The duplocale() function can also be used in conjunction with
    uselocale((locale_t)0). This returns the locale in effect for
    the calling thread, but can have the value LC_GLOBAL_LOCALE.
    Passing LC_GLOBAL_LOCALE to functions such as isalnum_l()
    results in undefined behavior, but applications can convert
    it into a usable locale object by using 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.

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


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


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

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

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2821 bytes --]

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.

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

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

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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