[PATCH] istream_iterator: unexpected read in ctor

[PATCH] istream_iterator: unexpected read in ctor

[Petr Ovtchenkov]

istream_iterator do unexpected read from stream
when initialized by istream&.

It is not required from increment operators of istream_iterator
that _M_ok will be true as precondition.
---
 libstdc++-v3/include/bits/stream_iterator.h        | 19 +++++-----
 .../24_iterators/istream_iterator/81964.cc         | 42 ++++++++++++++++++++++
 2 files changed, 50 insertions(+), 11 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc

diff --git a/libstdc++-v3/include/bits/stream_iterator.h b/libstdc++-v3/include/bits/stream_iterator.h
index f9c6ba6..26959ce 100644
--- a/libstdc++-v3/include/bits/stream_iterator.h
+++ b/libstdc++-v3/include/bits/stream_iterator.h
@@ -56,8 +56,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
     private:
       istream_type*	_M_stream;
-      _Tp		_M_value;
-      bool		_M_ok;
+      mutable _Tp	_M_value;
+      mutable bool	_M_ok;
 
     public:
       ///  Construct end of input stream iterator.
@@ -66,8 +66,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       ///  Construct start of input stream iterator.
       istream_iterator(istream_type& __s)
-      : _M_stream(&__s)
-      { _M_read(); }
+      : _M_stream(&__s), _M_value(), _M_ok(false)
+      { }
 
       istream_iterator(const istream_iterator& __obj)
       : _M_stream(__obj._M_stream), _M_value(__obj._M_value),
@@ -77,6 +77,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       const _Tp&
       operator*() const
       {
+	if (!_M_ok) {
+	  _M_read();
+	}
 	__glibcxx_requires_cond(_M_ok,
 				_M_message(__gnu_debug::__msg_deref_istream)
 				._M_iterator(*this));
@@ -89,9 +92,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istream_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(_M_ok,
-				_M_message(__gnu_debug::__msg_inc_istream)
-				._M_iterator(*this));
 	_M_read();
 	return *this;
       }
@@ -99,9 +99,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istream_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(_M_ok,
-				_M_message(__gnu_debug::__msg_inc_istream)
-				._M_iterator(*this));
 	istream_iterator __tmp = *this;
 	_M_read();
 	return __tmp;
@@ -113,7 +110,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
     private:
       void
