Re: #define __MUSL__ in features.h

Re: #define __MUSL__ in features.h

[dgutson .]

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

> On Fri, Mar 29, 2013 at 09:44:05PM +0100, Daniel Cegiełka wrote:
> > Is it possible to add to the features.h __MUSL__ definition?
> >
> > glibc can be identified by __GLIBC__, uclibc through __UCLIBC__ etc.
>
> Is this question in the FAQ yet? If not, it really should be. The
> answer is no, it won't be added, because it's a bug to assume a
> certain implementation has particular properties rather than testing.

That is a beautiful theory in an ideal world, but in the real world,

implementations have bugs, and sometimes we need to workaround these bugs.

(e.g. the FD* issue reported by Martin Galvan).

So when writing code that should work with different implementations, these

macros are needed to apply workarounds for implementation-specific bugs.

That's why all the rest of the C lib implementations do provide an identifying

macro, something that I think musl should also do, IMVHO.


    Daniel.


> So far, every time somebody's asked for this with a particular usage
> case in mind, the usage case was badly wrong, and would have broken
> support for the next release of musl...

>

> Rich



-- 
Who’s got the sweetest disposition?
One guess, that’s who?
Who’d never, ever start an argument?
Who never shows a bit of temperament?
Who's never wrong but always right?
Who'd never dream of starting a fight?
Who get stuck with all the bad luck?

[-- Attachment #2: Type: text/html, Size: 5457 bytes --]

Re: Re: #define __MUSL__ in features.h

[Rich Felker]

On Thu, Mar 15, 2018 at 12:55:29PM -0300, dgutson . wrote:
> > On Fri, Mar 29, 2013 at 09:44:05PM +0100, Daniel Cegiełka wrote:
> > > Is it possible to add to the features.h __MUSL__ definition?
> > >
> > > glibc can be identified by __GLIBC__, uclibc through __UCLIBC__ etc.
> >
> > Is this question in the FAQ yet? If not, it really should be. The
> > answer is no, it won't be added, because it's a bug to assume a
> > certain implementation has particular properties rather than testing.
> 
> That is a beautiful theory in an ideal world, but in the real world,
> 
> implementations have bugs, and sometimes we need to workaround these bugs.

If there's an actual bug you need to work around, detect it.
Hard-coding "musl is buggy" is not beneficial to us; rather it leads
to broken hacks lingering long after the bug is fixed.

> (e.g. the FD* issue reported by Martin Galvan).

That's not a bug. It's compiler warnings being wrongly produced for a
system header, probably because someone added -I/usr/include or
similar (normally GCC suppresses these).

The musl policy regarding not having a macro like __MUSL__ is doing
exactly what it's intended to do: encouraging developers and package
maintainers to come to us (or investigate on their own) and fix the
underlying portability problems (and sometimes musl bugs) rather than
writing hacks to a specific version of musl that will be wrong a few
versions later.

Rich

Re: Re: #define __MUSL__ in features.h

[Martin Galvan]

2018-03-15 15:39 GMT-03:00 Rich Felker <dalias@libc.org>:
>> (e.g. the FD* issue reported by Martin Galvan).
>
> That's not a bug. It's compiler warnings being wrongly produced for a
> system header, probably because someone added -I/usr/include or
> similar (normally GCC suppresses these).

I'm certain we didn't add -I/usr/include or something similar. Could
you test this yourself to confirm it's not a bug?

The compiler warnings aren't being wrongly produced. musl will indeed
perform a signed-to-unsigned conversion here.

> The musl policy regarding not having a macro like __MUSL__ is doing
> exactly what it's intended to do: encouraging developers and package
> maintainers to come to us (or investigate on their own) and fix the
> underlying portability problems (and sometimes musl bugs) rather than
> writing hacks to a specific version of musl that will be wrong a few
> versions later.

So whenever we find a bug on musl we should just stop all our
development until you've fixed the bug?

Re: Re: #define __MUSL__ in features.h

[dgutson .]

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

On Thu, Mar 15, 2018 at 3:39 PM, Rich Felker <dalias@libc.org> wrote:

> On Thu, Mar 15, 2018 at 12:55:29PM -0300, dgutson . wrote:
> > > On Fri, Mar 29, 2013 at 09:44:05PM +0100, Daniel Cegiełka wrote:
> > > > Is it possible to add to the features.h __MUSL__ definition?
> > > >
> > > > glibc can be identified by __GLIBC__, uclibc through __UCLIBC__ etc.
> > >
> > > Is this question in the FAQ yet? If not, it really should be. The
> > > answer is no, it won't be added, because it's a bug to assume a
> > > certain implementation has particular properties rather than testing.
> >
> > That is a beautiful theory in an ideal world, but in the real world,
> >
> > implementations have bugs, and sometimes we need to workaround these
> bugs.
>
> If there's an actual bug you need to work around, detect it.
> Hard-coding "musl is buggy" is not beneficial to us; rather it leads
> to broken hacks lingering long after the bug is fixed.
>
> > (e.g. the FD* issue reported by Martin Galvan).
>
> That's not a bug. It's compiler warnings being wrongly produced for a
> system header, probably because someone added -I/usr/include or
> similar (normally GCC suppresses these).
>
> The musl policy regarding not having a macro like __MUSL__ is doing
> exactly what it's intended to do: encouraging developers and package
> maintainers to come to us (or investigate on their own) and fix the
> underlying portability problems (and sometimes musl bugs) rather than
> writing hacks to a specific version of musl that will be wrong a few
> versions later.
>

Fact #1: Software is not perfect. Bugs may show up. And sometimes in bad
timing when doing a release.
So, user developers have two options: hack the library with a workaround,
and release with a
hacked library untested and unverified by its supporting community, or they
can leave the library as-is, and
implement the workaround in the using code (which requires the macro in
order to limit the impact to the library implementation).

Fact #2: Fixes take time, because community projects have their own agenda
and triaging policies.

So, in the hypothetical bug of a library (no matter this particular case I
referred to, there were, there are, and there will always be bugs)
the macro will work as an escape hatch until the fix of the hypothetical
bug is ready upstream.

I'm writing this from a very practical and industry point of view (who have
worked in FLOSS projects before and commercial projects).


