This program goes into an infinite recursion in basic_filebuf::overflow: #include <fstream> #include <cstring> using std::memset; int main() { { std::ofstream s("test.txt"); char data[BUFSIZ]; memset(data, 'A', sizeof(data)); s.write(data, sizeof(data)); } std::fstream s("test.txt"); char buf[BUFSIZ]; memset(buf, 0, sizeof(buf)); s.read(buf, sizeof(buf)); #ifdef FIX s.seekg(sizeof(buf)); #endif s << 'B'; } The standard requires the seek performed when FIX is defined, so we're allowed to give unexpected results. Even so, it would be preferable to not crash when that's missing. The problem is that the write calls overflow which starts with: if (__testout) { if (_M_reading) { _M_destroy_pback(); const int __gptr_off = _M_get_ext_pos(_M_state_last); if (_M_seek(__gptr_off, ios_base::cur, _M_state_last) == pos_type(off_type(-1))) return __ret; } That call to _M_seek calls _M_terminate_output which starts with: // Part one: update the output sequence. bool __testvalid = true; if (this->pbase() < this->pptr()) { const int_type __tmp = this->overflow(); if (traits_type::eq_int_type(__tmp, traits_type::eof())) __testvalid = false; } So we go back to overflow() and then recurse.
(In reply to Jonathan Wakely from comment #0) > The problem is that the write calls overflow which starts with: Correction: the destructor calls overflow in this testcase. In the original testcase from a customer there was a write of BUFSIZ, which triggered overflow after filling the put area. A more direct way to reproduce it is by changing the final s << 'B'; to: s.write("B", 1); s.flush(); I can prevent the recursion with either: @@ -515,6 +515,7 @@ if (_M_reading) { _M_destroy_pback(); + _M_reading = false; const int __gptr_off = _M_get_ext_pos(_M_state_last); if (_M_seek(__gptr_off, ios_base::cur, _M_state_last) == pos_type(off_type(-1))) or: @@ -920,7 +925,7 @@ { // Part one: update the output sequence. bool __testvalid = true; - if (this->pbase() < this->pptr()) + if (this->pbase() < this->pptr() && __builtin_expect(!_M_reading, 1)) { const int_type __tmp = this->overflow(); if (traits_type::eq_int_type(__tmp, traits_type::eof())) But why are we getting into this state anyway? We have a non-empty output sequence when _M_reading is true, meaning we're in the middle of an uncommitted read. At the end of basic_filebuf::xsgetn after the read we do: if (__n == 0) { _M_set_buffer(0); _M_reading = true; } And _M_set_buffer(0) on a bidirectional filebuf sets up the put area: if (__testout && __off == 0 && _M_buf_size > 1 ) this->setp(_M_buf, _M_buf + _M_buf_size - 1); else this->setp(0, 0); This means the next write inserts straight into the put area, and then the next overflow() finds that _M_reading is true but there is also a pending output sequence.
I'm testing this patch: --- a/libstdc++-v3/include/bits/fstream.tcc +++ b/libstdc++-v3/include/bits/fstream.tcc @@ -699,7 +699,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (__n == 0) { - _M_set_buffer(0); + _M_set_buffer(-1); _M_reading = true; } else if (__len == 0)
This started in 4.6.0 with r87453 Paolo, does the change in comment 2 look right to you?
Seems weird: -1 means uncommitted (per the comment before _M_set_buffer) and we also set _M_reading? I don't think we do that anywhere else. But it's a long time... Note that I clearly remember somebody suggesting that in fact those _M_reading / _M_writing flags aren't necessary, we could in princinple do without, I even remember seeing a draft patch about that. Certainly, anyway, if we think that uncomitted mode is fine here, there is a way to adjust (or leave alone) both not to cause problems...
PR 45708 suggested that _M_reading and _M_writing aren't needed. I haven't found a patch to implement that, but I haven't looked too hard. You make a good point that going into uncommitted mode with _M_reading == true isn't done elsewhere. I don't understand the state machine, so I'm very unsure about changing anything here. But _M_set_buffer(0) seems wrong, since that causes an unwanted(?) call to this->setp(_M_buf, _M_buf + _M_buf_size - 1); from xsgetn.
The relevant xsgetn code essentially is an optimization, right? Shouldn't be too difficult to figure out what would happen in the slow, correct case... What's wrong with a normal uncommitted case, thus _M_reading = false? Performance? Correctness?
(In reply to Jonathan Wakely from comment #3) > This started in 4.6.0 with r87453 It did start with 4.6.0 but that commit was already in 4.5.4 so it can't have been that one. It seems to be the change to overflow in r164529 i.e. the fix for PR 45628, which did this to basic_filebuf::overflow, introducing the potential recursion: @@ -410,8 +426,16 @@ int_type __ret = traits_type::eof(); const bool __testeof = traits_type::eq_int_type(__c, __ret); const bool __testout = _M_mode & ios_base::out; - if (__testout && !_M_reading) + if (__testout) { + if (_M_reading) + { + _M_destroy_pback(); + const int __gptr_off = _M_get_ext_pos(_M_state_last); + if (_M_seek(__gptr_off, ios_base::cur, _M_state_last) + == pos_type(off_type(-1))) + return __ret; + } if (this->pbase() < this->pptr()) { // If appropriate, append the overflow char.
(In reply to Paolo Carlini from comment #6) > The relevant xsgetn code essentially is an optimization, right? Shouldn't be > too difficult to figure out what would happen in the slow, correct case... > What's wrong with a normal uncommitted case, thus _M_reading = false? > Performance? Correctness? I have **no** idea :-)
(In reply to Paolo Carlini from comment #4) > Seems weird: -1 means uncommitted (per the comment before _M_set_buffer) and > we also set _M_reading? I don't think we do that anywhere else. But we also don't use _M_set_buffer(0) (write mode) and _M_reading=true (read mode) anywhere else, and doing that makes no sense to me. This comment says that _M_set_buffer(0) means write mode: /** * This function sets the pointers of the internal buffer, both get * and put areas. Typically: * * __off == egptr() - eback() upon underflow/uflow (@b read mode); * __off == 0 upon overflow (@b write mode); * __off == -1 upon open, setbuf, seekoff/pos (@b uncommitted mode). * * NB: epptr() - pbase() == _M_buf_size - 1, since _M_buf_size * reflects the actual allocated memory and the last cell is reserved * for the overflow char of a full put area. */ void _M_set_buffer(streamsize __off) But we call that from xsgetn which is obviously a read operation. And this comment says _M_reading == true means read mode: /** * _M_reading == false && _M_writing == false for @b uncommitted mode; * _M_reading == true for @b read mode; * _M_writing == true for @b write mode; * * NB: _M_reading == true && _M_writing == true is unused. */ bool _M_reading; bool _M_writing; And it also says we don't use both true at once. So calling _M_set_buffer(0) and setting _M_reading == true is a contradiction, or the comments are wrong.
Yes, I agree with your analysis, something seems wrong. I suspsect going to a normal uncommitted mode, thus not setting _M_reading would largely work, but I fear performance implications, etc. I would strongly recommend analyzing next what can wrong if _M_reading remains false after that (a) reading operation...
I don't think uncommitted mode is correct there, because stdio requires a seek on the underlying FILE before writing to it. Setting _M_reading ensures that will happen before the next write. Uncommitted mode would cause that seek to be skipped. So maybe this workaround (from comment 1) is a reasonable solution: @@ -920,7 +925,7 @@ { // Part one: update the output sequence. bool __testvalid = true; - if (this->pbase() < this->pptr()) + if (this->pbase() < this->pptr() && __builtin_expect(!_M_reading, 1)) { const int_type __tmp = this->overflow(); if (traits_type::eq_int_type(__tmp, traits_type::eof())) This just prevents the infinite recursion, by not trying to perform a pending write if we're currently in read mode.
Hmm, except that if we *do* have a pending output sequence there (i.e. data in the put area), we'd discard it, losing data. I'll try to verify that with a testcase. So we want to avoid getting into a state where we have anything in the put area while _M_reading is true, which is what comment 2 does. It looks strange, because usually _M_set_buffer(-1) is used for uncommitted mode, but what it's actually doing is nuking the buffers. The next read would need to do an underflow to refill the get area (which is correct), or the next write would need to do a seek and then an overflow to make a put area available (which is correct).
Hi, > It looks strange, because usually _M_set_buffer(-1) is used for uncommitted > mode, but what it's actually doing is nuking the buffers. The next read > would need to do an underflow to refill the get area (which is correct), or > the next write would need to do a seek and then an overflow to make a put > area available (which is correct). Indeed, you are making explicit some vague thoughts of mine, which, by and large pointed in the direction that uncommited mode may well be fine, modulo possible performance implications, like, again very vague thoughts, we end up re-reading something which we already read. Those cases I was hoping you could investigate a bit ;)
Author: redi Date: Tue Jul 18 23:39:34 2017 New Revision: 250328 URL: https://gcc.gnu.org/viewcvs?rev=250328&root=gcc&view=rev Log: PR libstdc++/81395 fix crash when write follows large read PR libstdc++/81395 * include/bits/fstream.tcc (basic_filebuf::xsgetn): Don't set buffer pointers for write mode after reading. * testsuite/27_io/basic_filebuf/sgetn/char/81395.cc: New. Added: trunk/libstdc++-v3/testsuite/27_io/basic_filebuf/sgetn/char/81395.cc Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/bits/fstream.tcc
Fixed on trunk? (adjust summary and known-to-fail/work)
Done.
GCC 5 branch is being closed
Author: redi Date: Mon Oct 23 17:16:38 2017 New Revision: 254015 URL: https://gcc.gnu.org/viewcvs?rev=254015&root=gcc&view=rev Log: PR libstdc++/81395 fix crash when write follows large read Backport from mainline 2017-07-18 Jonathan Wakely <jwakely@redhat.com> PR libstdc++/81395 * include/bits/fstream.tcc (basic_filebuf::xsgetn): Don't set buffer pointers for write mode after reading. * testsuite/27_io/basic_filebuf/sgetn/char/81395.cc: New. Added: branches/gcc-7-branch/libstdc++-v3/testsuite/27_io/basic_filebuf/sgetn/char/81395.cc Modified: branches/gcc-7-branch/libstdc++-v3/ChangeLog branches/gcc-7-branch/libstdc++-v3/include/bits/fstream.tcc
Author: redi Date: Mon Oct 23 17:47:10 2017 New Revision: 254018 URL: https://gcc.gnu.org/viewcvs?rev=254018&root=gcc&view=rev Log: PR libstdc++/81395 fix crash when write follows large read Backport from mainline 2017-07-18 Jonathan Wakely <jwakely@redhat.com> PR libstdc++/81395 * include/bits/fstream.tcc (basic_filebuf::xsgetn): Don't set buffer pointers for write mode after reading. * testsuite/27_io/basic_filebuf/sgetn/char/81395.cc: New. Added: branches/gcc-6-branch/libstdc++-v3/testsuite/27_io/basic_filebuf/sgetn/char/81395.cc Modified: branches/gcc-6-branch/libstdc++-v3/ChangeLog branches/gcc-6-branch/libstdc++-v3/include/bits/fstream.tcc
Fixed for 6.5 and 7.3