[committed] libstdc++: Replace operator>>(istream&, char*) [LWG 2499]

[Martin Sebor]

On 8/6/20 8:45 AM, Jonathan Wakely via Libstdc++ wrote:
> 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<typename _CharT, typename _Traits>
>>>>    __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<streamsize>::__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<char>]',
>      inlined from 'std::basic_istream<_CharT, _Traits>& 
> std::operator>>(std::basic_istream<_CharT, _Traits>&, _CharT*) [with 
> _CharT = char; _Traits = std::char_traits<char>]' 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<char>]',
>      inlined from 'std::basic_istream<_CharT, _Traits>& 
> std::operator>>(std::basic_istream<_CharT, _Traits>&, _CharT*) [with 
> _CharT = char; _Traits = std::char_traits<char>]' 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.

Sorry.  I don't see this exercise as a complete waste of time
(but I understand why it feels like that to you).

What it highlights is the fact that the warning infrastructure
we have in place is far from optimal for C++ in general (with
its heavy reliance on ilining and templates) and the standard
library in particular (especially with -Wno-system-headers).
We should make an effort to do better.

Setting aside the effort to clean up the library so that it can
be used even with -Wsystem-headers, warnings about out of bounds
accesses should trigger even with -Wno-system-headers.  If one
doesn't I'd tend to view it as a bug.  I added code to have some
trigger despite it but I'm pretty sure there are more places where
the middle end needs to do the same gymnastics to enable it.

Martin