[PATCH v2] libstdc++: Rework istreambuf_iterator::_M_sbuf handling to slince null-dereference warning [PR105580]
Tomasz Kaminski
tkaminsk@redhat.com
Fri Apr 24 07:49:24 GMT 2026
On Fri, Apr 24, 2026 at 7:38 AM Tomasz Kaminski <tkaminsk@redhat.com> wrote:
>
>
> On Mon, Mar 2, 2026 at 8:29 AM Tomasz Kamiński <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.
>>
> I removed this line because no changes were made to this file. It depended
on pragmas before,
and they are no longer needed. I could { dg-add-options no_pch } from it.
Let me know if I should do it before merging to master.
>
>> Signed-off-by: Tomasz Kamiński <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?
>>
> OK for GCC17?
>
>>
>> .../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
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20260424/4a365c15/attachment-0001.htm>
More information about the Libstdc++
mailing list