[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 Richard Biener changed: What|Removed |Added Keywords||wrong-code Priority|P3 |P2 --- Comment #15 from Richard Biener --- Fixed on trunk? (adjust summary and known-to-fail/work)
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 --- Comment #14 from Jonathan Wakely --- Author: redi Date: Tue Jul 18 23:39:34 2017 New Revision: 250328 URL: https://gcc.gnu.org/viewcvs?rev=250328=gcc=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
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 --- Comment #13 from Paolo Carlini --- 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 ;)
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 --- Comment #12 from Jonathan Wakely --- 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).
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395
--- Comment #11 from Jonathan Wakely ---
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.
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 --- Comment #10 from Paolo Carlini --- 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...
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 --- Comment #9 from Jonathan Wakely --- (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.
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 --- Comment #8 from Jonathan Wakely --- (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 :-)
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395
--- Comment #7 from Jonathan Wakely ---
(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.
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 --- Comment #6 from Paolo Carlini --- 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?
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 --- Comment #5 from Jonathan Wakely --- 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.
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 --- Comment #4 from Paolo Carlini --- 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...
[Bug libstdc++/81395] [5/6/7/8 Regression] basic_filebuf::overflow recurses and overflows stack
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81395 Jonathan Wakely changed: What|Removed |Added Status|NEW |ASSIGNED CC||paolo.carlini at oracle dot com Known to work||4.5.4 Assignee|unassigned at gcc dot gnu.org |redi at gcc dot gnu.org Target Milestone|--- |5.5 Summary|basic_filebuf::overflow |[5/6/7/8 Regression] |recurses and overflows |basic_filebuf::overflow |stack |recurses and overflows ||stack Known to fail||4.6.4, 4.7.4, 4.8.5, 4.9.4, ||5.4.0, 6.3.0, 7.0 --- Comment #3 from Jonathan Wakely --- This started in 4.6.0 with r87453 Paolo, does the change in comment 2 look right to you?
