| Message ID | 4c480186-d9eb-9749-88aa-6a9a6e5eec3e@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [_GLIBCXX_DEBUG] Improve valid_range check | expand |
On 22/11/19 18:38 +0100, François Dumont wrote: >Hi > > I noticed that we are not checking that iterators are not singular >in valid_range. Moreover __check_singular signature for pointers is >not intercepting all kind of pointers in terms of qualification. > > I'd like to commit it next week but considering we are in stage 3 >I need proper acceptance. > > * include/debug/functions.h: Remove <bits/move.h> include. > (__check_singular_aux, __check_singular): Move... > * include/debug/helper_functions.h: > (__check_singular_aux, __check_singular): ...here. > (__valid_range_aux): Adapt to use latter. > * testsuite/25_algorithms/copy/debug/2_neg.cc: New. > >Tested under Linux x86_64 normal and debug modes. OK for trunk, thanks.
On 22/11/2019 18:59, Jonathan Wakely wrote: > On 22/11/19 18:38 +0100, François Dumont wrote: >> I noticed that we are not checking that iterators are not singular >> in valid_range. Moreover __check_singular signature for pointers is >> not intercepting all kind of pointers in terms of qualification. >> >> I'd like to commit it next week but considering we are in stage 3 >> I need proper acceptance. >> >> * include/debug/functions.h: Remove <bits/move.h> include. >> (__check_singular_aux, __check_singular): Move... >> * include/debug/helper_functions.h: >> (__check_singular_aux, __check_singular): ...here. >> (__valid_range_aux): Adapt to use latter. >> * testsuite/25_algorithms/copy/debug/2_neg.cc: New. >> >> Tested under Linux x86_64 normal and debug modes. > > OK for trunk, thanks. The curly braces... > diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h > index c3e7478f649..5a858754875 100644 > --- a/libstdc++-v3/include/debug/helper_functions.h > +++ b/libstdc++-v3/include/debug/helper_functions.h [...] > @@ -138,14 +156,23 @@ namespace __gnu_debug > inline bool > __valid_range_aux(_InputIterator __first, _InputIterator __last, > std::input_iterator_tag) > - { return true; } > + { > + if (__first != __last) > + return !__check_singular(__first) && !__check_singular(__last); > + > + return true; > + } > > template<typename _InputIterator> > _GLIBCXX_CONSTEXPR > inline bool > __valid_range_aux(_InputIterator __first, _InputIterator __last, > std::random_access_iterator_tag) > - { return __first <= __last; } > + { > + return > + __valid_range_aux(__first, __last, std::input_iterator_tag{}) ...^^^ here... > + && __first <= __last; > + } > > /** We have iterators, so figure out what kind of iterators they are > * to see if we can check the range ahead of time. > @@ -167,6 +194,9 @@ namespace __gnu_debug > typename _Distance_traits<_InputIterator>::__type& __dist, > std::__false_type) > { > + if (!__valid_range_aux(__first, __last, std::input_iterator_tag{})) ...and ^^^ here are not allowed pre C++11. Replacing those with std::input_iterator_tag() should fix it. > + return false; > + > __dist = __get_distance(__first, __last); > switch (__dist.second) > {
On 26/11/19 12:33 +0100, Stephan Bergmann wrote: >On 22/11/2019 18:59, Jonathan Wakely wrote: >>On 22/11/19 18:38 +0100, François Dumont wrote: >>> I noticed that we are not checking that iterators are not >>>singular in valid_range. Moreover __check_singular signature for >>>pointers is not intercepting all kind of pointers in terms of >>>qualification. >>> >>> I'd like to commit it next week but considering we are in >>>stage 3 I need proper acceptance. >>> >>> * include/debug/functions.h: Remove <bits/move.h> include. >>> (__check_singular_aux, __check_singular): Move... >>> * include/debug/helper_functions.h: >>> (__check_singular_aux, __check_singular): ...here. >>> (__valid_range_aux): Adapt to use latter. >>> * testsuite/25_algorithms/copy/debug/2_neg.cc: New. >>> >>>Tested under Linux x86_64 normal and debug modes. >> >>OK for trunk, thanks. > >The curly braces... > >>diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h >>index c3e7478f649..5a858754875 100644 >>--- a/libstdc++-v3/include/debug/helper_functions.h >>+++ b/libstdc++-v3/include/debug/helper_functions.h >[...] >>@@ -138,14 +156,23 @@ namespace __gnu_debug >> inline bool >> __valid_range_aux(_InputIterator __first, _InputIterator __last, >> std::input_iterator_tag) >>- { return true; } >>+ { >>+ if (__first != __last) >>+ return !__check_singular(__first) && !__check_singular(__last); >>+ >>+ return true; >>+ } >> template<typename _InputIterator> >> _GLIBCXX_CONSTEXPR >> inline bool >> __valid_range_aux(_InputIterator __first, _InputIterator __last, >> std::random_access_iterator_tag) >>- { return __first <= __last; } >>+ { >>+ return >>+ __valid_range_aux(__first, __last, std::input_iterator_tag{}) > >...^^^ here... > >>+ && __first <= __last; >>+ } >> /** We have iterators, so figure out what kind of iterators they are >> * to see if we can check the range ahead of time. >>@@ -167,6 +194,9 @@ namespace __gnu_debug >> typename _Distance_traits<_InputIterator>::__type& __dist, >> std::__false_type) >> { >>+ if (!__valid_range_aux(__first, __last, std::input_iterator_tag{})) > >...and ^^^ here are not allowed pre C++11. Replacing those with > > std::input_iterator_tag() > >should fix it. Indeed. We should also have tests that use "-std=gnu++98 -D_GLIBCXX_DEBUG" so they'd catch this. François, can you take care of the fix please?
On 11/26/19 1:21 PM, Jonathan Wakely wrote: > On 26/11/19 12:33 +0100, Stephan Bergmann wrote: >> On 22/11/2019 18:59, Jonathan Wakely wrote: >>> On 22/11/19 18:38 +0100, François Dumont wrote: >>>> I noticed that we are not checking that iterators are not >>>> singular in valid_range. Moreover __check_singular signature for >>>> pointers is not intercepting all kind of pointers in terms of >>>> qualification. >>>> >>>> I'd like to commit it next week but considering we are in stage >>>> 3 I need proper acceptance. >>>> >>>> * include/debug/functions.h: Remove <bits/move.h> include. >>>> (__check_singular_aux, __check_singular): Move... >>>> * include/debug/helper_functions.h: >>>> (__check_singular_aux, __check_singular): ...here. >>>> (__valid_range_aux): Adapt to use latter. >>>> * testsuite/25_algorithms/copy/debug/2_neg.cc: New. >>>> >>>> Tested under Linux x86_64 normal and debug modes. >>> >>> OK for trunk, thanks. >> >> The curly braces... >> >>> diff --git a/libstdc++-v3/include/debug/helper_functions.h >>> b/libstdc++-v3/include/debug/helper_functions.h >>> index c3e7478f649..5a858754875 100644 >>> --- a/libstdc++-v3/include/debug/helper_functions.h >>> +++ b/libstdc++-v3/include/debug/helper_functions.h >> [...] >>> @@ -138,14 +156,23 @@ namespace __gnu_debug >>> inline bool >>> __valid_range_aux(_InputIterator __first, _InputIterator __last, >>> std::input_iterator_tag) >>> - { return true; } >>> + { >>> + if (__first != __last) >>> + return !__check_singular(__first) && !__check_singular(__last); >>> + >>> + return true; >>> + } >>> template<typename _InputIterator> >>> _GLIBCXX_CONSTEXPR >>> inline bool >>> __valid_range_aux(_InputIterator __first, _InputIterator __last, >>> std::random_access_iterator_tag) >>> - { return __first <= __last; } >>> + { >>> + return >>> + __valid_range_aux(__first, __last, std::input_iterator_tag{}) >> >> ...^^^ here... >> >>> + && __first <= __last; >>> + } >>> /** We have iterators, so figure out what kind of iterators they are >>> * to see if we can check the range ahead of time. >>> @@ -167,6 +194,9 @@ namespace __gnu_debug >>> typename _Distance_traits<_InputIterator>::__type& >>> __dist, >>> std::__false_type) >>> { >>> + if (!__valid_range_aux(__first, __last, >>> std::input_iterator_tag{})) >> >> ...and ^^^ here are not allowed pre C++11. Replacing those with >> >> std::input_iterator_tag() >> >> should fix it. > > Indeed. We should also have tests that use "-std=gnu++98 > -D_GLIBCXX_DEBUG" so they'd catch this. > > François, can you take care of the fix please? > > > Sure, I am about to do so. However I wasn't sure about this syntax before the commit so I had run the new 2_neg.cc with: make CXXFLAGS=-std=c++98 check-debug and it worked fine and still is ! I also try -std=gnu++98 and made sure that pch had been updated by re-building libstdc++ first. I just can't get the expected compilation error. Maybe I need to rebuild the whole stuff to get an error... Sorry
On 26/11/2019 20:07, François Dumont wrote: > However I wasn't sure about this syntax before the commit so I had run > the new 2_neg.cc with: > > make CXXFLAGS=-std=c++98 check-debug > > and it worked fine and still is ! > > I also try -std=gnu++98 and made sure that pch had been updated by > re-building libstdc++ first. I just can't get the expected compilation > error. > > Maybe I need to rebuild the whole stuff to get an error... I saw the error with recent Clang and -std=gnu++98. Recent GCC indeed seems to give at most a warning.
On 11/26/19 9:49 PM, Stephan Bergmann wrote: > On 26/11/2019 20:07, François Dumont wrote: >> However I wasn't sure about this syntax before the commit so I had >> run the new 2_neg.cc with: >> >> make CXXFLAGS=-std=c++98 check-debug >> >> and it worked fine and still is ! >> >> I also try -std=gnu++98 and made sure that pch had been updated by >> re-building libstdc++ first. I just can't get the expected >> compilation error. >> >> Maybe I need to rebuild the whole stuff to get an error... > > I saw the error with recent Clang and -std=gnu++98. Recent GCC indeed > seems to give at most a warning. > > Ok, thanks for the feedback, gcc might be more permissive then. Whatever I committed the fix an hour ago.
On 26/11/19 20:07 +0100, François Dumont wrote: >On 11/26/19 1:21 PM, Jonathan Wakely wrote: >>On 26/11/19 12:33 +0100, Stephan Bergmann wrote: >>>On 22/11/2019 18:59, Jonathan Wakely wrote: >>>>On 22/11/19 18:38 +0100, François Dumont wrote: >>>>> I noticed that we are not checking that iterators are not >>>>>singular in valid_range. Moreover __check_singular signature >>>>>for pointers is not intercepting all kind of pointers in terms >>>>>of qualification. >>>>> >>>>> I'd like to commit it next week but considering we are in >>>>>stage 3 I need proper acceptance. >>>>> >>>>> * include/debug/functions.h: Remove <bits/move.h> include. >>>>> (__check_singular_aux, __check_singular): Move... >>>>> * include/debug/helper_functions.h: >>>>> (__check_singular_aux, __check_singular): ...here. >>>>> (__valid_range_aux): Adapt to use latter. >>>>> * testsuite/25_algorithms/copy/debug/2_neg.cc: New. >>>>> >>>>>Tested under Linux x86_64 normal and debug modes. >>>> >>>>OK for trunk, thanks. >>> >>>The curly braces... >>> >>>>diff --git a/libstdc++-v3/include/debug/helper_functions.h >>>>b/libstdc++-v3/include/debug/helper_functions.h >>>>index c3e7478f649..5a858754875 100644 >>>>--- a/libstdc++-v3/include/debug/helper_functions.h >>>>+++ b/libstdc++-v3/include/debug/helper_functions.h >>>[...] >>>>@@ -138,14 +156,23 @@ namespace __gnu_debug >>>> inline bool >>>> __valid_range_aux(_InputIterator __first, _InputIterator __last, >>>> std::input_iterator_tag) >>>>- { return true; } >>>>+ { >>>>+ if (__first != __last) >>>>+ return !__check_singular(__first) && !__check_singular(__last); >>>>+ >>>>+ return true; >>>>+ } >>>> template<typename _InputIterator> >>>> _GLIBCXX_CONSTEXPR >>>> inline bool >>>> __valid_range_aux(_InputIterator __first, _InputIterator __last, >>>> std::random_access_iterator_tag) >>>>- { return __first <= __last; } >>>>+ { >>>>+ return >>>>+ __valid_range_aux(__first, __last, std::input_iterator_tag{}) >>> >>>...^^^ here... >>> >>>>+ && __first <= __last; >>>>+ } >>>> /** We have iterators, so figure out what kind of iterators they are >>>> * to see if we can check the range ahead of time. >>>>@@ -167,6 +194,9 @@ namespace __gnu_debug >>>> typename _Distance_traits<_InputIterator>::__type& >>>>__dist, >>>> std::__false_type) >>>> { >>>>+ if (!__valid_range_aux(__first, __last, >>>>std::input_iterator_tag{})) >>> >>>...and ^^^ here are not allowed pre C++11. Replacing those with >>> >>> std::input_iterator_tag() >>> >>>should fix it. >> >>Indeed. We should also have tests that use "-std=gnu++98 >>-D_GLIBCXX_DEBUG" so they'd catch this. >> >>François, can you take care of the fix please? >> >> >> >Sure, I am about to do so. > >However I wasn't sure about this syntax before the commit so I had run >the new 2_neg.cc with: > >make CXXFLAGS=-std=c++98 check-debug > >and it worked fine and still is ! The testsuite doesn't use CXXFLAGS. >I also try -std=gnu++98 and made sure that pch had been updated by >re-building libstdc++ first. I just can't get the expected compilation >error. > >Maybe I need to rebuild the whole stuff to get an error... No, you need to pass the right flags so they are used by the testsuite. This will do it: make -C testsuite/ check-debug debug_flags=unix/-D_GLIBCXX_DEBUG/-std=gnu++98/-Wsystem-headers But since it only shows up with -Wsystem-headers, there's no point trying to test for it.
On 11/26/19 10:52 PM, Jonathan Wakely wrote: > On 26/11/19 20:07 +0100, François Dumont wrote: >> Sure, I am about to do so. >> >> However I wasn't sure about this syntax before the commit so I had >> run the new 2_neg.cc with: >> >> make CXXFLAGS=-std=c++98 check-debug >> >> and it worked fine and still is ! > > The testsuite doesn't use CXXFLAGS. > Did you mean CPPFLAGS ? I do see the option added to the compiler call in libstdc++.log file. And I used it to build for instance pretty-printers tests in _GLIBCXX_DEBUG mode with success. > >> I also try -std=gnu++98 and made sure that pch had been updated by >> re-building libstdc++ first. I just can't get the expected >> compilation error. >> >> Maybe I need to rebuild the whole stuff to get an error... > > No, you need to pass the right flags so they are used by the > testsuite. This will do it: > > make -C testsuite/ check-debug > debug_flags=unix/-D_GLIBCXX_DEBUG/-std=gnu++98/-Wsystem-headers I'll have to keep your mail to remember it ! > > But since it only shows up with -Wsystem-headers, there's no point > trying to test for it. > > Ok, makes sens. Thanks
On 27/11/19 06:37 +0100, François Dumont wrote: >On 11/26/19 10:52 PM, Jonathan Wakely wrote: >>On 26/11/19 20:07 +0100, François Dumont wrote: >>>Sure, I am about to do so. >>> >>>However I wasn't sure about this syntax before the commit so I had >>>run the new 2_neg.cc with: >>> >>>make CXXFLAGS=-std=c++98 check-debug >>> >>>and it worked fine and still is ! >> >>The testsuite doesn't use CXXFLAGS. >> >Did you mean CPPFLAGS ? No, I mean CXXFLAGS. >I do see the option added to the compiler call in libstdc++.log file. >And I used it to build for instance pretty-printers tests in >_GLIBCXX_DEBUG mode with success. Ah, it works if you run it *inside* the $target/libstdc++-v3/testsuite directory. I usually run the tests from the parent directory. >> >>>I also try -std=gnu++98 and made sure that pch had been updated by >>>re-building libstdc++ first. I just can't get the expected >>>compilation error. >>> >>>Maybe I need to rebuild the whole stuff to get an error... >> >>No, you need to pass the right flags so they are used by the >>testsuite. This will do it: >> >>make -C testsuite/ check-debug >>debug_flags=unix/-D_GLIBCXX_DEBUG/-std=gnu++98/-Wsystem-headers >I'll have to keep your mail to remember it ! Just look in testsuite/Makefile to see what variables are used by the check-debug target. That's what determines the flags used, not my email! :-) >>But since it only shows up with -Wsystem-headers, there's no point >>trying to test for it. >> >> >Ok, makes sens. > >Thanks >
diff --git a/libstdc++-v3/include/debug/functions.h b/libstdc++-v3/include/debug/functions.h index 8c385b87244..12df745b573 100644 --- a/libstdc++-v3/include/debug/functions.h +++ b/libstdc++-v3/include/debug/functions.h @@ -29,7 +29,6 @@ #ifndef _GLIBCXX_DEBUG_FUNCTIONS_H #define _GLIBCXX_DEBUG_FUNCTIONS_H 1 -#include <bits/move.h> // for __addressof #include <bits/stl_function.h> // for less #if __cplusplus >= 201103L @@ -49,23 +48,6 @@ namespace __gnu_debug template<typename _Sequence> struct _Is_contiguous_sequence : std::__false_type { }; - // An arbitrary iterator pointer is not singular. - inline bool - __check_singular_aux(const void*) { return false; } - - // We may have an iterator that derives from _Safe_iterator_base but isn't - // a _Safe_iterator. - template<typename _Iterator> - inline bool - __check_singular(const _Iterator& __x) - { return __check_singular_aux(std::__addressof(__x)); } - - /** Non-NULL pointers are nonsingular. */ - template<typename _Tp> - inline bool - __check_singular(const _Tp* __ptr) - { return __ptr == 0; } - /* Checks that [first, last) is a valid range, and then returns * __first. This routine is useful when we can't use a separate * assertion statement because, e.g., we are in a constructor. diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h index c3e7478f649..5a858754875 100644 --- a/libstdc++-v3/include/debug/helper_functions.h +++ b/libstdc++-v3/include/debug/helper_functions.h @@ -29,6 +29,7 @@ #ifndef _GLIBCXX_DEBUG_HELPER_FUNCTIONS_H #define _GLIBCXX_DEBUG_HELPER_FUNCTIONS_H 1 +#include <bits/move.h> // for __addressof #include <bits/stl_iterator_base_types.h> // for iterator_traits, // categories and _Iter_base #include <bits/cpp_type_traits.h> // for __is_integer @@ -112,6 +113,23 @@ namespace __gnu_debug __get_distance(_Iterator __lhs, _Iterator __rhs) { return __get_distance(__lhs, __rhs, std::__iterator_category(__lhs)); } + // An arbitrary iterator pointer is not singular. + inline bool + __check_singular_aux(const void*) { return false; } + + // We may have an iterator that derives from _Safe_iterator_base but isn't + // a _Safe_iterator. + template<typename _Iterator> + inline bool + __check_singular(_Iterator const& __x) + { return __check_singular_aux(std::__addressof(__x)); } + + /** Non-NULL pointers are nonsingular. */ + template<typename _Tp> + inline bool + __check_singular(_Tp* const& __ptr) + { return __ptr == 0; } + /** We say that integral types for a valid range, and defer to other * routines to realize what to do with integral types instead of * iterators. @@ -138,14 +156,23 @@ namespace __gnu_debug inline bool __valid_range_aux(_InputIterator __first, _InputIterator __last, std::input_iterator_tag) - { return true; } + { + if (__first != __last) + return !__check_singular(__first) && !__check_singular(__last); + + return true; + } template<typename _InputIterator> _GLIBCXX_CONSTEXPR inline bool __valid_range_aux(_InputIterator __first, _InputIterator __last, std::random_access_iterator_tag) - { return __first <= __last; } + { + return + __valid_range_aux(__first, __last, std::input_iterator_tag{}) + && __first <= __last; + } /** We have iterators, so figure out what kind of iterators they are * to see if we can check the range ahead of time. @@ -167,6 +194,9 @@ namespace __gnu_debug typename _Distance_traits<_InputIterator>::__type& __dist, std::__false_type) { + if (!__valid_range_aux(__first, __last, std::input_iterator_tag{})) + return false; + __dist = __get_distance(__first, __last); switch (__dist.second) { diff --git a/libstdc++-v3/testsuite/25_algorithms/copy/debug/2_neg.cc b/libstdc++-v3/testsuite/25_algorithms/copy/debug/2_neg.cc new file mode 100644 index 00000000000..8bbf873de96 --- /dev/null +++ b/libstdc++-v3/testsuite/25_algorithms/copy/debug/2_neg.cc @@ -0,0 +1,37 @@ +// Copyright (C) 2019 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/>. + +// 25.2.1 [lib.alg.copy] Copy. + +// { dg-do run { xfail *-*-* } } +// { dg-require-debug-mode "" } + +#include <algorithm> + +void +test01() +{ + int arr[] = { 0, 1, 2, 3, 4 }; + std::copy((int*)0, arr + 5, arr); +} + +int +main() +{ + test01(); + return 0; +}