Re: musl bugs found through gnulib

Re: musl bugs found through gnulib

[Bruno Haible]

[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
  checking whether printf supports the 'ls' directive... no
  checking whether printf survives out-of-memory conditions... no

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

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

Replacement of futimens, because of
  checking whether futimens works... 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

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

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)

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

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

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

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

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

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

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

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

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

Segmentation fault
FAIL: test-localename

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

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

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

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

Bruno

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

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

[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

[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

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

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

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: [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: 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: musl, fdopen test

[Bruno Haible]

Rich Felker wrote:
> > >> 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.

Indeed, the possibility that fdopen(invalid fd, ...) can succeed is supported
by POSIX. When looking at the gnulib documentation
  doc/posix-functions/fdopen.texi
and the unit test
  tests/test-fdopen.c
it appears that it was not the intent of gnulib to prohibit this behaviour.
Rather, musl is the first platform to exhibit this behaviour, and gnulib's
intent was to make sure that fdopen(invalid fd, ...)
  1. does not crash,
  2. sets errno when it fails.

Eric Blake replied:
> > 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 glibc documentation contains this warning:

     In some other systems, `fdopen' may fail to detect that the modes
     for file descriptor do not permit the access specified by
     `opentype'.  The GNU C library always checks for this.

So, I think few programmers will explicitly want to exploit this glibc
specific behaviour.

Rich Felker replied:
> 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.

No, gnulib does not intend to encourage this kind of lazy programming.

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

Sounds reasonable.

Here's a proposed patch to remove gnulib's unintentional requirement.


2012-06-19  Bruno Haible  <bruno@clisp.org>

	fdopen: Allow implementations that don't reject invalid fd arguments.
	* m4/fdopen.m4 (gl_FUNC_FDOPEN): Let the test pass if fdopen(-1,...)
	succeeds.
	Reported by Rich Felker <dalias@aerifal.cx>.

--- m4/fdopen.m4.orig	Tue Jun 19 13:00:23 2012
+++ m4/fdopen.m4	Tue Jun 19 13:00:05 2012
@@ -1,4 +1,4 @@
-# fdopen.m4 serial 2
+# fdopen.m4 serial 3
 dnl Copyright (C) 2011-2012 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -25,10 +25,8 @@
   FILE *fp;
   errno = 0;
   fp = fdopen (-1, "r");
-  if (fp != NULL)
+  if (fp == NULL && errno == 0)
     return 1;
-  if (errno == 0)
-    return 2;
   return 0;
 }]])],
           [gl_cv_func_fdopen_works=yes],

Re: musl, fdopen test

[Jim Meyering]

Bruno Haible wrote:
...
>> 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,
>
> Sounds reasonable.
>
> Here's a proposed patch to remove gnulib's unintentional requirement.
>
>
> 2012-06-19  Bruno Haible  <bruno@clisp.org>
>
> 	fdopen: Allow implementations that don't reject invalid fd arguments.
> 	* m4/fdopen.m4 (gl_FUNC_FDOPEN): Let the test pass if fdopen(-1,...)
> 	succeeds.
> 	Reported by Rich Felker <dalias@aerifal.cx>.

Thanks, Bruno.
That patch looks perfect.

Re: musl, fdopen test

[Bruno Haible]

> > 2012-06-19  Bruno Haible  <bruno@clisp.org>
> >
> >       fdopen: Allow implementations that don't reject invalid fd arguments.
> >       * m4/fdopen.m4 (gl_FUNC_FDOPEN): Let the test pass if fdopen(-1,...)
> >       succeeds.
> >       Reported by Rich Felker <dalias@aerifal.cx>.
> 
> Thanks, Bruno.
> That patch looks perfect.

I have applied the patch.

Bruno

Re: musl, printf out-of-memory test

[Bruno Haible]

Rich Felker wrote:
> > 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??

Strange indeed. With a testdir of all of gnulib, I got

  configure:17615: checking whether printf survives out-of-memory conditions
  configure:17786: /arch/x86-linux/inst-musl/bin/musl-gcc -std=gnu99 -o conftest -g -O2 -Wall  conftest.c  >&5
  configure:17789: $? = 0
  configure:17837: result: yes

but with a testdir of only the POSIX related modules of gnulib, I got

  configure:13657: checking whether printf survives out-of-memory conditions
  configure:13828: /arch/x86-linux/inst-musl/bin/musl-gcc -std=gnu99 -o conftest -g -O2 -Wall  conftest.c  >&5
  configure:13831: $? = 0
  configure:13879: result: no

