Changing policy about -Werror

classic Classic list List threaded Threaded
119 messages Options
1234 ... 6
Reply | Threaded
Open this post in threaded view
|

Changing policy about -Werror

Andrew Savchenko
Hi!

Our current -Werror policy demands unconditional removal:
https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed

I think this is wrong, see bugs 665464, 665538 for a recent
discussion why.

My point is that in *most* cases -Werror indeed should be removed,
because upstream rarely can keep up with all possible configure,
*FLAGS, compiler versions and arch combinations. But! In some cases
— especially for security oriented software — this flag may be
pertain and may be kept at maintainer's discretion.

The rationale is that -Werror usually points to dangerous
situations like uninitialized variables, pointer type mismatch or
implicit function declaration (and much more) which may lead to
serious security implications.

So, if maintainer has enough manpower to support this flag, we
should allow to keep it. Of course if it will cause long-standing
troubles (e.g. bugs opened for a long time) QA should have power to
remove it or demand its removal.

So my proposal is:

1) Deprecate QA policy with unconditional demand of -Werror removal.
2) Add to devmanual's chapter on -Werror an exception clause about
security-oriented software and maintainer's right to make final
decision.

Best regards,
Andrew Savchenko

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Thomas Deutschmann
Hi,

I disagree. Either discuss to drop the entire policy about "-Werror" or
don't but please do _not_ enter the game of differentiating between
"normal" and something you call "security-orientated" packages.

You will lose this game in the end.

If there's really a reason to allow "-Werror" it applies to *any*
package or there isn't a good reason. _Any_ package can be part of a
chained attack. Saying "Uh, this is a security-orientated package, we
must keep '-Werror' for..." -- for WHAT?! You are probably creating a
false sense of security...

Let me remind you of something like
https://daniel.haxx.se/blog/2016/10/14/a-single-byte-write-opened-a-root-execution-exploit/

No, "-Werror" wouldn't have prevent this, that's not my point. My point
is, that there's nothing like "security-orientated" packages. And in the
end you deal with chained attacks involving vectors you haven't thought
of before involving otherwise harmless packages.


Regarding a general drop of that policy: No, I wouldn't change that
policy at all. Gentoo is a rolling distribution and "-Werror" creates
undesired problems in most cases. Given that we have another rule that
any package must respect user's CFLAGS any user or dev who care can add
"-Werror" back to his/her CFLAGS... but don't force every user of Gentoo
to deal with that.


--
Regards,
Thomas Deutschmann / Gentoo Linux Developer
C4DD 695F A713 8F24 2AA1 5638 5849 7EE5 1D5D 74A5


signature.asc (650 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Andrew Savchenko
On Sun, 9 Sep 2018 15:03:11 +0200 Thomas Deutschmann wrote:
> Hi,
>
> I disagree. Either discuss to drop the entire policy about "-Werror" or
> don't but please do _not_ enter the game of differentiating between
> "normal" and something you call "security-orientated" packages.

You got me wrong. I'm not trying to build special rules for
security packages (since there is no margin between them and other
packages and you rightfully pointed out that any vulnerability may
play a role in a chained attack); they were just an example.

What I'm trying to do is to allow maintainers to keep -Werror if
they really want to do this, understand what they are doing and
have enough manpower to support this.

As can be seen from aforementioned bugs right now developer and
upstream support this to their best and yet QA team tries to
enforce -Werror drop using the brute force and ignoring active best
effort support. This should not happen.

Best regards,
Andrew Savchenko

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Jeroen Roovers-3
In reply to this post by Andrew Savchenko
On Sun, 9 Sep 2018 14:32:21 +0300
Andrew Savchenko <[hidden email]> wrote:

> Hi!
>
> Our current -Werror policy demands unconditional removal:
> https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed

Which is great.

> I think this is wrong, see bugs 665464, 665538 for a recent
> discussion why.

That's just QA failing to be human in one report and then again in
another report. It happens all the time and cannot be fixed, so you seem
to instead suggest banning -Werror wrong because some developers think
they can sanely cat-herd -Werror's overreaching effects.

> My point is that in *most* cases -Werror indeed should be removed,
> because upstream rarely can keep up with all possible configure,
> *FLAGS, compiler versions and arch combinations. But! In some cases
> — especially for security oriented software — this flag may be
> pertain and may be kept at maintainer's discretion.
  ^^^^^^^
Pertinent, you meant to say, I assume.

If you really have to support this mistaken (upstream) sense of
security, instead go for -Werror=<warning> (see gcc(1)) i.e. turn very
specific warnings into errors instead of turning _all_ warnings into
errors.

> The rationale is that -Werror usually points to dangerous
> situations like uninitialized variables, pointer type mismatch or
> implicit function declaration (and much more)

No it does not. It merely turns warnings (non-fatal) emitted by the
actual checks into errors (fatal). It does not cause any checks to be
performed or warnings to be issued by itself.

Setting or allowing a blanket -Werror therefore causes innocuous
warnings like this:

   warning: format not a string literal, argument types not checked
   [-Wformat-nonliteral]
   (In other words: FIXME: I didn't check this format because it was not
   a string literal and I cannot yet resolve those into an actual format
   defined elsewhere.*)

and this:

   # hppa2.0-unknown-linux-gnu-gcc -fstack-protector main.c
   cc1: warning: -fstack-protector not supported for this target

into hilarious errors.

To some those might look like succeeding security checks, but they're
really failing sanity checks.

> which may lead to serious security implications.
>
> So, if maintainer has enough manpower to support this flag, we
> should allow to keep it. Of course if it will cause long-standing
> troubles (e.g. bugs opened for a long time) QA should have power to
> remove it or demand its removal.
>
> So my proposal is:
>
> 1) Deprecate QA policy with unconditional demand of -Werror removal.

