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

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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 4769 bytes --]

P0487R1 resolved LWG 2499 for C++20 by removing the operator>> overloads
that have high risk of buffer overflows. They were replaced by
equivalents that only accept a reference to an array, and so can
guarantee not to write past the end of the array.

In order to support both the old and new functionality, this patch
introduces a new overloaded __istream_extract function which takes a
maximum length. The new operator>> overloads use the array size as the
maximum length. The old overloads now use __builtin_object_size to
determine the available buffer size if available (which requires -O2) or
use numeric_limits<streamsize>::max()/sizeof(char_type) otherwise. This
is a change in behaviour, as the old overloads previously always used
numeric_limits<streamsize>::max(), without considering sizeof(char_type)
and without attempting to prevent overflows.

Because they now do little more than call __istream_extract, the old
operator>> overloads are very small inline functions. This means there
is no advantage to explicitly instantiating them in the library (in fact
that would prevent the __builtin_object_size checks from ever working).
As a result, the explicit instantiation declarations can be removed from
the header. The explicit instantiation definitions are still needed, for
backwards compatibility with existing code that expects to link to the
definitions in the library.

While working on this change I noticed that src/c++11/istream-inst.cc
has the following explicit instantiation definition:
  template istream& operator>>(istream&, char*);
This had no effect (and so should not have been present in that file),
because there was an explicit specialization declared in <istream> and
defined in src/++98/istream.cc. However, this change removes the
explicit specialization, and now the explicit instantiation definition
is necessary to ensure the symbol gets defined in the library.

libstdc++-v3/ChangeLog:

	* config/abi/pre/gnu.ver (GLIBCXX_3.4.29): Export new symbols.
	* include/bits/istream.tcc (__istream_extract): New function
	template implementing both of operator>>(istream&, char*) and
	operator>>(istream&, char(&)[N]). Add explicit instantiation
	declaration for it. Remove explicit instantiation declarations
	for old function templates.
	* include/std/istream (__istream_extract): Declare.
	(operator>>(basic_istream<C,T>&, C*)): Define inline and simply
	call __istream_extract.
	(operator>>(basic_istream<char,T>&, signed char*)): Likewise.
	(operator>>(basic_istream<char,T>&, unsigned char*)): Likewise.
	(operator>>(basic_istream<C,T>&, C(7)[N])): Define for LWG 2499.
	(operator>>(basic_istream<char,T>&, signed char(&)[N])):
	Likewise.
	(operator>>(basic_istream<char,T>&, unsigned char(&)[N])):
	Likewise.
	* include/std/streambuf (basic_streambuf): Declare char overload
	of __istream_extract as a friend.
	* src/c++11/istream-inst.cc: Add explicit instantiation
	definition for wchar_t overload of __istream_extract. Remove
	explicit instantiation definitions of old operator>> overloads
	for versioned-namespace build.
	* src/c++98/istream.cc (operator>>(istream&, char*)): Replace
	with __istream_extract(istream&, char*, streamsize).
	* testsuite/27_io/basic_istream/extractors_character/char/3.cc:
	Do not use variable-length array.
	* testsuite/27_io/basic_istream/extractors_character/char/4.cc:
	Do not run test for C++20.
	* testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc:
	Do not test writing to pointers for C++20.
	* testsuite/27_io/basic_istream/extractors_character/char/9826.cc:
	Use array instead of pointer.
	* testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc:
	Do not use variable-length array.
	* testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc:
	Do not run test for C++20.
	* testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc:
	Do not test writing to pointers for C++20.
	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499.cc:
	New test.
	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc:
	New test.
	* testsuite/27_io/basic_istream/extractors_character/char/overflow.cc:
	New test.
	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499.cc:
	New test.
	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc:
	New test.

Tested powerpc64le-linux. Committed to trunk.

Martin, Jakub, could you please double-check the usage of
__builtin_object_size? (line 805 in libstdc++-v3/include/std/istream)
Do you see any problems with using it here? If it can't tell the size
then we just assume it's larger than the string to be extracted, which
is what the old code did anyway. I do have an idea for stronger
checking in debug mode, which I'll post in a minute.



[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 29743 bytes --]

commit 17abcc773415848dce593016512636cda3de20d5
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Aug 5 22:17:18 2020

    libstdc++: Replace operator>>(istream&, char*) [LWG 2499]
    
    P0487R1 resolved LWG 2499 for C++20 by removing the operator>> overloads
    that have high risk of buffer overflows. They were replaced by
    equivalents that only accept a reference to an array, and so can
    guarantee not to write past the end of the array.
    
    In order to support both the old and new functionality, this patch
    introduces a new overloaded __istream_extract function which takes a
    maximum length. The new operator>> overloads use the array size as the
    maximum length. The old overloads now use __builtin_object_size to
    determine the available buffer size if available (which requires -O2) or
    use numeric_limits<streamsize>::max()/sizeof(char_type) otherwise. This
    is a change in behaviour, as the old overloads previously always used
    numeric_limits<streamsize>::max(), without considering sizeof(char_type)
    and without attempting to prevent overflows.
    
    Because they now do little more than call __istream_extract, the old
    operator>> overloads are very small inline functions. This means there
    is no advantage to explicitly instantiating them in the library (in fact
    that would prevent the __builtin_object_size checks from ever working).
    As a result, the explicit instantiation declarations can be removed from
    the header. The explicit instantiation definitions are still needed, for
    backwards compatibility with existing code that expects to link to the
    definitions in the library.
    
    While working on this change I noticed that src/c++11/istream-inst.cc
    has the following explicit instantiation definition:
      template istream& operator>>(istream&, char*);
    This had no effect (and so should not have been present in that file),
    because there was an explicit specialization declared in <istream> and
    defined in src/++98/istream.cc. However, this change removes the
    explicit specialization, and now the explicit instantiation definition
    is necessary to ensure the symbol gets defined in the library.
    
    libstdc++-v3/ChangeLog:
    
            * config/abi/pre/gnu.ver (GLIBCXX_3.4.29): Export new symbols.
            * include/bits/istream.tcc (__istream_extract): New function
            template implementing both of operator>>(istream&, char*) and
            operator>>(istream&, char(&)[N]). Add explicit instantiation
            declaration for it. Remove explicit instantiation declarations
            for old function templates.
            * include/std/istream (__istream_extract): Declare.
            (operator>>(basic_istream<C,T>&, C*)): Define inline and simply
            call __istream_extract.
            (operator>>(basic_istream<char,T>&, signed char*)): Likewise.
            (operator>>(basic_istream<char,T>&, unsigned char*)): Likewise.
            (operator>>(basic_istream<C,T>&, C(7)[N])): Define for LWG 2499.
            (operator>>(basic_istream<char,T>&, signed char(&)[N])):
            Likewise.
            (operator>>(basic_istream<char,T>&, unsigned char(&)[N])):
            Likewise.
            * include/std/streambuf (basic_streambuf): Declare char overload
            of __istream_extract as a friend.
            * src/c++11/istream-inst.cc: Add explicit instantiation
            definition for wchar_t overload of __istream_extract. Remove
            explicit instantiation definitions of old operator>> overloads
            for versioned-namespace build.
            * src/c++98/istream.cc (operator>>(istream&, char*)): Replace
            with __istream_extract(istream&, char*, streamsize).
            * testsuite/27_io/basic_istream/extractors_character/char/3.cc:
            Do not use variable-length array.
            * testsuite/27_io/basic_istream/extractors_character/char/4.cc:
            Do not run test for C++20.
            * testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc:
            Do not test writing to pointers for C++20.
            * testsuite/27_io/basic_istream/extractors_character/char/9826.cc:
            Use array instead of pointer.
            * testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc:
            Do not use variable-length array.
            * testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc:
            Do not run test for C++20.
            * testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc:
            Do not test writing to pointers for C++20.
            * testsuite/27_io/basic_istream/extractors_character/char/lwg2499.cc:
            New test.
            * testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc:
            New test.
            * testsuite/27_io/basic_istream/extractors_character/char/overflow.cc:
            New test.
            * testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499.cc:
            New test.
            * testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc:
            New test.

diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 17aff5d907b..b6ce76c1f20 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -2304,6 +2304,11 @@ GLIBCXX_3.4.29 {
     # std::from_chars
     _ZSt10from_charsPKcS0_R[def]St12chars_format;
 
+    # std::__istream_extract(istream&, char*, streamsize)
+    _ZSt17__istream_extractRSiPc[ilx];
+    # std::__istream_extract(wistream&, wchar_t*, streamsize)
+    _ZSt17__istream_extractIwSt11char_traitsIwEEvRSt13basic_istreamIT_T0_EPS3_[ilx];
+
 } GLIBCXX_3.4.28;
 
 # Symbols in the support library (libsupc++) have their own tag.
diff --git a/libstdc++-v3/include/bits/istream.tcc b/libstdc++-v3/include/bits/istream.tcc
index 0289867c50b..b8f530f6ef5 100644
--- a/libstdc++-v3/include/bits/istream.tcc
+++ b/libstdc++-v3/include/bits/istream.tcc
@@ -986,8 +986,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     }
 
   template<typename _CharT, typename _Traits>