The '$? = 0' line prints only the linker's exit code, not the runtime
exit code. I'm adding a second output line for the runtime exit code.
Then I get:

  configure:8919: checking whether printf survives out-of-memory conditions
  configure:9090: /arch/x86-linux/inst-musl/bin/musl-gcc -o conftest -g -O2 -Wall  conftest.c  >&5
  configure:9093: $? = 0
  configure:9097: $? = 1
  configure:9142: result: no

After adding a printf to stderr: Once I get

  configure:8919: checking whether printf survives out-of-memory conditions
  configure:9093: /arch/x86-linux/inst-musl/bin/musl-gcc -o conftest -g -O2 -Wall  conftest.c  >&5
  configure:9096: $? = 0
  printf's return value = 5000002, errno = 0
  configure:9100: $? = 0
  configure:9145: result: yes

In another configure run I get:

  configure:8919: checking whether printf survives out-of-memory conditions
  configure:9093: /arch/x86-linux/inst-musl/bin/musl-gcc -o conftest -g -O2 -Wall  conftest.c  >&5
  configure:9096: $? = 0
  configure:9100: $? = 1
  configure:9145: result: no

So, the exit code 1 must have come from the crash handler. Without this crash
handler: 7x I get

  configure:8919: checking whether printf survives out-of-memory conditions
  configure:8979: /arch/x86-linux/inst-musl/bin/musl-gcc -o conftest -g -O2 -Wall  conftest.c  >&5
  configure:8982: $? = 0
  printf's return value = 5000002, errno = 0
  configure:8986: $? = 0
  configure:9031: result: yes

but once I get

  configure:8979: /arch/x86-linux/inst-musl/bin/musl-gcc -o conftest -g -O2 -Wall  conftest.c  >&5
  configure:8982: $? = 0
  configure:8986: $? = 139
  configure:9031: result: no

So, apparently, under memory stress, musl's printf has a probability of
between 10% and 50% of crashing with SIGSEGV (139 = 128 + 11).

Bruno


2012-06-19  Bruno Haible  <bruno@clisp.org>

	*printf-posix: Put more info into config.log.
	* m4/printf.m4 (gl_PRINTF_ENOMEM): Emit conftest's error output and
	exit code into config.log.

--- m4/printf.m4.orig	Tue Jun 19 12:41:56 2012
+++ m4/printf.m4	Tue Jun 19 12:41:53 2012
@@ -1,4 +1,4 @@
-# printf.m4 serial 48
+# printf.m4 serial 49
 dnl Copyright (C) 2003, 2007-2012 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -1028,8 +1028,9 @@
 changequote([,])dnl
           ])])
           if AC_TRY_EVAL([ac_link]) && test -s conftest$ac_exeext; then