-      _M_read()
+      _M_read() const
       {
 	_M_ok = (_M_stream && *_M_stream) ? true : false;
 	if (_M_ok)
diff --git a/libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc b/libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc
new file mode 100644
index 0000000..f58fc87
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc
@@ -0,0 +1,42 @@
+// { dg-options "-std=gnu++11" }
+
+// Copyright (C) 2017 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/>.
+
+#include <sstream>
+#include <iterator>
+#include <testsuite_hooks.h>
+
+// libstdc++/81964
+void test0x()
+{
+  using namespace std;
+  bool test __attribute__((unused)) = true;
+
+  std::istringstream s("1 2");
+  std::istream_iterator<int> ii1(s);
+  std::istream_iterator<int> ii2(s);
+
+  VERIFY( *ii2 == 1 );
+  VERIFY( *ii1 == 2 );
+}
+
+int main()
+{
+  test0x();
+  return 0;
+}
-- 
2.10.1

Re: [PATCH] istream_iterator: unexpected read in ctor

[Petr Ovtchenkov]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81964

On Thu, 24 Aug 2017 11:55:58 +0300
Petr Ovtchenkov <ptr@void-ptr.info> wrote:

> istream_iterator do unexpected read from stream
> when initialized by istream&.
> 
> It is not required from increment operators of istream_iterator
> that _M_ok will be true as precondition.
> ---
>  libstdc++-v3/include/bits/stream_iterator.h        | 19 +++++-----
>  .../24_iterators/istream_iterator/81964.cc         | 42 ++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 11 deletions(-)
>  create mode 100644 libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc
> 
> diff --git a/libstdc++-v3/include/bits/stream_iterator.h
> b/libstdc++-v3/include/bits/stream_iterator.h index f9c6ba6..26959ce 100644
> --- a/libstdc++-v3/include/bits/stream_iterator.h
> +++ b/libstdc++-v3/include/bits/stream_iterator.h
> @@ -56,8 +56,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>  
>      private:
>        istream_type*	_M_stream;
> -      _Tp		_M_value;
> -      bool		_M_ok;
> +      mutable _Tp	_M_value;
> +      mutable bool	_M_ok;
>  
>      public:
>        ///  Construct end of input stream iterator.
> @@ -66,8 +66,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>  
>        ///  Construct start of input stream iterator.
>        istream_iterator(istream_type& __s)
> -      : _M_stream(&__s)
> -      { _M_read(); }
> +      : _M_stream(&__s), _M_value(), _M_ok(false)
> +      { }
>  
>        istream_iterator(const istream_iterator& __obj)
>        : _M_stream(__obj._M_stream), _M_value(__obj._M_value),
> @@ -77,6 +77,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        const _Tp&
>        operator*() const
>        {
> +	if (!_M_ok) {
> +	  _M_read();
> +	}
>  	__glibcxx_requires_cond(_M_ok,
>  				_M_message(__gnu_debug::__msg_deref_istream)
>  				._M_iterator(*this));
> @@ -89,9 +92,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        istream_iterator&
>        operator++()
>        {
> -	__glibcxx_requires_cond(_M_ok,
> -				_M_message(__gnu_debug::__msg_inc_istream)
> -				._M_iterator(*this));
>  	_M_read();
>  	return *this;
>        }
> @@ -99,9 +99,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        istream_iterator
>        operator++(int)
>        {
> -	__glibcxx_requires_cond(_M_ok,
> -				_M_message(__gnu_debug::__msg_inc_istream)
> -				._M_iterator(*this));
>  	istream_iterator __tmp = *this;
>  	_M_read();
>  	return __tmp;
> @@ -113,7 +110,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>  
>      private:
>        void
> -      _M_read()
> +      _M_read() const
>        {
>  	_M_ok = (_M_stream && *_M_stream) ? true : false;
>  	if (_M_ok)
> diff --git a/libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc
> b/libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc new file mode 100644
> index 0000000..f58fc87
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc
> @@ -0,0 +1,42 @@
> +// { dg-options "-std=gnu++11" }
> +
> +// Copyright (C) 2017 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/>.
> +
> +#include <sstream>
> +#include <iterator>
> +#include <testsuite_hooks.h>
> +
> +// libstdc++/81964
> +void test0x()
> +{
> +  using namespace std;
> +  bool test __attribute__((unused)) = true;
> +
> +  std::istringstream s("1 2");
> +  std::istream_iterator<int> ii1(s);
> +  std::istream_iterator<int> ii2(s);
> +
> +  VERIFY( *ii2 == 1 );
> +  VERIFY( *ii1 == 2 );
> +}
> +
> +int main()
> +{
> +  test0x();
> +  return 0;
> +}
> -- 
> 2.10.1
> 

Re: [PATCH] istream_iterator: unexpected read in ctor

[François Dumont]

On 24/08/2017 10:55, Petr Ovtchenkov wrote:
> istream_iterator do unexpected read from stream
> when initialized by istream&.
>
> It is not required from increment operators of istream_iterator
> that _M_ok will be true as precondition.

I think it does through those words:

"After it is constructed, and every time ++ is used, the iterator reads 
and stores a value of T. If the iterator fails to read and store a value 
of T (fail() on the stream returns true), the iterator becomes equal to 
the end-of-stream iterator value."

and later:

"The result of operator* on an end-of-stream iterator is not defined."

The assertion is here to help in detecting this undefined behavior.

> ---
>   libstdc++-v3/include/bits/stream_iterator.h        | 19 +++++-----
>   .../24_iterators/istream_iterator/81964.cc         | 42 ++++++++++++++++++++++
>   2 files changed, 50 insertions(+), 11 deletions(-)
>   create mode 100644 libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc
>
> diff --git a/libstdc++-v3/include/bits/stream_iterator.h b/libstdc++-v3/include/bits/stream_iterator.h
> index f9c6ba6..26959ce 100644
> --- a/libstdc++-v3/include/bits/stream_iterator.h
> +++ b/libstdc++-v3/include/bits/stream_iterator.h
> @@ -56,8 +56,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   
>       private:
>         istream_type*	_M_stream;
> -      _Tp		_M_value;
> -      bool		_M_ok;
> +      mutable _Tp	_M_value;
> +      mutable bool	_M_ok;

Each time I see the mutable keywork it rings my alert bell.

>   
>       public:
>         ///  Construct end of input stream iterator.
> @@ -66,8 +66,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   
>         ///  Construct start of input stream iterator.
>         istream_iterator(istream_type& __s)
> -      : _M_stream(&__s)
> -      { _M_read(); }
> +      : _M_stream(&__s), _M_value(), _M_ok(false)
> +      { }
>   
>         istream_iterator(const istream_iterator& __obj)
>         : _M_stream(__obj._M_stream), _M_value(__obj._M_value),
> @@ -77,6 +77,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>         const _Tp&
>         operator*() const
>         {
> +	if (!_M_ok) {
> +	  _M_read();
> +	}
>   	__glibcxx_requires_cond(_M_ok,
>   				_M_message(__gnu_debug::__msg_deref_istream)
>   				._M_iterator(*this));
> @@ -89,9 +92,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>         istream_iterator&
>         operator++()
>         {
> -	__glibcxx_requires_cond(_M_ok,
> -				_M_message(__gnu_debug::__msg_inc_istream)
> -				._M_iterator(*this));
>   	_M_read();
>   	return *this;
>         }
> @@ -99,9 +99,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>         istream_iterator
>         operator++(int)
>         {
> -	__glibcxx_requires_cond(_M_ok,
> -				_M_message(__gnu_debug::__msg_inc_istream)
> -				._M_iterator(*this));
>   	istream_iterator __tmp = *this;
>   	_M_read();
>   	return __tmp;
> @@ -113,7 +110,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   
>       private:
>         void
> -      _M_read()
> +      _M_read() const
>         {
>   	_M_ok = (_M_stream && *_M_stream) ? true : false;
>   	if (_M_ok)
> diff --git a/libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc b/libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc
> new file mode 100644
> index 0000000..f58fc87
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/24_iterators/istream_iterator/81964.cc
> @@ -0,0 +1,42 @@
> +
> +#include <sstream>
> +#include <iterator>
> +#include <testsuite_hooks.h>
> +
> +// libstdc++/81964
> +void test0x()
> +{
> +  using namespace std;
> +  bool test __attribute__((unused)) = true;
> +
> +  std::istringstream s("1 2");
> +  std::istream_iterator<int> ii1(s);

Try to add here a:
std::istream_iterator<int> end;
VERIFY( ii1 != end );

Your patch would make a freshly created istream_iterator looks like an 
end-of-stream iterator.

I am surprised if no test in the testsuite is catching that by the way.

François

Re: [PATCH] istream_iterator: unexpected read in ctor

[Tim Song]

On Thu, Aug 24, 2017 at 4:55 AM, Petr Ovtchenkov <ptr@void-ptr.info> wrote:
> istream_iterator do unexpected read from stream
> when initialized by istream&.
>

This is pretty much required by the specification. See the discussion
in http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0738r0.html

And the "fix" is also wrong because it makes operator* not
thread-safe, which it is required to be (as a const member function).

Re: [PATCH] istream_iterator: unexpected read in ctor

[Petr Ovtchenkov]

Summary:

  Standard say (with a bit misoriented words):

     27.6.1  Class template istream_iterator [istream.iterator]
     1 The class template istream_iterator is an input iterator (27.2.3)
       that reads (using operator>>) successive elements from the input
       stream for which it was constructed.
    ->    After it is constructed, and every time ++ is used, the iterator
    ->    reads and stores a value of T.
     ...

     27.6.1.1 istream_iterator constructors and destructor [istream.iterator.cons]
     ...
     istream_iterator(istream_type& s);
    ->  3 Effects: Initializes in_stream with addressof(s). value may be initialized
    ->    during construction or the first time it is referenced.
     4 Postconditions: in_stream == addressof(s).

  Current position is "After it is constructed ... the iterator reads and stores
  a value of T.": http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0738r0.html

IMO the test (modified according words above) is still useful, I will re-submit it ASAP.

--

   - ptr