You have yet to give arguments for its removal. Like, proper and
particular examples of where -Werror benefits everyone. So far I've
seen only some hand-waving of the (in)security banner.

Unless you meant to say that security is improved when you can't
install the software when of -Werror prevents it from being built,
because then you've already solved the entire problem of the software's
purported vulnerabilities, indeed.

> 2) Add to devmanual's chapter on -Werror an exception clause about
> security-oriented software and maintainer's right to make final
> decision.

That clause will be the laughing stock of the security community.


Regards,
     jer


* We have plenty of bug reports already where one "security researcher"
points out that the build fails when the former warning is triggered
when -Werror is injected, and it might indeed look like a lurking
security issue if it weren't for the fact that the format sanity check
never took place.

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Richard Yao-2
In reply to this post by Andrew Savchenko


> On Sep 9, 2018, at 7:32 AM, Andrew Savchenko <[hidden email]> wrote:
>
> Hi!
>
> Our current -Werror policy demands unconditional removal:
> https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed
>
> I think this is wrong, see bugs 665464, 665538 for a recent
> discussion why.
>
> My point is that in *most* cases -Werror indeed should be removed,
> because upstream rarely can keep up with all possible configure,
> *FLAGS, compiler versions and arch combinations. But! In some cases
> — especially for security oriented software — this flag may be
> pertain and may be kept at maintainer's discretion.
>
> The rationale is that -Werror usually points to dangerous
> situations like uninitialized variables, pointer type mismatch or
> implicit function declaration (and much more) which may lead to
> serious security implications.
>
> So, if maintainer has enough manpower to support this flag, we
> should allow to keep it. Of course if it will cause long-standing
> troubles (e.g. bugs opened for a long time) QA should have power to
> remove it or demand its removal.
>
> So my proposal is:
>
> 1) Deprecate QA policy with unconditional demand of -Werror removal.
> 2) Add to devmanual's chapter on -Werror an exception clause about
> security-oriented software and maintainer's right to make final
> decision.

-Werror has caught bugs that could have resulted in data loss in ZFS in the past thanks to it being built in userspace as part of zdb. So it is useful for integrity too, not just security (although arguably, integrity is part of security).

Currently, sys-fs/zfs turns on -Werror when USE=debug is set. So far, nobody has complained about USE=debug enforcing -Werror. USE=debug by definition ought to be an exception.

Perhaps we could have another USE flag for -Werror where it is a security feature. e.g. USE=strict-compile-checks
>
> Best regards,
> Andrew Savchenko


Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Michał Górny-5
On Sun, 2018-09-09 at 11:22 -0400, Richard Yao wrote:

