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

[Jonathan Wakely <jwakely@redhat.com>]

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