-            (./conftest
+            (./conftest 2>&AS_MESSAGE_LOG_FD
              result=$?
+             _AS_ECHO_LOG([\$? = $result])
              if test $result != 0 && test $result != 77; then result=1; fi
              exit $result
             ) >/dev/null 2>/dev/null

Re: Re: musl, printf out-of-memory test

[Rich Felker]

On Tue, Jun 19, 2012 at 12:45:50PM +0200, Bruno Haible wrote:
> So, the exit code 1 must have come from the crash handler. Without this crash
> handler: 7x I get
> 
>   configure:8919: checking whether printf survives out-of-memory conditions
>   configure:8979: /arch/x86-linux/inst-musl/bin/musl-gcc -o conftest -g -O2 -Wall  conftest.c  >&5
>   configure:8982: $? = 0
>   printf's return value = 5000002, errno = 0
>   configure:8986: $? = 0
>   configure:9031: result: yes
> 
> but once I get
> 
>   configure:8979: /arch/x86-linux/inst-musl/bin/musl-gcc -o conftest -g -O2 -Wall  conftest.c  >&5
>   configure:8982: $? = 0
>   configure:8986: $? = 139
>   configure:9031: result: no
> 
> So, apparently, under memory stress, musl's printf has a probability of
> between 10% and 50% of crashing with SIGSEGV (139 = 128 + 11).

musl's printf does not do anything with memory except using a small
constant amount of stack space (a few hundred bytes for non-float,
somewhere around 5-7k for floating point). This is completely
independent of the width/padding/precision; the implementation
actually goes to a good bit of trouble to ensure that it can print any
amount of padding efficiently without large or unbounded stack space
usage.

Is there any way the rlimits put in place could be preventing the
stack from expanding beyond even one page the current number of pages,
etc.?

Rich

Re: musl, printf out-of-memory test

[Bruno Haible]

Rich Felker wrote:
> > but once I get
> > 
> >   configure:8979: /arch/x86-linux/inst-musl/bin/musl-gcc -o conftest -g -O2 -Wall  conftest.c  >&5
> >   configure:8982: $? = 0
> >   configure:8986: $? = 139
> >   configure:9031: result: no
> > 
> > So, apparently, under memory stress, musl's printf has a probability of
> > between 10% and 50% of crashing with SIGSEGV (139 = 128 + 11).
> 
> musl's printf does not do anything with memory except using a small
> constant amount of stack space (a few hundred bytes for non-float,
> somewhere around 5-7k for floating point). This is completely
> independent of the width/padding/precision; the implementation
> actually goes to a good bit of trouble to ensure that it can print any
> amount of padding efficiently without large or unbounded stack space
> usage.
> 
> Is there any way the rlimits put in place could be preventing the
> stack from expanding beyond even one page the current number of pages,
> etc.?

I can reduce the program and the compilation options:

=============================== conftest.c =============================
#include <stdio.h>
#include <errno.h>
int main()
{
  int ret;
  int err;
  ret = printf ("%.5000000f", 1.0);
  err = errno;
  fprintf (stderr, "printf's return value = %d, errno = %d\n", ret, err);
  return !(ret == 5000002 || (ret < 0 && err == ENOMEM));
}
========================================================================
$ musl-gcc -g -Wall  conftest.c -o conftest
$ ./conftest > /dev/null ; echo $?
printf's return value = 5000002, errno = 0
0
$ ./conftest > /dev/null ; echo $?
printf's return value = 5000002, errno = 0
0
$ ./conftest > /dev/null ; echo $?
printf's return value = 5000002, errno = 0
0
$ ./conftest > /dev/null ; echo $?
Speicherzugriffsfehler (Speicherabzug geschrieben)
139
$ ./conftest > /dev/null ; echo $?
Speicherzugriffsfehler (Speicherabzug geschrieben)
139

I couldn't get useful info from gdb.

This is on Linux, 32-bit mode on a 64-bit system. Can you reproduce this?

Bruno

Re: musl, printf out-of-memory test

[Rich Felker]

On Tue, Jun 19, 2012 at 10:04:57PM +0200, Bruno Haible wrote:
> I can reduce the program and the compilation options:
> 
> =============================== conftest.c =============================
> #include <stdio.h>
> #include <errno.h>
> int main()
> {
>   int ret;
>   int err;
>   ret = printf ("%.5000000f", 1.0);
>   err = errno;
>   fprintf (stderr, "printf's return value = %d, errno = %d\n", ret, err);
>   return !(ret == 5000002 || (ret < 0 && err == ENOMEM));
> }
> ========================================================================
> $ musl-gcc -g -Wall  conftest.c -o conftest
> $ ./conftest > /dev/null ; echo $?
> printf's return value = 5000002, errno = 0
> 0
> $ ./conftest > /dev/null ; echo $?
> printf's return value = 5000002, errno = 0
> 0
> $ ./conftest > /dev/null ; echo $?
> printf's return value = 5000002, errno = 0
> 0
> $ ./conftest > /dev/null ; echo $?
> Speicherzugriffsfehler (Speicherabzug geschrieben)
> 139
> $ ./conftest > /dev/null ; echo $?
> Speicherzugriffsfehler (Speicherabzug geschrieben)
> 139
> 
> I couldn't get useful info from gdb.
> 
> This is on Linux, 32-bit mode on a 64-bit system. Can you reproduce this?

I can't reproduce it. Do you have a dynamic-linked musl or just
static? I tried both and couldn't reproduce with either. Did you set
resource limits before running it? Are you using any strange kernel
mods? I once heard of a patched kernel setting up other mappings over
top of the not-yet-expanded-into stack space, but I'd be surprised if
more weren't breaking on such a system...

What happened in gdb? Were you unable to get it to crash? What if you
run it under strace?

Rich

Re: musl, printf out-of-memory test

[Bruno Haible]

Rich Felker wrote:
> Do you have a dynamic-linked musl or just static?

Dynamically linked:

$ readelf -d conftest

Dynamic section at offset 0xf3c contains 18 entries:
  Tag        Type                         Name/Value
 0x00000001 (NEEDED)                     Shared library: [libc.so]
 0x0000000c (INIT)                       0x804832c
 0x0000000d (FINI)                       0x80484ec
 0x00000004 (HASH)                       0x80481a0
 0x6ffffef5 (GNU_HASH)                   0x80481dc
 0x00000005 (STRTAB)                     0x80482b0
 0x00000006 (SYMTAB)                     0x8048210
 0x0000000a (STRSZ)                      83 (bytes)
 0x0000000b (SYMENT)                     16 (bytes)
 0x00000015 (DEBUG)                      0x0
 0x00000003 (PLTGOT)                     0x8049ff4
 0x00000002 (PLTRELSZ)                   32 (bytes)
 0x00000014 (PLTREL)                     REL
 0x00000017 (JMPREL)                     0x804830c
 0x00000011 (REL)                        0x8048304
 0x00000012 (RELSZ)                      8 (bytes)
 0x00000013 (RELENT)                     8 (bytes)
 0x00000000 (NULL)                       0x0
$ readelf -l conftest

Elf file type is EXEC (Executable file)
Entry point 0x8048390
There are 9 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000034 0x08048034 0x08048034 0x00120 0x00120 R E 0x4
  INTERP         0x000154 0x08048154 0x08048154 0x00026 0x00026 R   0x1
      [Requesting program interpreter: /arch/x86-linux/inst-musl/lib/libc.so]
  LOAD           0x000000 0x08048000 0x08048000 0x00578 0x00578 R E 0x1000
  LOAD           0x000f28 0x08049f28 0x08049f28 0x000ec 0x000f8 RW  0x1000
  DYNAMIC        0x000f3c 0x08049f3c 0x08049f3c 0x000b8 0x000b8 RW  0x4
  NOTE           0x00017c 0x0804817c 0x0804817c 0x00024 0x00024 R   0x4
  GNU_EH_FRAME   0x000528 0x08048528 0x08048528 0x00014 0x00014 R   0x4
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RWE 0x4
  GNU_RELRO      0x000f28 0x08049f28 0x08049f28 0x000d8 0x000d8 R   0x1

 Section to Segment mapping:
  Segment Sections...
   00     
   01     .interp 
   02     .interp .note.gnu.build-id .hash .gnu.hash .dynsym .dynstr .rel.dyn .rel.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame 
   03     .ctors .dtors .jcr .dynamic .got.plt .data .bss 
   04     .dynamic 
   05     .note.gnu.build-id 
   06     .eh_frame_hdr 
   07     
   08     .ctors .dtors .jcr .dynamic 
$ readelf --dyn-syms conftest

Symbol table '.dynsym' contains 10 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 00000000     0 FUNC    GLOBAL DEFAULT  UND printf
     2: 00000000     0 FUNC    GLOBAL DEFAULT  UND fprintf
     3: 00000000     0 FUNC    GLOBAL DEFAULT  UND __errno_location
     4: 00000000     0 FUNC    GLOBAL DEFAULT  UND __libc_start_main
     5: 0804a014     0 NOTYPE  GLOBAL DEFAULT  ABS _edata
     6: 0804a020     0 NOTYPE  GLOBAL DEFAULT  ABS _end
     7: 08048390     0 NOTYPE  GLOBAL DEFAULT   11 _start
     8: 0804a014     0 NOTYPE  GLOBAL DEFAULT  ABS __bss_start
     9: 0804a014     4 OBJECT  GLOBAL DEFAULT   22 stderr

> Did you set resource limits before running it?

No.
$ ulimit -a
core file size          (blocks, -c) unlimited
data seg size           (kbytes, -d) unlimited
scheduling priority             (-e) 0
file size               (blocks, -f) unlimited
pending signals                 (-i) 29019
max locked memory       (kbytes, -l) 64
max memory size         (kbytes, -m) unlimited
open files                      (-n) 1024
pipe size            (512 bytes, -p) 8
POSIX message queues     (bytes, -q) 819200
real-time priority              (-r) 0
stack size              (kbytes, -s) 8192
cpu time               (seconds, -t) unlimited
max user processes              (-u) 29019
virtual memory          (kbytes, -v) unlimited
file locks                      (-x) unlimited

> Are you using any strange kernel mods?

No. Stock openSUSE 12.1.
$ uname -srv
Linux 3.1.10-1.9-desktop #1 SMP PREEMPT Thu Apr 5 18:48:38 UTC 2012 (4a97ec8)

> What happened in gdb?

The stack trace in gdb is unusable.
$ gdb conftest
GNU gdb (GDB) SUSE (7.3-41.1.2)
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-suse-linux".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /data/bruno/tmp/testdir3/conftest...done.
(gdb) set solib-search-path /arch/x86-linux/inst-musl/lib
(gdb) run
Starting program: /data/bruno/tmp/testdir3/conftest 
warning: Could not load shared library symbols for linux-gate.so.1.
Do you need "set solib-search-path" or "set sysroot"?

Program received signal SIGSEGV, Segmentation fault.
0xf7fc76c3 in fmt_fp () from /data/arch/x86-linux/inst-musl/lib/libc.so
(gdb) where
#0  0xf7fc76c3 in fmt_fp () from /data/arch/x86-linux/inst-musl/lib/libc.so
#1  0x00000000 in ?? ()

This is a bit useless, since libc.so is compiled without debugging information.
If I rebuild with "-O1 -g" instead of "-Os" and "-O3", I get this stack trace:

$ gdb conftest
GNU gdb (GDB) SUSE (7.3-41.1.2)
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-suse-linux".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /data/bruno/tmp/testdir3/conftest...done.
(gdb) set solib-search-path /arch/x86-linux/inst-musl/lib
(gdb) run
Starting program: /data/bruno/tmp/testdir3/conftest 
warning: Could not load shared library symbols for linux-gate.so.1.
Do you need "set solib-search-path" or "set sysroot"?

Program received signal SIGSEGV, Segmentation fault.
fmt_fp (f=0xf7ff9200, y=0, w=0, p=5000000, fl=0, t=102) at src/stdio/vfprintf.c:326
326                     x = *d % i;
(gdb) where
#0  fmt_fp (f=0xf7ff9200, y=0, w=0, p=5000000, fl=0, t=102) at src/stdio/vfprintf.c:326
#1  0xf7fcacf3 in printf_core (f=0xf7ff9200, fmt=<optimized out>, ap=0xffffc13c, nl_arg=0xffffc09c, 
    nl_type=0xffffc114) at src/stdio/vfprintf.c:614