> > On Sep 9, 2018, at 7:32 AM, Andrew Savchenko <[hidden email]> wrote:
> >
> > Hi!
> >
> > Our current -Werror policy demands unconditional removal:
> > https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed
> >
> > I think this is wrong, see bugs 665464, 665538 for a recent
> > discussion why.
> >
> > My point is that in *most* cases -Werror indeed should be removed,
> > because upstream rarely can keep up with all possible configure,
> > *FLAGS, compiler versions and arch combinations. But! In some cases
> > — especially for security oriented software — this flag may be
> > pertain and may be kept at maintainer's discretion.
> >
> > The rationale is that -Werror usually points to dangerous
> > situations like uninitialized variables, pointer type mismatch or
> > implicit function declaration (and much more) which may lead to
> > serious security implications.
> >
> > So, if maintainer has enough manpower to support this flag, we
> > should allow to keep it. Of course if it will cause long-standing
> > troubles (e.g. bugs opened for a long time) QA should have power to
> > remove it or demand its removal.
> >
> > So my proposal is:
> >
> > 1) Deprecate QA policy with unconditional demand of -Werror removal.
> > 2) Add to devmanual's chapter on -Werror an exception clause about
> > security-oriented software and maintainer's right to make final
> > decision.
>
> -Werror has caught bugs that could have resulted in data loss in ZFS in the past thanks to it being built in userspace as part of zdb. So it is useful for integrity too, not just security (although arguably, integrity is part of security).
>
> Currently, sys-fs/zfs turns on -Werror when USE=debug is set. So far, nobody has complained about USE=debug enforcing -Werror. USE=debug by definition ought to be an exception.
Now that you know that you're violating a policy, please kindly fix
that.

> Perhaps we could have another USE flag for -Werror where it is a security feature. e.g. USE=strict-compile-checks

Perhaps people could learn that Gentoo lets them alter CFLAGS, and stop
inventing USE flags for every flag the compiler supports.

> >
> > Best regards,
> > Andrew Savchenko
>
>

--
Best regards,
Michał Górny

