[2/2] libio: Perform write-only fflush as part of freopen (bug 30731)

Message ID 54125d816ab3d073364d760ec5ec4ac0f98d3338.1691499154.git.fweimer@redhat.com
State Rejected
Headers
Series [1/2] libio: Introduce _IO_JUMPS_UNVALIDATED |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
redhat-pt-bot/TryBot-still_applies warning Patch no longer applies to master

Commit Message

Florian Weimer Aug. 8, 2023, 12:53 p.m. UTC
  For standard streams that have a file descriptor, the read flush
may change the file offset of the underlying file descriptor.  This
is undeseriable because it makes using freopen after fork unsafe
even for read-only files.

Tested on i686-linux-gnu and x86_64-linux-gnu.

---
 libio/Makefile              |  2 +-
 libio/fileops.c             | 22 +++++++++
 libio/freopen.c             |  3 +-
 libio/freopen64.c           |  3 +-
 libio/libio.h               |  4 ++
 libio/tst-freopen-no-seek.c | 91 +++++++++++++++++++++++++++++++++++++
 6 files changed, 120 insertions(+), 5 deletions(-)
 create mode 100644 libio/tst-freopen-no-seek.c
  

Comments

Andreas Schwab Aug. 8, 2023, 1:49 p.m. UTC | #1
On Aug 08 2023, Florian Weimer via Libc-alpha wrote:

> For standard streams that have a file descriptor, the read flush
> may change the file offset of the underlying file descriptor.  This
> is undeseriable because it makes using freopen after fork unsafe
> even for read-only files.

The POSIX standard says that fflush should be called unconditionally,
which means that it expects all side effects to happen.
  
Florian Weimer Aug. 8, 2023, 1:59 p.m. UTC | #2
* Andreas Schwab:

> On Aug 08 2023, Florian Weimer via Libc-alpha wrote:
>
>> For standard streams that have a file descriptor, the read flush
>> may change the file offset of the underlying file descriptor.  This
>> is undeseriable because it makes using freopen after fork unsafe
>> even for read-only files.
>
> The POSIX standard says that fflush should be called unconditionally,
> which means that it expects all side effects to happen.

Hmm.  I think we might be in undefined territory here.  But taking a
second look at it, the current (unpatched) behavior is probably more
generally useful.

I would like to apply the first patch as a cleanup, though.

Thanks,
Florian
  

Patch

diff --git a/libio/Makefile b/libio/Makefile
index 287ec11338..0ed1781792 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -83,7 +83,7 @@  tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \
 	tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \
 	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \
-	tst-wfile-sync tst-bz28828 tst-getdelim
+	tst-wfile-sync tst-bz28828 tst-getdelim tst-freopen-no-seek
 
 tests-internal = tst-vtables tst-vtables-interposed
 
diff --git a/libio/fileops.c b/libio/fileops.c
index 1c1113e339..92c4368bb9 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -816,6 +816,28 @@  _IO_new_file_sync (FILE *fp)
 }
 libc_hidden_ver (_IO_new_file_sync, _IO_file_sync)
 
