[PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc PR109849
François Dumont
frs.dumont@gmail.com
Mon Jun 3 04:56:08 GMT 2024
I hadn't try to make my patch as limited as possible to fix the problem,
indeed.
libstdc++: Fix -Wstringop-overflow warning coming from std::vector
[PR109849]
libstdc++-v3/ChangeLog:
PR libstdc++/109849
* include/bits/vector.tcc
(std::vector<>::_M_range_insert(iterator, _FwdIt, _FwdIt,
forward_iterator_tag)): Add __builtin_unreachable
expression to tell
the compiler that the allocated buffer is large enough to
receive current
elements plus the range to insert.
Tested under Linux x64, ok to commit ?
François
On 30/05/2024 13:07, Jonathan Wakely wrote:
> On Thu, 30 May 2024 at 06:11, François Dumont <frs.dumont@gmail.com> wrote:
>> Looks like this new version works the same to fix the warning without
>> the issues reported here.
>>
>> All 23_containers/vector tests run in C++98/14/20 so far.
>>
>> Ok to commit once I've complete the testsuite (or some bot did it for me
>> !) ?
> There seem to be two unrelated changes here. One is to make the local
> variables usable in both branches, but I don't understand why that
> matters because the first branch doesn't reallocate, so there's no
> call to operator new that would confuse the compiler.
>
> The second change is to add the __builtin_unreachable() to tell the
> compiler that __len is correct and did not wrap around. Which should
> not be needed because
>
> Are both changes necessary to fix the FAIL for c++98 mode?
>
> I've just done a quick check, and it seems that this smaller change
> fixes the FAIL:
>
> --- a/libstdc++-v3/include/bits/vector.tcc
> +++ b/libstdc++-v3/include/bits/vector.tcc
> @@ -933,6 +933,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>
> const size_type __len =
> _M_check_len(__n, "vector::_M_range_insert");
> + if (__len < (__n + (__old_finish - __old_start)))
> + __builtin_unreachable();
> +
> pointer __new_start(this->_M_allocate(__len));
> pointer __new_finish(__new_start);
> __try
>
> We don't need the rest of the change, which makes sense because the
> first branch doesn't reallocate so the compiler doesn't think the
> pointers can be invalidated.
>
>
>
>> I'll look for a PR to associate, if you have one in mind do not hesitate
>> to tell me.
> It's discussed in PR 109849.
>
>> François
>>
>>
>> On 28/05/2024 12:28, Jonathan Wakely wrote:
>>> On 27/05/24 22:07 +0200, François Dumont wrote:
>>>> In C++98 this test fails with:
>>>>
>>>> Excess errors:
>>>> /home/fdumont/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:452:
>>>> warning: 'void* __builtin_memcpy(void*, const void*, long unsigned
>>>> int)' writing between 2 and 9223372036854775806 bytes into a region
>>>> of size 0 overflows the destination [-Wstringop-overflow=]
>>>>
>>>> The attached patch avoids this warning.
>>>>
>>>> libstdc++: Fix -Wstringop-overflow warning coming from std::vector
>>>>
>>>> Make vector<>::_M_range_insert implementation more transparent to
>>>> the compiler checks.
>>>>
>>>> Extend local copies of members to the whole method scope so that
>>>> all branches benefit
>>>> from those.
>>>>
>>>> libstdc++-v3/ChangeLog:
>>>>
>>>> * include/bits/vector.tcc
>>>> (std::vector<>::_M_range_insert(iterator, _FwdIt, _FwdIt,
>>>> forward_iterator_tag)):
>>>> Use local copies of members to call the different
>>>> algorithms.
>>>>
>>>> Ok to commit if all tests passes ?
>>>>
>>>> François
>>>> diff --git a/libstdc++-v3/include/bits/vector.tcc
>>>> b/libstdc++-v3/include/bits/vector.tcc
>>>> index 36b27dce7b9..671929dee55 100644
>>>> --- a/libstdc++-v3/include/bits/vector.tcc
>>>> +++ b/libstdc++-v3/include/bits/vector.tcc
>>>> @@ -885,83 +885,80 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>>> {
>>>> if (__first != __last)
>>>> {
>>>> + // Make local copies of these members because the compiler
>>>> + // thinks the allocator can alter them if 'this' is globally
>>>> + // reachable.
>>>> + pointer __start = this->_M_impl._M_start;
>>>> + pointer __end = this->_M_impl._M_end_of_storage;
>>>> + pointer __finish = this->_M_impl._M_finish;
>>>> + pointer __pos = __position.base();
>>>> + _Tp_alloc_type& __allocator = _M_get_Tp_allocator();
>>>> +
>>>> + if (__pos < __start || __finish < __pos)
>>>> + __builtin_unreachable();
>>> I don't think we should use __builtin_unreachable for something which
>>> is not an invariant of the class. The __position argument is supplied
>>> by the user, so we should not make promises about it being valid,
>>> because we can't know that.
>>>
>>> We can promise that __start <= __finish, and that __finish <= end,
>>> because we control those. We can't promise the user won't pass in a
>>> bad __position. Although it's undefined for the user to do that, using
>>> __builtin_unreachable() here makes the effects worse, and makes it
>>> harder to debug.
>>>
>>> Also, (__pos < __start) might already trigger undefined behaviour for
>>> fancy pointers, if they don't point to the same memory region.
>>>
>>> So this change is not OK.
>>>
>>>
>>>> +
>>>> const size_type __n = std::distance(__first, __last);
>>>> - if (size_type(this->_M_impl._M_end_of_storage
>>>> - - this->_M_impl._M_finish) >= __n)
>>>> + if (size_type(__end - __finish) >= __n)
>>>> {
>>>> - const size_type __elems_after = end() - __position;
>>>> - pointer __old_finish(this->_M_impl._M_finish);
>>>> + const size_type __elems_after = __end - __pos;
>>>> + pointer __old_finish(__finish);
>>>> if (__elems_after > __n)
>>>> {
>>>> _GLIBCXX_ASAN_ANNOTATE_GROW(__n);
>>>> - std::__uninitialized_move_a(this->_M_impl._M_finish - __n,
>>>> - this->_M_impl._M_finish,
>>>> - this->_M_impl._M_finish,
>>>> - _M_get_Tp_allocator());
>>>> - this->_M_impl._M_finish += __n;
>>>> + __finish = std::__uninitialized_move_a
>>>> + (__finish - __n, __finish, __finish, __allocator);
>>>> _GLIBCXX_ASAN_ANNOTATE_GREW(__n);
>>>> - _GLIBCXX_MOVE_BACKWARD3(__position.base(),
>>>> - __old_finish - __n, __old_finish);
>>>> - std::copy(__first, __last, __position);
>>>> + _GLIBCXX_MOVE_BACKWARD3
>>>> + (__pos, __old_finish - __n, __old_finish);
>>>> + std::copy(__first, __last, __pos);
>>>> }
>>>> else
>>>> {
>>>> _ForwardIterator __mid = __first;
>>>> std::advance(__mid, __elems_after);
>>>> _GLIBCXX_ASAN_ANNOTATE_GROW(__n);
>>>> - std::__uninitialized_copy_a(__mid, __last,
>>>> - this->_M_impl._M_finish,
>>>> - _M_get_Tp_allocator());
>>>> - this->_M_impl._M_finish += __n - __elems_after;
>>>> + __finish = std::__uninitialized_copy_a
>>>> + (__mid, __last, __finish, __allocator);
>>>> _GLIBCXX_ASAN_ANNOTATE_GREW(__n - __elems_after);
>>>> - std::__uninitialized_move_a(__position.base(),
>>>> - __old_finish,
>>>> - this->_M_impl._M_finish,
>>>> - _M_get_Tp_allocator());
>>>> - this->_M_impl._M_finish += __elems_after;
>>>> + __finish = std::__uninitialized_move_a
>>>> + (__pos, __old_finish, __finish, __allocator);
>>>> _GLIBCXX_ASAN_ANNOTATE_GREW(__elems_after);
>>>> - std::copy(__first, __mid, __position);
>>>> + std::copy(__first, __mid, __pos);
>>>> }
>>>> +
>>>> + this->_M_impl._M_finish = __finish;
>>>> }
>>>> else
>>>> {
>>>> - // Make local copies of these members because the compiler
>>>> - // thinks the allocator can alter them if 'this' is globally
>>>> - // reachable.
>>>> - pointer __old_start = this->_M_impl._M_start;
>>>> - pointer __old_finish = this->_M_impl._M_finish;
>>>> -
>>>> + const size_type __size = size_type(__finish - __start);
>>>> const size_type __len =
>>>> _M_check_len(__n, "vector::_M_range_insert");
>>>> + if (__len < __n + __size)
>>>> + __builtin_unreachable();
>>>> +
>>>> pointer __new_start(this->_M_allocate(__len));
>>>> pointer __new_finish(__new_start);
>>>> __try
>>>> {
>>>> __new_finish
>>>> = std::__uninitialized_move_if_noexcept_a
>>>> - (__old_start, __position.base(),
>>>> - __new_start, _M_get_Tp_allocator());
>>>> + (__start, __pos, __new_start, __allocator);
>>>> __new_finish
>>>> - = std::__uninitialized_copy_a(__first, __last,
>>>> - __new_finish,
>>>> - _M_get_Tp_allocator());
>>>> + = std::__uninitialized_copy_a
>>>> + (__first, __last, __new_finish, __allocator);
>>>> __new_finish
>>>> = std::__uninitialized_move_if_noexcept_a
>>>> - (__position.base(), __old_finish,
>>>> - __new_finish, _M_get_Tp_allocator());
>>>> + (__pos, __finish, __new_finish, __allocator);
>>>> }
>>>> __catch(...)
>>>> {
>>>> - std::_Destroy(__new_start, __new_finish,
>>>> - _M_get_Tp_allocator());
>>>> + std::_Destroy(__new_start, __new_finish, __allocator);
>>>> _M_deallocate(__new_start, __len);
>>>> __throw_exception_again;
>>>> }
>>>> - std::_Destroy(__old_start, __old_finish,
>>>> - _M_get_Tp_allocator());
>>>> + std::_Destroy(__start, __finish, __allocator);
>>>> _GLIBCXX_ASAN_ANNOTATE_REINIT;
>>>> - _M_deallocate(__old_start,
>>>> - this->_M_impl._M_end_of_storage - __old_start);
>>>> + _M_deallocate(__start, __end - __start);
>>>> this->_M_impl._M_start = __new_start;
>>>> this->_M_impl._M_finish = __new_finish;
>>>> this->_M_impl._M_end_of_storage = __new_start + __len;
-------------- next part --------------
diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index 36b27dce7b9..e5a12c1d608 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -933,6 +933,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
const size_type __len =
_M_check_len(__n, "vector::_M_range_insert");
+ if (!__builtin_constant_p(__len) &&
+ __len < (__n + (__old_start - __old_finish)))
+ __builtin_unreachable();
+
pointer __new_start(this->_M_allocate(__len));
pointer __new_finish(__new_start);
__try
More information about the Libstdc++
mailing list