From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-2.mimecast.com [207.211.31.81]) by sourceware.org (Postfix) with ESMTP id 0132D384C005 for ; Thu, 6 Aug 2020 14:45:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0132D384C005 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-409-5_UxGBfdPUKaMdLg0GCBSQ-1; Thu, 06 Aug 2020 10:45:29 -0400 X-MC-Unique: 5_UxGBfdPUKaMdLg0GCBSQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0612A1005504; Thu, 6 Aug 2020 14:45:28 +0000 (UTC) Received: from localhost (unknown [10.33.36.145]) by smtp.corp.redhat.com (Postfix) with ESMTP id 78A3D5F21A; Thu, 6 Aug 2020 14:45:27 +0000 (UTC) Date: Thu, 6 Aug 2020 15:45:26 +0100 From: Jonathan Wakely To: Jakub Jelinek Cc: Martin Sebor , libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [committed] libstdc++: Replace operator>>(istream&, char*) [LWG 2499] Message-ID: <20200806144526.GB3400@redhat.com> References: <20200805212500.GA2013665@redhat.com> <5a18ba7e-e550-72b9-150f-ce7abed339fd@redhat.com> <20200806131448.GY3400@redhat.com> <20200806132648.GW2363@tucnak> <20200806140114.GA3400@redhat.com> MIME-Version: 1.0 In-Reply-To: <20200806140114.GA3400@redhat.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Aug 2020 14:45:40 -0000 On 06/08/20 15:01 +0100, Jonathan Wakely wrote: >On 06/08/20 15:26 +0200, Jakub Jelinek via Libstdc++ wrote: >>On Thu, Aug 06, 2020 at 02:14:48PM +0100, Jonathan Wakely wrote: >>> template >>> __attribute__((__nonnull__(2), __access__(__write_only__, 2))) >>> inline basic_istream<_CharT, _Traits>& >>> operator>>(basic_istream<_CharT, _Traits>& __in, _CharT* __s) >>> { >>> size_t __n = __builtin_object_size(__s, 0); >>> if (__builtin_expect(__n < sizeof(_CharT), false)) >>> { >>> // not even space for null terminator >>> __glibcxx_assert(__n >= sizeof(_CharT)); >>> __in.width(0); >>> __in.setstate(ios_base::failbit); >>> } >>> else >>> { >>> if (__n == (size_t)-1) >>> __n = __gnu_cxx::__numeric_traits::__max; >>> std::__istream_extract(__in, __s, __n / sizeof(_CharT)); >>> } >>> return __in; >>> } >>> >>>This will give a -Wstringop-overflow warning at -O0 and then overflow >>>the buffer, with undefined behaviour. And it will give no warning but >>>avoid the overflow when optimising. This isn't my preferred outcome, >>>I'd prefer to always get a warning, *and* be able to avoid the >>>overflow when optimising and the size is known. >> >>A way to get warning even at -O2 would be to call some external function >>in the if (__bos0 < sizeof(_CharT)) block, which wouldn't be optimized away >>and would have __attribute__((warning ("..."))) on it. >>See e.g. how glibc uses __warndecl e.g. in >>/usr/include/bits/string_fortified.h. >>One can use alias attribute to have different warnings for the same external >>call (which could do e.g. what part of __glibcxx_assert does, call vprintf >>+ abort. > >Every time I've tried that I've found the requirement for an external >function to be frustrating. It means adding a new symbol to the >library, because it doesn't work for inline functions or function >templates, even with __attribute__((noinline)). > >And we don't necessarily want it to abort, because that depends on a >macro defined by users, which isn't visible inside the library. > >It shouldn't be this hard. The function with __attribute__(__warning__(""))) only warns when -Wsystem-headers is on, which makes it useless. And when it's on, it warns twice for a single call: In file included from /home/jwakely/gcc/11/include/c++/11.0.0/sstream:38, from of.cc:1: In function 'std::basic_istream<_CharT, _Traits>& std::operator>>(std::basic_istream<_CharT, _Traits>&, _CharT*) [with _CharT = char; _Traits = std::char_traits]', inlined from 'std::basic_istream<_CharT, _Traits>& std::operator>>(std::basic_istream<_CharT, _Traits>&, _CharT*) [with _CharT = char; _Traits = std::char_traits]' at /home/jwakely/gcc/11/include/c++/11.0.0/istream:808:5, inlined from 'void test01(std::istream&)' at of.cc:7:16: /home/jwakely/gcc/11/include/c++/11.0.0/istream:814:26: warning: call to 'std::__diag_overflow' declared with attribute warning: buffer overflow detected [-Wattribute-warning] 814 | __diag_overflow(); | ~~~~~~~~~~~~~~~^~ In function 'std::basic_istream<_CharT, _Traits>& std::operator>>(std::basic_istream<_CharT, _Traits>&, _CharT*) [with _CharT = char; _Traits = std::char_traits]', inlined from 'std::basic_istream<_CharT, _Traits>& std::operator>>(std::basic_istream<_CharT, _Traits>&, _CharT*) [with _CharT = char; _Traits = std::char_traits]' at /home/jwakely/gcc/11/include/c++/11.0.0/istream:808:5, inlined from 'void test01(std::istream&)' at of.cc:7:16, inlined from 'int main()' at of.cc:13:9: /home/jwakely/gcc/11/include/c++/11.0.0/istream:814:26: warning: call to 'std::__diag_overflow' declared with attribute warning: buffer overflow detected [-Wattribute-warning] 814 | __diag_overflow(); | ~~~~~~~~~~~~~~~^~ Adding attributes to __istream_extract is useless, because that's only called by the library, so again, needs -Wsystem-headers to do anything. Adding attributes to operator>> works well, but only at -O0 because otherwise it gets inlined and the attributes are ignored. The functions that get called by the inlined function don't warn because they're in system headers. This is unusable, and a waste of a day.