signature.asc (981 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Michał Górny-5
In reply to this post by Andrew Savchenko
On Sun, 2018-09-09 at 14:32 +0300, Andrew Savchenko wrote:

> Hi!
>
> Our current -Werror policy demands unconditional removal:
> https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed
>
> I think this is wrong, see bugs 665464, 665538 for a recent
> discussion why.
>
> My point is that in *most* cases -Werror indeed should be removed,
> because upstream rarely can keep up with all possible configure,
> *FLAGS, compiler versions and arch combinations. But! In some cases
> — especially for security oriented software — this flag may be
> pertain and may be kept at maintainer's discretion.
>
> The rationale is that -Werror usually points to dangerous
> situations like uninitialized variables, pointer type mismatch or
> implicit function declaration (and much more) which may lead to
> serious security implications.
It may also point to a different coding style, user's flags (like
clang's 'argument unused during X' warnings.  Are you suggesting that
upstream is going to detect all those situations and prevent them from
occurring, or are you going to WONTFIX the resulting bugs?

>
> So, if maintainer has enough manpower to support this flag, we
> should allow to keep it. Of course if it will cause long-standing
> troubles (e.g. bugs opened for a long time) QA should have power to
> remove it or demand its removal.

What you're saying basically boils down to 'it's fine that the package
randomly fails to build if somebody will fix it'.  However, some people
actually use Gentoo on real systems and really prefer when things
*build*.  While resolving warnings etc. is usually a worthwhile goal,
not at the cost of having users repeatedly hit failures, have to report
bugs about them and wait for the maintainer to fix them.

Not to mention that those fixes only work for new versions,
and therefore this whole idea turns downgrading (however undesirable you
might consider it) into a pointless effort of chasing old warnings.

This is just another example of writing programs for a single toolchain,
and adding more hacks every time someone tests with another one.

--
Best regards,
Michał Górny

signature.asc (981 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Ulrich Mueller-2
In reply to this post by Andrew Savchenko
>>>>> On Sun, 09 Sep 2018, Andrew Savchenko wrote:

> What I'm trying to do is to allow maintainers to keep -Werror if
> they really want to do this, understand what they are doing and
> have enough manpower to support this.

Bug 665464 has just proven that this doesn't work. That bug would not
have happened if the policy had been followed. Also its fix (removal of
an unused variable) should have been applied only upstream. I don't see
a good reason for adding downstream patches that will make no difference
for the resulting binary. At least not when the upstream package is
maintained, and the issue will likely go away with one of the next
releases.

> As can be seen from aforementioned bugs right now developer and
> upstream support this to their best and yet QA team tries to
> enforce -Werror drop using the brute force and ignoring active best
> effort support. This should not happen.

See flameeyes's old blog post for the rationale why the current policy
is in place:
https://flameeyes.blog/2009/02/25/future-proof-your-code-dont-use-werror/

Ulrich

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Richard Yao-2
In reply to this post by Michał Górny-5


> On Sep 9, 2018, at 12:11 PM, Michał Górny <[hidden email]> wrote:
>
> On Sun, 2018-09-09 at 11:22 -0400, Richard Yao wrote:
>>> On Sep 9, 2018, at 7:32 AM, Andrew Savchenko <[hidden email]> wrote:
>>>
>>> Hi!
>>>
>>> Our current -Werror policy demands unconditional removal:
>>> https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed
>>>
>>> I think this is wrong, see bugs 665464, 665538 for a recent
>>> discussion why.
>>>
>>> My point is that in *most* cases -Werror indeed should be removed,
>>> because upstream rarely can keep up with all possible configure,
>>> *FLAGS, compiler versions and arch combinations. But! In some cases
>>> — especially for security oriented software — this flag may be
>>> pertain and may be kept at maintainer's discretion.
>>>
>>> The rationale is that -Werror usually points to dangerous
>>> situations like uninitialized variables, pointer type mismatch or
>>> implicit function declaration (and much more) which may lead to
>>> serious security implications.
>>>
>>> So, if maintainer has enough manpower to support this flag, we
>>> should allow to keep it. Of course if it will cause long-standing
>>> troubles (e.g. bugs opened for a long time) QA should have power to
>>> remove it or demand its removal.
>>>
>>> So my proposal is:
>>>
>>> 1) Deprecate QA policy with unconditional demand of -Werror removal.
>>> 2) Add to devmanual's chapter on -Werror an exception clause about
>>> security-oriented software and maintainer's right to make final
>>> decision.
>>
>> -Werror has caught bugs that could have resulted in data loss in ZFS in the past thanks to it being built in userspace as part of zdb. So it is useful for integrity too, not just security (although arguably, integrity is part of security).
>>
>> Currently, sys-fs/zfs turns on -Werror when USE=debug is set. So far, nobody has complained about USE=debug enforcing -Werror. USE=debug by definition ought to be an exception.
>
> Now that you know that you're violating a policy, please kindly fix
> that.
I already knew about the policy. However, USE=debug is by definition meant for debug purposes. -Werror is helpful for debugging. There is nothing wrong with turning it on for debugging. The normal builds don’t have USE=debug set and -Werror is not used there.

>
>> Perhaps we could have another USE flag for -Werror where it is a security feature. e.g. USE=strict-compile-checks
>
> Perhaps people could learn that Gentoo lets them alter CFLAGS, and stop
> inventing USE flags for every flag the compiler supports.
>
>>>
>>> Best regards,
>>> Andrew Savchenko
>>
>>
>
> --
> Best regards,
> Michał Górny


Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Richard Yao-2
In reply to this post by Michał Górny-5