#2  0xf7fcb0eb in vfprintf (f=0xf7ff9200, fmt=0x80484f4 "%.5000000f", ap=0xffffc1a4 "") at src/stdio/vfprintf.c:659
#3  0xf7fcd967 in vprintf (fmt=0x80484f4 "%.5000000f", ap=0xffffc1a4 "") at src/stdio/vprintf.c:5
#4  0xf7fc8463 in printf (fmt=0x80484f4 "%.5000000f") at src/stdio/printf.c:9
#5  0x0804845f in main () at conftest.c:7
(gdb) info locals
x = <optimized out>
big = {524288, 0 <repeats 1750 times>, 4160552156, 0, 0, 0, 0, 0, 0, 0, 4160720884, 8, 8, 134513329, 4160343432, 
  134513332, 4160609540, 1, 0 <repeats 46 times>, 134513908, 4160721408, 4160517969, 4160727464, 134513908, 0, 0, 0, 
  0, 0, 4160720884, 4160711907, 0, 0, 4160524786}
a = 0xffffa2b0
d = 0x218b40
r = 0xffffa2b0
z = 0x218b44
e2 = 0
e = 0
i = <optimized out>
j = 9
l = <optimized out>
buf = '\000' <repeats 24 times>
s = <optimized out>
prefix = 0xf7ff6cb4 "0X+0X 0X-0x+0x 0x"
pl = 0
ebuf0 = '\000' <repeats 11 times>
ebuf = 0xffffa293 ""
estr = <optimized out>
(gdb) up
#1  0xf7fcacf3 in printf_core (f=0xf7ff9200, fmt=<optimized out>, ap=0xffffc13c, nl_arg=0xffffc09c, 
    nl_type=0xffffc114) at src/stdio/vfprintf.c:614
