[Bug libstdc++/81751] __basic_file::sync() may flush _all_ files

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81751

--- Comment #7 from Jonathan Wakely  ---
Author: redi
Date: Wed Aug  9 17:52:10 2017
New Revision: 250993

URL: https://gcc.gnu.org/viewcvs?rev=250993=gcc=rev
Log:
PR libstdc++/81751 don't call fflush(NULL)

PR libstdc++/79820
PR libstdc++/81751
* config/io/basic_file_stdio.cc (sys_open(FILE*, ios_base::openmode)):
Call fflush on the stream instead of calling sync() while _M_cfile is
null. Restore original value of errno.
* testsuite/ext/stdio_filebuf/char/79820.cc: New.
* testsuite/ext/stdio_filebuf/char/81751.cc: New.

Added:
trunk/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc
trunk/libstdc++-v3/testsuite/ext/stdio_filebuf/char/81751.cc
Modified:
trunk/libstdc++-v3/ChangeLog
trunk/libstdc++-v3/config/io/basic_file_stdio.cc

[Bug libstdc++/81751] __basic_file::sync() may flush _all_ files

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81751

--- Comment #6 from Jonathan Wakely  ---
Or just call fflush(__file) directly:

https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00673.html

[Bug libstdc++/81751] __basic_file::sync() may flush _all_ files

[bugzilla.volker at kabelmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81751

--- Comment #5 from Volker Wehrs  ---
I'm sorry but I ignored the first if-clause in sys_open(). That if-clause makes
sure there is no open file referenced by the __basic_file, otherwise sys_open()
fails.

Then the sync() is called before the new __file is set, so in the current
implementation sync() can only see a _M_cfile set to NULL. Thus sync() is wrong
here.

Note that currently the new file (in __file) is implicitly flushed by the call
to fflush("NULL"), i.e. "flush all". So the new file should be sync'd
explicitly.

So sys_open() might look like this:

  __basic_file*
  __basic_file::sys_open(__c_file* __file, ios_base::openmode)
  {
__basic_file* __ret = NULL;
if (!this->is_open() && __file)
  {
_M_cfile = __file;
_M_cfile_created = false;
__ret = this;
this->sync();
  }
return __ret;
  }

[...]

  int
  __basic_file::sync()
  {
int __err;
do
  __err = fflush(_M_cfile);
while ( __err && errno == EINTR );
return __err;
  }

[Bug libstdc++/81751] __basic_file::sync() may flush _all_ files

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81751

--- Comment #4 from Jonathan Wakely  ---
But there is an open FILE and it might have pending writes or ungetc'd data
that should be flushed. I think that's why it's there. Consider:

  FILE* f = std::fopen("81751.txt", "w+");
  std::fwrite("Some words.", 1, 10, f);
  __gnu_cxx::stdio_filebuf buf(f, std::ios::in|std::ios::out, BUFSIZ);
  int c = buf.sgetc();

If the constructor doesn't flush then the read performed by sgetc will follow
the write without an intervening seek or flush, which is undefined behaviour.

The constructor taking a file descriptor creates a new FILE using fdopen, so
there are no buffered writes on the stream, and a flush isn't needed.

[Bug libstdc++/81751] __basic_file::sync() may flush _all_ files

[bugzilla.volker at kabelmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81751

--- Comment #3 from Volker Wehrs  ---
Why is sync() (alias fflush()) called in sys_open(FILE*,...) at all?

1. It is not called in sys_open(int,...)
2. Both sys_open() functions are called in the constructor of stdio_filebuf
_only_.

So, as __basic_file seem to be intended as an internal class, maybe the call to
sync() can completely be omitted as there definitively is no open file.

[Bug libstdc++/81751] __basic_file::sync() may flush _all_ files

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81751

--- Comment #2 from Jonathan Wakely  ---
The current __basic_file::sys_open behaviour came from the PR 17215 fix
(r86756). Before that change we called sync() after doing _M_cfile=__file, but
after we call sync() while it's still null.

I think rather than changing sync(), the correct fix is to not call it while
_M_cfile is null. Instead we should directly call fflush(__file) in sys_open.
That should restore the behaviour that was present before GCC 3.4.4

[Bug libstdc++/81751] __basic_file::sync() may flush _all_ files

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81751

Jonathan Wakely  changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
   Last reconfirmed||2017-08-07
 Ever confirmed|0   |1

--- Comment #1 from Jonathan Wakely  ---
That seems like a sensible change.