[2/2] libio: Perform write-only fflush as part of freopen (bug 30731)
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
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
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.
* 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
@@ -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
@@ -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)
{
@@ -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;
@@ -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;
@@ -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) \
new file mode 100644
@@ -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>