614                             l = fmt_fp(f, arg.f, w, p, fl, t);
(gdb) info locals
a = <optimized out>
z = 0xffffbff0 ""
s = 0x80484fe ""
l10n = 0
litpct = <optimized out>
fl = 0
w = 0
p = 5000000
arg = {i = 9223372036854775808, f = 1, p = 0x0}
argpos = -1
st = <optimized out>
ps = 0
cnt = 0
l = 0
i = <optimized out>
buf = "A\370\367\374\371\370\367\000\000\000\000\021", '\000' <repeats 27 times>, "\377", <incomplete sequence \367>
prefix = 0xf7ff6cd2 "-+   0X0x"
t = 102
pl = 0
wc = L"\xf7f9c62d\xf7f899ac"
ws = <optimized out>
mb = "\271\202\004\b"
(gdb) up
#2  0xf7fcb0eb in vfprintf (f=0xf7ff9200, fmt=0x80484f4 "%.5000000f", ap=0xffffc1a4 "") at src/stdio/vfprintf.c:659
659             ret = printf_core(f, fmt, &ap2, nl_arg, nl_type);
(gdb) info locals
ap2 = 0xffffc1ac ""
nl_type = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0}
nl_arg = {{i = 150189233701, f = 0, p = 0xf7f9d625}, {i = 4307434622, f = <invalid float value>, p = 0xbe3c7e}, {
    i = 4024693728518132, f = 0, p = 0x8049ff4}, {i = 0, f = <invalid float value>, p = 0x0}, {i = 98599429607984, 
    f = 0, p = 0xf7fa1230}, {i = 17868614760971370496, f = -0, p = 0x0}, {i = 17870160128724931592, f = 0, 
    p = 0xf7ff9408}, {i = 13791, f = 0, p = 0x35df}, {i = 47244701668, f = <invalid float value>, p = 0xefe4}, {
    i = 824633720832, f = 0, p = 0x0}}
