PR libstdc++/81751 don't call fflush(NULL)

Submitted by Jonathan Wakely on Aug. 9, 2017, 3:56 p.m.

Details

Message ID 20170809155605.GL15340@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Aug. 9, 2017, 3:56 p.m.
This fixes a couple of problems in __gnu_cxx::stdio_filebuf,
specifically in the __basic_file::sys_open(FILE*, openmode) function
it uses when constructed from an existing FILE stream.

Firstly, r86756 changed __basic_file::sys_open(FILE*, openmode) to put
the call to sync() before the assignment that sets _M_cfile. This
means the fflush(_M_cfile) call has a null argument, and flushes all
open streams. I don't think that was intentional, and we only want to
flush the FILE we're constructing the streambuf with. (I think this is
a regression from 3.4.3, which just flushed the one stream).

Secondly, we zero errno so that we can tell if fflush sets it. We need
to restore the original value to meet the promise we make at
https://gcc.gnu.org/onlinedocs/libstdc++/manual/errno.html

Paolo, does the this->sync() to fflush(__file) change look right?

	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.

Tested powerpc64le-linux, not yet committed.
commit 72565d197cc91a04882a398d9d242a0581d6cc09
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Aug 8 23:20:40 2017 +0100

    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.

Comments

Paolo Carlini Aug. 9, 2017, 4:39 p.m.
Hi,

> On 9 Aug 2017, at 17:56, Jonathan Wakely <jwakely@redhat.com> wrote:
> 
> This fixes a couple of problems in __gnu_cxx::stdio_filebuf,
> specifically in the __basic_file::sys_open(FILE*, openmode) function
> it uses when constructed from an existing FILE stream.
> 
> Firstly, r86756 changed __basic_file::sys_open(FILE*, openmode) to put
> the call to sync() before the assignment that sets _M_cfile. This
> means the fflush(_M_cfile) call has a null argument, and flushes all
> open streams. I don't think that was intentional, and we only want to
> flush the FILE we're constructing the streambuf with. (I think this is
> a regression from 3.4.3, which just flushed the one stream).
> 
> Secondly, we zero errno so that we can tell if fflush sets it. We need
> to restore the original value to meet the promise we make at
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/errno.html
> 
> Paolo, does the this->sync() to fflush(__file) change look right?

Yes, I went through the audit trail of libstdc++/81751 and the change looks good. Certainly we didn't intentionally want the behavior of fflush(0)!

Thanks!
Paolo
Jonathan Wakely Aug. 9, 2017, 5:52 p.m.
On 09/08/17 18:39 +0200, Paolo Carlini wrote:
>Hi,
>
>> On 9 Aug 2017, at 17:56, Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>> This fixes a couple of problems in __gnu_cxx::stdio_filebuf,
>> specifically in the __basic_file::sys_open(FILE*, openmode) function
>> it uses when constructed from an existing FILE stream.
>>
>> Firstly, r86756 changed __basic_file::sys_open(FILE*, openmode) to put
>> the call to sync() before the assignment that sets _M_cfile. This
>> means the fflush(_M_cfile) call has a null argument, and flushes all
>> open streams. I don't think that was intentional, and we only want to
>> flush the FILE we're constructing the streambuf with. (I think this is
>> a regression from 3.4.3, which just flushed the one stream).
>>
>> Secondly, we zero errno so that we can tell if fflush sets it. We need
>> to restore the original value to meet the promise we make at
>> https://gcc.gnu.org/onlinedocs/libstdc++/manual/errno.html
>>
>> Paolo, does the this->sync() to fflush(__file) change look right?
>
>Yes, I went through the audit trail of libstdc++/81751 and the change looks good. Certainly we didn't intentionally want the behavior of fflush(0)!

OK, thanks - I've committed the change to trunk now.

Patch hide | download patch | download mbox

diff --git a/libstdc++-v3/config/io/basic_file_stdio.cc b/libstdc++-v3/config/io/basic_file_stdio.cc
index e736701..eeb1e5e 100644
--- a/libstdc++-v3/config/io/basic_file_stdio.cc
+++ b/libstdc++-v3/config/io/basic_file_stdio.cc
@@ -195,11 +195,13 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __basic_file* __ret = NULL;
     if (!this->is_open() && __file)
       {
-	int __err;
+	int __err, __save_errno = errno;
+	// POSIX guarantees that fflush sets errno on error, but C doesn't.
 	errno = 0;
 	do
-	  __err = this->sync();
+	  __err = fflush(__file);
 	while (__err && errno == EINTR);
+	errno = __save_errno;
 	if (!__err)
 	  {
 	    _M_cfile = __file;
diff --git a/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc b/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc
new file mode 100644
index 0000000..ba566f8
--- /dev/null
+++ b/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc
@@ -0,0 +1,39 @@ 
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-require-fileio "" }
+
+#include <ext/stdio_filebuf.h>
+#include <cstdio>
+#include <cerrno>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  FILE* f = std::fopen("79820.txt", "w");
+  std::fclose(f);
+  errno = 127;
+  __gnu_cxx::stdio_filebuf<char> b(f, std::ios::out, BUFSIZ);
+  VERIFY(errno == 127); // PR libstdc++/79820
+}
+
+int
+main()
+{
+  test01();
+}
diff --git a/libstdc++-v3/testsuite/ext/stdio_filebuf/char/81751.cc b/libstdc++-v3/testsuite/ext/stdio_filebuf/char/81751.cc
new file mode 100644
index 0000000..22191dcb
--- /dev/null
+++ b/libstdc++-v3/testsuite/ext/stdio_filebuf/char/81751.cc
@@ -0,0 +1,53 @@ 
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-require-fileio "" }
+
+#include <ext/stdio_filebuf.h>
+#include <cstdio>
+#include <cerrno>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  FILE* out = std::fopen("81751.txt", "w");
+  std::fwrite("Some words.", 1, 10, out);
+
+  FILE* in1 = std::fopen("81751.txt", "r");
+  __gnu_cxx::stdio_filebuf<char> buf1(in1, std::ios::in, BUFSIZ);
+  int c = buf1.sgetc();
+  VERIFY( c == std::char_traits<char>::eof() ); // PR libstdc++/81751
+
+  std::fflush(out);
+  FILE* in2 = std::fopen("81751.txt", "r");
+  __gnu_cxx::stdio_filebuf<char> buf2(in2, std::ios::in, BUFSIZ);
+  c = buf2.sgetc();
+  VERIFY( c == 'S' );
+
+  buf1.close();
+  buf2.close();
+  std::fclose(in1);
+  std::fclose(in2);
+  std::fclose(out);
+}
+
+int
+main()
+{
+  test01();
+}