> On Sep 9, 2018, at 12:11 PM, Michał Górny <[hidden email]> wrote:
>
> On Sun, 2018-09-09 at 11:22 -0400, Richard Yao wrote:
>>> On Sep 9, 2018, at 7:32 AM, Andrew Savchenko <[hidden email]> wrote:
>>>
>>> Hi!
>>>
>>> Our current -Werror policy demands unconditional removal:
>>> https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed
>>>
>>> I think this is wrong, see bugs 665464, 665538 for a recent
>>> discussion why.
>>>
>>> My point is that in *most* cases -Werror indeed should be removed,
>>> because upstream rarely can keep up with all possible configure,
>>> *FLAGS, compiler versions and arch combinations. But! In some cases
>>> — especially for security oriented software — this flag may be
>>> pertain and may be kept at maintainer's discretion.
>>>
>>> The rationale is that -Werror usually points to dangerous
>>> situations like uninitialized variables, pointer type mismatch or
>>> implicit function declaration (and much more) which may lead to
>>> serious security implications.
>>>
>>> So, if maintainer has enough manpower to support this flag, we
>>> should allow to keep it. Of course if it will cause long-standing
>>> troubles (e.g. bugs opened for a long time) QA should have power to
>>> remove it or demand its removal.
>>>
>>> So my proposal is:
>>>
>>> 1) Deprecate QA policy with unconditional demand of -Werror removal.
>>> 2) Add to devmanual's chapter on -Werror an exception clause about
>>> security-oriented software and maintainer's right to make final
>>> decision.
>>
>> -Werror has caught bugs that could have resulted in data loss in ZFS in the past thanks to it being built in userspace as part of zdb. So it is useful for integrity too, not just security (although arguably, integrity is part of security).
>>
>> Currently, sys-fs/zfs turns on -Werror when USE=debug is set. So far, nobody has complained about USE=debug enforcing -Werror. USE=debug by definition ought to be an exception.
>
> Now that you know that you're violating a policy, please kindly fix
> that.
>
>> Perhaps we could have another USE flag for -Werror where it is a security feature. e.g. USE=strict-compile-checks
>
> Perhaps people could learn that Gentoo lets them alter CFLAGS, and stop
> inventing USE flags for every flag the compiler supports.

Do that and watch nearly everything break. If a package really ought to have -Werror due to a very good reason and is properly maintained to support it, then there is nothing wrong with inventing a USE flag to give users the option of enforcing that. It is better than letting users discover that via random trial and error. That just wastes people’s time.

>
>>>
>>> Best regards,
>>> Andrew Savchenko
>>
>>
>
> --
> Best regards,
> Michał Górny


Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Richard Yao-2
In reply to this post by Ulrich Mueller-2


On Sep 9, 2018, at 12:32 PM, Ulrich Mueller <[hidden email]> wrote:

>>>>>> On Sun, 09 Sep 2018, Andrew Savchenko wrote:
>
>> What I'm trying to do is to allow maintainers to keep -Werror if
>> they really want to do this, understand what they are doing and
>> have enough manpower to support this.
>
> Bug 665464 has just proven that this doesn't work. That bug would not
> have happened if the policy had been followed. Also its fix (removal of
> an unused variable) should have been applied only upstream. I don't see
> a good reason for adding downstream patches that will make no difference
> for the resulting binary. At least not when the upstream package is
> maintained, and the issue will likely go away with one of the next
> releases.
>
>> As can be seen from aforementioned bugs right now developer and
>> upstream support this to their best and yet QA team tries to
>> enforce -Werror drop using the brute force and ignoring active best
>> effort support. This should not happen.
>
> See flameeyes's old blog post for the rationale why the current policy
> is in place:
> https://flameeyes.blog/2009/02/25/future-proof-your-code-dont-use-werror/
For every pointless check that fails -Werror, there is likely one that actually does matter. An unused variable could go either way if upstream intended to use that variable, but used another one by mistake (it happens).

Allowing users to opt into -Werror behavior on specific packages whose maintainers have a good reason to do it and are keeping up with things would be alright.
>
> Ulrich
>


Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Richard Yao-2
In reply to this post by Richard Yao-2

