On Fri, 27 Feb 2026 at 17:37, Tomasz Kaminski <[email protected]> wrote:
>
>
>
> On Fri, Feb 27, 2026 at 5:12 PM Tomasz Kaminski <[email protected]> wrote:
>>
>>
>>
>> On Fri, Feb 27, 2026 at 4:48 PM Jonathan Wakely <[email protected]> wrote:
>>>
>>> On Wed, 07 Jan 2026 at 15:30 +0100, Tomasz Kamiński 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
>>> >sitution
>>> >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 not or positive have performance impact on the usual
>>> >iteration
>>>
>>> I think this should be:
>>>
>>> "This change should have zero or positive performance impact ..."
>>>
>>> >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 again 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.
>>>
>>> This should be ++ not __
>>>
>>> > (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.
>>> >---
>>> >This warning turned out to be false-positive (as explained in the commit
>>> >decription), so we can just add add pragrams to address that. But this
>>> >change seem to be beneficial anyway, as we are no longer causing
>>> >data-races.
>>> >
>>> >Tested on x86_64-linux. Locally tested with -Wnull-dereference,
>>> >and the istream_iterator warning in 24_iterators/istreambuf_iterator/2.cc
>>> >disappears (there are some other warning present I haven't looked into
>>> >that yet).
>>>
>>> Can we add a new testsuite/24_iterators/istreambuf_iterator/105580.cc
>>> using exactly the testcase from the bug, with -Wnull-dereference in
>>> the dg-options? I don't think the additions to 2.cc cover it. I'd like
>>> us to have a test that gives the warning today and doesn't warn after
>>> the fix.
>>
>> OK will do.
>>>
>>>
>>> > .../include/bits/streambuf_iterator.h | 20 +++----
>>> > .../24_iterators/istreambuf_iterator/2.cc | 58 +++++++++++++++++++
>>> > 2 files changed, 66 insertions(+), 12 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;
>>>
>>> I've been trying to figure out if this change (removing 'mutable')
>>> could be unsafe if we mix objects compiled with old and new GCC.
>>>
>>> In a TU compiled by an old GCC, _M_sbuf could change by changed when
>>> the iterator reaches EOF. In a TU compiled by the new GCC, could the
>>> compiler optimize based on the fact that if only const member
>>> functionss are used, the member can't change?
>>>
>>> I don't think it's a problem, because the new compiler has to assume
>>> that any non-inline code could already do a const_cast on the iterator
>>> and so call non-const members that way. The only way it can optimize
>>> by assuming _M_sbuf never, ever changes is if it can see the original
>>> definition of the iterator and it was defined as const. But in that
>>> case, nobody can ever call non-const members on it anyway.
>>
>> I thought about it, and the only situation when removing the mutable could
>> have
>> any impact, is that you declare an iterator as a const object, so the
>> compiler can in
>> that case now can propagate _M_sbuf value. But in that case, there is no way
>> to
>> mutate _M_sbuf without UB.
>>
>>>
>>>
>>> So I think this is safe. I would feel more comfortable if we left the
>>> 'mutable' for GCC 16 with a comment saying it's not needed now, and
>>> then remove it for GCC 17 early in stage 1.
>>
>> Given that the warning is FP, and is emitted from system headers, do we even
>> need to have fix for GCC16? Maybe I should simply add pragma to ignore
>> warnings
>> for now, and postpone the whole endeavor for GCC17? Or just leave the
>> warning as is.
>> is emitted from system headers anyway.
>
> I also feel uneasy about touching istream iterator now. In the beginning, we
> prioritized the
> change because we thought this was a true bug, so I think a better approach
> now may be to
> add test emmiting and pragmas disabling the warning now (with backports)
> and land the changes to GCC17.
OK, that seems fine (and also safe for backports to older branches) as
long as putting the pragmas in istreambuf_iterator actually fixes it.
The warnings come from here, not from the iterator:
/home/jwakely/gcc/16/include/c++/16.0.1/streambuf:494:30: warning:
potential null pointer dereference [-Wnull-dereference]
494 | gptr() const { return _M_in_cur; }
| ^~~~~~~~~
So I'm not sure if putting pragmas in the iterator will prevent that.