+void
+__fflush_for_freopen (FILE *fp)
+{
+  void *vtable = _IO_JUMPS_UNVALIDATED (fp);
+  if (vtable == &_IO_file_jumps
+      || vtable == &_IO_file_jumps_mmap
+      || vtable == &_IO_file_jumps_maybe_mmap
+      || vtable == &_IO_wfile_jumps
+      || vtable == &_IO_wfile_jumps_mmap
+      || vtable == &_IO_wfile_jumps_maybe_mmap)
+    {
+      /* For a regular FILE * backed by a file descriptor, only
+	 perform the write flush.  A read flush may cause a seek that
+	 only has unwanted side effects because of the immediately
+	 following freopen operation.  */
+      if (fp->_IO_write_ptr > fp->_IO_write_base)
+	_IO_do_flush(fp);
+    }
+  else
+    _IO_SYNC (fp);
+}
+
 int
 _IO_file_sync_mmap (FILE *fp)
 {
diff --git a/libio/freopen.c b/libio/freopen.c
index 91ff5ec1f9..45e6d2da45 100644
--- a/libio/freopen.c
+++ b/libio/freopen.c
@@ -42,8 +42,7 @@  freopen (const char *filename, const char *mode, FILE *fp)
   CHECK_FILE (fp, NULL);
 
   _IO_acquire_lock (fp);
-  /* First flush the stream (failure should be ignored).  */
-  _IO_SYNC (fp);
+  __fflush_for_freopen (fp);
 
   if (!(fp->_flags & _IO_IS_FILEBUF))
     goto end;
diff --git a/libio/freopen64.c b/libio/freopen64.c
index 9a74f33dd4..c6e6c43d18 100644
--- a/libio/freopen64.c
+++ b/libio/freopen64.c
@@ -41,8 +41,7 @@  freopen64 (const char *filename, const char *mode, FILE *fp)
   CHECK_FILE (fp, NULL);
 
   _IO_acquire_lock (fp);
-  /* First flush the stream (failure should be ignored).  */
-  _IO_SYNC (fp);
+  __fflush_for_freopen (fp);
 
   if (!(fp->_flags & _IO_IS_FILEBUF))
     goto end;
diff --git a/libio/libio.h b/libio/libio.h
index 3d71bc1063..0cc5128e0b 100644
--- a/libio/libio.h
+++ b/libio/libio.h
@@ -160,6 +160,10 @@  extern wint_t __wunderflow (FILE *);
 extern wint_t __wuflow (FILE *);
 extern wint_t __woverflow (FILE *, wint_t);
 
+/* Special fflush operation immediately before freopen FILE *
+   replacement.  Avoids a read flush for descriptor-backed streams.  */
+void __fflush_for_freopen (FILE *) attribute_hidden;
+
 #define _IO_getc_unlocked(_fp) __getc_unlocked_body (_fp)
 #define _IO_peekc_unlocked(_fp)						\
   (__glibc_unlikely ((_fp)->_IO_read_ptr >= (_fp)->_IO_read_end)	\
diff --git a/libio/tst-freopen-no-seek.c b/libio/tst-freopen-no-seek.c
new file mode 100644
index 0000000000..00fd37c3c1
--- /dev/null
+++ b/libio/tst-freopen-no-seek.c
@@ -0,0 +1,91 @@ 
+/* Test that freopen does not seek unnecessarily (bug 30731).
+   Copyright (C) 2023 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+
+static void
+test (bool do_fdopen, bool do_wide, bool do_freopen64)
+{
+  char *path;
+  int fd = create_temp_file ("tst-freopen-no-seek-XXXXXX", &path);
+  TEST_VERIFY_EXIT (fd >= 0);
+
+  support_write_file_string (path, "freopen test file\n");
+
+  FILE *fp;
+  if (do_fdopen)
+    {
+      fp = fdopen (fd, "r");
+      if (fp == NULL)
+        FAIL_EXIT1 ("fdopen: %m");
+    }
+  else
+    {
+      xclose (fd);
+      fp = xfopen (path, "r");
+    }
+  free (path);
+
+  /* The fd descriptor shares the same underlying file description as the descriptor underlying fp.  In particular, the file offset is shared.  */
+  fd = dup (fileno (fp));
+  if (fd < 0)
+    FAIL_EXIT1 ("dup: %m");
+
+  if (do_wide)
+    TEST_COMPARE (fgetc (fp), 'f');
+  else
+    TEST_COMPARE (fgetc (fp), 'f');
+  TEST_COMPARE (xlseek (fd, 0, SEEK_CUR), strlen ("freopen test file\n"));
+
+  if (do_freopen64)
+    fp = freopen64 ("/dev/null", "r", fp);
+  else
+    fp = freopen ("/dev/null", "r", fp);
+  if (fp == NULL)
+    FAIL_EXIT1 ("freopen (\"/dev/null\", \"r\"): %m");
+
+  /* The freopen call should not have changed the seek offset of the
+     underlying file description because there is nothing to flush as
+     the stream is opened read-only, and changing the seek offset on a
+     file descriptor that is closed immediately afterwards is only
+     visible through unwanted side effects.  */
+  TEST_COMPARE (xlseek (fd, 0, SEEK_CUR), strlen ("freopen test file\n"));
+
+  xclose (fd);
+  fclose (fp);
+}
+
+static int
+do_test (void)
+{
+  for (int do_fdopen = 0; do_fdopen < 2; ++do_fdopen)
+    for (int do_wide = 0; do_wide < 2; ++do_wide)
+      for (int do_freopen64 = 0; do_freopen64 < 2; ++do_freopen64)
+        test (do_fdopen, do_wide, do_freopen64);
+  return 0;
+}
+
+#include <support/test-driver.c>