| Message ID | 4FC8A0DE.7030104@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 01, 2012 at 01:00:46PM +0200, Florian Weimer wrote: > I forgot to send this to the libstdc++ list the first time. > > This patch evaluates _FORTIFY_SOURCE in a way similar to GNU libc. > If set, std::vector::operator[] throws if the index is out of bounds. > This is compliant with the standard because such usage triggers > undefined behavior. _FORTIFY_SOURCE users expect some performance hit. > > In contrast to debugging mode, this does not change ABI and is more > widely applicable. > > Okay for trunk? Have you looked at the assembly differences with this in? I wonder e.g. if we don't need to add __builtin_expect around the _M_range_check test. As it needs to read in many cases two pointers, subtract them (+ divide by size of entry (or shift right if power of two)) and compare that to something likely in a register, it is likely to be more expensive than most of other _FORTIFY_SOURCE checks, but perhaps bearable. Also, I wonder if the addition of a throw spot (even in cold .text chunk) doesn't add too much cost (code having to deal with possible EH even when otherwise it wouldn't be expected) and whether calling __chk_fail () in that case instead wouldn't be cheaper. Even in C++ memcpy etc. _FORTIFY_SOURCE failures will result in __chk_fail, not throwing some exception, so operator[] on std::vector could work the same. operator[] is documented as not doing the range check (unlike ->at()), so you probably need to tweak the documentation too. > 2012-05-29 Florian Weimer <fweimer@redhat.com> > > * include/bits/stl_vector.h (vector::_M_fortify_range_check): > New. > * (vector::operator[]): Call it. > * testsuite/23_containers/vector/element_access/2.cc: New. > > Index: libstdc++-v3/include/bits/stl_vector.h > =================================================================== > --- libstdc++-v3/include/bits/stl_vector.h (revision 187951) > +++ libstdc++-v3/include/bits/stl_vector.h (working copy) > @@ -768,7 +768,10 @@ > */ > reference > operator[](size_type __n) > - { return *(this->_M_impl._M_start + __n); } > + { > + _M_fortify_range_check(__n); > + return *(this->_M_impl._M_start + __n); > + } > > /** > * @brief Subscript access to the data contained in the %vector. > @@ -783,7 +786,10 @@ > */ > const_reference > operator[](size_type __n) const > - { return *(this->_M_impl._M_start + __n); } > + { > + _M_fortify_range_check(__n); > + return *(this->_M_impl._M_start + __n); > + } > > protected: > /// Safety check used only from at(). > @@ -794,6 +800,16 @@ > __throw_out_of_range(__N("vector::_M_range_check")); > } > > + /// Range check used by operator[]. > + /// No-op unless _FORTIFY_SOURCE is enabled. > + void > + _M_fortify_range_check(size_type __n) const > + { > +#if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0 > + _M_range_check(__n); > +#endif > + } > + > public: > /** > * @brief Provides access to the data contained in the %vector. Jakub
Hi
a bunch of minor comments, first:
1- What happens with make check-debug? I suppose the debug-mode checks
trigger before the check in normal code, and the testcase doesn't pass.
Maybe for this kind of work you need a // { dg-require-normal-mode "" }
2- Something sound weird with debug-mode which doesn't throw, just
errors out, and the new check in normal-mode which throws. If there are
other reasons to not throw, per Jakub's comment, I would support the idea.
2- Make sure to have the test variable in the testcases.
3- We definitely need a comment explaining *who* provided _M_range_check
4- Maybe I'm misremembering, sorry in case, but Isn't:
#if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0
the same as:
#if _FORTIFY_SOURCE > 0
?
Thanks,
Paolo.
On 06/01/2012 01:42 PM, Paolo Carlini wrote: > Hi > > a bunch of minor comments, first: > 1- What happens with make check-debug? I suppose the debug-mode checks > trigger before the check in normal code, and the testcase doesn't pass. > Maybe for this kind of work you need a // { dg-require-normal-mode "" } I'm now running "make check-debug", didn't know about it. > 2- Something sound weird with debug-mode which doesn't throw, just > errors out, and the new check in normal-mode which throws. If there are > other reasons to not throw, per Jakub's comment, I would support the idea. Yes, that's true. > 2- Make sure to have the test variable in the testcases. I've seen the variable, but I don't understand how to use it. I would like to add something about it to the testing documentation. > 3- We definitely need a comment explaining *who* provided _M_range_check Sorry, I don't understand this suggestion. > 4- Maybe I'm misremembering, sorry in case, but Isn't: > > #if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0 > > the same as: > > #if _FORTIFY_SOURCE > 0 > > ? I think you're right, but I'm just copying literally what GNU libc is doing. I can change it to the shorter version if you want me to.
Hi, On 06/01/2012 02:53 PM, Florian Weimer wrote: > I've seen the variable, but I don't understand how to use it. I would > like to add something about it to the testing documentation. Just copy what *all* the other testcase in the library testsuite have, just define it, with attribute unused. If you are curious why, just look inside testsuite_hooks.h > 3- We definitely need a comment explaining *who* provided _M_range_check > > Sorry, I don't understand this suggestion. You are right, sorry, I went through your changes too quickly and didn't realize that you are just using the existing _M_range_check. Anyway, I confirm that probably we want something more consistent with debug-mode, thus not throwing in this case. Maybe just a __builtin_abort(), I don't know. Or adjust the code in c++config to make available a non-trivial __glibcxx_assert also when _FORTIFY_SOURCE > 0. >> 4- Maybe I'm misremembering, sorry in case, but Isn't: >> >> #if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0 >> >> the same as: >> >> #if _FORTIFY_SOURCE > 0 >> >> ? > > I think you're right, but I'm just copying literally what GNU libc is > doing. I can change it to the shorter version if you want me to. I think we should yes. Paolo.
On 06/01/2012 03:09 PM, Paolo Carlini wrote: > You are right, sorry, I went through your changes too quickly and > didn't realize that you are just using the existing _M_range_check. > Anyway, I confirm that probably we want something more consistent with > debug-mode, thus not throwing in this case. Maybe just a > __builtin_abort(), I don't know. Or adjust the code in c++config to > make available a non-trivial __glibcxx_assert also when > _FORTIFY_SOURCE > 0. Then, if you do this, you probably don't need a new _M_fortify_range_check function. Paolo.
On 06/01/2012 02:09 PM, Paolo Carlini wrote: >>> 4- Maybe I'm misremembering, sorry in case, but Isn't: >>> >>> #if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0 >>> >>> the same as: >>> >>> #if _FORTIFY_SOURCE > 0 >>> >>> ? >> >> I think you're right, but I'm just copying literally what GNU libc is doing. I can change it to the shorter version if you want me to. > I think we should yes. The shorter version triggers an -Wundef warning if _FORTIFY_SOURCE is not defined.
On Fri, Jun 01, 2012 at 03:09:19PM +0200, Paolo Carlini wrote: > On 06/01/2012 02:53 PM, Florian Weimer wrote: > >I've seen the variable, but I don't understand how to use it. I > >would like to add something about it to the testing documentation. > Just copy what *all* the other testcase in the library testsuite > have, just define it, with attribute unused. If you are curious why, > just look inside testsuite_hooks.h > >3- We definitely need a comment explaining *who* provided _M_range_check > > > >Sorry, I don't understand this suggestion. > You are right, sorry, I went through your changes too quickly and > didn't realize that you are just using the existing _M_range_check. > Anyway, I confirm that probably we want something more consistent > with debug-mode, thus not throwing in this case. Maybe just a > __builtin_abort(), I don't know. Or adjust the code in c++config to > make available a non-trivial __glibcxx_assert also when > _FORTIFY_SOURCE > 0. The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO if this is presented as _FORTIFY_SOURCE check, it should call that and not some other function. You'd need to use #if __USE_FORTIFY_LEVEL > 0 test instead (as __chk_fail is only provided by glibcs that on _FORTIFY_SOURCE definition sets __USE_FORTIFY_LEVEL), but it would be consistent with all other fortification failures (and, even -fstack-protector failures are similar). Or of course if you want it to do something else on failures, better enable it using a different macro. Jakub
On 06/01/2012 03:22 PM, Pedro Alves wrote: > The shorter version triggers an -Wundef warning if _FORTIFY_SOURCE is > not defined. Interesting, but does it happen in system headers too? Paolo.
On 06/01/2012 03:34 PM, Jakub Jelinek wrote: > The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO if > this is presented as _FORTIFY_SOURCE check, it should call that and > not some other function. I understand. I don't know much about -D_FORTIFY_SOURCE, honestly. I hope the diagnostics provided by __chk_fail is good enough. And well, then we really do have to explain in a comment where __chk_fail is coming from ;) Paolo.
On Fri, Jun 01, 2012 at 03:39:12PM +0200, Paolo Carlini wrote: > On 06/01/2012 03:34 PM, Jakub Jelinek wrote: > >The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO > >if this is presented as _FORTIFY_SOURCE check, it should call that > >and not some other function. > I understand. I don't know much about -D_FORTIFY_SOURCE, honestly. I > hope the diagnostics provided by __chk_fail is good enough. And > well, then we really do have to explain in a comment where > __chk_fail is coming from ;) The default error output is like: *** buffer overflow detected ***: /tmp/a terminated ======= Backtrace: ========= /lib64/libc.so.6(__fortify_fail+0x37)[0x388e3097e7] /lib64/libc.so.6[0x388e3079a0] /tmp/a[0x40050a] /lib64/libc.so.6(__libc_start_main+0xf5)[0x388e221735] /tmp/a[0x400419] ======= Memory map: ======== 00400000-00401000 r-xp 00000000 08:02 147893 /tmp/a 00600000-00601000 rw-p 00000000 08:02 147893 /tmp/a 01b63000-01b84000 rw-p 00000000 00:00 0 [heap] 388de00000-388de20000 r-xp 00000000 08:02 1201564 /usr/lib64/ld-2.15.so 388e01f000-388e020000 r--p 0001f000 08:02 1201564 /usr/lib64/ld-2.15.so 388e020000-388e021000 rw-p 00020000 08:02 1201564 /usr/lib64/ld-2.15.so 388e021000-388e022000 rw-p 00000000 00:00 0 388e200000-388e3ac000 r-xp 00000000 08:02 1201757 /usr/lib64/libc-2.15.so 388e3ac000-388e5ac000 ---p 001ac000 08:02 1201757 /usr/lib64/libc-2.15.so 388e5ac000-388e5b0000 r--p 001ac000 08:02 1201757 /usr/lib64/libc-2.15.so 388e5b0000-388e5b2000 rw-p 001b0000 08:02 1201757 /usr/lib64/libc-2.15.so 388e5b2000-388e5b7000 rw-p 00000000 00:00 0 3891200000-3891215000 r-xp 00000000 08:02 1201763 /usr/lib64/libgcc_s-4.7.0-20120507.so.1 3891215000-3891414000 ---p 00015000 08:02 1201763 /usr/lib64/libgcc_s-4.7.0-20120507.so.1 3891414000-3891415000 rw-p 00014000 08:02 1201763 /usr/lib64/libgcc_s-4.7.0-20120507.so.1 7feee518f000-7feee5192000 rw-p 00000000 00:00 0 7feee51a5000-7feee51a7000 rw-p 00000000 00:00 0 7fff27a66000-7fff27a87000 rw-p 00000000 00:00 0 [stack] 7fff27bff000-7fff27c00000 r-xp 00000000 00:00 0 [vdso] ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall] Aborted (core dumped) Jakub
Hi, On 06/01/2012 03:49 PM, Jakub Jelinek wrote: > On Fri, Jun 01, 2012 at 03:39:12PM +0200, Paolo Carlini wrote: >> On 06/01/2012 03:34 PM, Jakub Jelinek wrote: >>> The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO >>> if this is presented as _FORTIFY_SOURCE check, it should call that >>> and not some other function. >> I understand. I don't know much about -D_FORTIFY_SOURCE, honestly. I >> hope the diagnostics provided by __chk_fail is good enough. And >> well, then we really do have to explain in a comment where >> __chk_fail is coming from ;) > The default error output is like: I understand it's some sort of "standard" thus certainly I'm not going to insist, but I would like to see more clearly displayed the line number of the library assertion failing, see what I mean? Thanks, Paolo.
On Fri, Jun 01, 2012 at 03:52:24PM +0200, Paolo Carlini wrote: > On 06/01/2012 03:49 PM, Jakub Jelinek wrote: > >On Fri, Jun 01, 2012 at 03:39:12PM +0200, Paolo Carlini wrote: > >>On 06/01/2012 03:34 PM, Jakub Jelinek wrote: > >>>The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO > >>>if this is presented as _FORTIFY_SOURCE check, it should call that > >>>and not some other function. > >>I understand. I don't know much about -D_FORTIFY_SOURCE, honestly. I > >>hope the diagnostics provided by __chk_fail is good enough. And > >>well, then we really do have to explain in a comment where > >>__chk_fail is coming from ;) > >The default error output is like: > I understand it's some sort of "standard" thus certainly I'm not > going to insist, but I would like to see more clearly displayed the > line number of the library assertion failing, see what I mean? Well, you have the core file often (unless disabled), or could use addr2line to decode the addresses. The point is that the fortification checks must be very lightweight (so that people can enable them by default for everything), and e.g. storing __LINE__ and __FILE__ everywhere where operator[] is used would be too expensive (grow .rodata too much). Jakub
On 06/01/2012 02:36 PM, Paolo Carlini wrote: > On 06/01/2012 03:22 PM, Pedro Alves wrote: >> The shorter version triggers an -Wundef warning if _FORTIFY_SOURCE is not defined. > Interesting, but does it happen in system headers too? Good point. I just tried and it doesn't.
On Fri, Jun 01, 2012 at 03:11:51PM +0100, Pedro Alves wrote: > On 06/01/2012 02:36 PM, Paolo Carlini wrote: > > > On 06/01/2012 03:22 PM, Pedro Alves wrote: > >> The shorter version triggers an -Wundef warning if _FORTIFY_SOURCE is not defined. > > Interesting, but does it happen in system headers too? > > > Good point. I just tried and it doesn't. But with -Wsystem-headers it does again. It doesn't hurt to add that defined ... && check to quiet it up. Jakub
On 06/01/2012 04:04 PM, Jakub Jelinek wrote: > Well, you have the core file often (unless disabled), or could use > addr2line to decode the addresses. The point is that the fortification > checks must be very lightweight (so that people can enable them by > default for everything), and e.g. storing __LINE__ and __FILE__ > everywhere where operator[] is used would be too expensive (grow > .rodata too much). I see. Then, I'm coming to the conclusion that if we start deploying such lightweight checks in the normal-mode library we should document the thing in the library documentation, under "debugging". Just a few sentences explaining what you and Florian just clarified, vs debug-mode too. Paolo.
On 06/01/2012 04:13 PM, Jakub Jelinek wrote: > On Fri, Jun 01, 2012 at 03:11:51PM +0100, Pedro Alves wrote: >> On 06/01/2012 02:36 PM, Paolo Carlini wrote: >> >>> On 06/01/2012 03:22 PM, Pedro Alves wrote: >>>> The shorter version triggers an -Wundef warning if _FORTIFY_SOURCE is not defined. >>> Interesting, but does it happen in system headers too? >> >> Good point. I just tried and it doesn't. > But with -Wsystem-headers it does again. ... together with another TON of warnings everywhere, yes ;) Paolo.
On 06/01/2012 03:34 PM, Jakub Jelinek wrote: > The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO > if this is presented as _FORTIFY_SOURCE check, it should call that > and not some other function. You'd need to use > #if __USE_FORTIFY_LEVEL> 0 > test instead (as __chk_fail is only provided by glibcs that on > _FORTIFY_SOURCE definition sets __USE_FORTIFY_LEVEL), but it would be > consistent with all other fortification failures (and, even > -fstack-protector failures are similar). __chk_fail it is, then. This means that the test case will be specific to GNU libc platforms. How can I mark it as such? > Or of course if you want it to do something else on failures, better > enable it using a different macro. I'm aiming for a consistent developer experience. There is little documentation for _FORTIFY_SOURCE, and we plan to change that. However, due to the way most additional checks are implemented (reliance upon __builtin_object_size in particular), it will always be magic you cannot rely on, which makes good documentation difficult. But we should at least explain that! (Obviously, the std::vector check doesn't share this problem.)
On 06/01/2012 05:00 PM, Florian Weimer wrote: > On 06/01/2012 03:34 PM, Jakub Jelinek wrote: >> The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO >> if this is presented as _FORTIFY_SOURCE check, it should call that >> and not some other function. You'd need to use >> #if __USE_FORTIFY_LEVEL> 0 >> test instead (as __chk_fail is only provided by glibcs that on >> _FORTIFY_SOURCE definition sets __USE_FORTIFY_LEVEL), but it would be >> consistent with all other fortification failures (and, even >> -fstack-protector failures are similar). > > __chk_fail it is, then. This means that the test case will be > specific to GNU libc platforms. How can I mark it as such? If nothing else comes to mind, you can always add a check_v3_target_glibc to libstdc++.exp and then a dg-require-glibc using it to dg-options.exp. Inside the former, just check that __GLIBC__ is defined, maybe with an appropriate value. Then you can use Paolo.
On Fri, Jun 01, 2012 at 05:00:58PM +0200, Florian Weimer wrote: > On 06/01/2012 03:34 PM, Jakub Jelinek wrote: > >The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO > >if this is presented as _FORTIFY_SOURCE check, it should call that > >and not some other function. You'd need to use > >#if __USE_FORTIFY_LEVEL> 0 > >test instead (as __chk_fail is only provided by glibcs that on > >_FORTIFY_SOURCE definition sets __USE_FORTIFY_LEVEL), but it would be > >consistent with all other fortification failures (and, even > >-fstack-protector failures are similar). > > __chk_fail it is, then. This means that the test case will be > specific to GNU libc platforms. How can I mark it as such? { target *-*-linux* } or so? > >Or of course if you want it to do something else on failures, better > >enable it using a different macro. > > I'm aiming for a consistent developer experience. For other platforms there is always libssp included with gcc, which can be used instead (users then have to link -lssp if they compile with -D_FORTIFY_SOURCE). Note, __chk_fail () isn't prototyped in glibc headers, so you want probably in the checking method declare it in some __gnu* namespace as extern "C" __chk_fail () __attribute__((unused)); and then use. Jakub
On 06/01/2012 05:09 PM, Jakub Jelinek wrote: >> __chk_fail it is, then. This means that the test case will be >> specific to GNU libc platforms. How can I mark it as such? > > { target *-*-linux* } or so? Wouldn't this match Bionic libc environments, too? > Note, __chk_fail () isn't prototyped in glibc headers, so you want > probably in the checking method declare it in some __gnu* namespace > as extern "C" __chk_fail () __attribute__((unused)); > and then use. Good point, thanks. I'm asking the libc folks if we may use this symbol from libstdc++, just to be on the safe side.
Index: libstdc++-v3/include/bits/stl_vector.h =================================================================== --- libstdc++-v3/include/bits/stl_vector.h (revision 187951) +++ libstdc++-v3/include/bits/stl_vector.h (working copy) @@ -768,7 +768,10 @@ */ reference operator[](size_type __n) - { return *(this->_M_impl._M_start + __n); } + { + _M_fortify_range_check(__n); + return *(this->_M_impl._M_start + __n); + } /** * @brief Subscript access to the data contained in the %vector. @@ -783,7 +786,10 @@ */ const_reference operator[](size_type __n) const - { return *(this->_M_impl._M_start + __n); } + { + _M_fortify_range_check(__n); + return *(this->_M_impl._M_start + __n); + } protected: /// Safety check used only from at(). @@ -794,6 +800,16 @@ __throw_out_of_range(__N("vector::_M_range_check")); } + /// Range check used by operator[]. + /// No-op unless _FORTIFY_SOURCE is enabled. + void + _M_fortify_range_check(size_type __n) const + { +#if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0 + _M_range_check(__n); +#endif + } + public: /** * @brief Provides access to the data contained in the %vector. Index: libstdc++-v3/testsuite/23_containers/vector/element_access/2.cc =================================================================== --- libstdc++-v3/testsuite/23_containers/vector/element_access/2.cc (revision 0) +++ libstdc++-v3/testsuite/23_containers/vector/element_access/2.cc (revision 0) @@ -0,0 +1,71 @@ +// Copyright (C) 2012 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// 23.2.4 vector + +// { dg-add-options no_pch } + +#undef _FORTIFY_SOURCE +#define _FORTIFY_SOURCE 2 + +#include <vector> +#include <stdexcept> +#include <testsuite_hooks.h> + +void test01() +{ + std::vector<int> v(5); + try + { + v[5]; + VERIFY( false ); + } + catch(std::out_of_range& err) + { + VERIFY( true ); + } + catch(...) + { + VERIFY( false ); + } +} + +void test02() +{ + std::vector<int> v(5); + const std::vector<int> u(v); + try + { + u[5]; + VERIFY( false ); + } + catch(std::out_of_range& err) + { + VERIFY( true ); + } + catch(...) + { + VERIFY( false ); + } +} + +int main() +{ + test01(); + test02(); + return 0; +}