> On Sep 9, 2018, at 1:09 PM, Richard Yao <[hidden email]> wrote:
>
>
>
>> On Sep 9, 2018, at 12:11 PM, Michał Górny <[hidden email]> wrote:
>>
>> On Sun, 2018-09-09 at 11:22 -0400, Richard Yao wrote:
>>>> On Sep 9, 2018, at 7:32 AM, Andrew Savchenko <[hidden email]> wrote:
>>>>
>>>> Hi!
>>>>
>>>> Our current -Werror policy demands unconditional removal:
>>>> https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed
>>>>
>>>> I think this is wrong, see bugs 665464, 665538 for a recent
>>>> discussion why.
>>>>
>>>> My point is that in *most* cases -Werror indeed should be removed,
>>>> because upstream rarely can keep up with all possible configure,
>>>> *FLAGS, compiler versions and arch combinations. But! In some cases
>>>> — especially for security oriented software — this flag may be
>>>> pertain and may be kept at maintainer's discretion.
>>>>
>>>> The rationale is that -Werror usually points to dangerous
>>>> situations like uninitialized variables, pointer type mismatch or
>>>> implicit function declaration (and much more) which may lead to
>>>> serious security implications.
>>>>
>>>> So, if maintainer has enough manpower to support this flag, we
>>>> should allow to keep it. Of course if it will cause long-standing
>>>> troubles (e.g. bugs opened for a long time) QA should have power to
>>>> remove it or demand its removal.
>>>>
>>>> So my proposal is:
>>>>
>>>> 1) Deprecate QA policy with unconditional demand of -Werror removal.
>>>> 2) Add to devmanual's chapter on -Werror an exception clause about
>>>> security-oriented software and maintainer's right to make final
>>>> decision.
>>>
>>> -Werror has caught bugs that could have resulted in data loss in ZFS in the past thanks to it being built in userspace as part of zdb. So it is useful for integrity too, not just security (although arguably, integrity is part of security).
>>>
>>> Currently, sys-fs/zfs turns on -Werror when USE=debug is set. So far, nobody has complained about USE=debug enforcing -Werror. USE=debug by definition ought to be an exception.
>>
>> Now that you know that you're violating a policy, please kindly fix
>> that.
> I already knew about the policy. However, USE=debug is by definition meant for debug purposes. -Werror is helpful for debugging. There is nothing wrong with turning it on for debugging. The normal builds don’t have USE=debug set and -Werror is not used there.

By the way, if people insist on applying this policy to USE=debug, I am inclined to mask the USE flag. This would satisfy the policy, but I don’t think that is better than how things are now. Users already are warned not to set USE=debug globally and if they are setting it on a specific package, they are opting into the package’s definition of debug functionality.

I don’t see a problem with having -Werror as part of USE=debug. USE=debug on that package is meant to be an aid to upstream development and upstream in this case wants to know about any warnings.

>>> Perhaps we could have another USE flag for -Werror where it is a security feature. e.g. USE=strict-compile-checks
>>
>> Perhaps people could learn that Gentoo lets them alter CFLAGS, and stop
>> inventing USE flags for every flag the compiler supports.
>>
>>>>
>>>> Best regards,
>>>> Andrew Savchenko
>>>
>>>
>>
>> --
>> Best regards,
>> Michał Górny
>
>


Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Michael Orlitzky
In reply to this post by Andrew Savchenko
On 09/09/2018 07:32 AM, Andrew Savchenko wrote:
> Hi!
>
> Our current -Werror policy demands unconditional removal:
> https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed
>
> I think this is wrong, see bugs 665464, 665538 for a recent
> discussion why.
>
> ...
I agree with the QA team on this. For the upstream maintainer, -Werror
is useful and deserves to be enabled. For the end-user, on the other
hand, it has no real benefit. And for users of a source-based
distribution, it is actively harmful. Here are some random points:

  * A -Werror failure doesn't actually prevent me from installing a
    package, it only prevents me from installing a package with a newer
    compiler (that often provides other security improvements, like
    Spectre mitigation). So if you're using -Werror to prevent a
    "vulnerable" package from being installed, it doesn't work, and can
    actually be harmful if it prevents me from using a better compiler.

  * The build failures from -Werror don't occur only with new installs.
    They also occur during rebuilds for things like USE changes or
    library ABI updates, leaving you with a broken system.

  * Upstream maintainers can't retroactively fix Gentoo versions. If
    some old version foo-1.0 builds with gcc-8.x and is stable, but then
    breaks with gcc-9.x due to a new warning, how is upstream going to
    fix that? They aren't -- and you aren't either without patching a
    supposedly stable package in-place.

  * Breakage with -Werror prevents upgrades of an already-installed
    package. If there's a security vulnerability in an old version and
    if -Werror is preventing me from upgrading (thanks to a gcc upgrade
    in the meantime), then you've just made things much worse.

And so on.

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Matt Turner-5
In reply to this post by Andrew Savchenko
On Sun, Sep 9, 2018 at 4:32 AM Andrew Savchenko <[hidden email]> wrote:
>
> Hi!
>
> Our current -Werror policy demands unconditional removal:
> https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed
>
> I think this is wrong, see bugs 665464, 665538 for a recent
> discussion why.

