From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x632.google.com (mail-ej1-x632.google.com [IPv6:2a00:1450:4864:20::632]) by sourceware.org (Postfix) with ESMTPS id F25A3388C039; Thu, 11 Mar 2021 17:51:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F25A3388C039 Received: by mail-ej1-x632.google.com with SMTP id c10so47959379ejx.9; Thu, 11 Mar 2021 09:51:10 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=xOT1/tYwB4gbMfyiGzRuVOPk34neeO/6gs2lQLKDjl0=; b=SjC1Yoq3Z+SyBZEFtO/Kwz6WmVOl/Q4fIG6mXqlcsHfYwc4svBsZ1CckYyqBCQcbyc 5XEpmHRQuk3RLebAmFKlBjpr3IxP4DZJFDbZXDAnqkr53LlL91ZFLwRb+hWg1fN2iVHw boi/E2sRpd//X38t5xme1Y2vtxNukvsfqaW3nzi7BSmHB1FdbFRqqLXyF7lLe2B6kcqG xhu2LJMDz0YMmM6JP2sPrVAfxbdi09ec8HhLY+TgFNdLqcTjggGTZjw7MkiM83PeON2i HvJr7hdihFf4BzNcWM/1kR3rYx+qvKXzDrKyQ0tmkHrngE5pNxe/oWyjN7/NKJPdIOBQ h8Rw== X-Gm-Message-State: AOAM530G0WTuk+BHyS5n5POkTV08d3JH6jw7TqlgCb9Ma2a+0dKEpzSN SH6BbnoIAUo/BF4H2tkujQUPnqVJCzg= X-Google-Smtp-Source: ABdhPJzm6OtuDex1VN0DRR5eQjfwIzY+KPZ/WP+tnDEcOglsu5bYebecljBRqpuhRkbxpqVpov9Jlg== X-Received: by 2002:a17:906:37db:: with SMTP id o27mr4358020ejc.60.1615485069137; Thu, 11 Mar 2021 09:51:09 -0800 (PST) Received: from [10.64.5.165] ([109.190.253.15]) by smtp.googlemail.com with ESMTPSA id eo22sm1711183ejc.0.2021.03.11.09.51.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 11 Mar 2021 09:51:08 -0800 (PST) Subject: Re: [Bug libstdc++/99402] [10/11 Regression] std::copy creates _GLIBCXX_DEBUG false positive for attempt to subscript a dereferenceable (start-of-sequence) iterator From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= To: "libstdc++@gcc.gnu.org" , gcc-patches References: Message-ID: <5a5ae1ac-f3e3-70a4-c37d-80dd7baf6d05@gmail.com> Date: Thu, 11 Mar 2021 18:51:06 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------C6B199D5685ABA6C1B0A2D22" Content-Language: fr X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Mar 2021 17:51:13 -0000 This is a multi-part message in MIME format. --------------C6B199D5685ABA6C1B0A2D22 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit I eventually prefer to propose this version. Compared to the previous one I have the _M_can_advance calling the former one with correct number of elements to check for advance. And the former _M_can_advance is also properly making use of the __dp_sign_max_size precision. Here is the revisited git log:     libstdc++: [_GLIBCXX_DEBUG] Fix management of __dp_sign_max_size [PR 99402]     __dp_sign precision indicates that we found out what iterator comes first or     last in the range. __dp_sign_max_size is the same plus it gives the information     of the max size of the range that is to say the max_size value such that     distance(lhs, rhs) < max_size.     Thanks to this additional information we are able to tell when a copy of n elements     to that range will fail even if we do not know exactly how large it is.     This patch makes sure that we are properly using this information.     libstdc++-v3/ChangeLog:             PR libstdc++/99402             * include/debug/helper_functions.h (__can_advance(_InputIterator,             const std::pair<_Diff, _Distance_precision>&, int)): New.             (__can_advance(const _Safe_iterator<>&,             const std::pair<_Diff, _Distance_precision>&, int)): New.             * include/debug/macros.h (__glibcxx_check_can_increment_dist): New,             use latter.             (__glibcxx_check_can_increment_range): Adapt to use latter.             (__glibcxx_check_can_decrement_range): Likewise.             * include/debug/safe_iterator.h             (_Safe_iterator<>::_M_can_advance(const std::pair<_Diff, _Distance_precision>&,             int)): New.             (__can_advance(const _Safe_iterator<>&,             const std::pair<_Diff, _Distance_precision>&, int)): New.             * include/debug/safe_iterator.tcc             (_Safe_iterator<>::_M_can_advance(const std::pair<_Diff, _Distance_precision>&,             int)): New.             (_Safe_iterator<>::_M_valid_range(const _Safe_iterator<>&,             std::pair&, bool)): Adapt for             __dp_sign_max_size.             (__copy_move_a): Adapt to use __glibcxx_check_can_increment_dist.             (__copy_move_backward_a): Likewise.             (__equal_aux): Likewise.             * include/debug/stl_iterator.h (__can_advance(const std::reverse_iterator<>&,             const std::pair<_Diff, _Distance_precision>&, int)): New.             (__can_advance(const std::move_iterator<>&,             const std::pair<_Diff, _Distance_precision>&, int)): New.             * testsuite/25_algorithms/copy/debug/99402.cc: New test. Ok to commit if tests are all PASS ? François On 07/03/21 10:30 pm, François Dumont wrote: > Here is the patch to correctly deal with the new __dp_sign_max_size. > > I prefer to introduce new __can_advance overloads for this to > correctly deal with the _Distance_precision in it. _M_valid_range was > also ignoring __dp_sign_max_size. > >     libstdc++: [_GLIBCXX_DEBUG] Fix management of __dp_sign_max_size > [PR 99402] > >     libstdc++-v3/ChangeLog: > >             PR libstdc++/99402 >             * include/debug/helper_functions.h > (__can_advance(_InputIterator, >             const std::pair<_Diff, _Distance_precision>&, int)): New. >             (__can_advance(const _Safe_iterator<>&, >             const std::pair<_Diff, _Distance_precision>&, int)): New. >             * include/debug/macros.h > (__glibcxx_check_can_increment_dist): New, >             use latter. >             (__glibcxx_check_can_increment_range): Adapt to use latter. >             (__glibcxx_check_can_decrement_range): Likewise. >             * include/debug/safe_iterator.h >             (_Safe_iterator<>::_M_can_advance(const std::pair<_Diff, > _Distance_precision>&, >             int)): New. >             (__can_advance(const _Safe_iterator<>&, >             const std::pair<_Diff, _Distance_precision>&, int)): New. >             * include/debug/safe_iterator.tcc >             (_Safe_iterator<>::_M_can_advance(const std::pair<_Diff, > _Distance_precision>&, >             int)): New. >             (_Safe_iterator<>::_M_valid_range(const _Safe_iterator<>&, >             std::pair&, bool)): > Adapt for >             __dp_sign_max_size. >             (__copy_move_a): Adapt to use > __glibcxx_check_can_increment_dist. >             (__copy_move_backward_a): Likewise. >             (__equal_aux): Likewise. >             * include/debug/stl_iterator.h (__can_advance(const > std::reverse_iterator<>&, >             const std::pair<_Diff, _Distance_precision>&, int)): New. >             (__can_advance(const std::move_iterator<>&, >             const std::pair<_Diff, _Distance_precision>&, int)): New. >             * testsuite/25_algorithms/copy/debug/99402.cc: New test. > > Tested under Linux x86_64. > > Ok to commit ? > > François > --------------C6B199D5685ABA6C1B0A2D22 Content-Type: text/x-patch; charset=UTF-8; name="pr99402.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="pr99402.patch" diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h index 513e5460eba..c0144ced979 100644 --- a/libstdc++-v3/include/debug/helper_functions.h +++ b/libstdc++-v3/include/debug/helper_functions.h @@ -291,6 +291,18 @@ namespace __gnu_debug __can_advance(const _Safe_iterator<_Iterator, _Sequence, _Category>&, _Size); + template + _GLIBCXX_CONSTEXPR + inline bool + __can_advance(_InputIterator, const std::pair<_Diff, _Distance_precision>&, int) + { return true; } + + template + bool + __can_advance(const _Safe_iterator<_Iterator, _Sequence, _Category>&, + const std::pair<_Diff, _Distance_precision>&, int); + /** Helper function to extract base iterator of random access safe iterator * in order to reduce performance impact of debug mode. Limited to random * access iterator because it is the only category for which it is possible diff --git a/libstdc++-v3/include/debug/macros.h b/libstdc++-v3/include/debug/macros.h index 0988437046f..9ac52ebd09d 100644 --- a/libstdc++-v3/include/debug/macros.h +++ b/libstdc++-v3/include/debug/macros.h @@ -94,6 +94,12 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__can_advance(_First, _Size), \ ._M_iterator(_First, #_First) \ ._M_integer(_Size, #_Size)) +#define __glibcxx_check_can_increment_dist(_First,_Dist,_Way) \ + _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__can_advance(_First, _Dist, _Way), \ + _M_message(__gnu_debug::__msg_iter_subscript_oob) \ + ._M_iterator(_First, #_First) \ + ._M_integer(_Way * _Dist.first, #_Dist)) + #define __glibcxx_check_can_increment_range(_First1,_Last1,_First2) \ do \ { \ @@ -105,7 +111,7 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__can_advance(_First, _Size), \ ._M_iterator(_Last1, #_Last1), \ __FILE__,__LINE__,__PRETTY_FUNCTION__); \ _GLIBCXX_DEBUG_VERIFY_AT_F( \ - __gnu_debug::__can_advance(_First2, __dist.first),\ + __gnu_debug::__can_advance(_First2, __dist, 1), \ _M_message(__gnu_debug::__msg_iter_subscript_oob)\ ._M_iterator(_First2, #_First2) \ ._M_integer(__dist.first), \ @@ -123,7 +129,7 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__can_advance(_First, _Size), \ ._M_iterator(_Last1, #_Last1), \ __FILE__,__LINE__,__PRETTY_FUNCTION__); \ _GLIBCXX_DEBUG_VERIFY_AT_F( \ - __gnu_debug::__can_advance(_First2, -__dist.first),\ + __gnu_debug::__can_advance(_First2, __dist, -1), \ _M_message(__gnu_debug::__msg_iter_subscript_oob)\ ._M_iterator(_First2, #_First2) \ ._M_integer(-__dist.first), \ diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h index a10df190969..8e138fd32e5 100644 --- a/libstdc++-v3/include/debug/safe_iterator.h +++ b/libstdc++-v3/include/debug/safe_iterator.h @@ -407,6 +407,12 @@ namespace __gnu_debug bool _M_can_advance(difference_type __n, bool __strict = false) const; + // Can we advance the iterator using @p __dist in @p __way direction. + template + bool + _M_can_advance(const std::pair<_Diff, _Distance_precision>& __dist, + int __way) const; + // Is the iterator range [*this, __rhs) valid? bool _M_valid_range(const _Safe_iterator& __rhs, @@ -958,6 +964,14 @@ namespace __gnu_debug _Size __n) { return __it._M_can_advance(__n); } + template + inline bool + __can_advance(const _Safe_iterator<_Iterator, _Sequence, _Category>& __it, + const std::pair<_Diff, _Distance_precision>& __dist, + int __way) + { return __it._M_can_advance(__dist, __way); } + template _Iterator __base(const _Safe_iterator<_Iterator, _Sequence, diff --git a/libstdc++-v3/include/debug/safe_iterator.tcc b/libstdc++-v3/include/debug/safe_iterator.tcc index 81deb10125b..6f14c40bf41 100644 --- a/libstdc++-v3/include/debug/safe_iterator.tcc +++ b/libstdc++-v3/include/debug/safe_iterator.tcc @@ -92,22 +92,30 @@ namespace __gnu_debug if (__n == 0) return true; + std::pair __dist = __n < 0 + ? _M_get_distance_from_begin() + : _M_get_distance_to_end(); + if (__n < 0) - { - std::pair __dist = - _M_get_distance_from_begin(); - return __dist.second == __dp_exact - ? __dist.first >= -__n + __n = -__n; + + return __dist.second > __dp_sign + ? __dist.first >= __n : !__strict && __dist.first > 0; } - else + + template + template + bool + _Safe_iterator<_Iterator, _Sequence, _Category>:: + _M_can_advance(const std::pair<_Diff, _Distance_precision>& __dist, + int __way) const { - std::pair __dist = - _M_get_distance_to_end(); return __dist.second == __dp_exact - ? __dist.first >= __n - : !__strict && __dist.first > 0; - } + ? _M_can_advance(__way * __dist.first) + : _M_can_advance(__way * (__dist.first == 0 + ? 0 + : __dist.first < 0 ? -1 : 1)); } template @@ -194,20 +202,15 @@ namespace __gnu_debug switch (__dist.second) { case __dp_equality: - if (__dist.first == 0) + // Assume that this is a valid range; we can't check anything else. return true; - break; - case __dp_sign: - case __dp_exact: + default: // If range is not empty first iterator must be dereferenceable. - if (__dist.first > 0) - return !__check_dereferenceable || _M_dereferenceable(); - return __dist.first == 0; + return __dist.first == 0 + || (__dist.first > 0 + && (!__check_dereferenceable || _M_dereferenceable())); } - - // Assume that this is a valid range; we can't check anything else. - return true; } template @@ -225,9 +228,8 @@ namespace __gnu_debug __dist = std::make_pair(__rhs.base() - this->base(), __dp_exact); // If range is not empty first iterator must be dereferenceable. - if (__dist.first > 0) - return this->_M_dereferenceable(); - return __dist.first == 0; + return __dist.first == 0 + || (__dist.first > 0 && this->_M_dereferenceable()); } } // namespace __gnu_debug @@ -251,7 +253,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { typename ::__gnu_debug::_Distance_traits<_Ite>::__type __dist; __glibcxx_check_valid_range2(__first, __last, __dist); - __glibcxx_check_can_increment(__result, __dist.first); + __glibcxx_check_can_increment_dist(__result, __dist, 1); if (__dist.second > ::__gnu_debug::__dp_equality) return std::__copy_move_a<_IsMove>(__first.base(), __last.base(), @@ -268,7 +270,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { typename ::__gnu_debug::_Distance_traits<_II>::__type __dist; __glibcxx_check_valid_range2(__first, __last, __dist); - __glibcxx_check_can_increment(__result, __dist.first); + __glibcxx_check_can_increment_dist(__result, __dist, 1); if (__dist.second > ::__gnu_debug::__dp_sign && __result._M_can_advance(__dist.first, true)) @@ -290,7 +292,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { typename ::__gnu_debug::_Distance_traits<_IIte>::__type __dist; __glibcxx_check_valid_range2(__first, __last, __dist); - __glibcxx_check_can_increment(__result, __dist.first); + __glibcxx_check_can_increment_dist(__result, __dist, 1); if (__dist.second > ::__gnu_debug::__dp_equality) { @@ -318,7 +320,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { typename ::__gnu_debug::_Distance_traits<_Ite>::__type __dist; __glibcxx_check_valid_range2(__first, __last, __dist); - __glibcxx_check_can_increment(__result, -__dist.first); + __glibcxx_check_can_increment_dist(__result, __dist, -1); if (__dist.second > ::__gnu_debug::__dp_equality) return std::__copy_move_backward_a<_IsMove>( @@ -335,7 +337,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { typename ::__gnu_debug::_Distance_traits<_II>::__type __dist; __glibcxx_check_valid_range2(__first, __last, __dist); - __glibcxx_check_can_increment(__result, -__dist.first); + __glibcxx_check_can_increment_dist(__result, __dist, -1); if (__dist.second > ::__gnu_debug::__dp_sign && __result._M_can_advance(-__dist.first, true)) @@ -358,7 +360,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { typename ::__gnu_debug::_Distance_traits<_IIte>::__type __dist; __glibcxx_check_valid_range2(__first, __last, __dist); - __glibcxx_check_can_increment(__result, -__dist.first); + __glibcxx_check_can_increment_dist(__result, __dist, -1); if (__dist.second > ::__gnu_debug::__dp_equality) { @@ -423,7 +425,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { typename ::__gnu_debug::_Distance_traits<_II1>::__type __dist; __glibcxx_check_valid_range2(__first1, __last1, __dist); - __glibcxx_check_can_increment(__first2, __dist.first); + __glibcxx_check_can_increment_dist(__first2, __dist, 1); if (__dist.second > ::__gnu_debug::__dp_equality) return std::__equal_aux(__first1.base(), __last1.base(), __first2); @@ -438,7 +440,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { typename ::__gnu_debug::_Distance_traits<_II1>::__type __dist; __glibcxx_check_valid_range2(__first1, __last1, __dist); - __glibcxx_check_can_increment(__first2, __dist.first); + __glibcxx_check_can_increment_dist(__first2, __dist, 1); if (__dist.second > ::__gnu_debug::__dp_sign && __first2._M_can_advance(__dist.first, true)) @@ -457,7 +459,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { typename ::__gnu_debug::_Distance_traits<_II1>::__type __dist; __glibcxx_check_valid_range2(__first1, __last1, __dist); - __glibcxx_check_can_increment(__first2, __dist.first); + __glibcxx_check_can_increment_dist(__first2, __dist, 1); if (__dist.second > ::__gnu_debug::__dp_equality) { diff --git a/libstdc++-v3/include/debug/stl_iterator.h b/libstdc++-v3/include/debug/stl_iterator.h index a9dd5b6c08d..edeb42ebe98 100644 --- a/libstdc++-v3/include/debug/stl_iterator.h +++ b/libstdc++-v3/include/debug/stl_iterator.h @@ -52,6 +52,13 @@ namespace __gnu_debug __can_advance(const std::reverse_iterator<_Iterator>& __it, _Size __n) { return __can_advance(__it.base(), -__n); } + template + inline bool + __can_advance(const std::reverse_iterator<_Iterator>& __it, + const std::pair<_Diff, _Distance_precision>& __dist, + int __way) + { return __can_advance(__it.base(), __dist, -__way); } + template inline std::reverse_iterator<_Iterator> __base(const std::reverse_iterator<_Safe_iterator< @@ -101,6 +108,13 @@ namespace __gnu_debug __can_advance(const std::move_iterator<_Iterator>& __it, _Size __n) { return __can_advance(__it.base(), __n); } + template + inline bool + __can_advance(const std::move_iterator<_Iterator>& __it, + const std::pair<_Diff, _Distance_precision>& __dist, + int __way) + { return __can_advance(__it.base(), __dist, __way); } + template inline auto __unsafe(const std::move_iterator<_Iterator>& __it) diff --git a/libstdc++-v3/testsuite/25_algorithms/copy/debug/99402.cc b/libstdc++-v3/testsuite/25_algorithms/copy/debug/99402.cc new file mode 100644 index 00000000000..041d222d079 --- /dev/null +++ b/libstdc++-v3/testsuite/25_algorithms/copy/debug/99402.cc @@ -0,0 +1,35 @@ +// Copyright (C) 2021 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 +// . + +// { dg-options "-D_GLIBCXX_DEBUG" } +// { dg-do run } + +#include +#include +#include + +// PR libstdc++/99402 + +using namespace std; + +int main() +{ + // any container with non-random access iterators: + const set source = { 0, 1 }; + vector dest(1); + copy(source.begin(), ++source.begin(), dest.begin()); +} --------------C6B199D5685ABA6C1B0A2D22--