internal_buf = "h\334\375\367", '\000' <repeats 12 times>"\364, \217\377\367\340\216\377\367\270\300\377\377\"\000\000\000:\310\371\367\270\300\377\377\000\000\000\000\210\000\000\000\260\202\004\b\000\224\377\367\000\000\000\000\000\000\000\000\364\217\377\367H\224\377\367@\301\377\377\000\340\377\377"
saved_buf = 0x0
ret = <optimized out>
__need_unlock = 0
(gdb) up
#3  0xf7fcd967 in vprintf (fmt=0x80484f4 "%.5000000f", ap=0xffffc1a4 "") at src/stdio/vprintf.c:5
5               return vfprintf(stdout, fmt, ap);
(gdb) info locals
No locals.
(gdb) up
#4  0xf7fc8463 in printf (fmt=0x80484f4 "%.5000000f") at src/stdio/printf.c:9
9               ret = vprintf(fmt, ap);
(gdb) info locals
ret = 9
ap = 0xffffc1a4 ""
(gdb) up
#5  0x0804845f in main () at conftest.c:7
7         ret = printf ("%.5000000f", 1.0);
(gdb) info locals
ret = 0
err = 0

The SIGSEGV occurs because d = 0x218b40 but the address ranges are these:
08048000-08049000 r-xp 00000000 08:05 26174991                           /data/bruno/tmp/testdir3/conftest
08049000-0804b000 rwxp 00000000 08:05 26174991                           /data/bruno/tmp/testdir3/conftest
f7f84000-f7ff8000 r-xp 00000000 08:05 26168372                           /data/arch/x86-linux/inst-musl/lib/libc.so
f7ff8000-f7ffa000 rwxp 00073000 08:05 26168372                           /data/arch/x86-linux/inst-musl/lib/libc.so
f7ffa000-f7ffe000 rwxp 00000000 00:00 0 
fffdc000-ffffe000 rwxp 00000000 00:00 0                                  [stack]
ffffe000-fffff000 r-xp 00000000 00:00 0                                  [vdso]

> What if you run it under strace?

Yes. When it succeeds, the strace output looks normal. When it fails,
it's this:

$ strace ./conftest
execve("./conftest", ["./conftest"], [/* 133 vars */]) = 0
[ Process PID=2858 runs in 32 bit mode. ]
--- {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=0xe7664} (Segmentation fault) ---
+++ killed by SIGSEGV (core dumped) +++
Speicherzugriffsfehler (Speicherabzug geschrieben)

Hope this helps.

Bruno

Re: musl, printf out-of-memory test

[Rich Felker]

On Tue, Jun 19, 2012 at 11:17:33PM +0200, Bruno Haible wrote:
> [...]
> The SIGSEGV occurs because d = 0x218b40 but the address ranges are these:
> 08048000-08049000 r-xp 00000000 08:05 26174991                           /data/bruno/tmp/testdir3/conftest
> 08049000-0804b000 rwxp 00000000 08:05 26174991                           /data/bruno/tmp/testdir3/conftest
> f7f84000-f7ff8000 r-xp 00000000 08:05 26168372                           /data/arch/x86-linux/inst-musl/lib/libc.so
> f7ff8000-f7ffa000 rwxp 00073000 08:05 26168372                           /data/arch/x86-linux/inst-musl/lib/libc.so
> f7ffa000-f7ffe000 rwxp 00000000 00:00 0 
> fffdc000-ffffe000 rwxp 00000000 00:00 0                                  [stack]
> ffffe000-fffff000 r-xp 00000000 00:00 0                                  [vdso]
> 
> > What if you run it under strace?
> 
> Yes. When it succeeds, the strace output looks normal. When it fails,
> it's this:
> 
> $ strace ./conftest
> execve("./conftest", ["./conftest"], [/* 133 vars */]) = 0
> [ Process PID=2858 runs in 32 bit mode. ]
> --- {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=0xe7664} (Segmentation fault) ---
> +++ killed by SIGSEGV (core dumped) +++
> Speicherzugriffsfehler (Speicherabzug geschrieben)
> 
> Hope this helps.