Bug 665464 supports the exact opposite conclusion. Werror turned a
trivial warning into a build failure.

> My point is that in *most* cases -Werror indeed should be removed,
> because upstream rarely can keep up with all possible configure,
> *FLAGS, compiler versions and arch combinations. But! In some cases
> — especially for security oriented software — this flag may be
> pertain and may be kept at maintainer's discretion.
>
> The rationale is that -Werror usually points to dangerous
> situations like uninitialized variables, pointer type mismatch or
> implicit function declaration (and much more) which may lead to
> serious security implications.

Warnings are often over unimportant details (like in this bug). It is
certainly not the case that they "usually point to dangerous
situations".

> So, if maintainer has enough manpower to support this flag, we
> should allow to keep it. Of course if it will cause long-standing
> troubles (e.g. bugs opened for a long time) QA should have power to
> remove it or demand its removal.

In the bug that started this, it was the case that the maintainer
himself had not built the package with this configuration. Nor had any
arch team that recently stabilized the package (x86, amd64, ia64, ppc,
ppc64, arm).

So again, the bug supports the opposite conclusion.

The policy is sound, and I don't think we could have found a worse bug
as supporting evidence that we should revise the policy.

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Chí-Thanh Christopher Nguyễn
In reply to this post by Michał Górny-5
Michał Górny schrieb:
> Are you suggesting that
> upstream is going to detect all those situations and prevent them from
> occurring, or are you going to WONTFIX the resulting bugs?

No. With -Werror, upstream indicates that if a warning occurs, the build
should fail and the resulting code not be installed on user systems.

Instead, someone knowledgeable should look at the situation *first* and
determine whether it is a bogus warning, a trivial issue, or something which
warrants further attention.

I have long disagreed with QA policy on this, and think that ebuilds should
respect upstream here. Of course giving users the ability to override.


Best regards,
Chí-Thanh Christopher Nguyễn

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Chí-Thanh Christopher Nguyễn
In reply to this post by Andrew Savchenko
Andrew Savchenko schrieb:
> So my proposal is:
>
> 1) Deprecate QA policy with unconditional demand of -Werror removal.
> 2) Add to devmanual's chapter on -Werror an exception clause about
> security-oriented software and maintainer's right to make final
> decision.

Likely this proposal will not fly. I understand that -Werror is a
precautionary measure against installing broken code on user systems, and I
am all for using it when upstream says so. But it is understandably unwelcome
by many on Gentoo. If ebuilds started to use -Werror in greater numbers,
perceived tree quality would go down as build failures increase.

"But it's for your own good!" you would tell users who are angry again that
package XY didn't compile. -Werror gets in the way of users getting things
done, and if that happens too often, you might drive those users out.

So while I would very much like to see -Werror allowed, this is just not
going to happen.


Best regards,
Chí-Thanh Christopher Nguyễn

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Rich Freeman
In reply to this post by Michael Orlitzky
On Sun, Sep 9, 2018 at 1:50 PM Michael Orlitzky <[hidden email]> wrote:
>
>     So if you're using -Werror to prevent a
>     "vulnerable" package from being installed, it doesn't work, and can
>     actually be harmful if it prevents me from using a better compiler.
>

Whether or not the new compiler is better, it is clearly untested with
the package-version in question (otherwise these warnings would have
been addressed).  For something critical like a filesystem (zfs) that
could be useful to know.

I'm not convinced that this rule ought to be absolute.

That said, I do agree with your later comments that this creates a
messy situation by painting a user into a corner.  Other than sticking
painful ranged version dependencies on the toolchain into the package
I'm not sure if there is a cleaner solution that guarantees that the
package won't be built with a version of gcc that is untested with
that specific package without a user override.

I guess we could just have sensitive ebuilds output that it might eat
your data if you didn't add -Werror to your CFLAGS and then leave it
to the users to decide how much they care about build errors vs
unlikely sorry-I-lost-your-10PiB-array errors.

--
Rich

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Jason Zaman-2
In reply to this post by Chí-Thanh Christopher Nguyễn
On Mon, Sep 10, 2018 at 01:46:51AM +0200, Chí-Thanh Christopher Nguyễn wrote:

