[PATCH v2] libstdc++: Rework istreambuf_iterator::_M_sbuf handling to slince null-dereference warning [PR105580]
Nathan Myers
ncm@cantrip.org
Mon Mar 2 17:45:32 GMT 2026
I like this change. Besides eliminating a 'mutable', it
improves the cost structure of istreambuf_iterator operations.
LGTM
Nathan
On 3/2/26 5:18 AM, Tomasz Kaminski wrote:
>
>
> On Mon, Mar 2, 2026 at 8:29 AM Tomasz Kamiński <tkaminsk@redhat.com
> <mailto:tkaminsk@redhat.com>> wrote:
>
> The warning was produced by following seqeunce, given an
> istream_iterator<char>
> it, such that *it will result in hitting EoF in it->_M_get(), and
> thus clearing
> _M_sbuf, the subsequent call to ++it, will result in _M_sbuf-
> >sbumpc() call
> on null pointer dereference. This is however an false-positive, as
> in such
> situation it == istream_iteator() returns true, and the iterator
> should not
> be dereferenced in first place.
>
> This patch addresses the above by clearing the _M_sbuf in operator+
> +, instead
> of _M_get(). This removes the need for making _M_sbuf mutable, and
> thus make
> the implementation conforming with regards to C++11
> [res.on.data.races] p3.
>
> This change should have zero or positive performance impact on the usual
> iteration patterns, in form:
> while (it != end) { process(*it); ++it; }
> In case when it is end-of-stream iterator, the it != end returns in
> one call
> of _M_sbuf->sgetc() both before and after the change. However we do
> not modify
> _M_sbuf in this case. For non-empty range, we replace call to
> _M_sbuf->sbumpc()
> with _M_sbuf->snextc() in pre-increment, and extract the check
> against EoF from
> *it to ++it. However, as _M_sbuf is now cleared during increment, so
> last
> it != end check avoids _M_sbuf->sgetc() call to check against EoF.
>
> However, this change impact the behavior of the post-increment (*it+
> +), as
> we now load both current character (for return value) and next
> character (to
> check against EoF). In consequence we call both sgetc() and snextc(),
> in contrast to previous single sbumpc() call.
>
> PR libstdc++/105580
>
> libstdc++-v3/ChangeLog:
>
> * include/bits/streambuf_iterator.h
> (istreambuf_iterator::_M_sbuf):
> Remove mutable and adjust whitespace.
> (istreambuf_iterator::_M_c): Adjust whitespace.
> (istreambuf_iterator::operator++()): Clear _M_sbuf if next
> character
> is EoF.
> (istreambuf_iterator::operator++(int)): Use _M_sbuf->sgetc() to
> load current character, and define in terms of ++*this.
> (istreambuf_iterator::_M_get()): Do not clear _M_sbuf in
> case of EoF.
> * testsuite/24_iterators/istreambuf_iterator/2.cc: Test for
> using
> multiple iterators to same rdbuf.
> * testsuite/24_iterators/istreambuf_iterator/105580.cc:
> Remove check for
> warning being emitted.
>
> Signed-off-by: Tomasz Kamiński <tkaminsk@redhat.com
> <mailto:tkaminsk@redhat.com>>
> ---
> v2:
> - fixes typos in commit description
> - mentions LWG2366 in test_empty()
> - modifies already merged test for 105580
> I haven't reverted removal of mutable of _M_sbuf, as I thin we agreed
> it should not have impact.
>
> I will post a separate patch adding pragmas to silence warning, however
> they need to placed surrounbding gptr, egptr, gbump function in
> streambuf.
> However, after recheching the patch, I think that it seem to be safe for
> GCC16.
>
> Testing on x86_64-linux. istreambuf_iterator test passsed.
> OK for GCC16 (if not 17) if test passes?
>
> As we are merging pragmas, I will create a series with more changes
> (__proxy)
> for GCC 17.
>
>
> .../include/bits/streambuf_iterator.h | 20 +++---
> .../istreambuf_iterator/105580.cc | 2 -
> .../24_iterators/istreambuf_iterator/2.cc | 61 ++++++++++++++++++-
> 3 files changed, 68 insertions(+), 15 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/
> libstdc++-v3/include/bits/streambuf_iterator.h
> index 93d3dd24f93..095928ca4d8 100644
> --- a/libstdc++-v3/include/bits/streambuf_iterator.h
> +++ b/libstdc++-v3/include/bits/streambuf_iterator.h
> @@ -112,8 +112,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> // the "end of stream" iterator value.
> // NB: This implementation assumes the "end of stream" value
> // is EOF, or -1.
> - mutable streambuf_type* _M_sbuf;
> - int_type _M_c;
> + streambuf_type* _M_sbuf;
> + int_type _M_c;
>
> public:
> /// Construct end of input stream iterator.
> @@ -172,7 +172,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
>
> - _M_sbuf->sbumpc();
> + if (_S_is_eof(_M_sbuf->snextc()))
> + _M_sbuf = 0;
> _M_c = traits_type::eof();
> return *this;
> }
> @@ -181,14 +182,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> istreambuf_iterator
> operator++(int)
> {
> - __glibcxx_requires_cond(_M_sbuf &&
> - (!_S_is_eof(_M_c) || !
> _S_is_eof(_M_sbuf->sgetc())),
> -
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> - ._M_iterator(*this));
> -
> istreambuf_iterator __old = *this;
> - __old._M_c = _M_sbuf->sbumpc();
> - _M_c = traits_type::eof();
> + __old._M_c = _M_sbuf->sgetc();
> + ++*this;
> return __old;
> }
>
> @@ -206,8 +202,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _M_get() const
> {
> int_type __ret = _M_c;
> - if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret =
> _M_sbuf->sgetc()))
> - _M_sbuf = 0;
> + if (_M_sbuf && _S_is_eof(__ret))
> + __ret = _M_sbuf->sgetc();
> return __ret;
> }
>
> diff --git a/libstdc++-v3/testsuite/24_iterators/
> istreambuf_iterator/105580.cc b/libstdc++-v3/testsuite/24_iterators/
> istreambuf_iterator/105580.cc
> index 85f888b86e7..87edf999ffc 100644
> --- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/105580.cc
> +++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/105580.cc
> @@ -1,5 +1,4 @@
> // { dg-compile }
> -// { dg-require-normal-mode "" }
> // { dg-additional-options "-Wnull-dereference" }
>
> #include <string>
> @@ -12,5 +11,4 @@ int main()
> std::string ss(it, end);
> return 0;
> }
> -// { dg-warning ".*null pointer dereference" "" { target *-*-* } 0 }
>
> diff --git a/libstdc++-v3/testsuite/24_iterators/
> istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/
> istreambuf_iterator/2.cc
> index 78701d71cee..cdf4a0df0f4 100644
> --- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
> +++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
> @@ -48,7 +48,7 @@ void test02(void)
> cistreambuf_iter istrb_it05(istrs01);
> cistreambuf_iter istrb_it06(istrs01.rdbuf());
> VERIFY( istrb_it05 == istrb_it06 );
> -
> +
> // bool equal(istreambuf_iter& b)
> cistreambuf_iter istrb_it07(0);
> cistreambuf_iter istrb_it08;
> @@ -109,8 +109,67 @@ void test02(void)
> }
> }
>
> +void
> +test_empty()
> +{
> + std::istreambuf_iterator<char> null(0), end;
> + VERIFY( null == end );
> +
> + std::istringstream ess;
> + // This specification hear seem to indicate that such iterators
> + // are not end-of-stream iterators, as rdbuf pointer is nullptr,
> + // (see LWG 2366: istreambuf_iterator end-of-stream equality).
> + // However we treat them as such, as otherwise we would produce
> + // a range containing single EoF character.
> + std::istreambuf_iterator<char> it1(ess), it2(ess);
> + VERIFY( it1 == end );
> + VERIFY( it2 == end );
> +}
> +
> +void
> +test_multi()
> +{
> + // C++98 (and later) define operator* in
> [istreambuf.iterator.ops] as:
> + // Returns: The character obtained via the streambuf member
> sbuf_->sgetc().
> + // This nails down behavior of multiple iterators to same sequence.
> + std::istringstream iss("abcd");
> + std::istreambuf_iterator<char> it1(iss), it2(iss), end;
> +
> + VERIFY( it1 != end );
> + VERIFY( it2 != end );
> + VERIFY( *it1 == 'a' );
> + VERIFY( *it2 == 'a' );
> + ++it1;
> +
> + VERIFY( it1 != end );
> + VERIFY( it2 != end );
> + VERIFY( *it1 == 'b' );
> + VERIFY( *it2 == 'b' );
> + ++it2;
> +
> + VERIFY( it1 != end );
> + VERIFY( it2 != end );
> + VERIFY( *it1 == 'c' );
> + VERIFY( *it2 == 'c' );
> + ++it2;
> +
> + VERIFY( it1 != end );
> + VERIFY( it2 != end );
> + VERIFY( *it1 == 'd' );
> + VERIFY( *it2 == 'd' );
> + // second dereference
> + VERIFY( *it1 == 'd' );
> + VERIFY( *it2 == 'd' );
> + ++it1;
> +
> + VERIFY( it1 == end );
> + VERIFY( it2 == end );
> +}
> +
> int main()
> {
> test02();
> + test_empty();
> + test_multi();
> return 0;
> }
> --
> 2.53.0
>
More information about the Libstdc++
mailing list