1:28 p.m.
The upstream KiCAD project has requested that I remove GLIBCXX_ASSERTIONS from the Fedora
package, as described here: https://bugs.launchpad.net/kicad/+bug/1838448
What is the best way to do that? I can add "%undefine _hardened_build" (which I
am testing now) but I think that will remove other hardening features that I might want to
leave enabled.
Steve
10:09 a.m.
On 01/08/2019 19:28, Steven A. Falco wrote:
The upstream KiCAD project has requested that I remove
GLIBCXX_ASSERTIONS from the Fedora package, as described here:
https://bugs.launchpad.net/kicad/+bug/1838448
What is the best way to do that? I can add "%undefine _hardened_build" (which
I am testing now) but I think that will remove other hardening features that I might want
to leave enabled.
Well you just need to add -U_GLIBCXX_ASSERTIONS to the end of the
compiler flags.
But I think upstream is giving very bad advice...
That define does not "add extra crashes" in the way that they
seem to think - well I mean it does literally but those crashes
are reports of program errors on their part.
Specifically in this case they appear to be accessing a
std::vector at an index beyond the end, so they are accessing
memory that may not be allocated at all, and if it is does
not belong to the vector in question. So the program is quite
likely to crash there one day anyway, the extra assertion just
makes sure it always does.
Tom
--
Tom Hughes (tom(a)compton.nu)
http://compton.nu/
10:14 a.m.
I personally use:
https://src.fedoraproject.org/rpms/galera/blob/master/f/galera.spec#_40
| %build
| %{set_build_flags}
|
| # FTBFS with the GLIBCXX_ASSERTIONS; #1546787
| CPPFLAGS=`echo $CPPFLAGS| sed -e "s|-Wp,-D_GLIBCXX_ASSERTIONS||g" `
| CFLAGS=`echo $CFLAGS| sed -e "s|-Wp,-D_GLIBCXX_ASSERTIONS||g" `
| CXXFLAGS=`echo $CXXFLAGS| sed -e "s|-Wp,-D_GLIBCXX_ASSERTIONS||g" `
| export CPPFLAGS CFLAGS CXXFLAGS
until it's solved.
Of course, if somebody has some more elegant solution, I'll be happy
to adopt it.
Michal
--
Michal Schorm
Software Engineer
Core Services - Databases Team
Red Hat
--
On Fri, Aug 2, 2019 at 5:01 PM Steven A. Falco <stevenfalco(a)gmail.com> wrote:
>
> The upstream KiCAD project has requested that I remove GLIBCXX_ASSERTIONS from the
Fedora package, as described here: https://bugs.launchpad.net/kicad/+bug/1838448
>
> What is the best way to do that? I can add "%undefine _hardened_build"
(which I am testing now) but I think that will remove other hardening features that I
might want to leave enabled.
>
> Steve
> _______________________________________________
> devel mailing list -- devel(a)lists.fedoraproject.org
> To unsubscribe send an email to devel-leave(a)lists.fedoraproject.org
> Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives:
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org
10:15 a.m.
Hello, Steven A. Falco.
Thu, 1 Aug 2019 14:28:31 -0400 you wrote:
What is the best way to do that? I can add "%undefine
_hardened_build" (which I am testing now) but I think that will remove other
hardening features that I might want to leave enabled.
%global optflags %(echo
%{optflags} | sed 's/-Wp,-D_GLIBCXX_ASSERTIONS//')
--
Sincerely,
Vitaly Zaitsev (vitaly(a)easycoding.org)
10:18 a.m.
On 8/2/19 11:09 AM, Tom Hughes wrote:
On 01/08/2019 19:28, Steven A. Falco wrote:
> The upstream KiCAD project has requested that I remove GLIBCXX_ASSERTIONS from the
Fedora package, as described here: https://bugs.launchpad.net/kicad/+bug/1838448
>
> What is the best way to do that? I can add "%undefine _hardened_build"
(which I am testing now) but I think that will remove other hardening features that I
might want to leave enabled.
Well you just need to add -U_GLIBCXX_ASSERTIONS to the end of the
compiler flags.
But I think upstream is giving very bad advice...
That define does not "add extra crashes" in the way that they
seem to think - well I mean it does literally but those crashes
are reports of program errors on their part.
Specifically in this case they appear to be accessing a
std::vector at an index beyond the end, so they are accessing
memory that may not be allocated at all, and if it is does
not belong to the vector in question. So the program is quite
likely to crash there one day anyway, the extra assertion just
makes sure it always does.
I agree that it sounds like bad advice, and I've raised that exact issue in comment
#22 (https://bugs.launchpad.net/kicad/+bug/1838448/comments/22). We'll see if I can
convince upstream to rethink this.
Steve
10:39 a.m.
* Steven A. Falco:
The upstream KiCAD project has requested that I remove
GLIBCXX_ASSERTIONS from the Fedora package, as described here:
https://bugs.launchpad.net/kicad/+bug/1838448
I commented on the bug. I think removal of these security checks is not
the right thing to do.
Thanks,
Florian
10:44 a.m.
On Thu, Aug 1, 2019 at 1:28 PM, Steven A. Falco <stevenfalco(a)gmail.com>
wrote:
What is the best way to do that?
Don't. As Florian has pointed out in Launchpad, this would convert the
nice crash into a security vulnerability. The assertions have a
negligible impact on performance, so better leave them enabled and
focus on fixing your software bug instead.
Michael
10:49 a.m.
On 8/1/19 9:28 PM, Steven A. Falco wrote:
The upstream KiCAD project has requested that I remove
GLIBCXX_ASSERTIONS from the Fedora package, as described here:
https://bugs.launchpad.net/kicad/+bug/1838448
What is the best way to do that? I can add "%undefine
_hardened_build" (which I am testing now) but I think that will
remove other hardening features that I might want to leave enabled.
Looks like you're using CMake. This should do the trick
export CFLAGS=$(echo "%{optflags}" |
sed "s|-Wp,-D_GLIBCXX_ASSERTIONS||g")
export CXXFLAGS=$(echo "%{optflags}" |
sed "s|-Wp,-D_GLIBCXX_ASSERTIONS||g")
--
Susi Lehtola
Fedora Project Contributor
jussilehtola(a)fedoraproject.org
10:50 a.m.
On 8/2/19 11:39 AM, Florian Weimer wrote:
* Steven A. Falco:
> The upstream KiCAD project has requested that I remove
> GLIBCXX_ASSERTIONS from the Fedora package, as described here:
> https://bugs.launchpad.net/kicad/+bug/1838448
I commented on the bug. I think removal of these security checks is not
the right thing to do.
Thanks,
Florian
Thanks for doing that. The more I think about it, the less I like removing the
GLIBCXX_ASSERTIONS.
Steve
11:10 a.m.
On Thu, 2019-08-01 at 14:28 -0400, Steven A. Falco wrote:
The upstream KiCAD project has requested that I remove
GLIBCXX_ASSERTIONS from the Fedora package, as described here:
https://bugs.launchpad.net/kicad/+bug/1838448
What is the best way to do that? I can add "%undefine
_hardened_build" (which I am testing now) but I think that will
remove other hardening features that I might want to leave enabled.
The best way is "Do not do that".
These flags are finding *actual* bugs in the program (out of bound
errors). Upstream should find out what is causing those and fix the
source of the bug, not hide it (it may cause memory corruption or worse
down the road).
Simo.
--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc
11:11 a.m.
Steve, as you said in that thread, actually those assertions have
helped uncover a bug (tagged as "critical")! I don't see any way in
which they could "add additional crashes" if upstream does their
homework. So I don't think it's a good idea to remove them.
Iñaki
On Fri, 2 Aug 2019 at 17:47, Steven A. Falco <stevenfalco(a)gmail.com> wrote:
The upstream KiCAD project has requested that I remove GLIBCXX_ASSERTIONS from the Fedora
package, as described here: https://bugs.launchpad.net/kicad/+bug/1838448
What is the best way to do that? I can add "%undefine _hardened_build" (which
I am testing now) but I think that will remove other hardening features that I might want
to leave enabled.
Steve
_______________________________________________
devel mailing list -- devel(a)lists.fedoraproject.org
To unsubscribe send an email to devel-leave(a)lists.fedoraproject.org
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org
--
Iñaki Úcar
12:13 p.m.
Am Donnerstag, den 01.08.2019, 14:28 -0400 schrieb Steven A. Falco:
The upstream KiCAD project has requested that I remove
GLIBCXX_ASSERTIONS from the Fedora package, as described here:
https://bugs.launchpad.net/kicad/+bug/1838448
What is the best way to do that? I can add "%undefine
_hardened_build" (which I am testing now) but I think that will remove
other hardening features that I might want to leave enabled.
Steve
Simply add this to your spec file:
# Upstream recommends to turn off _GLIBCXX_ASSERTIONS.
# See: $UPSTREAM_BUG
%global optflags %optflags -U_GLIBCXX_ASSERTIONS
3:36 p.m.
I think this is what you want:
%global optflags %(echo %{optflags} | sed 's/-Wp,-D_GLIBCXX_ASSERTIONS / /')
Tom
On Fri, Aug 2, 2019 at 11:00 AM Steven A. Falco <stevenfalco(a)gmail.com>
wrote:
The upstream KiCAD project has requested that I remove
GLIBCXX_ASSERTIONS
from the Fedora package, as described here:
https://bugs.launchpad.net/kicad/+bug/1838448
What is the best way to do that? I can add "%undefine _hardened_build"
(which I am testing now) but I think that will remove other hardening
features that I might want to leave enabled.
Steve
_______________________________________________
devel mailing list -- devel(a)lists.fedoraproject.org
To unsubscribe send an email to devel-leave(a)lists.fedoraproject.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org
5:46 a.m.
Steven A. Falco wrote:
The upstream KiCAD project has requested that I remove
GLIBCXX_ASSERTIONS
from the Fedora package, as described here:
https://bugs.launchpad.net/kicad/+bug/1838448
What is the best way to do that? I can add "%undefine _hardened_build"
(which I am testing now) but I think that will remove other hardening
features that I might want to leave enabled.
Just don't do it. That buffer overflow needs fixing either way. And the
security implications of disabling the assertions have already been pointed
out in the upstream bug report.
Kevin Kofler
2:03 p.m.
On 8/2/19 1:13 PM, Björn 'besser82' Esser wrote:
Am Donnerstag, den 01.08.2019, 14:28 -0400 schrieb Steven A. Falco:
> The upstream KiCAD project has requested that I remove
> GLIBCXX_ASSERTIONS from the Fedora package, as described here:
> https://bugs.launchpad.net/kicad/+bug/1838448
>
> What is the best way to do that? I can add "%undefine
> _hardened_build" (which I am testing now) but I think that will remove
> other hardening features that I might want to leave enabled.
>
> Steve
Simply add this to your spec file:
# Upstream recommends to turn off _GLIBCXX_ASSERTIONS.
# See: $UPSTREAM_BUG
%global optflags %optflags -U_GLIBCXX_ASSERTIONS
Thanks for all the replies! I will resist turning off the glibcxx assertions - I am
convinced now that it would be a bad idea.
But at least now I know how to do that sort of thing, if I ever do need to modify the
default flags in the future.
Steve
6:29 p.m.
Tom Hughes writes:
But I think upstream is giving very bad advice...
That define does not "add extra crashes" in the way that they
seem to think - well I mean it does literally but those crashes
are reports of program errors on their part.
Specifically in this case they appear to be accessing a
std::vector at an index beyond the end, so they are accessing
memory that may not be allocated at all, and if it is does
not belong to the vector in question. So the program is quite
likely to crash there one day anyway, the extra assertion just
makes sure it always does.
I believe that the following construct trips this assertion:
# std::vector<int> foo;
#
# std::vector<int> bar;
#
# // Populate foo with something.
#
# std::copy(&foo[0], &foo[foo.size()], std::back_insert_iterator{bar});
There's nothing wrong with this. There is no out of bounds access. I do not
believe that this is undefined behavior. The defined semantics of
std::vector, and its operator[], are well defined here.
I ran into these new assertions with my own code as well, after updating to
F28 (where they were enabled by default the first time, IIRC, not sure why
this shows up only now, for that package).
I ended up tweaking my code to avoid the assertions, rather than disabling
them. For this particular situation, my original change was to try
std::copy(&foo[0], &foo[0]+foo.size(), std::back_insert_iterator{bar});
But that still tripped the assertion when the foo vector was empty, so I had
wrap this inside an if().
This was a somewhat silly excersize. But, I do agree that these assertions
are better to have, than have not.
7:24 p.m.
On Sat, Aug 3, 2019 at 4:30 PM Sam Varshavchik <mrsam(a)courier-mta.com> wrote:
Tom Hughes writes:
> But I think upstream is giving very bad advice...
>
> That define does not "add extra crashes" in the way that they
> seem to think - well I mean it does literally but those crashes
> are reports of program errors on their part.
>
> Specifically in this case they appear to be accessing a
> std::vector at an index beyond the end, so they are accessing
> memory that may not be allocated at all, and if it is does
> not belong to the vector in question. So the program is quite
> likely to crash there one day anyway, the extra assertion just
> makes sure it always does.
I believe that the following construct trips this assertion:
# std::vector<int> foo;
#
# std::vector<int> bar;
#
# // Populate foo with something.
#
# std::copy(&foo[0], &foo[foo.size()], std::back_insert_iterator{bar});
There's nothing wrong with this. There is no out of bounds access.
You just formed a reference past the end of an array. I doubt that is
valid according to the standard. It certainly fails the smell test.
I *think* the standard defines &foo[foo.size] as being equivalent to
&*(foo.begin() + foo.size()), which certainly appears to be invalid.
On a cursory search of the standard, I couldn't find where it says
what operator* on this type of iterator does at all, let alone whether
it's valid for one-past-the-end iterators, but I'm pretty sure that
your code is, indeed, wrong.
I do not
believe that this is undefined behavior. The defined semantics of
std::vector, and its operator[], are well defined here.
I ran into these new assertions with my own code as well, after updating to
F28 (where they were enabled by default the first time, IIRC, not sure why
this shows up only now, for that package).
I ended up tweaking my code to avoid the assertions, rather than disabling
them. For this particular situation, my original change was to try
std::copy(&foo[0], &foo[0]+foo.size(), std::back_insert_iterator{bar});
What you want is foo.begin() and foo.end() or, if you really want to
play with pointers, foo.data().
10:14 p.m.
Andrew Lutomirski writes:
On a cursory search of the standard, I couldn't find where it
says
what operator* on this type of iterator does at all, let alone whether
it's valid for one-past-the-end iterators, but I'm pretty sure that
your code is, indeed, wrong.
This finally piqued my curiosity enough to take some time to look into this.
That expression applied the "*" operator to a random access iterator. So,
going down the path:
# [random.access.iterators]
#
# A class or pointer type X satisfies the requirements of a random access
# iterator if, in addition to satisfying the requirements for bidirectional
# iterators, ...
In a similar fashion, a bidirectional iterator delegates some requirements
to a forward iterator.
The forward iterator delegates some of its requirements to an input iterator.
At the end of the road, in [input.iterators]:
# *a reference, convertible to T Requires: a is dereferenceable.
Then, finally, in [iterator.requirements.general]:
# Values of an iterator i for which the expression *i is defined are called
# dereferenceable.
This reads to me like a definition that circularly defines itself.
[input.iterators] says "*a requires that a is dereferencable". And
[iterator.requirements.general] says that something is dereferencable if "*"
for it is defined. That certainly clears that up…
Full disclosure: it's true that the next sentence is:
# The library never assumes that past-the-end values are dereferenceable.
But this only states that the library assumes that, it doesn't
authoritatively state that.
But going back to the previous point, if we also want to see what going down
"*i refers the unary * operator" route, as a means of avoiding the self-
definition, we see that:
# [expr.unary.op]
#
# The unary * operator performs indirection: … the result is an lvalue
Nothing here requires the pointer to be valid, just that "*" gives you an
lvaule. Nothing more, nothing else. That's it. Whether the pointer is valid,
this gets punted only when the lvalue-to-rvalue conversion take place:
# [conv.lval]
#
# ... if the object to which the glvalue refers contains an invalid pointer
# value the behavior is implementation-defined.
But if you immediately apply the & operator, this conversion never takes
place.
The C standard is even more explicit, and even blesses this
construct, verbatim:
# The unary & operator yields the address of its operand.
# [ … ]
#
# if the operand is the result of a [] operator, neither the & operator
# nor the unary * that is implied by the [] is evaluated and the result is as
# if the & operator were removed and the [] operator were changed to a +
# operator
I could not find some similar verbiage in the C++ standard, but based on all
of the above I have to conclude that this is …too late today, and I should
really get some sleep…
But not after rereading the specification for a vector itself, where I found
something I glossed over the first time:
# [vector]
#
# A vector satisfies all of the requirements of a container and … of a
# contiguous container.
The reference from "contiguous container" goes to:
# [container.requirements.general]
#
# A contiguous container is a container that supports random access iterators
# and whose member types iterator and const_iterator are contiguous iterators.
The reference from "contiguous iterators" goes to:
# [iterator.requirements.general]
#
# Iterators that further satisfy the requirement that, for integral values
# n and dereferenceable iterator values a and (a + n), *(a + n) is
# equivalent to *(addressof(*a) + n), are called contiguous iterators.
There might be more to the "contiguous" semantics that reflects on whether
or not "&foo[foo.size()]" is defined behavior, or not, but here
addressof(*a) already puts us squarely in pointer equivalence territory, and
seems to again go into the unary "*" operator direction here, despite the
conflicting wording.
…now it's definitely too late in the day.
12:51 a.m.
On Sat, Aug 03, 2019 at 07:29:34PM -0400, Sam Varshavchik wrote:
I believe that the following construct trips this assertion:
# std::vector<int> foo;
#
# std::vector<int> bar;
#
# // Populate foo with something.
#
# std::copy(&foo[0], &foo[foo.size()], std::back_insert_iterator{bar});
It's true that something like
copy(&foo[0], &foo[foo.size()], back_inserter(bar));
triggers an assertion when compiled with -D_GLIBCXX_ASSERTIONS. Because
the checking code just can't look at the context of its invocation.
Which is fine, because you really don't need to use such constructs.
There's nothing wrong with this. There is no out of bounds
access. I do not
believe that this is undefined behavior. The defined semantics of
std::vector, and its operator[], are well defined here.
It's defined behaviour but there is something wrong with it: it isn't
idiomatic C++ and it's tedious and error-prone.
I ran into these new assertions with my own code as well, after
updating to
F28 (where they were enabled by default the first time, IIRC, not sure why
this shows up only now, for that package).
I ended up tweaking my code to avoid the assertions, rather than disabling
them. For this particular situation, my original change was to try
std::copy(&foo[0], &foo[0]+foo.size(), std::back_insert_iterator{bar});
But that still tripped the assertion when the foo vector was empty, so I had
wrap this inside an if().
But why don't you use the idiomatic
copy(foo.begin(), foo.end(), back_inserter(bar));
?
No need to wrap it into an extra if statement.
With GCC, the generated code is basically the same as when using:
copy(foo.data(), foo.data()+foo.size(), back_inserter(bar));
vector::data() is available since C++11. This doesn't trigger
any assertion and works with an empty source vector, as well.
What also works in general and doesn't trigger any assertion is the
'ultra ugly':
copy(&*foo.begin(), &*foo.end(), back_inserter(bar));
But there is also really no need to use an back_insert_iterator here - a
direct solution:
bar.insert(bar.end(), foo.begin(), foo.end());
Best regards
Georg
8:14 a.m.
Georg Sauthoff writes:
> I ended up tweaking my code to avoid the assertions, rather than
disabling
> them. For this particular situation, my original change was to try
>
> std::copy(&foo[0], &foo[0]+foo.size(), std::back_insert_iterator{bar});
>
> But that still tripped the assertion when the foo vector was empty, so I
had
> wrap this inside an if().
But why don't you use the idiomatic
copy(foo.begin(), foo.end(), back_inserter(bar));
?
No need to wrap it into an extra if statement.
I'm well aware of the alternatives. That's not the point.
The point is that there's nothing wrong with this specific form of existing
code that now throws exceptions when the hardened build gets turned on.
There is no buffer overruns, and nothing to exploit.
IIRC, the hardened build did find one real bug in the upstream package that
was the original subject matter here, but all of the rest were these kinds
of false positives. Now you have a situation when the existing code is known
to be working, but needs changing in order to use a hardened build. There's
some level of risk of regression in any change, and that gets weighed
against the benefits of having a hardened build.
The above was just a basic example of this. It is not true that exceptions
from hardened code are always evidence of potentially exploitable problem.
Sometimes/most of the time, but sometimes they are false positives.
12:29 p.m.
On Sun, 4 Aug 2019 at 16:21, Sam Varshavchik <mrsam(a)courier-mta.com> wrote:
I'm well aware of the alternatives. That's not the point.
The point is that there's nothing wrong with this specific form of existing
code that now throws exceptions when the hardened build gets turned on.
There is no buffer overruns, and nothing to exploit.
IIRC, the hardened build did find one real bug in the upstream package that
was the original subject matter here, but all of the rest were these kinds
of false positives. Now you have a situation when the existing code is known
to be working, but needs changing in order to use a hardened build. There's
some level of risk of regression in any change, and that gets weighed
against the benefits of having a hardened build.
The above was just a basic example of this. It is not true that exceptions
from hardened code are always evidence of potentially exploitable problem.
Sometimes/most of the time, but sometimes they are false positives.
Georg's point (please, correct me if I'm wrong) is: how many false
positives are there (I'd say, very few) and how many of them leave us
with no reasonable (and even more idiomatic, as in your basic example)
options (I'd say, very very few to none) compared to the protection
these assertions provide with a very small performance cost?
I do think that there's something wrong with unnecessarily complex and
non-idiomatic constructs when there are reasonable, fast, idiomatic
alternatives, even if the code throws no exceptions and is technically
correct. I do think there's something wrong if you need to navigate
various C++ standards to understand the container implementation at a
low level to be sure that memory is contiguous and the code is doing
the correct thing.
But anyway, this turned into an off-topic. This flag is not an issue
for KiCad, and it wasn't a false positive. The issue was a critical
bug uncovered thanks to this flag.
Iñaki
1:53 a.m.
On Fri, 2019-08-02 at 19:13 +0200, Björn 'besser82' Esser wrote:
Am Donnerstag, den 01.08.2019, 14:28 -0400 schrieb Steven A. Falco:
> The upstream KiCAD project has requested that I remove
> GLIBCXX_ASSERTIONS from the Fedora package, as described here:
> https://bugs.launchpad.net/kicad/+bug/1838448
>
> What is the best way to do that? I can add "%undefine
> _hardened_build" (which I am testing now) but I think that will remove
> other hardening features that I might want to leave enabled.
>
> Steve
Simply add this to your spec file:
# Upstream recommends to turn off _GLIBCXX_ASSERTIONS.
# See: $UPSTREAM_BUG
%global optflags %optflags -U_GLIBCXX_ASSERTIONS
Am I the only one worried of seeing way too many people jumping on and
telling how to suppress these hardening features without a word of
cautiousness ?
I feel like we should scan all spec files for this kind of stuff and
raise bugs if we find more than a handful of packages with this kind of
"workarounds" ...
Simo.
6:45 a.m.
On Mon, Aug 5, 2019 at 3:49 AM Simo Sorce <simo(a)redhat.com> wrote:
On Fri, 2019-08-02 at 19:13 +0200, Björn 'besser82' Esser wrote:
> Am Donnerstag, den 01.08.2019, 14:28 -0400 schrieb Steven A. Falco:
> > The upstream KiCAD project has requested that I remove
> > GLIBCXX_ASSERTIONS from the Fedora package, as described here:
> > https://bugs.launchpad.net/kicad/+bug/1838448
> >
> > What is the best way to do that? I can add "%undefine
> > _hardened_build" (which I am testing now) but I think that will remove
> > other hardening features that I might want to leave enabled.
> >
> > Steve
>
> Simply add this to your spec file:
>
> # Upstream recommends to turn off _GLIBCXX_ASSERTIONS.
> # See: $UPSTREAM_BUG
> %global optflags %optflags -U_GLIBCXX_ASSERTIONS
Am I the only one worried of seeing way too many people jumping on and
telling how to suppress these hardening features without a word of
cautiousness ?
I feel like we should scan all spec files for this kind of stuff and
raise bugs if we find more than a handful of packages with this kind of
"workarounds" ...
Not many people are equipped to figure these out, and usually when
these new flags are introduced, they come with no help and no
information from the people introducing them.
The last time I had to deal with this, I didn't even know this was the
culprit initially, and I was stubborn and got upstream to help fix the
issue. Koschei logs of dependency shifts with successes and failures
helped me pinpoint the flaw.
My experience with these issues is why I've never proposed increasing
our default hardening. It's far too difficult to debug and the
compiler people are unable or unwilling to help in most circumstances.
Thankfully with *my* packages, I've gotten rid of most of filter-outs.
But my D language packages require me to filter out quite a bit since
our flags are enforced regardless of compiler, and currently all D
software uses LDC, not GDC. We should probably fix that, but it's not
the only language that forces us to deal with the "problem child" that
is LLVM and the Clang stack.
--
真実はいつも一つ！/ Always, there's only one truth!
10:22 a.m.
On Sun, Aug 4, 2019 at 6:16 AM Sam Varshavchik <mrsam(a)courier-mta.com> wrote:
Georg Sauthoff writes:
> > I ended up tweaking my code to avoid the assertions, rather than disabling
> > them. For this particular situation, my original change was to try
> >
> > std::copy(&foo[0], &foo[0]+foo.size(),
std::back_insert_iterator{bar});
> >
> > But that still tripped the assertion when the foo vector was empty, so I
> had
> > wrap this inside an if().
>
> But why don't you use the idiomatic
>
> copy(foo.begin(), foo.end(), back_inserter(bar));
>
> ?
>
> No need to wrap it into an extra if statement.
I'm well aware of the alternatives. That's not the point.
The point is that there's nothing wrong with this specific form of existing
code that now throws exceptions when the hardened build gets turned on.
There is no buffer overruns, and nothing to exploit.
IIRC, the hardened build did find one real bug in the upstream package that
was the original subject matter here, but all of the rest were these kinds
of false positives. Now you have a situation when the existing code is known
to be working, but needs changing in order to use a hardened build. There's
some level of risk of regression in any change, and that gets weighed
against the benefits of having a hardened build.
The above was just a basic example of this. It is not true that exceptions
from hardened code are always evidence of potentially exploitable problem.
Sometimes/most of the time, but sometimes they are false positives.
I filed an upstream bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91357
Depending on the resolution of that bug, I suggest that Fedora
consider dropping _GLIBCXX_ASSERTIONS from the default hardened build
options. IMO Fedora's default RPM build options should not cause
crashes on valid, if less-than-stylistically-great, code. I don't
think that package maintainers should need to update package source to
use C++ in a more polite way.
11:36 a.m.
On Mon, Aug 5, 2019 at 8:22 AM Andrew Lutomirski <luto(a)mit.edu> wrote:
On Sun, Aug 4, 2019 at 6:16 AM Sam Varshavchik <mrsam(a)courier-mta.com> wrote:
>
> Georg Sauthoff writes:
>
> > > I ended up tweaking my code to avoid the assertions, rather than
disabling
> > > them. For this particular situation, my original change was to try
> > >
> > > std::copy(&foo[0], &foo[0]+foo.size(),
std::back_insert_iterator{bar});
> > >
> > > But that still tripped the assertion when the foo vector was empty, so I
> > had
> > > wrap this inside an if().
> >
> > But why don't you use the idiomatic
> >
> > copy(foo.begin(), foo.end(), back_inserter(bar));
> >
> > ?
> >
> > No need to wrap it into an extra if statement.
>
> I'm well aware of the alternatives. That's not the point.
>
> The point is that there's nothing wrong with this specific form of existing
> code that now throws exceptions when the hardened build gets turned on.
> There is no buffer overruns, and nothing to exploit.
>
> IIRC, the hardened build did find one real bug in the upstream package that
> was the original subject matter here, but all of the rest were these kinds
> of false positives. Now you have a situation when the existing code is known
> to be working, but needs changing in order to use a hardened build. There's
> some level of risk of regression in any change, and that gets weighed
> against the benefits of having a hardened build.
>
> The above was just a basic example of this. It is not true that exceptions
> from hardened code are always evidence of potentially exploitable problem.
> Sometimes/most of the time, but sometimes they are false positives.
>
>
I filed an upstream bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91357
Depending on the resolution of that bug, I suggest that Fedora
consider dropping _GLIBCXX_ASSERTIONS from the default hardened build
options. IMO Fedora's default RPM build options should not cause
crashes on valid, if less-than-stylistically-great, code. I don't
think that package maintainers should need to update package source to
use C++ in a more polite way.
And the bug has spoken. v[v.size()] is undefined behavior. Don't do it!
11:48 a.m.
On Mon, 5 Aug 2019 at 18:23, Andrew Lutomirski <luto(a)mit.edu> wrote:
On Sun, Aug 4, 2019 at 6:16 AM Sam Varshavchik <mrsam(a)courier-mta.com> wrote:
>
> Georg Sauthoff writes:
>
> > > I ended up tweaking my code to avoid the assertions, rather than
disabling
> > > them. For this particular situation, my original change was to try
> > >
> > > std::copy(&foo[0], &foo[0]+foo.size(),
std::back_insert_iterator{bar});
> > >
> > > But that still tripped the assertion when the foo vector was empty, so I
> > had
> > > wrap this inside an if().
> >
> > But why don't you use the idiomatic
> >
> > copy(foo.begin(), foo.end(), back_inserter(bar));
> >
> > ?
> >
> > No need to wrap it into an extra if statement.
>
> I'm well aware of the alternatives. That's not the point.
>
> The point is that there's nothing wrong with this specific form of existing
> code that now throws exceptions when the hardened build gets turned on.
> There is no buffer overruns, and nothing to exploit.
>
> IIRC, the hardened build did find one real bug in the upstream package that
> was the original subject matter here, but all of the rest were these kinds
> of false positives. Now you have a situation when the existing code is known
> to be working, but needs changing in order to use a hardened build. There's
> some level of risk of regression in any change, and that gets weighed
> against the benefits of having a hardened build.
>
> The above was just a basic example of this. It is not true that exceptions
> from hardened code are always evidence of potentially exploitable problem.
> Sometimes/most of the time, but sometimes they are false positives.
>
>
I filed an upstream bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91357
Depending on the resolution of that bug, I suggest that Fedora
consider dropping _GLIBCXX_ASSERTIONS from the default hardened build
options. IMO Fedora's default RPM build options should not cause
crashes on valid, if less-than-stylistically-great, code. I don't
think that package maintainers should need to update package source to
use C++ in a more polite way.
Upstream response has been quick:
"It's UB, no question. The table of "Optional sequence container
operations" says a[n] is equivalent to *(a.begin() + n) so when
n==a.size() you dereference the past-the-end iterator, which is UB."
"The assertion is entirely correct. Calling operator[] with an invalid
value is UB, it doesn't matter what you do (or don't do) with the
result."
Iñaki
3:37 a.m.
On Mon, Aug 05, 2019 at 09:36:47AM -0700, Andrew Lutomirski wrote:
On Mon, Aug 5, 2019 at 8:22 AM Andrew Lutomirski <luto(a)mit.edu>
wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91357
>
> Depending on the resolution of that bug, I suggest that Fedora
> consider dropping _GLIBCXX_ASSERTIONS from the default hardened build
> options. IMO Fedora's default RPM build options should not cause
> crashes on valid, if less-than-stylistically-great, code. I don't
> think that package maintainers should need to update package source to
> use C++ in a more polite way.
And the bug has spoken. v[v.size()] is undefined behavior. Don't do it!
Ok, but does that mean the program has to abort? Could gcc do anything
dangerous here? If we were actually trying to catch undefined behavior
(e.g. with -fsanitize=undefined), I suspect Fedora wouldn't even boot
without a crash.
--
Miroslav Lichvar
4:08 a.m.
On 06/08/2019 09:37, Miroslav Lichvar wrote:
On Mon, Aug 05, 2019 at 09:36:47AM -0700, Andrew Lutomirski wrote:
> On Mon, Aug 5, 2019 at 8:22 AM Andrew Lutomirski <luto(a)mit.edu> wrote:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91357
>>
>> Depending on the resolution of that bug, I suggest that Fedora
>> consider dropping _GLIBCXX_ASSERTIONS from the default hardened build
>> options. IMO Fedora's default RPM build options should not cause
>> crashes on valid, if less-than-stylistically-great, code. I don't
>> think that package maintainers should need to update package source to
>> use C++ in a more polite way.
>
> And the bug has spoken. v[v.size()] is undefined behavior. Don't do it!
Ok, but does that mean the program has to abort? Could gcc do anything
dangerous here? If we were actually trying to catch undefined behavior
(e.g. with -fsanitize=undefined), I suspect Fedora wouldn't even boot
without a crash.
Well of course the program doesn't have to abort - we have chosen to
compile in a mode where it does.
We do that because by default this is invoking the nasal daemons clause
and the compiler is allowed to do absolutely anything it feels like.
Yes at the moment you might well find that in fact the compiler does
what people "expect" and, because it's only taking a reference, it never
tries to read the memory - that can change at any time however.
For an example see what happened when gcc got cleverer about assuming
that this won't be null in a method and started making optimisations
around that and things which had "worked" suddenly started crashing as
a result. Because they had been invoking undefined behaviour all along.
Obviously there is a trade off, which is why we don't compile everything
with sanitisers, because it would make them much slower - you should
absolutely build your code with them in development though and run all
your tests like that.
Presumably in this case the performance penalty was considered small
enough that it was worth building even production code with this mode
enabled.
Tom
--
Tom Hughes (tom(a)compton.nu)
http://compton.nu/
6:09 a.m.
Tom Hughes writes:
On 06/08/2019 09:37, Miroslav Lichvar wrote:
>
> Ok, but does that mean the program has to abort? Could gcc do anything
> dangerous here? If we were actually trying to catch undefined behavior
> (e.g. with -fsanitize=undefined), I suspect Fedora wouldn't even boot
> without a crash.
Well of course the program doesn't have to abort - we have chosen to
compile in a mode where it does.
We do that because by default this is invoking the nasal daemons clause
and the compiler is allowed to do absolutely anything it feels like.
Earlier in the thread I posted some conflicting references from the
standard, related to the additional requirements of contiguous containers,
and the specification of the "*" operator, that leaves some room open for
interpretation, here.
If you go by the formal specification of container and iterator
requirements, this is undefined behavior. But std::vector is also specified
as having "additional" requirements of a "contiguous container", whose
semantics are specified in terms of std::addressof behavior, and that rabbit
hole goes in a slightly different direction.
4:32 a.m.
New subject: Performance cost of GLIBCXX_ASSERTIONS? (Was: How do I remove GLIBCXX_ASSERTIONS?)
>>>> "TH" == Tom Hughes <tom(a)compton.nu>
writes:
TH> Presumably in this case the performance penalty was considered small
TH> enough that it was worth building even production code with this
TH> mode enabled.
I'd like to know if any performance analysis was done about this,
because the upstream of a package I help maintain (prusa-slicer) has
also indicated to me that they aren't in favor of compiling with
GLIBCXX_ASSERTIONS in code which is computationally expensive and not
security sensitive. (This after the assertion found an actual bug in
their code.) The only answer I could give was that any code
which can open downloaded files is "security sensitive" in some fashion,
and presumably the gcc/glibc developers have evaluated the performance
impact.
Is there any reference I could use to answer such questions?
- J<
4:53 a.m.
New subject: Performance cost of GLIBCXX_ASSERTIONS?
* Jason Tibbitts:
>>>>> "TH" == Tom Hughes
<tom(a)compton.nu> writes:
TH> Presumably in this case the performance penalty was considered small
TH> enough that it was worth building even production code with this
TH> mode enabled.
I'd like to know if any performance analysis was done about this,
because the upstream of a package I help maintain (prusa-slicer) has
also indicated to me that they aren't in favor of compiling with
GLIBCXX_ASSERTIONS in code which is computationally expensive and not
security sensitive. (This after the assertion found an actual bug in
their code.) The only answer I could give was that any code
which can open downloaded files is "security sensitive" in some fashion,
and presumably the gcc/glibc developers have evaluated the performance
impact.
Is there any reference I could use to answer such questions?
For loops which use the correct type for the index type
(size_t/std::vector<T>::size_type), the bounds check is optimized away
by the compiler. There is one SPEC2006 regression, but it uses the
wrong index type:
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85929>
Such 32-bit/64-bit mismatches are often bugs anyway and easily fixed in
the source code. This case is difficult to optimize for the compiler
because by using the wrong type, the behavior of the code is different
for large arrays.
For non-sequential access, the compiler cannot optimize away the bounds
check. Whether the additional comparison is visible in profiles depends
on many, many factors. I haven't seen it show up so far.
Thanks,
Florian
2:38 a.m.
On 03/08/19 19:29 -0400, Sam Varshavchik wrote:
Tom Hughes writes:
>But I think upstream is giving very bad advice...
>
>That define does not "add extra crashes" in the way that they
>seem to think - well I mean it does literally but those crashes
>are reports of program errors on their part.
>
>Specifically in this case they appear to be accessing a
>std::vector at an index beyond the end, so they are accessing
>memory that may not be allocated at all, and if it is does
>not belong to the vector in question. So the program is quite
>likely to crash there one day anyway, the extra assertion just
>makes sure it always does.
I believe that the following construct trips this assertion:
# std::vector<int> foo;
#
# std::vector<int> bar;
#
# // Populate foo with something.
#
# std::copy(&foo[0], &foo[foo.size()], std::back_insert_iterator{bar});
There's nothing wrong with this. There is no out of bounds access. I
do not believe that this is undefined behavior. The defined semantics
of std::vector, and its operator[], are well defined here.
The semantics are defined only when the index is < foo.size(), meaning
foo[foo.size()] is undefined.
I ran into these new assertions with my own code as well, after
updating to F28 (where they were enabled by default the first time,
IIRC, not sure why this shows up only now, for that package).
I ended up tweaking my code to avoid the assertions, rather than
disabling them. For this particular situation, my original change was
to try
std::copy(&foo[0], &foo[0]+foo.size(), std::back_insert_iterator{bar});
But that still tripped the assertion when the foo vector was empty, so
I had wrap this inside an if().
Or:
std::copy(foo.begin(), foo.end(), ...);
Or:
std::copy(foo.data(), foo.data()+foo.size(), ...);
3:28 a.m.
On 03/08/19 23:14 -0400, Sam Varshavchik wrote:
Andrew Lutomirski writes:
>On a cursory search of the standard, I couldn't find where it says
>what operator* on this type of iterator does at all, let alone whether
>it's valid for one-past-the-end iterators, but I'm pretty sure that
>your code is, indeed, wrong.
This finally piqued my curiosity enough to take some time to look into
this. That expression applied the "*" operator to a random access
iterator. So, going down the path:
# [random.access.iterators]
#
# A class or pointer type X satisfies the requirements of a random
access# iterator if, in addition to satisfying the requirements for
bidirectional# iterators, ...
In a similar fashion, a bidirectional iterator delegates some
requirements to a forward iterator.
The forward iterator delegates some of its requirements to an input iterator.
At the end of the road, in [input.iterators]:
# *a reference, convertible to T Requires: a is dereferenceable.
Then, finally, in [iterator.requirements.general]:
# Values of an iterator i for which the expression *i is defined are
called# dereferenceable.
This reads to me like a definition that circularly defines itself.
[input.iterators] says "*a requires that a is dereferencable". And
[iterator.requirements.general] says that something is dereferencable
if "*" for it is defined. That certainly clears that up…
Full disclosure: it's true that the next sentence is:
# The library never assumes that past-the-end values are dereferenceable.
But this only states that the library assumes that, it doesn't
authoritatively state that.
Because some past-the-end iterators are dereferenceable. The
past-the-end iterator for a sequence [a,b) which is subsequence of
[a,c) will be dereferenceable is b!=c.
But the end() iterator for a container is both past-the-end and
non-dereferenceable, because it's not a subsequence of some larger
sequence. It really is past the end and there is no object at that
position.
But going back to the previous point, if we also want to see what
going down "*i refers the unary * operator" route, as a means of
avoiding the self-definition, we see that:
# [expr.unary.op]
#
# The unary * operator performs indirection: … the result is an lvalue
That's irrelevant in this case. See [expr.pre] which says "Subclause
[expr.post] defines the effects of operators when applied to types for
which they have not been overloaded."
GCC's std::vector::iterator is not a pointer, it's a class with an
overloaded operator*, so its semantics are not required to be the same
as the built-in * on pointers.
Nothing here requires the pointer to be valid, just that "*"
gives you
an lvaule. Nothing more, nothing else. That's it. Whether the pointer
Wrong. An lvalue can only refer to a valid object.
is valid, this gets punted only when the lvalue-to-rvalue conversion
take place:
Nope ...
# [conv.lval]
#
# ... if the object to which the glvalue refers contains an invalid pointer
# value the behavior is implementation-defined.
No, that is not saying you can have an lvalue formed by dereferencing
an invalid pointer value. It's saying that applying the
lvalue-to-rvalue conversion to an invalid pointer value is
implementation defined. i.e.
void lvalue_to_rvalue_conv(int*);
int* ptr = new int(0);
delete ptr;
lvalue_to_rvalue_conv(ptr); // implementation defined
That wording was added by https://wg21.link/cwg616 and is irrelevant
here. Dereferencing invalid or null pointer values is not allowed in
C++.
But if you immediately apply the & operator, this conversion never
takes place.
True in C, not in C++.
The C standard is even more explicit, and even blesses this construct,
verbatim:
# The unary & operator yields the address of its operand.
# [ … ]
#
# if the operand is the result of a [] operator, neither the &
operator # nor the unary * that is implied by the [] is evaluated and
the result is as # if the & operator were removed and the [] operator
were changed to a + # operator
I could not find some similar verbiage in the C++ standard, but based
You can't find it because there is no such rule in C++. There is an
open issue asking whether it should be allowed, but it's been open
(and inactive) for many years. See https://wg21.link/cwg232
on all of the above I have to conclude that this is …too late today,
and I should really get some sleep…
Anyway, even if that issue was resolved, it would be irrelevant when
considering an overloaded operator*.
Applying & to the result of an overloaded operator* still has to
evaluate the overloaded operator* and then evaluate & (which might
also use an overloaded operator&). So any rule that said that &*p is a
no-op when p is a pointer would not apply when p is a class type, like
GCC's std::vector::iterator.
But not after rereading the specification for a vector itself, where I
found something I glossed over the first time:
# [vector]
#
# A vector satisfies all of the requirements of a container and … of a#
contiguous container.
The reference from "contiguous container" goes to:
# [container.requirements.general]
#
# A contiguous container is a container that supports random access iterators
# and whose member types iterator and const_iterator are contiguous iterators.
The reference from "contiguous iterators" goes to:
# [iterator.requirements.general]
#
# Iterators that further satisfy the requirement that, for integral values
# n and dereferenceable iterator values a and (a + n), *(a + n) is
# equivalent to *(addressof(*a) + n), are called contiguous iterators.
If n is foo.size() then (a + n) is not a dereferenceable iterator, so
the *(addressof(*a) + n) equivalence doesn't hold. Because *a is
undefined.
There might be more to the "contiguous" semantics that
reflects on
whether or not "&foo[foo.size()]" is defined behavior, or not, but
There isn't.
here addressof(*a) already puts us squarely in pointer equivalence
territory, and seems to again go into the unary "*" operator direction
Nope, it does whatever std::vector::iterator::operator* does. Which is
not necessarily the same as the built-in unary * for pointers.
here, despite the conflicting wording.
It's undefined. GCC's assertion is correct. Fix the code.
>…now it's definitely too late in the day.
1694
days inactive
1727
days old
32 comments
19 participants
participants (19)
-
Andrew Lutomirski -
Björn 'besser82' Esser -
Florian Weimer -
Georg Sauthoff -
Iñaki Ucar -
Jason Tibbitts -
Jonathan Wakely -
Kevin Kofler -
mcatanzaro＠gnome.org -
Michal Schorm -
Miroslav Lichvar -
Neal Gompa -
Sam Varshavchik -
Simo Sorce -
Steven A. Falco -
Susi Lehtola -
Tom Callaway -
Tom Hughes -
Vitaly Zaitsev