>
> Rich
>



-- 
Who’s got the sweetest disposition?
One guess, that’s who?
Who’d never, ever start an argument?
Who never shows a bit of temperament?
Who's never wrong but always right?
Who'd never dream of starting a fight?
Who get stuck with all the bad luck?

[-- Attachment #2: Type: text/html, Size: 3721 bytes --]

Re: Re: #define __MUSL__ in features.h

[Rich Felker]

On Thu, Mar 15, 2018 at 03:48:32PM -0300, Martin Galvan wrote:
> 2018-03-15 15:39 GMT-03:00 Rich Felker <dalias@libc.org>:
> >> (e.g. the FD* issue reported by Martin Galvan).
> >
> > That's not a bug. It's compiler warnings being wrongly produced for a
> > system header, probably because someone added -I/usr/include or
> > similar (normally GCC suppresses these).
> 
> I'm certain we didn't add -I/usr/include or something similar. Could
> you test this yourself to confirm it's not a bug?

In any case it's not a bug in musl. The code is perfectly valid C. If
the compiler is producing a warning for it, either ignore it or ask
the compiler to stop.

> The compiler warnings aren't being wrongly produced. musl will indeed
> perform a signed-to-unsigned conversion here.

Because that's how the C language works.

> > The musl policy regarding not having a macro like __MUSL__ is doing
> > exactly what it's intended to do: encouraging developers and package
> > maintainers to come to us (or investigate on their own) and fix the
> > underlying portability problems (and sometimes musl bugs) rather than
> > writing hacks to a specific version of musl that will be wrong a few
> > versions later.
> 
> So whenever we find a bug on musl we should just stop all our
> development until you've fixed the bug?

No. As noted above, if you need to support systems that might have bug
X, you write a test (configure-time or run-time as appropriate) to
detect bug X and handle it.

Rich

Re: Re: #define __MUSL__ in features.h

[dgutson .]

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

On Thu, Mar 15, 2018 at 3:53 PM, Rich Felker <dalias@libc.org> wrote:

> On Thu, Mar 15, 2018 at 03:48:32PM -0300, Martin Galvan wrote:
> > 2018-03-15 15:39 GMT-03:00 Rich Felker <dalias@libc.org>:
> > >> (e.g. the FD* issue reported by Martin Galvan).
> > >
> > > That's not a bug. It's compiler warnings being wrongly produced for a
> > > system header, probably because someone added -I/usr/include or
> > > similar (normally GCC suppresses these).
> >
> > I'm certain we didn't add -I/usr/include or something similar. Could
> > you test this yourself to confirm it's not a bug?
>
> In any case it's not a bug in musl. The code is perfectly valid C. If
> the compiler is producing a warning for it, either ignore it or ask
> the compiler to stop.
>
> > The compiler warnings aren't being wrongly produced. musl will indeed
> > perform a signed-to-unsigned conversion here.
>
> Because that's how the C language works.
>

it is a potential vulnerability:
https://cwe.mitre.org/data/definitions/195.html
https://wiki.sei.cmu.edu/confluence/display/c/INT31-C.+Ensure+that+integer+conversions+do+not+result+in+lost+or+misinterpreted+data
https://wiki.sei.cmu.edu/confluence/display/c/INT30-C.+Ensure+that+unsigned+integer+operations+do+not+wrap

Can you ensure it is rocksolid and the signed integer will NEVER be a
negative value?


>
> > > The musl policy regarding not having a macro like __MUSL__ is doing
> > > exactly what it's intended to do: encouraging developers and package
> > > maintainers to come to us (or investigate on their own) and fix the
> > > underlying portability problems (and sometimes musl bugs) rather than
> > > writing hacks to a specific version of musl that will be wrong a few
> > > versions later.
> >
> > So whenever we find a bug on musl we should just stop all our
> > development until you've fixed the bug?
>
> No. As noted above, if you need to support systems that might have bug
> X, you write a test (configure-time or run-time as appropriate) to
> detect bug X and handle it.
>
> Rich
>



-- 
Who’s got the sweetest disposition?
One guess, that’s who?
Who’d never, ever start an argument?
Who never shows a bit of temperament?
Who's never wrong but always right?
Who'd never dream of starting a fight?
Who get stuck with all the bad luck?

[-- Attachment #2: Type: text/html, Size: 3696 bytes --]

Re: Re: #define __MUSL__ in features.h

[Martin Galvan]

2018-03-15 15:53 GMT-03:00 Rich Felker <dalias@libc.org>:
> In any case it's not a bug in musl. The code is perfectly valid C. If
> the compiler is producing a warning for it, either ignore it or ask
> the compiler to stop.

Just because some code is valid C, it doesn't mean it's not buggy.

>> The compiler warnings aren't being wrongly produced. musl will indeed
>> perform a signed-to-unsigned conversion here.
>
> Because that's how the C language works.

Yes. And gcc has checks to try and make up for C's weak typing.

While your definition of "bug" is debatable, IMHO if a commonly used
option causes application builds to break due to some library, the
library has a usability issue. The issue is even bigger when we're
talking about something as core as the standard C library.

>> So whenever we find a bug on musl we should just stop all our
>> development until you've fixed the bug?
>
> No. As noted above, if you need to support systems that might have bug
> X, you write a test (configure-time or run-time as appropriate) to
> detect bug X and handle it.

Precisely, and __MUSL__ would be really useful for this.

Re: Re: #define __MUSL__ in features.h

[dgutson .]

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

On Thu, Mar 15, 2018 at 4:00 PM, dgutson . <danielgutson@gmail.com> wrote:

>
>
> On Thu, Mar 15, 2018 at 3:53 PM, Rich Felker <dalias@libc.org> wrote:
>
>> On Thu, Mar 15, 2018 at 03:48:32PM -0300, Martin Galvan wrote:
>> > 2018-03-15 15:39 GMT-03:00 Rich Felker <dalias@libc.org>:
>> > >> (e.g. the FD* issue reported by Martin Galvan).
>> > >
>> > > That's not a bug. It's compiler warnings being wrongly produced for a
>> > > system header, probably because someone added -I/usr/include or
>> > > similar (normally GCC suppresses these).
>> >
>> > I'm certain we didn't add -I/usr/include or something similar. Could
>> > you test this yourself to confirm it's not a bug?
>>
>> In any case it's not a bug in musl. The code is perfectly valid C. If
>> the compiler is producing a warning for it, either ignore it or ask
>> the compiler to stop.
>>
>> > The compiler warnings aren't being wrongly produced. musl will indeed
>> > perform a signed-to-unsigned conversion here.
>>
>> Because that's how the C language works.
>>
>
> it is a potential vulnerability:
> https://cwe.mitre.org/data/definitions/195.html
> https://wiki.sei.cmu.edu/confluence/display/c/INT31-C.+
> Ensure+that+integer+conversions+do+not+result+in+
> lost+or+misinterpreted+data
> https://wiki.sei.cmu.edu/confluence/display/c/INT30-C.+
> Ensure+that+unsigned+integer+operations+do+not+wrap
>

Just to add: the code doesn't not comply with MISRA 2004 because it
violates 6.10.3. IOW, at least this issue turns musl MISRA non-compliant.


>
>
> Can you ensure it is rocksolid and the signed integer will NEVER be a
> negative value?
>
>
>>
>> > > The musl policy regarding not having a macro like __MUSL__ is doing
>> > > exactly what it's intended to do: encouraging developers and package
>> > > maintainers to come to us (or investigate on their own) and fix the
>> > > underlying portability problems (and sometimes musl bugs) rather than
>> > > writing hacks to a specific version of musl that will be wrong a few
>> > > versions later.
>> >
>> > So whenever we find a bug on musl we should just stop all our
>> > development until you've fixed the bug?
>>
>> No. As noted above, if you need to support systems that might have bug
>> X, you write a test (configure-time or run-time as appropriate) to
>> detect bug X and handle it.
>>
>> Rich
>>
>
>
>
> --
> Who’s got the sweetest disposition?
> One guess, that’s who?
> Who’d never, ever start an argument?
> Who never shows a bit of temperament?
> Who's never wrong but always right?
> Who'd never dream of starting a fight?
> Who get stuck with all the bad luck?
>



-- 
Who’s got the sweetest disposition?
One guess, that’s who?
Who’d never, ever start an argument?
Who never shows a bit of temperament?
Who's never wrong but always right?
Who'd never dream of starting a fight?
Who get stuck with all the bad luck?

[-- Attachment #2: Type: text/html, Size: 5129 bytes --]

Re: Re: #define __MUSL__ in features.h

[Rich Felker]

On Thu, Mar 15, 2018 at 04:02:03PM -0300, Martin Galvan wrote:
> 2018-03-15 15:53 GMT-03:00 Rich Felker <dalias@libc.org>:
> > In any case it's not a bug in musl. The code is perfectly valid C. If
> > the compiler is producing a warning for it, either ignore it or ask
> > the compiler to stop.
> 
> Just because some code is valid C, it doesn't mean it's not buggy.

It's valid C that does exactly what it's intended to do.

> >> The compiler warnings aren't being wrongly produced. musl will indeed
> >> perform a signed-to-unsigned conversion here.
> >
> > Because that's how the C language works.
> 
> Yes. And gcc has checks to try and make up for C's weak typing.
> 
> While your definition of "bug" is debatable, IMHO if a commonly used
> option causes application builds to break due to some library, the
> library has a usability issue. The issue is even bigger when we're
> talking about something as core as the standard C library.

Perhaps this should be documented more explicitly, but there is no
guarantee that building with -Werror[=anything except warnings which
are constraint violations in C] will succeed, especially when GCC is
not honoring its usual promise not to produce warnings for code
expanded from macros from -isystem paths. I did just test and indeed
the warning is produced with gcc 6.3.0.

> >> So whenever we find a bug on musl we should just stop all our
> >> development until you've fixed the bug?
> >
> > No. As noted above, if you need to support systems that might have bug
> > X, you write a test (configure-time or run-time as appropriate) to
> > detect bug X and handle it.
> 
> Precisely, and __MUSL__ would be really useful for this.

Absolutely not. __MUSL__ would not tell you anything about whether bug
X is present. It would facilitate permanently assuming "musl has bug
X" because you observed bug X on musl at one point in the past.

FWIW, mixing these two issues in one thread is not very productive.
The warning issue is separate and should be discussed on its own.

Rich

Re: Re: #define __MUSL__ in features.h

[dgutson .]

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

On Thu, Mar 15, 2018 at 4:32 PM, Rich Felker <dalias@libc.org> wrote:

> On Thu, Mar 15, 2018 at 04:02:03PM -0300, Martin Galvan wrote:
> > 2018-03-15 15:53 GMT-03:00 Rich Felker <dalias@libc.org>:
> > > In any case it's not a bug in musl. The code is perfectly valid C. If
> > > the compiler is producing a warning for it, either ignore it or ask
> > > the compiler to stop.
> >
> > Just because some code is valid C, it doesn't mean it's not buggy.
>
> It's valid C that does exactly what it's intended to do.
>
> > >> The compiler warnings aren't being wrongly produced. musl will indeed
> > >> perform a signed-to-unsigned conversion here.
> > >
> > > Because that's how the C language works.
> >
> > Yes. And gcc has checks to try and make up for C's weak typing.
> >
> > While your definition of "bug" is debatable, IMHO if a commonly used
> > option causes application builds to break due to some library, the
> > library has a usability issue. The issue is even bigger when we're
> > talking about something as core as the standard C library.
>
> Perhaps this should be documented more explicitly, but there is no
> guarantee that building with -Werror[=anything except warnings which
> are constraint violations in C] will succeed, especially when GCC is
> not honoring its usual promise not to produce warnings for code
> expanded from macros from -isystem paths. I did just test and indeed
> the warning is produced with gcc 6.3.0.
>
> > >> So whenever we find a bug on musl we should just stop all our
> > >> development until you've fixed the bug?
> > >
> > > No. As noted above, if you need to support systems that might have bug
> > > X, you write a test (configure-time or run-time as appropriate) to
> > > detect bug X and handle it.
> >
> > Precisely, and __MUSL__ would be really useful for this.
>
> Absolutely not. __MUSL__ would not tell you anything about whether bug
> X is present. It would facilitate permanently assuming "musl has bug
> X" because you observed bug X on musl at one point in the past.
>

Then turn __MUSL__ a number holding the version, as in cplusplus, etc, so
people can do

#if __MUSL__ < someversion
#endif

and it will be clear what happens and will solve the chronology issue.


>
> FWIW, mixing these two issues in one thread is not very productive.
> The warning issue is separate and should be discussed on its own.
>
> Rich
>



-- 
Who’s got the sweetest disposition?
One guess, that’s who?
Who’d never, ever start an argument?
Who never shows a bit of temperament?
Who's never wrong but always right?
Who'd never dream of starting a fight?
Who get stuck with all the bad luck?

[-- Attachment #2: Type: text/html, Size: 3775 bytes --]

Re: Re: #define __MUSL__ in features.h

[Rich Felker]

On Thu, Mar 15, 2018 at 04:00:37PM -0300, dgutson . wrote:
> On Thu, Mar 15, 2018 at 3:53 PM, Rich Felker <dalias@libc.org> wrote:
> 
> > On Thu, Mar 15, 2018 at 03:48:32PM -0300, Martin Galvan wrote:
> > > 2018-03-15 15:39 GMT-03:00 Rich Felker <dalias@libc.org>:
> > > >> (e.g. the FD* issue reported by Martin Galvan).
> > > >
> > > > That's not a bug. It's compiler warnings being wrongly produced for a
> > > > system header, probably because someone added -I/usr/include or
> > > > similar (normally GCC suppresses these).
> > >
> > > I'm certain we didn't add -I/usr/include or something similar. Could
> > > you test this yourself to confirm it's not a bug?
> >
> > In any case it's not a bug in musl. The code is perfectly valid C. If
> > the compiler is producing a warning for it, either ignore it or ask
> > the compiler to stop.
> >
> > > The compiler warnings aren't being wrongly produced. musl will indeed
> > > perform a signed-to-unsigned conversion here.
> >
> > Because that's how the C language works.
> >
> 
> it is a potential vulnerability:
> https://cwe.mitre.org/data/definitions/195.html
> https://wiki.sei.cmu.edu/confluence/display/c/INT31-C.+Ensure+that+integer+conversions+do+not+result+in+lost+or+misinterpreted+data
> https://wiki.sei.cmu.edu/confluence/display/c/INT30-C.+Ensure+that+unsigned+integer+operations+do+not+wrap
> 
> Can you ensure it is rocksolid and the signed integer will NEVER be a
> negative value?

FD_* have undefined behavior if the argument is outside the range of
FD_SETSIZE. We could trap this (and if you use fortify headers, they
do) but doing so breaks applications that wrongly allocate larger
space for fd_set buffers for the sake of intentionally using larger fd
values than are possible with the select API.

The behavior of the code with or without the cast to unsigned added is
_exactly the same_. There is no bug here that is fixed by the proposed
patch. The warning is telling you that, if you don't understand how
integer promotions work in C, the code might not do what you expected
it to do. The fact that you seem to think adding a cast "fixes a bug"
is demonstrating how harmful the whole cult around compiler warnings
is: it's not about using them as hints to check your code and make
sure it's doing what you want, but instead about making the warning go
away without actually changing anything.

Rich

Re: Re: #define __MUSL__ in features.h

[Rich Felker]

On Thu, Mar 15, 2018 at 04:13:44PM -0300, dgutson . wrote:
> On Thu, Mar 15, 2018 at 4:00 PM, dgutson . <danielgutson@gmail.com> wrote:
> 
> >
> >
> > On Thu, Mar 15, 2018 at 3:53 PM, Rich Felker <dalias@libc.org> wrote:
> >
> >> On Thu, Mar 15, 2018 at 03:48:32PM -0300, Martin Galvan wrote:
> >> > 2018-03-15 15:39 GMT-03:00 Rich Felker <dalias@libc.org>:
> >> > >> (e.g. the FD* issue reported by Martin Galvan).
> >> > >
> >> > > That's not a bug. It's compiler warnings being wrongly produced for a
> >> > > system header, probably because someone added -I/usr/include or
> >> > > similar (normally GCC suppresses these).
> >> >
> >> > I'm certain we didn't add -I/usr/include or something similar. Could
> >> > you test this yourself to confirm it's not a bug?
> >>
> >> In any case it's not a bug in musl. The code is perfectly valid C. If
> >> the compiler is producing a warning for it, either ignore it or ask
> >> the compiler to stop.
> >>
> >> > The compiler warnings aren't being wrongly produced. musl will indeed
> >> > perform a signed-to-unsigned conversion here.
> >>
> >> Because that's how the C language works.
> >>
> >
> > it is a potential vulnerability:
> > https://cwe.mitre.org/data/definitions/195.html
> > https://wiki.sei.cmu.edu/confluence/display/c/INT31-C.+
> > Ensure+that+integer+conversions+do+not+result+in+
> > lost+or+misinterpreted+data
> > https://wiki.sei.cmu.edu/confluence/display/c/INT30-C.+
> > Ensure+that+unsigned+integer+operations+do+not+wrap
> >
> 
> Just to add: the code doesn't not comply with MISRA 2004 because it
> violates 6.10.3. IOW, at least this issue turns musl MISRA non-compliant.

musl does not comply or aim to comply with MISRA.

MISRA certainly has a place, as a set of very strict practices that
you can verify with static checks and that force you to be very
explicit about things that might not be obvious to programmers not
really familiar with C. But it also makes C much more verbose and hard
to read if you are familiar with the language. I don't fault
people/orgs using MISRA for doing safety-critical embedded work that
has to be done in C, but I don't want to be the one dealing with it.

Rich

Re: Re: #define __MUSL__ in features.h

[dgutson .]

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

On Thu, Mar 15, 2018 at 4:37 PM, Rich Felker <dalias@libc.org> wrote:

> On Thu, Mar 15, 2018 at 04:00:37PM -0300, dgutson . wrote:
> > On Thu, Mar 15, 2018 at 3:53 PM, Rich Felker <dalias@libc.org> wrote:
> >
> > > On Thu, Mar 15, 2018 at 03:48:32PM -0300, Martin Galvan wrote:
> > > > 2018-03-15 15:39 GMT-03:00 Rich Felker <dalias@libc.org>:
> > > > >> (e.g. the FD* issue reported by Martin Galvan).
> > > > >
> > > > > That's not a bug. It's compiler warnings being wrongly produced
> for a
> > > > > system header, probably because someone added -I/usr/include or
> > > > > similar (normally GCC suppresses these).
> > > >
> > > > I'm certain we didn't add -I/usr/include or something similar. Could
> > > > you test this yourself to confirm it's not a bug?
> > >
> > > In any case it's not a bug in musl. The code is perfectly valid C. If
> > > the compiler is producing a warning for it, either ignore it or ask
> > > the compiler to stop.
> > >
> > > > The compiler warnings aren't being wrongly produced. musl will indeed
> > > > perform a signed-to-unsigned conversion here.
> > >
> > > Because that's how the C language works.
> > >
> >
> > it is a potential vulnerability:
> > https://cwe.mitre.org/data/definitions/195.html
> > https://wiki.sei.cmu.edu/confluence/display/c/INT31-C.+
> Ensure+that+integer+conversions+do+not+result+in+
> lost+or+misinterpreted+data
> > https://wiki.sei.cmu.edu/confluence/display/c/INT30-C.+
> Ensure+that+unsigned+integer+operations+do+not+wrap
> >
> > Can you ensure it is rocksolid and the signed integer will NEVER be a
> > negative value?
>
> FD_* have undefined behavior if the argument is outside the range of
> FD_SETSIZE. We could trap this (and if you use fortify headers, they
> do) but doing so breaks applications that wrongly allocate larger
> space for fd_set buffers for the sake of intentionally using larger fd
> values than are possible with the select API.
>
> The behavior of the code with or without the cast to unsigned added is
> _exactly the same_. There is no bug here that is fixed by the proposed
> patch. The warning is telling you that, if you don't understand how
>

You are talking to the wrong guy. I did not propose the patch and I did not
propose the cast.


> integer promotions work in C, the code might not do what you expected
> it to do. The fact that you seem to think adding a cast "fixes a bug"



> is demonstrating how harmful the whole cult around compiler warnings
> is: it's not about using them as hints to check your code and make
> sure it's doing what you want, but instead about making the warning go
> away without actually changing anything.
>
> Rich
>



-- 
Who’s got the sweetest disposition?
One guess, that’s who?
Who’d never, ever start an argument?
Who never shows a bit of temperament?
Who's never wrong but always right?
Who'd never dream of starting a fight?
Who get stuck with all the bad luck?

[-- Attachment #2: Type: text/html, Size: 4632 bytes --]

Re: Re: #define __MUSL__ in features.h

[Rich Felker]

On Thu, Mar 15, 2018 at 04:37:39PM -0300, dgutson . wrote:
> > > >> So whenever we find a bug on musl we should just stop all our
> > > >> development until you've fixed the bug?
> > > >
> > > > No. As noted above, if you need to support systems that might have bug
> > > > X, you write a test (configure-time or run-time as appropriate) to
> > > > detect bug X and handle it.
> > >
> > > Precisely, and __MUSL__ would be really useful for this.
> >
> > Absolutely not. __MUSL__ would not tell you anything about whether bug
> > X is present. It would facilitate permanently assuming "musl has bug
> > X" because you observed bug X on musl at one point in the past.
> >
> 
> Then turn __MUSL__ a number holding the version, as in cplusplus, etc, so
> people can do
> 
> #if __MUSL__ < someversion
> #endif
> 
> and it will be clear what happens and will solve the chronology issue.

This is a never-ending FAQ tarpit. Version numbers DO NOT WORK to
indicate presence or absence of bugs, because distros will backport
fixes. Apparently you never dealt with the hell of Redhat shipping
"2.6.x" kernels that had all the bugfixes from late 3.x, and
applications trying to infer stuff from the version number. DON'T DO
THAT. If you need to know if a bug or a feature is present, TEST FOR
IT.

Rich

Re: Re: #define __MUSL__ in features.h

[dgutson .]

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

On Thu, Mar 15, 2018 at 4:43 PM, Rich Felker <dalias@libc.org> wrote:

> On Thu, Mar 15, 2018 at 04:37:39PM -0300, dgutson . wrote:
> > > > >> So whenever we find a bug on musl we should just stop all our
> > > > >> development until you've fixed the bug?
> > > > >
> > > > > No. As noted above, if you need to support systems that might have
> bug
> > > > > X, you write a test (configure-time or run-time as appropriate) to
> > > > > detect bug X and handle it.
> > > >
> > > > Precisely, and __MUSL__ would be really useful for this.
> > >
> > > Absolutely not. __MUSL__ would not tell you anything about whether bug
> > > X is present. It would facilitate permanently assuming "musl has bug
> > > X" because you observed bug X on musl at one point in the past.
> > >
> >
> > Then turn __MUSL__ a number holding the version, as in cplusplus, etc, so
> > people can do
> >
> > #if __MUSL__ < someversion
> > #endif
> >
> > and it will be clear what happens and will solve the chronology issue.
>
> This is a never-ending FAQ tarpit. Version numbers DO NOT WORK to
> indicate presence or absence of bugs, because distros will backport
> fixes. Apparently you never dealt with the hell of Redhat shipping
> "2.6.x" kernels that had all the bugfixes from late 3.x, and
> applications trying to infer stuff from the version number. DON'T DO
> THAT. If you need to know if a bug or a feature is present, TEST FOR
> IT.
>

OK my last comment. If you are 100% positive that there will never be a
negative number or a wraparound due to
types conversion, then I suggest to add some compiler-dependant pragma
warning disabling locally in those lines.
We cannot afford to loose static checking capabilities of a compiler
because of a very specific occurrence.
And sorry for spamming this thread.


> Rich
>



-- 
Who’s got the sweetest disposition?
One guess, that’s who?
Who’d never, ever start an argument?
Who never shows a bit of temperament?
Who's never wrong but always right?
Who'd never dream of starting a fight?
Who get stuck with all the bad luck?

[-- Attachment #2: Type: text/html, Size: 3016 bytes --]

Re: Re: #define __MUSL__ in features.h

[u-uy74]

On Thu, Mar 15, 2018 at 04:13:44PM -0300, dgutson . wrote:
> Just to add: the code doesn't not comply with MISRA 2004 because it
> violates 6.10.3. IOW, at least this issue turns musl MISRA non-compliant.

According to some researchers (Delft University of Technology, 2008)

"... makes it possible that adherence to the MISRA standard as a whole
would have made the software less reliable. This observation is consistent
with Hatton’s earlier assessment of the MISRA C 2004 standard [10]."

Musl does not aim for the MISRA conformance. That's good.

Regards,
Rune

Re: Re: #define __MUSL__ in features.h

[u-uy74]

To be clear:

On Thu, Mar 15, 2018 at 09:16:11PM +0100, u-uy74@aetey.se wrote:

> Musl does not aim for the MISRA conformance.
 As stated by Rich.

>                                               That's good.
 My point.

Rune

Re: Re: #define __MUSL__ in features.h

[Markus Wichmann]

On Thu, Mar 15, 2018 at 03:51:22PM -0300, dgutson . wrote:
> Fact #1: Software is not perfect. Bugs may show up. And sometimes in bad
> timing when doing a release.
> So, user developers have two options: hack the library with a workaround,
> and release with a
> hacked library untested and unverified by its supporting community, or they
> can leave the library as-is, and
> implement the workaround in the using code (which requires the macro in
> order to limit the impact to the library implementation).
> 

You know, you could just write your own macro, right? It doesn't have to
be __MUSL__, it could be called HAVE_BUGGY_SOMETHING, and it could be
defined in a config.h. And it is entirely up to you how it gets there.

Let's say musl had a bug in glob() somewhere. What you want to do is:

#ifdef __MUSL__
assume glob() is buggy
#else
use glob()
#endif

What Rich wants you to do is write a configure script that compiles a
program against the library, runs it and tests it output against two
alternatives. Then you can write

#ifdef HAVE_BUGGY_GLOB
assume glob() is buggy
#else
use glob()
#endif

The difference is about five minutes of work. And if you can't be
bothered to spend even those five minutes, you probably aren't going to
bother with the #ifdef stuff anyway. If it has to work HERE and NOW, you
just write the code conforming to the current situation. Might not work
tomorrow, but oh well, that's why it's called technical debt.

Oh, and if you're cross compiling, write a test program, run it on the
target, then create a config.h according to the results.

> Fact #2: Fixes take time, because community projects have their own agenda
> and triaging policies.
> 
> So, in the hypothetical bug of a library (no matter this particular case I
> referred to, there were, there are, and there will always be bugs)
> the macro will work as an escape hatch until the fix of the hypothetical
> bug is ready upstream.
> 

In a better world, that would be the case. In the real world, the
temporary solution is staying in place until it fails -- and it never
does. As soon as it works, you move on. Maybe the original author of the
workaround has left the company. Years later, some inquisitive soul will
ask why that #ifdef __MUSL__ strangeness is still in place, and no-one
will be able to answer, except with a cargo cult mentality. "Just let it
stay there, it doesn't hurt anything." Entire generations will grow up
reading that code and learning "musl has bug X", then move on to other
companies to spread the misinformation further. And all because you
couldn't be arsed to write a test!

I might have exaggerated a bit there.

> I'm writing this from a very practical and industry point of view (who have
> worked in FLOSS projects before and commercial projects).
> 

I have worked in embedded software for four years now, and what I have
seen does not inspire confidence in the continuous improvement processes
of the industry, to put it mildly.


Ciao,
Markus

Re: Re: #define __MUSL__ in features.h

[Szabolcs Nagy]

* Rich Felker <dalias@libc.org> [2018-03-15 15:32:44 -0400]:
> Perhaps this should be documented more explicitly, but there is no
> guarantee that building with -Werror[=anything except warnings which
> are constraint violations in C] will succeed, especially when GCC is
> not honoring its usual promise not to produce warnings for code
> expanded from macros from -isystem paths. I did just test and indeed
> the warning is produced with gcc 6.3.0.
> 

how did you reproduce it? -Wsign-conversion (or -Wconversion)
is not even enabled by

 -Wall -Wextra -pedantic

because it has so many false positives. if the user really
asked for this warning then i think it's reasonable to show
it even if it's expanded via a system header macro, since the
issue might be how the macro is called.

i think the patch with type casts is not acceptable: if there
is a real bug (negative fd is used) then instead of erroring
out (preferably at compile time if the negative int is visible)
the cast silently makes sure the bug goes undetected.

adding compiler version and platform dependent pragmas is
also unacceptable (the headers should be possible to parse
and understood by whatever tool that knows the c language).

what would be acceptable is some form of assertion that the
fd must be positive, however it seems nobody cares about this
warning so neither gcc nor clang tries to use available range
information to see if conversion can ever change the result:

  if (d > 0) FD_SET(d,s);

still warns.

if users care about this warning they should first make
sure the compilers dont emit false positives for such
trivial cases.

Re: Re: #define __MUSL__ in features.h

[Rich Felker]

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

On Thu, Mar 15, 2018 at 10:46:56PM +0100, Szabolcs Nagy wrote:
> * Rich Felker <dalias@libc.org> [2018-03-15 15:32:44 -0400]:
> > Perhaps this should be documented more explicitly, but there is no
> > guarantee that building with -Werror[=anything except warnings which
> > are constraint violations in C] will succeed, especially when GCC is
> > not honoring its usual promise not to produce warnings for code
> > expanded from macros from -isystem paths. I did just test and indeed
> > the warning is produced with gcc 6.3.0.
> > 
> 
> how did you reproduce it? -Wsign-conversion (or -Wconversion)
> is not even enabled by

Manually using -Wsign-conversion with the attached program.

> what would be acceptable is some form of assertion that the
> fd must be positive, however it seems nobody cares about this
> warning so neither gcc nor clang tries to use available range
> information to see if conversion can ever change the result:
> 
>   if (d > 0) FD_SET(d,s);
> 
> still warns.

This basically settles it that it's a buggy warning option.

> if users care about this warning they should first make
> sure the compilers dont emit false positives for such
> trivial cases.

Agreed.

Rich

[-- Attachment #2: signed-conversion.c --]
[-- Type: text/plain, Size: 111 bytes --]

#include <sys/select.h>

int main(int argc, char **argv)
{
	fd_set fds;
	FD_ZERO(&fds);
	FD_SET(argc, &fds);
}