From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 26E4338708C0 for ; Tue, 28 May 2024 10:28:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 26E4338708C0 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 26E4338708C0 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716892112; cv=none; b=PbXZLvJ4fh6S0V5fHtQnpntpEmVZa5XQmRtbK0HxfB85NloHNUQBmeKxxYa5kEh1zK8yLeZfCkSPUFpL2C5GhIswPKTaaDQXUZiEINTA7MK/5csC1GXdVSTXczTasQSty5h5Dxftiqc0RgO50nKHI71mvdDUu4HfFrkk7w4eDdo= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716892112; c=relaxed/simple; bh=BH/wHdmy0TEk37UJvmnfmxPvg4O5M55ZVBwWLAat7o8=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=CmX+CkezPkfqckY9eWk8qsGRqVlYblIFT32gWJE8KEsu7PDaLcsitQD6dm6/K2niF+GggGijI0MH3m8+beIHHhOfQ71FRCbWvoVS9OxpGYBF3snuOanT4buyaZJ5mhgzWMhT1h/O4PQVCMxavzr4uWTDQupkIQI5pXurqJTx1RY= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1716892109; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=OUODi5sBpm2qXro/XB3CdclKJt/7vOoT8uuj/C9+M50=; b=EYwMc6mm8GnJRa2K4w971xb5JLWOnZoL5ed3aoc2pKhlDQtctk/n5CsGly+6wuDOUoGDKb SpEXwUbUcg84j2V5qDZ04KHNiaCtKgZuOfyklAoqlziIHAeay5LY4dlGi5tcrjW3e0VNBz lQqEvXhY7BZcQHLhInieL5PWcrMAemI= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-682-YbsL9Vs4Ny6W3SenUtmyGQ-1; Tue, 28 May 2024 06:28:28 -0400 X-MC-Unique: YbsL9Vs4Ny6W3SenUtmyGQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 1EA458029EC; Tue, 28 May 2024 10:28:28 +0000 (UTC) Received: from localhost (unknown [10.42.28.51]) by smtp.corp.redhat.com (Postfix) with ESMTP id D1381C15BEA; Tue, 28 May 2024 10:28:27 +0000 (UTC) Date: Tue, 28 May 2024 11:28:27 +0100 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: libstdc++ , gcc-patches Subject: Re: [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc Message-ID: References: <4657346e-3046-424a-be1d-32a7eaecae32@gmail.com> MIME-Version: 1.0 In-Reply-To: <4657346e-3046-424a-be1d-32a7eaecae32@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00,BODY_8BITS,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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;