Yes, it helped a lot. Thanks! The problem was an obscure
pointer-arithmetic overflow that could only happen in 32-bit binaries
running on a 64-bit kernel where the stack pointer is near the 4GB
boundary. This is why I couldn't reproduce it: I'm on a 32-bit
kernel where the stack is at 3GB and there's no way an offset bounded
by INT_MAX/9 could reach past 4GB. That's my excuse for why it was
never noticed before, but it still doesn't justify the bug, which is a
nasty instance of UB (pointer arithmetic outside array bounds).

Anyway, it's fixed now.

Rich


P.S. I just realized - I meant to credit you for finding it in the
commit message but somehow I forgot to. Sorry about that!

Re: Re: musl, printf out-of-memory test

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2012-06-19 21:52:49 -0400]:
> On Tue, Jun 19, 2012 at 11:17:33PM +0200, Bruno Haible wrote:
> > Hope this helps.
> 
> Yes, it helped a lot. Thanks! The problem was an obscure
> pointer-arithmetic overflow that could only happen in 32-bit binaries
> running on a 64-bit kernel where the stack pointer is near the 4GB
> boundary. This is why I couldn't reproduce it: I'm on a 32-bit
> kernel where the stack is at 3GB and there's no way an offset bounded
> by INT_MAX/9 could reach past 4GB. That's my excuse for why it was
> never noticed before, but it still doesn't justify the bug, which is a
> nasty instance of UB (pointer arithmetic outside array bounds).
> 
> Anyway, it's fixed now.
> 

you mentioned another potential out of bound pointer arithmetics there
but it's not yet fixed:

diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c
index a3bf18d..116e1ce 100644
--- a/src/stdio/vfprintf.c
+++ b/src/stdio/vfprintf.c
@@ -319,7 +319,7 @@ static int fmt_fp(FILE *f, long double y, int w, int p, int fl, int t)
        if (j < 9*(z-r-1)) {
                uint32_t x;
                /* We avoid C's broken division of negative numbers */
-               d = r + 1 + (j+9*LDBL_MAX_EXP)/9 - LDBL_MAX_EXP;
+               d = r + 1 + ((j+9*LDBL_MAX_EXP)/9 - LDBL_MAX_EXP);
                j += 9*LDBL_MAX_EXP;
                j %= 9;
                for (i=10, j++; j<9; i*=10, j++);

Re: musl, printf out-of-memory test

[Bruno Haible]

Rich Felker wrote:
> The problem was an obscure pointer-arithmetic overflow ...
> where the stack pointer is near the 4GB boundary.

This explains also why it occurred only with a certain probability
outside gdb, but with 100% probability from within gdb: Apparently gdb
runs the program without address space layout randomization.

> Anyway, it's fixed now.

I confirm that
http://git.etalabs.net/cgi-bin/gitweb.cgi?p=musl;a=commitdiff;h=914949d321448bd2189bdcbce794dbae2c8ed16e
fixes the bug.

Bruno

Re: musl, printf out-of-memory test

[Jim Meyering]

Bruno Haible wrote:
> Rich Felker wrote:
>> The problem was an obscure pointer-arithmetic overflow ...
>> where the stack pointer is near the 4GB boundary.
>
> This explains also why it occurred only with a certain probability
> outside gdb, but with 100% probability from within gdb: Apparently gdb
> runs the program without address space layout randomization.

That is correct.  It is a feature of gdb-7.0 and newer.
You can inspect (watch/break-at/etc.) the same address and expect it
to refer to the same memory location in multiple invocations.
This makes gdb's command-line history even more useful.

Re: musl, printf out-of-memory test

[Tom Tromey]

>>>>> "Jim" == Jim Meyering <jim@meyering.net> writes:

Jim> That is correct.  It is a feature of gdb-7.0 and newer.
Jim> You can inspect (watch/break-at/etc.) the same address and expect it
Jim> to refer to the same memory location in multiple invocations.
Jim> This makes gdb's command-line history even more useful.

gdb defaults to this for development convenience.
You can change it though, see "set disable-randomization".

Tom

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: [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: 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: grantpt test

[Bruno Haible]

Rich Felker wrote:
> > 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...

Looking at the (few) callers of grantpt() in gnulib, it indeed seems
unlikely that people will want to rely on the failure for invalid file
descriptors. So I'm relaxing the requirements of gnulib.


2012-06-22  Bruno Haible  <bruno@clisp.org>

	grantpt: Relax requirement regarding invalid file descriptors.
	* lib/grantpt.c: Don't include <fcntl.h>.
	(grantpt): Don't verify the validity of the file descriptor.
	* modules/grantpt (Depends-on): Remove fcntl-h.
	* tests/test-grantpt.c (main): Allow grantpt to succeed for invalid
	file descriptors.
	* doc/posix-functions/grantpt.texi: Document more platforms on which
	grantpt succeeds for invalid file descriptors.
	Reported by Rich Felker <dalias@aerifal.cx>.

--- doc/posix-functions/grantpt.texi.orig	Fri Jun 22 12:33:55 2012
+++ doc/posix-functions/grantpt.texi	Fri Jun 22 12:33:52 2012
@@ -20,5 +20,5 @@
 IRIX 5.3.
 @item
 This function reports success for invalid file descriptors on some platforms:
-Cygwin 1.7.9.
+OpenBSD, Cygwin 1.7.9, musl libc.
 @end itemize
--- lib/grantpt.c.orig	Fri Jun 22 12:33:55 2012
+++ lib/grantpt.c	Fri Jun 22 12:14:57 2012
@@ -21,7 +21,6 @@
 
 #include <assert.h>
 #include <errno.h>
-#include <fcntl.h>
 #include <string.h>
 #include <sys/wait.h>
 #include <unistd.h>
@@ -50,8 +49,6 @@
 #if defined __OpenBSD__
   /* On OpenBSD, master and slave of a pseudo-terminal are allocated together,
      through an ioctl on /dev/ptm.  There is no need for grantpt().  */
-  if (fcntl (fd, F_GETFD) < 0)
-    return -1;
   return 0;
 #else
   /* This function is most often called from a process without 'root'
--- modules/grantpt.orig	Fri Jun 22 12:33:55 2012
+++ modules/grantpt	Fri Jun 22 12:15:14 2012
@@ -9,7 +9,6 @@
 Depends-on:
 stdlib
 extensions
-fcntl-h         [test $HAVE_GRANTPT = 0]
 pt_chown        [test $HAVE_GRANTPT = 0]
 waitpid         [test $HAVE_GRANTPT = 0]
 configmake      [test $HAVE_GRANTPT = 0]
--- tests/test-grantpt.c.orig	Fri Jun 22 12:33:55 2012
+++ tests/test-grantpt.c	Fri Jun 22 12:14:31 2012
@@ -28,22 +28,36 @@
 int
 main (void)
 {
-  /* Test behaviour for invalid file descriptors.  */
+  /* Test behaviour for invalid file descriptors.
+     These calls don't fail on OpenBSD (with gnulib's replacement) and on
+     musl libc.  */
   {
+    int ret;
+
     errno = 0;
-    ASSERT (grantpt (-1) == -1);
-    ASSERT (errno == EBADF
-            || errno == EINVAL /* seen on FreeBSD 6.4 */
-            || errno == 0 /* seen on Solaris 8 */
-           );
+    ret = grantpt (-1);
+    if (ret != 0)
+      {
+        ASSERT (ret == -1);
+        ASSERT (errno == EBADF
+                || errno == EINVAL /* seen on FreeBSD 6.4 */
+                || errno == 0 /* seen on Solaris 8 */
+               );
+      }
   }
   {
+    int ret;
+
     errno = 0;
-    ASSERT (grantpt (99) == -1);
-    ASSERT (errno == EBADF
-            || errno == EINVAL /* seen on FreeBSD 6.4 */
-            || errno == 0 /* seen on Solaris 8 */
-           );
+    ret = grantpt (99);
+    if (ret != 0)
+      {
+        ASSERT (ret == -1);
+        ASSERT (errno == EBADF
+                || errno == EINVAL /* seen on FreeBSD 6.4 */
+                || errno == 0 /* seen on Solaris 8 */
+               );
+      }
   }
 
   return 0;

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