On Fri, Feb 27, 2026 at 6:41 PM Jonathan Wakely <[email protected]> wrote:
> 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. > As you predicted, the pragmas will not lead to warnings being silenced, however adding unreachable check (before null-deference) solves that. if (!_M_sbuf) __builtin_unreachable(); I will post a 3 patch series with this as second patch, and we can decide which change we want to pick for each branch. > > 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. > >