> Michał Górny schrieb:
> > Are you suggesting that
> > upstream is going to detect all those situations and prevent them from
> > occurring, or are you going to WONTFIX the resulting bugs?
>
> No. With -Werror, upstream indicates that if a warning occurs, the build
> should fail and the resulting code not be installed on user systems.
>
> Instead, someone knowledgeable should look at the situation *first* and
> determine whether it is a bogus warning, a trivial issue, or something which
> warrants further attention.
>
> I have long disagreed with QA policy on this, and think that ebuilds should
> respect upstream here. Of course giving users the ability to override.

I disagree. -Werror means that upstream wants it to build without
warnings on their distro with their version of the compiler with their
versions of all the libraries. Even if upstream was using gentoo
(they're not) they'd need to be testing both stable and unstable
toolchains since there are frequently warnings that show up in one and
not the other and also between gcc/clang.

I agree with jer on this, if you want specific warnings to be errors use
-Werror=foo or something not a blanket -Werror.

There are things that upstream absolutely should be setting which make a
big difference for security like FORTIFY_SOURCE but hardened already has
that set so I get this and thus basically everything would fail to
compile.

$ gcc -O1 -D_FORTIFY_SOURCE=2 foo.c
<command-line>:0:0: warning: "_FORTIFY_SOURCE" redefined
<built-in>: note: this is the location of the previous definition

This all on amd64 too. If we start with other arches or cross compilers
or other things then -Werror is just not possible.

-- Jason

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Fabian Groffen-2
In reply to this post by Richard Yao-2
On 09-09-2018 11:22:41 -0400, Richard Yao wrote:
> -Werror has caught bugs that could have resulted in data loss in ZFS in the past thanks to it being built in userspace as part of zdb. So it is useful for integrity too, not just security (although arguably, integrity is part of security).

This is a misconception, as jer already pointed out.  Instead:

-Werror has forced you to take notice of problems that could have
resulted in data loss in ZFS ...

Also, consider that for -Werror to be "better", you also need -O3 in
order to activate the "proper" compiler checks like "variable set but
never used" ones.

> Perhaps we could have another USE flag for -Werror where it is a security feature. e.g. USE=strict-compile-checks

You better run a static code analyser, such as the one you can hook up
with Travis.  It usually points out real security problems such as
races, which GCC doesn't do yet, as far as I'm aware.  Let alone
trigger with -Werror.

Fabian


--
Fabian Groffen
Gentoo on a different level

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Chí-Thanh Christopher Nguyễn
In reply to this post by Jason Zaman-2
Jason Zaman schrieb:

>> No. With -Werror, upstream indicates that if a warning occurs, the build
>> should fail and the resulting code not be installed on user systems.
>>
>> Instead, someone knowledgeable should look at the situation *first* and
>> determine whether it is a bogus warning, a trivial issue, or something which
>> warrants further attention.
>>
>> I have long disagreed with QA policy on this, and think that ebuilds should
>> respect upstream here. Of course giving users the ability to override.
>
> I disagree. -Werror means that upstream wants it to build without
> warnings on their distro with their version of the compiler with their
> versions of all the libraries.

It means, upstream wants it to build without warnings everywhere. And if a
warning occurs (due to change in compiler, libraries, architecture, etc.),
have a developer look at it first before installing the code on user systems.

> There are things that upstream absolutely should be setting which make a
> big difference for security like FORTIFY_SOURCE but hardened already has
> that set so I get this and thus basically everything would fail to
> compile.
>
> $ gcc -O1 -D_FORTIFY_SOURCE=2 foo.c
> <command-line>:0:0: warning: "_FORTIFY_SOURCE" redefined
> <built-in>: note: this is the location of the previous definition
>
> This all on amd64 too. If we start with other arches or cross compilers
> or other things then -Werror is just not possible.

But you have looked at the issue, decided that it is harmless, and can now
make this particular warning non-fatal. Or report upstream, so they can do
the Right Thing™ and don't redefine.

$ gcc -O1 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 foo.c

That is all what is desired.


Best regards,
Chí-Thanh Christopher Nguyễn

1234 ... 6