-    basic_istream<_CharT, _Traits>&
-    operator>>(basic_istream<_CharT, _Traits>& __in, _CharT* __s)
+    void
+    __istream_extract(basic_istream<_CharT, _Traits>& __in, _CharT* __s,
+		      streamsize __num)
     {
       typedef basic_istream<_CharT, _Traits>		__istream_type;
       typedef basic_streambuf<_CharT, _Traits>          __streambuf_type;
@@ -1003,9 +1004,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  __try
 	    {
 	      // Figure out how many characters to extract.
-	      streamsize __num = __in.width();
-	      if (__num <= 0)
-		__num = __gnu_cxx::__numeric_traits<streamsize>::__max;
+	      streamsize __width = __in.width();
+	      if (0 < __width && __width < __num)
+		__num = __width;
 
 	      const __ctype_type& __ct = use_facet<__ctype_type>(__in.getloc());
 
@@ -1042,7 +1043,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	__err |= ios_base::failbit;
       if (__err)
 	__in.setstate(__err);
-      return __in;
     }
 
   // 27.6.1.4 Standard basic_istream manipulators
@@ -1075,11 +1075,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   extern template class basic_istream<char>;
   extern template istream& ws(istream&);
   extern template istream& operator>>(istream&, char&);
-  extern template istream& operator>>(istream&, char*);
   extern template istream& operator>>(istream&, unsigned char&);
   extern template istream& operator>>(istream&, signed char&);
-  extern template istream& operator>>(istream&, unsigned char*);
-  extern template istream& operator>>(istream&, signed char*);
 
   extern template istream& istream::_M_extract(unsigned short&);
   extern template istream& istream::_M_extract(unsigned int&);  
@@ -1101,7 +1098,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   extern template class basic_istream<wchar_t>;
   extern template wistream& ws(wistream&);
   extern template wistream& operator>>(wistream&, wchar_t&);
-  extern template wistream& operator>>(wistream&, wchar_t*);
+  extern template void __istream_extract(wistream&, wchar_t*, streamsize);
 
   extern template wistream& wistream::_M_extract(unsigned short&);
   extern template wistream& wistream::_M_extract(unsigned int&);  
diff --git a/libstdc++-v3/include/std/istream b/libstdc++-v3/include/std/istream
index 407c1ccda49..cb8e9f87c90 100644
--- a/libstdc++-v3/include/std/istream
+++ b/libstdc++-v3/include/std/istream
@@ -762,41 +762,52 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     { return (__in >> reinterpret_cast<char&>(__c)); }
   //@}
 
+
+  template<typename _CharT, typename _Traits>
+    void
+    __istream_extract(basic_istream<_CharT, _Traits>&, _CharT*, streamsize);
+
+  void __istream_extract(istream&, char*, streamsize);
+
   //@{
   /**
    *  @brief  Character string extractors
    *  @param  __in  An input stream.
-   *  @param  __s  A pointer to a character array.
+   *  @param  __s  A character array (or a pointer to an array before C++20).
    *  @return  __in
    *
    *  Behaves like one of the formatted arithmetic extractors described in
-   *  std::basic_istream.  After constructing a sentry object with good
-   *  status, this function extracts up to @c n characters and stores them
-   *  into the array starting at @a __s.  @c n is defined as:
+   *  `std::basic_istream`.  After constructing a sentry object with good
+   *  status, this function extracts up to `n` characters and stores them
+   *  into the array `__s`.  `n` is defined as:
    *
-   *  - if @c width() is greater than zero, @c n is width() otherwise
-   *  - @c n is <em>the number of elements of the largest array of *
-   *  - @c char_type that can store a terminating @c eos.</em>
-   *  - [27.6.1.2.3]/6
+   *  - if `width()` is greater than zero, `n` is `min(width(), n)`
+   *  - otherwise `n` is the number of elements of the array
+   *  - (before C++20 the pointer is assumed to point to an array of
+   *  - the largest possible size for an array of `char_type`).
    *
    *  Characters are extracted and stored until one of the following happens:
-   *  - @c n-1 characters are stored
+   *  - `n - 1` characters are stored
    *  - EOF is reached
    *  - the next character is whitespace according to the current locale
-   *  - the next character is a null byte (i.e., @c charT() )
+   *  - the next character is a null byte (i.e., `charT()`)
    *
-   *  @c width(0) is then called for the input stream.
+   *  `width(0)` is then called for the input stream.
    *
    *  If no characters are extracted, sets failbit.
   */
-  template<typename _CharT, typename _Traits>
-    basic_istream<_CharT, _Traits>&
-    operator>>(basic_istream<_CharT, _Traits>& __in, _CharT* __s);
 
-  // Explicit specialization declaration, defined in src/istream.cc.
-  template<>
-    basic_istream<char>&
-    operator>>(basic_istream<char>& __in, char* __s);
+#if __cplusplus <= 201703L
+  template<typename _CharT, typename _Traits>
+    inline basic_istream<_CharT, _Traits>&
+    operator>>(basic_istream<_CharT, _Traits>& __in, _CharT* __s)
+    {
+      streamsize __n = __builtin_object_size(__s, 2) / sizeof(_CharT);
+      if (__n == 0)
+	__n = __gnu_cxx::__numeric_traits<streamsize>::__max / sizeof(_CharT);
+      std::__istream_extract(__in, __s, __n);
+      return __in;
+    }
 
   template<class _Traits>
     inline basic_istream<char, _Traits>&
@@ -807,6 +818,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline basic_istream<char, _Traits>&
     operator>>(basic_istream<char, _Traits>& __in, signed char* __s)
     { return (__in >> reinterpret_cast<char*>(__s)); }
+#else
+  // _GLIBCXX_RESOLVE_LIB_DEFECTS
+  // 2499. operator>>(istream&, char*) makes it hard to avoid buffer overflows
+  template<typename _CharT, typename _Traits, size_t _Num>
+    inline basic_istream<_CharT, _Traits>&
+    operator>>(basic_istream<_CharT, _Traits>& __in, _CharT (&__s)[_Num])
+    {
+      static_assert(_Num <= __gnu_cxx::__numeric_traits<streamsize>::__max);
+      std::__istream_extract(__in, __s, _Num);
+      return __in;
+    }
+
+  template<class _Traits, size_t _Num>
+    inline basic_istream<char, _Traits>&
+    operator>>(basic_istream<char, _Traits>& __in, unsigned char (&__s)[_Num])
+    { return __in >> reinterpret_cast<char(&)[_Num]>(__s); }
+
+  template<class _Traits, size_t _Num>
+    inline basic_istream<char, _Traits>&
+    operator>>(basic_istream<char, _Traits>& __in, signed char (&__s)[_Num])
+    { return __in >> reinterpret_cast<char(&)[_Num]>(__s); }
+#endif
   //@}
 
   /**
diff --git a/libstdc++-v3/include/std/streambuf b/libstdc++-v3/include/std/streambuf
index f8e4cb9879c..3e512364b86 100644
--- a/libstdc++-v3/include/std/streambuf
+++ b/libstdc++-v3/include/std/streambuf
@@ -166,9 +166,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 					       void>::__type
         advance(istreambuf_iterator<_CharT2>&, _Distance);
 
-      template<typename _CharT2, typename _Traits2>
-        friend basic_istream<_CharT2, _Traits2>&
-        operator>>(basic_istream<_CharT2, _Traits2>&, _CharT2*);
+      friend void __istream_extract(istream&, char*, streamsize);
 
       template<typename _CharT2, typename _Traits2, typename _Alloc>
         friend basic_istream<_CharT2, _Traits2>&
diff --git a/libstdc++-v3/src/c++11/istream-inst.cc b/libstdc++-v3/src/c++11/istream-inst.cc
index 2ecfd512b73..2262db6f0cc 100644
--- a/libstdc++-v3/src/c++11/istream-inst.cc
+++ b/libstdc++-v3/src/c++11/istream-inst.cc
@@ -38,9 +38,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template istream& operator>>(istream&, char&);
   template istream& operator>>(istream&, unsigned char&);
   template istream& operator>>(istream&, signed char&);
+
+#if ! _GLIBCXX_INLINE_VERSION
+  // XXX GLIBCXX_ABI Deprecated
   template istream& operator>>(istream&, char*);
   template istream& operator>>(istream&, unsigned char*);
   template istream& operator>>(istream&, signed char*);
+#endif
 
   template istream& operator>>(istream&, _Setfill<char>);
   template istream& operator>>(istream&, _Setiosflags);
@@ -67,7 +71,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template class basic_istream<wchar_t>;
   template wistream& ws(wistream&);
   template wistream& operator>>(wistream&, wchar_t&);
+  template void __istream_extract(wistream&, wchar_t*, streamsize);
+
+#if ! _GLIBCXX_INLINE_VERSION
+  // XXX GLIBCXX_ABI Deprecated
   template wistream& operator>>(wistream&, wchar_t*);
+#endif
 
   template wistream& operator>>(wistream&, _Setfill<wchar_t>);
   template wistream& operator>>(wistream&, _Setiosflags);
diff --git a/libstdc++-v3/src/c++98/istream.cc b/libstdc++-v3/src/c++98/istream.cc
index d2c6794be73..7a48779d337 100644
--- a/libstdc++-v3/src/c++98/istream.cc
+++ b/libstdc++-v3/src/c++98/istream.cc
@@ -204,9 +204,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return *this;
     }
 
-  template<>
-    basic_istream<char>&
-    operator>>(basic_istream<char>& __in, char* __s)
+    void
+    __istream_extract(istream& __in, char* __s, streamsize __num)
     {
       typedef basic_istream<char>       	__istream_type;
       typedef __istream_type::int_type		__int_type;
@@ -223,9 +222,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  __try
 	    {
 	      // Figure out how many characters to extract.
-	      streamsize __num = __in.width();
-	      if (__num <= 0)
-		__num = __gnu_cxx::__numeric_traits<streamsize>::__max;
+	      streamsize __width = __in.width();
+	      if (0 < __width && __width < __num)
+		__num = __width;
 
 	      const __ctype_type& __ct = use_facet<__ctype_type>(__in.getloc());
 
@@ -282,7 +281,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	__err |= ios_base::failbit;
       if (__err)
 	__in.setstate(__err);
-      return __in;
     }
 
 #ifdef _GLIBCXX_USE_WCHAR_T
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/3.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/3.cc
index 2b7f086cbfb..32020b1ba00 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/3.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/3.cc
@@ -39,7 +39,7 @@ void test01()
 
   // template<_CharT, _Traits>
   //  basic_istream& operator>>(istream&, _CharT*)
-  int n = 20;
+  const int n = 20;
   char array1[n];
   typedef std::ios::traits_type ctraits_type;
 
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/4.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/4.cc
index c7c5401da38..9e427cc045f 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/4.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/4.cc
@@ -19,6 +19,7 @@
 
 // 27.6.1.2.3 basic_istream::operator>>
 
+// { dg-do run { target { ! c++20 } } }
 // { dg-require-fileio "" }
 
 #include <istream>
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc
index c38b7ea023d..d5d86c635df 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc
@@ -60,9 +60,11 @@ int main()
   testthrow(c);
   testthrow(uc);
   testthrow(sc);
+#if __cplusplus <= 201703L
   testthrow(cp);
   testthrow(scp);
   testthrow(ucp);
+#endif
 
   return 0;
 }
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/9826.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/9826.cc
index 07ee2511058..bc77f7bfb54 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/9826.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/9826.cc
@@ -39,7 +39,7 @@ void test02()
   sstr >> str;
 
   // 2
-  pod_char*  chr = 0;
+  pod_char  chr[1];
   sstr >> chr;
 
   // 3
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/lwg2499.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/lwg2499.cc
new file mode 100644
index 00000000000..d77b7114583
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/lwg2499.cc
@@ -0,0 +1,80 @@
+// Copyright (C) 2020 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
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++2a" }
+// { dg-do run { target c++2a } }
+
+// LWG 2499
+// operator>>(basic_istream&, CharT*) makes it hard to avoid buffer overflows
+
+#include <sstream>
+#include <testsuite_hooks.h>
+
+template<typename T>
+void
+test(std::basic_istream<char, T>& in)
+{
+  char pc[3];
+  in >> pc;
+  VERIFY( in.good() );
+  VERIFY( pc[0] == 'a' && pc[1] == 'b' && pc[2] == '\0' );
+
+  signed char sc[4];
+  in >> sc;
+  VERIFY( in.good() );
+  VERIFY( sc[0] == 'c' && sc[1] == 'd' && sc[2] == 'e' && sc[3] ==  '\0' );
+
+  unsigned char uc[4];
+  in >> uc;
+  VERIFY( in.good() );
+  VERIFY( uc[0] == 'f' && uc[1] == 'g' && uc[2] == 'h' && uc[3] ==  '\0' );
+
+  pc[2] = '#';
+  in >> pc;
+  VERIFY( in.good() );
+  VERIFY( pc[0] == 'i' && pc[1] == '\0' && pc[2] == '#' );
+
+  in >> pc;
+  VERIFY( in.good() );
+  VERIFY( pc[0] == 'j' && pc[1] == 'k' && pc[2] == '\0' );
+
+  pc[2] = '#';
+  in >> pc;
+  VERIFY( in.eof() );
+  VERIFY( pc[0] == 'l' && pc[1] == '\0' && pc[2] == '#' );
+}
+
+void
+test01()
+{
+  std::istringstream in("abcdefghi jk l");
+  test(in);
+}
+
+void
+test02()
+{
+  struct CT : std::char_traits<char> { };
+  std::basic_istringstream<char, CT> in("abcdefghi jk l");
+  test(in);
+}
+
+int main()
+{
+  test01();
+  test02();
+}
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc
new file mode 100644
index 00000000000..2c2bd521d5c
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc
@@ -0,0 +1,45 @@
+// Copyright (C) 2020 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
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++2a" }
+// { dg-do compile { target c++2a } }
+
+// LWG 2499
+// operator>>(basic_istream&, CharT*) makes it hard to avoid buffer overflows
+
+#include <istream>
+
+void
+test01(std::istream& in, char* pc, signed char* sc, unsigned char* uc)
+{
+  in >> pc; // { dg-error "here" }
+  in >> sc; // { dg-error "here" }
+  in >> uc; // { dg-error "here" }
+}
+
+struct CT : std::char_traits<char> { };
+
+void
+test02(std::basic_istream<char, CT>& in, char* pc, signed char* sc,
+       unsigned char* uc)
+{
+  in >> pc; // { dg-error "here" }
+  in >> sc; // { dg-error "here" }
+  in >> uc; // { dg-error "here" }
+}
+
+// { dg-excess-errors "" }
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/overflow.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/overflow.cc
new file mode 100644
index 00000000000..1141a41b208
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/overflow.cc
@@ -0,0 +1,64 @@
+// Copyright (C) 2020 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
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-O2 -std=gnu++98" }
+// { dg-do run }
+
+// This test checks that operator>> will avoid a buffer overflow when
+// reading into a buffer with a size that is known at compile time.
+
+// Since C++20 this is guaranteed (see LWG 2499), for previous standards
+// we try to check the buffer size as an extension (which depends on -O2).
+
+#include <sstream>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  std::istringstream in("foolish child");
+  char pc[5];
+  in >> pc;
+  VERIFY( in.good() );
+  VERIFY( std::string(pc) == "fool" );
+}
+
+void
+test02()
+{
+  std::istringstream in("foolish");
+  signed char sc[5];
+  in >> sc;
+  VERIFY( in.good() );
+  VERIFY( std::string((const char*)sc) == "fool" );
+}
+
+void
+test03()
+{
+  std::istringstream in("foolish");
+  unsigned char uc[5];
+  in >> uc;
+  VERIFY( in.good() );
+  VERIFY( std::string((const char*)uc) == "fool" );
+}
+
+int
+main()
+{
+  test01();
+}
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc
index 0ba58f74ac4..5ee3ee16692 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc
@@ -37,7 +37,7 @@ void test01()
 
   // template<_CharT, _Traits>
   //  basic_istream& operator>>(istream&, _CharT*)
-  int n = 20;
+  const int n = 20;
   wchar_t array1[n];
   typedef std::wios::traits_type ctraits_type;
 
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc
index 6c3ee647349..8414d62eee3 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc
@@ -20,6 +20,7 @@
 // 27.6.1.2.3 basic_istream::operator>>
 
 // { dg-options "-DMAX_SIZE=466" { target simulator } }
+// { dg-do run { target { ! c++20 } } }
 // { dg-require-fileio "" }
 
 #ifndef MAX_SIZE
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc
index c4935b7c6ca..1a9f9aa56ba 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc
@@ -54,7 +54,9 @@ int main()
   wchar_t* cp = &c;
 
   testthrow(c);
+#if __cplusplus <= 201703L
   testthrow(cp);
+#endif
 
   return 0;
 }
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499.cc
new file mode 100644
index 00000000000..e1d42b4bc42
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499.cc
@@ -0,0 +1,70 @@
+// Copyright (C) 2020 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
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++2a" }
+// { dg-do compile { target c++2a } }
+
+// LWG 2499
+// operator>>(basic_istream&, CharT*) makes it hard to avoid buffer overflows
+
+#include <sstream>
+#include <testsuite_hooks.h>
+
+template<typename T>
+void
+test(std::basic_istream<wchar_t, T>& in)
+{
+  wchar_t wc[3];
+  in >> wc;
+  VERIFY( in.good() );
+  VERIFY( wc[0] == L'a' && wc[1] == L'b' && wc[2] == L'\0' );
+
+  wc[2] = L'#';
+  in >> wc;
+  VERIFY( in.good() );
+  VERIFY( wc[0] == L'c' && wc[1] == L'\0' && wc[2] == L'#' );
+
+  in >> wc;
+  VERIFY( in.good() );
+  VERIFY( wc[0] == L'd' && wc[1] == L'\0' && wc[2] == L'#' );
+
+  wc[2] = L'#';
+  in >> wc;
+  VERIFY( in.eof() );
+  VERIFY( wc[0] == L'e' && wc[1] == L'\0' && wc[2] == L'#' );
+}
+
+void
+test01()
+{
+  std::wistringstream in(L"abc d e");
+  test(in);
+}
+
+void
+test02()
+{
+  struct WT : std::char_traits<wchar_t> { };
+  std::basic_istringstream<wchar_t, WT> in(L"abc d e");
+  test(in);
+}
+
+int main()
+{
+  test01();
+  test02();
+}
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc
new file mode 100644
index 00000000000..676cdee8297
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc
@@ -0,0 +1,40 @@
+// Copyright (C) 2020 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
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++2a" }
+// { dg-do compile { target c++2a } }
+
+// LWG 2499
+// operator>>(basic_istream&, CharT*) makes it hard to avoid buffer overflows
+
+#include <istream>
+
+void
+test01(std::wistream& in, wchar_t* wc)
+{
+  in >> wc; // { dg-error "here" }
+}
+
+struct WT : std::char_traits<wchar_t> { };
+
+void
+test02(std::basic_istream<wchar_t, WT>& in, wchar_t* wc)
+{
+  in >> wc; // { dg-error "here" }
+}
+
+// { dg-excess-errors "" }

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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2802 bytes --]

On 05/08/20 22:25 +0100, Jonathan Wakely wrote:
>P0487R1 resolved LWG 2499 for C++20 by removing the operator>> overloads
>that have high risk of buffer overflows. They were replaced by
>equivalents that only accept a reference to an array, and so can
>guarantee not to write past the end of the array.
>
>In order to support both the old and new functionality, this patch
>introduces a new overloaded __istream_extract function which takes a
>maximum length. The new operator>> overloads use the array size as the
>maximum length. The old overloads now use __builtin_object_size to
>determine the available buffer size if available (which requires -O2) or
>use numeric_limits<streamsize>::max()/sizeof(char_type) otherwise. This
>is a change in behaviour, as the old overloads previously always used
>numeric_limits<streamsize>::max(), without considering sizeof(char_type)
>and without attempting to prevent overflows.
>
>Because they now do little more than call __istream_extract, the old
>operator>> overloads are very small inline functions. This means there
>is no advantage to explicitly instantiating them in the library (in fact
>that would prevent the __builtin_object_size checks from ever working).
>As a result, the explicit instantiation declarations can be removed from
>the header. The explicit instantiation definitions are still needed, for
>backwards compatibility with existing code that expects to link to the
>definitions in the library.
>
>While working on this change I noticed that src/c++11/istream-inst.cc
>has the following explicit instantiation definition:
>  template istream& operator>>(istream&, char*);
>This had no effect (and so should not have been present in that file),
>because there was an explicit specialization declared in <istream> and
>defined in src/++98/istream.cc. However, this change removes the
>explicit specialization, and now the explicit instantiation definition
>is necessary to ensure the symbol gets defined in the library.

>Martin, Jakub, could you please double-check the usage of
>__builtin_object_size? (line 805 in libstdc++-v3/include/std/istream)
>Do you see any problems with using it here? If it can't tell the size
>then we just assume it's larger than the string to be extracted, which
>is what the old code did anyway. I do have an idea for stronger
>checking in debug mode, which I'll post in a minute.


The attached (uncommitted and not fully tested) patch would make the
new __istream_extract functions return a bool indicating whether
extraction stopped because the maximum number of characters was
reached (as opposed to reaching EOF or whitespace).

This would allow us to abort the program with a debug mode assertion
if limiting the size to the __builtin_object_size value prevented a
buffer overflow from occurring.




[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 4810 bytes --]

diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index b6ce76c1f20..a7d8ee83ce3 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -2307,7 +2307,7 @@ GLIBCXX_3.4.29 {
     # std::__istream_extract(istream&, char*, streamsize)
     _ZSt17__istream_extractRSiPc[ilx];
     # std::__istream_extract(wistream&, wchar_t*, streamsize)
-    _ZSt17__istream_extractIwSt11char_traitsIwEEvRSt13basic_istreamIT_T0_EPS3_[ilx];
+    _ZSt17__istream_extractIwSt11char_traitsIwEEbRSt13basic_istreamIT_T0_EPS3_[ilx];
 
 } GLIBCXX_3.4.28;
 
diff --git a/libstdc++-v3/include/bits/istream.tcc b/libstdc++-v3/include/bits/istream.tcc
index b8f530f6ef5..cb780999ff7 100644
--- a/libstdc++-v3/include/bits/istream.tcc
+++ b/libstdc++-v3/include/bits/istream.tcc
@@ -986,7 +986,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     }
 
   template<typename _CharT, typename _Traits>
-    void
+    bool
     __istream_extract(basic_istream<_CharT, _Traits>& __in, _CharT* __s,
 		      streamsize __num)
     {
@@ -1043,6 +1043,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	__err |= ios_base::failbit;
       if (__err)
 	__in.setstate(__err);
+      return __extracted == __num - 1;
     }
 
   // 27.6.1.4 Standard basic_istream manipulators
@@ -1098,7 +1099,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   extern template class basic_istream<wchar_t>;
   extern template wistream& ws(wistream&);
   extern template wistream& operator>>(wistream&, wchar_t&);
-  extern template void __istream_extract(wistream&, wchar_t*, streamsize);
+  extern template bool __istream_extract(wistream&, wchar_t*, streamsize);
 
   extern template wistream& wistream::_M_extract(unsigned short&);
   extern template wistream& wistream::_M_extract(unsigned int&);  
diff --git a/libstdc++-v3/include/std/istream b/libstdc++-v3/include/std/istream
index cb8e9f87c90..16562e74706 100644
--- a/libstdc++-v3/include/std/istream
+++ b/libstdc++-v3/include/std/istream
@@ -764,10 +764,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 
   template<typename _CharT, typename _Traits>
-    void
+    bool
     __istream_extract(basic_istream<_CharT, _Traits>&, _CharT*, streamsize);
 
-  void __istream_extract(istream&, char*, streamsize);
+  bool __istream_extract(istream&, char*, streamsize);
 
   //@{
   /**
@@ -805,6 +805,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       streamsize __n = __builtin_object_size(__s, 2) / sizeof(_CharT);
       if (__n == 0)
 	__n = __gnu_cxx::__numeric_traits<streamsize>::__max / sizeof(_CharT);
+#if _GLIBCXX_DEBUG
+      else
+	{
+	  streamsize __w = __in.width();
+	  if (0 <= __w || __w > __n)
+	    {
+	      if (std::__istream_extract(__in, __s, __n) && !__in.eof())
+		{
+		  _CharT __c = _Traits::to_char_type(__in.peek());
+		  _GLIBCXX_DEBUG_ASSERT(std::isspace(__c, __in.getloc()));
+		}
+	      return __in;
+	    }
+	}
+#endif
       std::__istream_extract(__in, __s, __n);
       return __in;
     }
diff --git a/libstdc++-v3/include/std/streambuf b/libstdc++-v3/include/std/streambuf
index 3e512364b86..07dff01344e 100644
--- a/libstdc++-v3/include/std/streambuf
+++ b/libstdc++-v3/include/std/streambuf
@@ -166,7 +166,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 					       void>::__type
         advance(istreambuf_iterator<_CharT2>&, _Distance);
 
-      friend void __istream_extract(istream&, char*, streamsize);
+      friend bool __istream_extract(istream&, char*, streamsize);
 
       template<typename _CharT2, typename _Traits2, typename _Alloc>
         friend basic_istream<_CharT2, _Traits2>&
diff --git a/libstdc++-v3/src/c++11/istream-inst.cc b/libstdc++-v3/src/c++11/istream-inst.cc
index 20d9158e770..a6bc79dd39c 100644
--- a/libstdc++-v3/src/c++11/istream-inst.cc
+++ b/libstdc++-v3/src/c++11/istream-inst.cc
@@ -71,7 +71,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template class basic_istream<wchar_t>;
   template wistream& ws(wistream&);
   template wistream& operator>>(wistream&, wchar_t&);
-  template void __istream_extract(wistream&, wchar_t*, streamsize);
+  template bool __istream_extract(wistream&, wchar_t*, streamsize);
 
 #ifndef _GLIBCXX_INLINE_VERSION
   // XXX GLIBCXX_ABI Deprecated
diff --git a/libstdc++-v3/src/c++98/istream.cc b/libstdc++-v3/src/c++98/istream.cc
index 7a48779d337..5e1119107bc 100644
--- a/libstdc++-v3/src/c++98/istream.cc
+++ b/libstdc++-v3/src/c++98/istream.cc
@@ -204,7 +204,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return *this;
     }
 
-    void
+    bool
     __istream_extract(istream& __in, char* __s, streamsize __num)
     {
       typedef basic_istream<char>       	__istream_type;
@@ -281,6 +281,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	__err |= ios_base::failbit;
       if (__err)
 	__in.setstate(__err);
+      return __extracted == __num - 1;
     }
 
 #ifdef _GLIBCXX_USE_WCHAR_T

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

[Martin Sebor]

On 8/5/20 3:25 PM, Jonathan Wakely wrote:
> P0487R1 resolved LWG 2499 for C++20 by removing the operator>> overloads
> that have high risk of buffer overflows. They were replaced by
> equivalents that only accept a reference to an array, and so can
> guarantee not to write past the end of the array.
> 
> In order to support both the old and new functionality, this patch
> introduces a new overloaded __istream_extract function which takes a
> maximum length. The new operator>> overloads use the array size as the
> maximum length. The old overloads now use __builtin_object_size to
> determine the available buffer size if available (which requires -O2) or
> use numeric_limits<streamsize>::max()/sizeof(char_type) otherwise. This
> is a change in behaviour, as the old overloads previously always used
> numeric_limits<streamsize>::max(), without considering sizeof(char_type)
> and without attempting to prevent overflows.
> 
> Because they now do little more than call __istream_extract, the old
> operator>> overloads are very small inline functions. This means there
> is no advantage to explicitly instantiating them in the library (in fact
> that would prevent the __builtin_object_size checks from ever working).
> As a result, the explicit instantiation declarations can be removed from
> the header. The explicit instantiation definitions are still needed, for
> backwards compatibility with existing code that expects to link to the
> definitions in the library.
> 
> While working on this change I noticed that src/c++11/istream-inst.cc
> has the following explicit instantiation definition:
>    template istream& operator>>(istream&, char*);
> This had no effect (and so should not have been present in that file),
> because there was an explicit specialization declared in <istream> and
> defined in src/++98/istream.cc. However, this change removes the
> explicit specialization, and now the explicit instantiation definition
> is necessary to ensure the symbol gets defined in the library.
> 
> libstdc++-v3/ChangeLog:
> 
> 	* config/abi/pre/gnu.ver (GLIBCXX_3.4.29): Export new symbols.
> 	* include/bits/istream.tcc (__istream_extract): New function
> 	template implementing both of operator>>(istream&, char*) and
> 	operator>>(istream&, char(&)[N]). Add explicit instantiation
> 	declaration for it. Remove explicit instantiation declarations
> 	for old function templates.
> 	* include/std/istream (__istream_extract): Declare.
> 	(operator>>(basic_istream<C,T>&, C*)): Define inline and simply
> 	call __istream_extract.
> 	(operator>>(basic_istream<char,T>&, signed char*)): Likewise.
> 	(operator>>(basic_istream<char,T>&, unsigned char*)): Likewise.
> 	(operator>>(basic_istream<C,T>&, C(7)[N])): Define for LWG 2499.
> 	(operator>>(basic_istream<char,T>&, signed char(&)[N])):
> 	Likewise.
> 	(operator>>(basic_istream<char,T>&, unsigned char(&)[N])):
> 	Likewise.
> 	* include/std/streambuf (basic_streambuf): Declare char overload
> 	of __istream_extract as a friend.
> 	* src/c++11/istream-inst.cc: Add explicit instantiation
> 	definition for wchar_t overload of __istream_extract. Remove
> 	explicit instantiation definitions of old operator>> overloads
> 	for versioned-namespace build.
> 	* src/c++98/istream.cc (operator>>(istream&, char*)): Replace
> 	with __istream_extract(istream&, char*, streamsize).
> 	* testsuite/27_io/basic_istream/extractors_character/char/3.cc:
> 	Do not use variable-length array.
> 	* testsuite/27_io/basic_istream/extractors_character/char/4.cc:
> 	Do not run test for C++20.
> 	* testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc:
> 	Do not test writing to pointers for C++20.
> 	* testsuite/27_io/basic_istream/extractors_character/char/9826.cc:
> 	Use array instead of pointer.
> 	* testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc:
> 	Do not use variable-length array.
> 	* testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc:
> 	Do not run test for C++20.
> 	* testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc:
> 	Do not test writing to pointers for C++20.
> 	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499.cc:
> 	New test.
> 	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc:
> 	New test.
> 	* testsuite/27_io/basic_istream/extractors_character/char/overflow.cc:
> 	New test.
> 	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499.cc:
> 	New test.
> 	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc:
> 	New test.
> 
> Tested powerpc64le-linux. Committed to trunk.
> 
> Martin, Jakub, could you please double-check the usage of
> __builtin_object_size? (line 805 in libstdc++-v3/include/std/istream)
> Do you see any problems with using it here? If it can't tell the size
> then we just assume it's larger than the string to be extracted, which
> is what the old code did anyway. I do have an idea for stronger
> checking in debug mode, which I'll post in a minute.

I've always found the second argument to __builtin_object_size
confusing for types above 1.  I don't see anything wrong in
the diff but I believe the most useful results are with type 1
for string functions and type 0 for raw memory functions like
memcpy (that's what _FORTIFY_SOURCE uses for the two sets of
functions).  In type 2 when the result is zero it means one of
two things: either the size of the array couldn't be determined
or it really is zero.  That's less than helpful in cases like:

   char a[8];
   strcpy (a + 8, s);

where it prevents detecting the buffer overflow.

I would suggest to use type 1 in the iostream functions instead.

Adding attribute access to the __istream_extract overloads should
also let GCC check for out-of-bounds accesses by the function and
issue warnings.  This goes beyond what __builtin_object_size makes
possible (it considers also variable sizes in known ranges).

Martin

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

[Jakub Jelinek]

On Wed, Aug 05, 2020 at 04:31:08PM -0600, Martin Sebor via Gcc-patches wrote:
> I've always found the second argument to __builtin_object_size
> confusing for types above 1.  I don't see anything wrong in
> the diff but I believe the most useful results are with type 1
> for string functions and type 0 for raw memory functions like
> memcpy (that's what _FORTIFY_SOURCE uses for the two sets of
> functions).  In type 2 when the result is zero it means one of
> two things: either the size of the array couldn't be determined
> or it really is zero.  That's less than helpful in cases like:
> 
>   char a[8];
>   strcpy (a + 8, s);
> 
> where it prevents detecting the buffer overflow.

I don't know what is confusing about it.
With the 0/1 arguments bos returns an upper bound for the object size
(and the don't know value is the maximum in that case, i.e. (size_t)-1),
while with 2/3 arguments bos returns an lower bound for the object size
(and thus the don't know value is the minimum value, i.e. 0).
The 2/3 modes are obviously not something you want to use in strcpy etc.
implementation, in those cases you want to abort the program only when
it is guaranteed to be invalid, i.e. when it will certainly overflow
the available size in any case, while with the 2/3 modes it would abort already
if there is a possibility the object might not be big enough.
One can e.g. use both modes to check if the object is known to have exactly
a particular size, when
__builtin_object_size (ptr, 0) == __builtin_object_size (ptr, 2)
and the bos returns say 25, then you know it is exactly 25 bytes.
E.g. if one has:
  ptr = flag ? malloc (32) : malloc (64);
  x[0] = __builtin_object_size (ptr, 0);
  x[1] = __builtin_object_size (ptr, 2);
then x[0] will be 64 as the maximum and x[1] to 32 as the minimum (of course
unless flag can be folded to constant, then both would be the same depending
on to which constant it is folded).

	Jakub

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

[Jonathan Wakely]

On 06/08/20 12:12 +0200, Jakub Jelinek wrote:
>On Wed, Aug 05, 2020 at 04:31:08PM -0600, Martin Sebor via Gcc-patches wrote:
>> I've always found the second argument to __builtin_object_size
>> confusing for types above 1.  I don't see anything wrong in
>> the diff but I believe the most useful results are with type 1
>> for string functions and type 0 for raw memory functions like
>> memcpy (that's what _FORTIFY_SOURCE uses for the two sets of
>> functions).  In type 2 when the result is zero it means one of
>> two things: either the size of the array couldn't be determined
>> or it really is zero.  That's less than helpful in cases like:
>>
>>   char a[8];
>>   strcpy (a + 8, s);
>>
>> where it prevents detecting the buffer overflow.
>
>I don't know what is confusing about it.

Personally I find the docs very confusing.

"The second bit determines if maximum or minimum of remaining bytes is
computed. "

OK, so is it maximum when the bit is set of maximum when the bit is
clear?

To answer that question I have to go back to the middle of the
previous paragraph and carefully parse it. 

"the returned number is the maximum of remaining byte counts in those
objects if type & 2 is 0 and minimum if nonzero."

This part talks about "if type & 2 is 0" and "nonzero", could we be
consistent and talk about a bit being clear/set, or use bitwise
operator notation, but not flip between the two? And use zero/nonzero
rather than 0/nonzero?

The inconsistency in presentation increases the mental load of parsing
it. I'll propose a patch for those docs when I get time.


>With the 0/1 arguments bos returns an upper bound for the object size
>(and the don't know value is the maximum in that case, i.e. (size_t)-1),
>while with 2/3 arguments bos returns an lower bound for the object size
>(and thus the don't know value is the minimum value, i.e. 0).
>The 2/3 modes are obviously not something you want to use in strcpy etc.
>implementation, in those cases you want to abort the program only when
>it is guaranteed to be invalid, i.e. when it will certainly overflow
>the available size in any case, while with the 2/3 modes it would abort already
>if there is a possibility the object might not be big enough.

For my case I'm not aborting, I'm deciding whether to use the result
from __builtin_object_size or just assume the array is as large as the
entire address space (which is the old behaviour).

I think Martin's right that I should use 0. Technically I could
probably use 1, because for struct S { char buf1[2]; char buf2[2]; };
it would be undefined to write 4 bytes into it, but it "worked" with
previous versions and so I'm choosing to let it keep "working". This
doesn't need to be 100% safe, because the API has been replaced by a
safer one for C++20 anyway.

>One can e.g. use both modes to check if the object is known to have exactly
>a particular size, when
>__builtin_object_size (ptr, 0) == __builtin_object_size (ptr, 2)
>and the bos returns say 25, then you know it is exactly 25 bytes.
>E.g. if one has:
>  ptr = flag ? malloc (32) : malloc (64);
>  x[0] = __builtin_object_size (ptr, 0);
>  x[1] = __builtin_object_size (ptr, 2);
>then x[0] will be 64 as the maximum and x[1] to 32 as the minimum (of course
>unless flag can be folded to constant, then both would be the same depending
>on to which constant it is folded).
>
>	Jakub

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

[Jonathan Wakely]

On 05/08/20 16:31 -0600, Martin Sebor via Libstdc++ wrote:
>On 8/5/20 3:25 PM, Jonathan Wakely wrote:
>>P0487R1 resolved LWG 2499 for C++20 by removing the operator>> overloads
>>that have high risk of buffer overflows. They were replaced by
>>equivalents that only accept a reference to an array, and so can
>>guarantee not to write past the end of the array.
>>
>>In order to support both the old and new functionality, this patch
>>introduces a new overloaded __istream_extract function which takes a
>>maximum length. The new operator>> overloads use the array size as the
>>maximum length. The old overloads now use __builtin_object_size to
>>determine the available buffer size if available (which requires -O2) or
>>use numeric_limits<streamsize>::max()/sizeof(char_type) otherwise. This
>>is a change in behaviour, as the old overloads previously always used
>>numeric_limits<streamsize>::max(), without considering sizeof(char_type)
>>and without attempting to prevent overflows.
>>
>>Because they now do little more than call __istream_extract, the old
>>operator>> overloads are very small inline functions. This means there
>>is no advantage to explicitly instantiating them in the library (in fact
>>that would prevent the __builtin_object_size checks from ever working).
>>As a result, the explicit instantiation declarations can be removed from
>>the header. The explicit instantiation definitions are still needed, for
>>backwards compatibility with existing code that expects to link to the
>>definitions in the library.
>>
>>While working on this change I noticed that src/c++11/istream-inst.cc
>>has the following explicit instantiation definition:
>>   template istream& operator>>(istream&, char*);
>>This had no effect (and so should not have been present in that file),
>>because there was an explicit specialization declared in <istream> and
>>defined in src/++98/istream.cc. However, this change removes the
>>explicit specialization, and now the explicit instantiation definition
>>is necessary to ensure the symbol gets defined in the library.
>>
>>libstdc++-v3/ChangeLog:
>>
>>	* config/abi/pre/gnu.ver (GLIBCXX_3.4.29): Export new symbols.
>>	* include/bits/istream.tcc (__istream_extract): New function
>>	template implementing both of operator>>(istream&, char*) and
>>	operator>>(istream&, char(&)[N]). Add explicit instantiation
>>	declaration for it. Remove explicit instantiation declarations
>>	for old function templates.
>>	* include/std/istream (__istream_extract): Declare.
>>	(operator>>(basic_istream<C,T>&, C*)): Define inline and simply
>>	call __istream_extract.
>>	(operator>>(basic_istream<char,T>&, signed char*)): Likewise.
>>	(operator>>(basic_istream<char,T>&, unsigned char*)): Likewise.
>>	(operator>>(basic_istream<C,T>&, C(7)[N])): Define for LWG 2499.
>>	(operator>>(basic_istream<char,T>&, signed char(&)[N])):
>>	Likewise.
>>	(operator>>(basic_istream<char,T>&, unsigned char(&)[N])):
>>	Likewise.
>>	* include/std/streambuf (basic_streambuf): Declare char overload
>>	of __istream_extract as a friend.
>>	* src/c++11/istream-inst.cc: Add explicit instantiation
>>	definition for wchar_t overload of __istream_extract. Remove
>>	explicit instantiation definitions of old operator>> overloads
>>	for versioned-namespace build.
>>	* src/c++98/istream.cc (operator>>(istream&, char*)): Replace
>>	with __istream_extract(istream&, char*, streamsize).
>>	* testsuite/27_io/basic_istream/extractors_character/char/3.cc:
>>	Do not use variable-length array.
>>	* testsuite/27_io/basic_istream/extractors_character/char/4.cc:
>>	Do not run test for C++20.
>>	* testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc:
>>	Do not test writing to pointers for C++20.
>>	* testsuite/27_io/basic_istream/extractors_character/char/9826.cc:
>>	Use array instead of pointer.
>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc:
>>	Do not use variable-length array.
>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc:
>>	Do not run test for C++20.
>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc:
>>	Do not test writing to pointers for C++20.
>>	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499.cc:
>>	New test.
>>	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc:
>>	New test.
>>	* testsuite/27_io/basic_istream/extractors_character/char/overflow.cc:
>>	New test.
>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499.cc:
>>	New test.
>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc:
>>	New test.
>>
>>Tested powerpc64le-linux. Committed to trunk.
>>
>>Martin, Jakub, could you please double-check the usage of
>>__builtin_object_size? (line 805 in libstdc++-v3/include/std/istream)
>>Do you see any problems with using it here? If it can't tell the size
>>then we just assume it's larger than the string to be extracted, which
>>is what the old code did anyway. I do have an idea for stronger
>>checking in debug mode, which I'll post in a minute.
>
>I've always found the second argument to __builtin_object_size
>confusing for types above 1.  I don't see anything wrong in
>the diff but I believe the most useful results are with type 1
>for string functions and type 0 for raw memory functions like
>memcpy (that's what _FORTIFY_SOURCE uses for the two sets of
>functions).  In type 2 when the result is zero it means one of
>two things: either the size of the array couldn't be determined
>or it really is zero.  That's less than helpful in cases like:
>
>  char a[8];
>  strcpy (a + 8, s);
>
>where it prevents detecting the buffer overflow.
>
>I would suggest to use type 1 in the iostream functions instead.
>
>Adding attribute access to the __istream_extract overloads should
>also let GCC check for out-of-bounds accesses by the function and
>issue warnings.  This goes beyond what __builtin_object_size makes
>possible (it considers also variable sizes in known ranges).

I tried the attached patch with a testcase like:

#include <istream>

void
test01(std::istream& in)
{
   char pc[1];
   in >> (pc + 1); // { dg-warning "writing 1 byte into a region of size 0" }
}

I get a -Wstringop-overflow warning with -O0, but not at -O2. Is that
because the call to operator>> gets inlined, and __builtin_object_size
returns 0, and we call __istream_extract(in, s, 0) which says we won't
write anything, so it's now considered safe?

That's unfortunate, because actually __istream_extract will always
write at least one byte as a null terminator.

So I tried various ways toi try and get the warning back at -O2 and
nothing worked e.g. when __builtin_object_size(__s, 0) returns 0 I
tried calling __istream_extract(__in, __s, 1) which should say I'm
going to write a character, and so should warn if the size is known to
be zero. But no wanring.

So now I'm considering this:

   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.

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

[Jakub Jelinek]

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.

	Jakub

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

[Jonathan Wakely]

On 06/08/20 14:14 +0100, Jonathan Wakely via Libstdc++ wrote:
>On 05/08/20 16:31 -0600, Martin Sebor via Libstdc++ wrote:
>>On 8/5/20 3:25 PM, Jonathan Wakely wrote:
>>>P0487R1 resolved LWG 2499 for C++20 by removing the operator>> overloads
>>>that have high risk of buffer overflows. They were replaced by
>>>equivalents that only accept a reference to an array, and so can
>>>guarantee not to write past the end of the array.
>>>
>>>In order to support both the old and new functionality, this patch
>>>introduces a new overloaded __istream_extract function which takes a
>>>maximum length. The new operator>> overloads use the array size as the
>>>maximum length. The old overloads now use __builtin_object_size to
>>>determine the available buffer size if available (which requires -O2) or
>>>use numeric_limits<streamsize>::max()/sizeof(char_type) otherwise. This
>>>is a change in behaviour, as the old overloads previously always used
>>>numeric_limits<streamsize>::max(), without considering sizeof(char_type)
>>>and without attempting to prevent overflows.
>>>
>>>Because they now do little more than call __istream_extract, the old
>>>operator>> overloads are very small inline functions. This means there
>>>is no advantage to explicitly instantiating them in the library (in fact
>>>that would prevent the __builtin_object_size checks from ever working).
>>>As a result, the explicit instantiation declarations can be removed from
>>>the header. The explicit instantiation definitions are still needed, for
>>>backwards compatibility with existing code that expects to link to the
>>>definitions in the library.
>>>
>>>While working on this change I noticed that src/c++11/istream-inst.cc
>>>has the following explicit instantiation definition:
>>>  template istream& operator>>(istream&, char*);
>>>This had no effect (and so should not have been present in that file),
>>>because there was an explicit specialization declared in <istream> and
>>>defined in src/++98/istream.cc. However, this change removes the
>>>explicit specialization, and now the explicit instantiation definition
>>>is necessary to ensure the symbol gets defined in the library.
>>>
>>>libstdc++-v3/ChangeLog:
>>>
>>>	* config/abi/pre/gnu.ver (GLIBCXX_3.4.29): Export new symbols.
>>>	* include/bits/istream.tcc (__istream_extract): New function
>>>	template implementing both of operator>>(istream&, char*) and
>>>	operator>>(istream&, char(&)[N]). Add explicit instantiation
>>>	declaration for it. Remove explicit instantiation declarations
>>>	for old function templates.
>>>	* include/std/istream (__istream_extract): Declare.
>>>	(operator>>(basic_istream<C,T>&, C*)): Define inline and simply
>>>	call __istream_extract.
>>>	(operator>>(basic_istream<char,T>&, signed char*)): Likewise.
>>>	(operator>>(basic_istream<char,T>&, unsigned char*)): Likewise.
>>>	(operator>>(basic_istream<C,T>&, C(7)[N])): Define for LWG 2499.
>>>	(operator>>(basic_istream<char,T>&, signed char(&)[N])):
>>>	Likewise.
>>>	(operator>>(basic_istream<char,T>&, unsigned char(&)[N])):
>>>	Likewise.
>>>	* include/std/streambuf (basic_streambuf): Declare char overload
>>>	of __istream_extract as a friend.
>>>	* src/c++11/istream-inst.cc: Add explicit instantiation
>>>	definition for wchar_t overload of __istream_extract. Remove
>>>	explicit instantiation definitions of old operator>> overloads
>>>	for versioned-namespace build.
>>>	* src/c++98/istream.cc (operator>>(istream&, char*)): Replace
>>>	with __istream_extract(istream&, char*, streamsize).
>>>	* testsuite/27_io/basic_istream/extractors_character/char/3.cc:
>>>	Do not use variable-length array.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/4.cc:
>>>	Do not run test for C++20.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc:
>>>	Do not test writing to pointers for C++20.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/9826.cc:
>>>	Use array instead of pointer.
>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc:
>>>	Do not use variable-length array.
>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc:
>>>	Do not run test for C++20.
>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc:
>>>	Do not test writing to pointers for C++20.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499.cc:
>>>	New test.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc:
>>>	New test.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/overflow.cc:
>>>	New test.
>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499.cc:
>>>	New test.
>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc:
>>>	New test.
>>>
>>>Tested powerpc64le-linux. Committed to trunk.
>>>
>>>Martin, Jakub, could you please double-check the usage of
>>>__builtin_object_size? (line 805 in libstdc++-v3/include/std/istream)
>>>Do you see any problems with using it here? If it can't tell the size
>>>then we just assume it's larger than the string to be extracted, which
>>>is what the old code did anyway. I do have an idea for stronger
>>>checking in debug mode, which I'll post in a minute.
>>
>>I've always found the second argument to __builtin_object_size
>>confusing for types above 1.  I don't see anything wrong in
>>the diff but I believe the most useful results are with type 1
>>for string functions and type 0 for raw memory functions like
>>memcpy (that's what _FORTIFY_SOURCE uses for the two sets of
>>functions).  In type 2 when the result is zero it means one of
>>two things: either the size of the array couldn't be determined
>>or it really is zero.  That's less than helpful in cases like:
>>
>> char a[8];
>> strcpy (a + 8, s);
>>
>>where it prevents detecting the buffer overflow.
>>
>>I would suggest to use type 1 in the iostream functions instead.
>>
>>Adding attribute access to the __istream_extract overloads should
>>also let GCC check for out-of-bounds accesses by the function and
>>issue warnings.  This goes beyond what __builtin_object_size makes
>>possible (it considers also variable sizes in known ranges).
>
>I tried the attached patch with a testcase like:
>
>#include <istream>
>
>void
>test01(std::istream& in)
>{
>  char pc[1];
>  in >> (pc + 1); // { dg-warning "writing 1 byte into a region of size 0" }
>}
>
>I get a -Wstringop-overflow warning with -O0, but not at -O2. Is that
>because the call to operator>> gets inlined, and __builtin_object_size
>returns 0, and we call __istream_extract(in, s, 0) which says we won't
>write anything, so it's now considered safe?
>
>That's unfortunate, because actually __istream_extract will always
>write at least one byte as a null terminator.
>
>So I tried various ways toi try and get the warning back at -O2 and
>nothing worked e.g. when __builtin_object_size(__s, 0) returns 0 I
>tried calling __istream_extract(__in, __s, 1) which should say I'm
>going to write a character, and so should warn if the size is known to
>be zero. But no wanring.

There is a warning, when calling __istream_extract, but it's
suppressed because it comes from within the library itself.

So optimisation means we inline the user's erroneous call, and
suppress the warning from the (still erroneous) call to the library
internals.

That sucks.

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

[Jonathan Wakely]

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.

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

[Jonathan Wakely]

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.

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

[Martin Sebor]

On 8/6/20 7:30 AM, Jonathan Wakely via Libstdc++ wrote:
> On 06/08/20 14:14 +0100, Jonathan Wakely via Libstdc++ wrote:
>> On 05/08/20 16:31 -0600, Martin Sebor via Libstdc++ wrote:
>>> On 8/5/20 3:25 PM, Jonathan Wakely wrote:
>>>> P0487R1 resolved LWG 2499 for C++20 by removing the operator>> 
>>>> overloads
>>>> that have high risk of buffer overflows. They were replaced by
>>>> equivalents that only accept a reference to an array, and so can
>>>> guarantee not to write past the end of the array.
>>>>
>>>> In order to support both the old and new functionality, this patch
>>>> introduces a new overloaded __istream_extract function which takes a
>>>> maximum length. The new operator>> overloads use the array size as the
>>>> maximum length. The old overloads now use __builtin_object_size to
>>>> determine the available buffer size if available (which requires 
>>>> -O2) or
>>>> use numeric_limits<streamsize>::max()/sizeof(char_type) otherwise. This
>>>> is a change in behaviour, as the old overloads previously always used
>>>> numeric_limits<streamsize>::max(), without considering 
>>>> sizeof(char_type)
>>>> and without attempting to prevent overflows.
>>>>
>>>> Because they now do little more than call __istream_extract, the old
>>>> operator>> overloads are very small inline functions. This means there
>>>> is no advantage to explicitly instantiating them in the library (in 
>>>> fact
>>>> that would prevent the __builtin_object_size checks from ever working).
>>>> As a result, the explicit instantiation declarations can be removed 
>>>> from
>>>> the header. The explicit instantiation definitions are still needed, 
>>>> for
>>>> backwards compatibility with existing code that expects to link to the
>>>> definitions in the library.
>>>>
>>>> While working on this change I noticed that src/c++11/istream-inst.cc
>>>> has the following explicit instantiation definition:
>>>>  template istream& operator>>(istream&, char*);
>>>> This had no effect (and so should not have been present in that file),
>>>> because there was an explicit specialization declared in <istream> and
>>>> defined in src/++98/istream.cc. However, this change removes the
>>>> explicit specialization, and now the explicit instantiation definition
>>>> is necessary to ensure the symbol gets defined in the library.
>>>>
>>>> libstdc++-v3/ChangeLog:
>>>>
>>>>     * config/abi/pre/gnu.ver (GLIBCXX_3.4.29): Export new symbols.
>>>>     * include/bits/istream.tcc (__istream_extract): New function
>>>>     template implementing both of operator>>(istream&, char*) and
>>>>     operator>>(istream&, char(&)[N]). Add explicit instantiation
>>>>     declaration for it. Remove explicit instantiation declarations
>>>>     for old function templates.
>>>>     * include/std/istream (__istream_extract): Declare.
>>>>     (operator>>(basic_istream<C,T>&, C*)): Define inline and simply
>>>>     call __istream_extract.
>>>>     (operator>>(basic_istream<char,T>&, signed char*)): Likewise.
>>>>     (operator>>(basic_istream<char,T>&, unsigned char*)): Likewise.
>>>>     (operator>>(basic_istream<C,T>&, C(7)[N])): Define for LWG 2499.
>>>>     (operator>>(basic_istream<char,T>&, signed char(&)[N])):
>>>>     Likewise.
>>>>     (operator>>(basic_istream<char,T>&, unsigned char(&)[N])):
>>>>     Likewise.
>>>>     * include/std/streambuf (basic_streambuf): Declare char overload
>>>>     of __istream_extract as a friend.
>>>>     * src/c++11/istream-inst.cc: Add explicit instantiation
>>>>     definition for wchar_t overload of __istream_extract. Remove
>>>>     explicit instantiation definitions of old operator>> overloads
>>>>     for versioned-namespace build.
>>>>     * src/c++98/istream.cc (operator>>(istream&, char*)): Replace
>>>>     with __istream_extract(istream&, char*, streamsize).
>>>>     * testsuite/27_io/basic_istream/extractors_character/char/3.cc:
>>>>     Do not use variable-length array.
>>>>     * testsuite/27_io/basic_istream/extractors_character/char/4.cc:
>>>>     Do not run test for C++20.
>>>>     * 
>>>> testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc:
>>>>     Do not test writing to pointers for C++20.
>>>>     * testsuite/27_io/basic_istream/extractors_character/char/9826.cc:
>>>>     Use array instead of pointer.
>>>>     * testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc:
>>>>     Do not use variable-length array.
>>>>     * testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc:
>>>>     Do not run test for C++20.
>>>>     * 
>>>> testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc:
>>>>     Do not test writing to pointers for C++20.
>>>>     * 
>>>> testsuite/27_io/basic_istream/extractors_character/char/lwg2499.cc:
>>>>     New test.
>>>>     * 
>>>> testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc:
>>>>     New test.
>>>>     * 
>>>> testsuite/27_io/basic_istream/extractors_character/char/overflow.cc:
>>>>     New test.
>>>>     * 
>>>> testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499.cc:
>>>>     New test.
>>>>     * 
>>>> testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc: 
>>>>
>>>>     New test.
>>>>
>>>> Tested powerpc64le-linux. Committed to trunk.
>>>>
>>>> Martin, Jakub, could you please double-check the usage of
>>>> __builtin_object_size? (line 805 in libstdc++-v3/include/std/istream)
>>>> Do you see any problems with using it here? If it can't tell the size
>>>> then we just assume it's larger than the string to be extracted, which
>>>> is what the old code did anyway. I do have an idea for stronger
>>>> checking in debug mode, which I'll post in a minute.
>>>
>>> I've always found the second argument to __builtin_object_size
>>> confusing for types above 1.  I don't see anything wrong in
>>> the diff but I believe the most useful results are with type 1
>>> for string functions and type 0 for raw memory functions like
>>> memcpy (that's what _FORTIFY_SOURCE uses for the two sets of
>>> functions).  In type 2 when the result is zero it means one of
>>> two things: either the size of the array couldn't be determined
>>> or it really is zero.  That's less than helpful in cases like:
>>>
>>> char a[8];
>>> strcpy (a + 8, s);
>>>
>>> where it prevents detecting the buffer overflow.
>>>
>>> I would suggest to use type 1 in the iostream functions instead.
>>>
>>> Adding attribute access to the __istream_extract overloads should
>>> also let GCC check for out-of-bounds accesses by the function and
>>> issue warnings.  This goes beyond what __builtin_object_size makes
>>> possible (it considers also variable sizes in known ranges).
>>
>> I tried the attached patch with a testcase like:
>>
>> #include <istream>
>>
>> void
>> test01(std::istream& in)
>> {
>>  char pc[1];
>>  in >> (pc + 1); // { dg-warning "writing 1 byte into a region of size 
>> 0" }
>> }
>>
>> I get a -Wstringop-overflow warning with -O0, but not at -O2. Is that
>> because the call to operator>> gets inlined, and __builtin_object_size
>> returns 0, and we call __istream_extract(in, s, 0) which says we won't
>> write anything, so it's now considered safe?
>>
>> That's unfortunate, because actually __istream_extract will always
>> write at least one byte as a null terminator.
>>
>> So I tried various ways toi try and get the warning back at -O2 and
>> nothing worked e.g. when __builtin_object_size(__s, 0) returns 0 I
>> tried calling __istream_extract(__in, __s, 1) which should say I'm
>> going to write a character, and so should warn if the size is known to
>> be zero. But no wanring.
> 
> There is a warning, when calling __istream_extract, but it's
> suppressed because it comes from within the library itself.
> 
> So optimisation means we inline the user's erroneous call, and
> suppress the warning from the (still erroneous) call to the library
> internals.
> 
> That sucks.
> 

Well, yes, I suppose it does.  Function attributes and inlining
don't mesh very well.  Once a function is inlined, the effects
of its attributes, are gone, not just on warnings but also on
subsequent optimizations.  The exceptions are attributes that
the inliner knows about.  For instance, attributes alloc_size
and malloc suffer from the same problem as access.

For the effect of attribute access in particular to persist
the inliner would need to record it somewhere.  For instance,
it could emit a call to a synthesized intrinsic function with
the same attribute and arguments that the expander would then
check as it does ordinary calls but then discard.

Other attributes would need their own special treatment.

FWIW, there are a whole number of contexts in the middle end
where attribute access could be used to good effect.  I have
been working to enhance a number of these areas but I confess
that the interaction with inlining hasn't been on my radar.
It's something to think about.

For this specific use case, I saw __istream_extract defined
as an ordinary (non-template) function in a .tcc file in
the patch so I thought it was out of line.  If it's inline
or if it's a template the only workaround I can think of
to retain the warning is to have it make a call to (no-op)
function with the attribute that is not inlined.  It's too
bad there is no attribute to tell the expander to avoid
emitting such a function (which would be the equivalent of
the idea I outlined in my second paragraph above).

Martin

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

[Jonathan Wakely]

On 06/08/20 08:56 -0600, Martin Sebor via Libstdc++ wrote:
>For this specific use case, I saw __istream_extract defined
>as an ordinary (non-template) function in a .tcc file in
>the patch so I thought it was out of line.  If it's inline

It's overloaded. One is a function template defined inline, the other
is a non-inline function defined in the library.

>or if it's a template the only workaround I can think of
>to retain the warning is to have it make a call to (no-op)
>function with the attribute that is not inlined.  It's too
>bad there is no attribute to tell the expander to avoid
>emitting such a function (which would be the equivalent of
>the idea I outlined in my second paragraph above).

That will still fail to warn because of -Wsystem-headers.

Attempting to use attributes here achieves absolutely nothing for the
problem scenarios I was concerned about.

The only case where it helps is passing a null pointer or a pointer to
a zero-sized buffer to operator>> and not optimising. All other
problematic cases fail to warn, and it's those other cases where a
warning would be helpful.

Re: [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

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

[Jonathan Wakely]

On 06/08/20 09:17 -0600, Martin Sebor via Libstdc++ wrote:
>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,

Yeah, it's an ongoing effort.

>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 agree. And __attribute__((__warning__(""))) too.

>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
>
>

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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 8175 bytes --]

On 06/08/20 14:14 +0100, Jonathan Wakely wrote:
>On 05/08/20 16:31 -0600, Martin Sebor via Libstdc++ wrote:
>>On 8/5/20 3:25 PM, Jonathan Wakely wrote:
>>>P0487R1 resolved LWG 2499 for C++20 by removing the operator>> overloads
>>>that have high risk of buffer overflows. They were replaced by
>>>equivalents that only accept a reference to an array, and so can
>>>guarantee not to write past the end of the array.
>>>
>>>In order to support both the old and new functionality, this patch
>>>introduces a new overloaded __istream_extract function which takes a
>>>maximum length. The new operator>> overloads use the array size as the
>>>maximum length. The old overloads now use __builtin_object_size to
>>>determine the available buffer size if available (which requires -O2) or
>>>use numeric_limits<streamsize>::max()/sizeof(char_type) otherwise. This
>>>is a change in behaviour, as the old overloads previously always used
>>>numeric_limits<streamsize>::max(), without considering sizeof(char_type)
>>>and without attempting to prevent overflows.
>>>
>>>Because they now do little more than call __istream_extract, the old
>>>operator>> overloads are very small inline functions. This means there
>>>is no advantage to explicitly instantiating them in the library (in fact
>>>that would prevent the __builtin_object_size checks from ever working).
>>>As a result, the explicit instantiation declarations can be removed from
>>>the header. The explicit instantiation definitions are still needed, for
>>>backwards compatibility with existing code that expects to link to the
>>>definitions in the library.
>>>
>>>While working on this change I noticed that src/c++11/istream-inst.cc
>>>has the following explicit instantiation definition:
>>>  template istream& operator>>(istream&, char*);
>>>This had no effect (and so should not have been present in that file),
>>>because there was an explicit specialization declared in <istream> and
>>>defined in src/++98/istream.cc. However, this change removes the
>>>explicit specialization, and now the explicit instantiation definition
>>>is necessary to ensure the symbol gets defined in the library.
>>>
>>>libstdc++-v3/ChangeLog:
>>>
>>>	* config/abi/pre/gnu.ver (GLIBCXX_3.4.29): Export new symbols.
>>>	* include/bits/istream.tcc (__istream_extract): New function
>>>	template implementing both of operator>>(istream&, char*) and
>>>	operator>>(istream&, char(&)[N]). Add explicit instantiation
>>>	declaration for it. Remove explicit instantiation declarations
>>>	for old function templates.
>>>	* include/std/istream (__istream_extract): Declare.
>>>	(operator>>(basic_istream<C,T>&, C*)): Define inline and simply
>>>	call __istream_extract.
>>>	(operator>>(basic_istream<char,T>&, signed char*)): Likewise.
>>>	(operator>>(basic_istream<char,T>&, unsigned char*)): Likewise.
>>>	(operator>>(basic_istream<C,T>&, C(7)[N])): Define for LWG 2499.
>>>	(operator>>(basic_istream<char,T>&, signed char(&)[N])):
>>>	Likewise.
>>>	(operator>>(basic_istream<char,T>&, unsigned char(&)[N])):
>>>	Likewise.
>>>	* include/std/streambuf (basic_streambuf): Declare char overload
>>>	of __istream_extract as a friend.
>>>	* src/c++11/istream-inst.cc: Add explicit instantiation
>>>	definition for wchar_t overload of __istream_extract. Remove
>>>	explicit instantiation definitions of old operator>> overloads
>>>	for versioned-namespace build.
>>>	* src/c++98/istream.cc (operator>>(istream&, char*)): Replace
>>>	with __istream_extract(istream&, char*, streamsize).
>>>	* testsuite/27_io/basic_istream/extractors_character/char/3.cc:
>>>	Do not use variable-length array.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/4.cc:
>>>	Do not run test for C++20.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/9555-ic.cc:
>>>	Do not test writing to pointers for C++20.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/9826.cc:
>>>	Use array instead of pointer.
>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/3.cc:
>>>	Do not use variable-length array.
>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/4.cc:
>>>	Do not run test for C++20.
>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/9555-ic.cc:
>>>	Do not test writing to pointers for C++20.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499.cc:
>>>	New test.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc:
>>>	New test.
>>>	* testsuite/27_io/basic_istream/extractors_character/char/overflow.cc:
>>>	New test.
>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499.cc:
>>>	New test.
>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc:
>>>	New test.
>>>
>>>Tested powerpc64le-linux. Committed to trunk.
>>>
>>>Martin, Jakub, could you please double-check the usage of
>>>__builtin_object_size? (line 805 in libstdc++-v3/include/std/istream)
>>>Do you see any problems with using it here? If it can't tell the size
>>>then we just assume it's larger than the string to be extracted, which
>>>is what the old code did anyway. I do have an idea for stronger
>>>checking in debug mode, which I'll post in a minute.
>>
>>I've always found the second argument to __builtin_object_size
>>confusing for types above 1.  I don't see anything wrong in
>>the diff but I believe the most useful results are with type 1
>>for string functions and type 0 for raw memory functions like
>>memcpy (that's what _FORTIFY_SOURCE uses for the two sets of
>>functions).  In type 2 when the result is zero it means one of
>>two things: either the size of the array couldn't be determined
>>or it really is zero.  That's less than helpful in cases like:
>>
>> char a[8];
>> strcpy (a + 8, s);
>>
>>where it prevents detecting the buffer overflow.
>>
>>I would suggest to use type 1 in the iostream functions instead.
>>
>>Adding attribute access to the __istream_extract overloads should
>>also let GCC check for out-of-bounds accesses by the function and
>>issue warnings.  This goes beyond what __builtin_object_size makes
>>possible (it considers also variable sizes in known ranges).
>
>I tried the attached patch with a testcase like:
>
>#include <istream>
>
>void
>test01(std::istream& in)
>{
>  char pc[1];
>  in >> (pc + 1); // { dg-warning "writing 1 byte into a region of size 0" }
>}
>
>I get a -Wstringop-overflow warning with -O0, but not at -O2. Is that
>because the call to operator>> gets inlined, and __builtin_object_size
>returns 0, and we call __istream_extract(in, s, 0) which says we won't
>write anything, so it's now considered safe?
>
>That's unfortunate, because actually __istream_extract will always
>write at least one byte as a null terminator.
>
>So I tried various ways toi try and get the warning back at -O2 and
>nothing worked e.g. when __builtin_object_size(__s, 0) returns 0 I
>tried calling __istream_extract(__in, __s, 1) which should say I'm
>going to write a character, and so should warn if the size is known to
>be zero. But no wanring.
>
>So now I'm considering this:
>
>  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.

The version above is what I've pushed (also attached).

Tested powerpc64le-linux.




[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 7016 bytes --]

commit 6251ea15f55ec57d6325c2e37e88b22315aba658
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Aug 6 16:16:33 2020

    libstdc++: Adjust overflow prevention to operator>>
    
    This adjusts the overflow prevention added to operator>> so that we can
    distinguish "unknown size" from "zero size", and avoid writing anything
    at all in to zero sized buffers.
    
    This also removes the incorrect comment saying extraction stops at a
    null byte.
    
    libstdc++-v3/ChangeLog:
    
            * include/std/istream (operator>>(istream&, char*)): Add
            attributes to get warnings for pointers that are null or known
            to point to the end of a buffer. Request upper bound from
            __builtin_object_size check and handle zero-sized buffer case.
            (operator>>(istream&, signed char))
            (operator>>(istream&, unsigned char*)): Add attributes.
            * testsuite/27_io/basic_istream/extractors_character/char/overflow.cc:
            Check extracting into the middle of a buffer.
            * testsuite/27_io/basic_istream/extractors_character/wchar_t/overflow.cc: New test.

diff --git a/libstdc++-v3/include/std/istream b/libstdc++-v3/include/std/istream
index cb8e9f87c90..20a455a0ef1 100644
--- a/libstdc++-v3/include/std/istream
+++ b/libstdc++-v3/include/std/istream
@@ -790,7 +790,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
    *  - `n - 1` characters are stored
    *  - EOF is reached
    *  - the next character is whitespace according to the current locale
-   *  - the next character is a null byte (i.e., `charT()`)
    *
    *  `width(0)` is then called for the input stream.
    *
@@ -799,25 +798,38 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 #if __cplusplus <= 201703L
   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)
     {
-      streamsize __n = __builtin_object_size(__s, 2) / sizeof(_CharT);
-      if (__n == 0)
-	__n = __gnu_cxx::__numeric_traits<streamsize>::__max / sizeof(_CharT);
-      std::__istream_extract(__in, __s, __n);
+      size_t __n = __builtin_object_size(__s, 0);
+      if (__builtin_expect(__n < sizeof(_CharT), false))
+	{
+	  // There is not even space for the required 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;
     }
 
   template<class _Traits>
+    __attribute__((__nonnull__(2), __access__(__write_only__, 2)))
     inline basic_istream<char, _Traits>&
     operator>>(basic_istream<char, _Traits>& __in, unsigned char* __s)
-    { return (__in >> reinterpret_cast<char*>(__s)); }
+    { return __in >> reinterpret_cast<char*>(__s); }
 
   template<class _Traits>
+    __attribute__((__nonnull__(2), __access__(__write_only__, 2)))
     inline basic_istream<char, _Traits>&
     operator>>(basic_istream<char, _Traits>& __in, signed char* __s)
-    { return (__in >> reinterpret_cast<char*>(__s)); }
+    { return __in >> reinterpret_cast<char*>(__s); }
 #else
   // _GLIBCXX_RESOLVE_LIB_DEFECTS
   // 2499. operator>>(istream&, char*) makes it hard to avoid buffer overflows
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/overflow.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/overflow.cc
index 1141a41b208..abbba8bb09b 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/overflow.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/char/overflow.cc
@@ -15,14 +15,14 @@
 // with this library; see the file COPYING3.  If not see
 // <http://www.gnu.org/licenses/>.
 
-// { dg-options "-O2 -std=gnu++98" }
+// { dg-options "-O2" }
 // { dg-do run }
 
 // This test checks that operator>> will avoid a buffer overflow when
 // reading into a buffer with a size that is known at compile time.
 
 // Since C++20 this is guaranteed (see LWG 2499), for previous standards
-// we try to check the buffer size as an extension (which depends on -O2).
+// checking the buffer size is an extension and depends on optimisation.
 
 #include <sstream>
 #include <testsuite_hooks.h>
@@ -30,11 +30,24 @@
 void
 test01()
 {
-  std::istringstream in("foolish child");
+  std::istringstream in("foolishly");
   char pc[5];
   in >> pc;
   VERIFY( in.good() );
   VERIFY( std::string(pc) == "fool" );
+
+#if __cplusplus <= 201703L
+  char* p = pc + 1;
+  in >> p;
+  VERIFY( in.good() );
+  VERIFY( std::string(pc) == "fish" );
+
+  p = pc + 4;
+  *p = '#';
+  in >> p;
+  VERIFY( in.fail() ); // if no characters are extracted, failbit is set
+  VERIFY( *p == '\0' );
+#endif
 }
 
 void
@@ -61,4 +74,6 @@ int
 main()
 {
   test01();
+  test02();
+  test03();
 }
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/overflow.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/overflow.cc
new file mode 100644
index 00000000000..6a23f1305a3
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_character/wchar_t/overflow.cc
@@ -0,0 +1,57 @@
+// Copyright (C) 2020 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
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-O2" }
+// { dg-do run }
+
+// This test checks that operator>> will avoid a buffer overflow when
+// reading into a buffer with a size that is known at compile time.
+
+// Since C++20 this is guaranteed (see LWG 2499), for previous standards
+// checking the buffer size is an extension and depends on optimisation.
+
+#include <sstream>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  std::wistringstream in(L"foolishly");
+  wchar_t pc[5];
+  in >> pc;
+  VERIFY( in.good() );
+  VERIFY( std::wstring(pc) == L"fool" );
+
+#if __cplusplus <= 201703L
+  wchar_t* p = pc + 1;
+  in >> p;
+  VERIFY( in.good() );
+  VERIFY( std::wstring(pc) == L"fish" );
+
+  p = pc + 4;
+  *p = L'#';
+  in >> p;
+  VERIFY( in.fail() ); // if no characters are extracted, failbit is set
+  VERIFY( *p == L'\0' );
+#endif
+}
+
+int
+main()
+{
+  test01();
+}

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

[Martin Sebor]

On 8/6/20 10:00 AM, Jonathan Wakely wrote:
> On 06/08/20 09:17 -0600, Martin Sebor via Libstdc++ wrote:
>> 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,
> 
> Yeah, it's an ongoing effort.
> 
>> 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 agree. And __attribute__((__warning__(""))) too.

I've opened four bugs to track some of the issues we've discussed:
96502, 96503, and 96505 for the lost attribute effect after
inlining, and 96508 for the system header interaction.  I wasn't
able to reproduce the problem you're having with the attribute
(calling an out-of-line function declared in a system header
does produce a warning) so if you're not completely put off
by your experience so far please take a look at it and see
what I may have missed.

Thanks
Martin

> 
>> 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
>>
>>
> 

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

[Jonathan Wakely]

On 06/08/20 12:45 -0600, Martin Sebor via Libstdc++ wrote:
>On 8/6/20 10:00 AM, Jonathan Wakely wrote:
>>On 06/08/20 09:17 -0600, Martin Sebor via Libstdc++ wrote:
>>>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,
>>
>>Yeah, it's an ongoing effort.
>>
>>>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 agree. And __attribute__((__warning__(""))) too.
>
>I've opened four bugs to track some of the issues we've discussed:
>96502, 96503, and 96505 for the lost attribute effect after
>inlining, and 96508 for the system header interaction.  I wasn't
>able to reproduce the problem you're having with the attribute
>(calling an out-of-line function declared in a system header
>does produce a warning) so if you're not completely put off
>by your experience so far please take a look at it and see
>what I may have missed.

This preprocessed code fails to warn without -Wsystem-headers:

# 1 "user.C"
# 1 "sys.h" 1

# 2 "sys.h" 3


# 3 "sys.h" 3
__attribute__((warning("badness")))
void diagnose_badness();

template<typename T>
void foo(T t)
{
   if (t < 0)
     diagnose_badness();
}
# 2 "user.C" 2


# 3 "user.C"
int main()
{
   int i = -1;
   foo(i);
}