* [PATCH 0/6] Fix issues around flushing and file offsets for input files
@ 2025-01-14 2:00 Joseph Myers
2025-01-14 2:02 ` [PATCH 1/6] Fix fflush after ungetc on input file (bug 5994) Joseph Myers
` (5 more replies)
0 siblings, 6 replies; 26+ messages in thread
From: Joseph Myers @ 2025-01-14 2:00 UTC (permalink / raw)
To: libc-alpha
There are several open bugs relating to flushing of FILE* streams
(with fflush and other operations) and their offsets (both the file
position indicator in the FILE*, and the offset in the underlying open
file description), especially after ungetc but not limited to that
case.
Fix five such bugs. Each has its own fix with associated test
specific to that bug; at the end of the patch series, add a test that
more systematically covers different combinations of cases for such
issues, with 25664 separate scenarios tested (which include examples
of all the five separate fixed bugs), all of which pass given the five
previous bug fixes. (None of the patches do anything about the case
of the "old" FILE structure, where that uses separate function
implementations.)
It's likely most of the patches would work on their own or with only a
subset of the previous patches rather than having strict dependencies,
though they haven't been tested that way (but can be retested like
that if approved out of order). In any case, these patches should
wait until after the 2.41 release to go in. (Note in particular that
the fix for bug 12724 may be high risk; a previous attempt was made to
fix that bug and subsequently reverted because it caused problems.)
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/6] Fix fflush after ungetc on input file (bug 5994)
2025-01-14 2:00 [PATCH 0/6] Fix issues around flushing and file offsets for input files Joseph Myers
@ 2025-01-14 2:02 ` Joseph Myers
2025-01-15 4:20 ` DJ Delorie
2025-01-14 2:03 ` [PATCH 2/6] Make fclose seek input file to right offset (bug 12724) Joseph Myers
` (4 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Joseph Myers @ 2025-01-14 2:02 UTC (permalink / raw)
To: libc-alpha
As discussed in bug 5994 (plus duplicates), POSIX requires fflush
after ungetc to discard pushed-back characters but preserve the file
position indicator. For this purpose, each ungetc decrements the file
position indicator by 1; it is unspecified after ungetc at the start
of the file, and after ungetwc, so no special handling is needed for
either of those cases.
This is fixed with appropriate logic in _IO_new_file_sync. I haven't
made any attempt to test or change things in this area for the "old"
functions; the case of files using mmap is addressed in a subsequent
patch (and there seem to be no problems in this area with files opened
with fmemopen).
Tested for x86_64.
---
libio/fileops.c | 5 +++
stdio-common/Makefile | 1 +
stdio-common/tst-ungetc-fflush.c | 64 ++++++++++++++++++++++++++++++++
3 files changed, 70 insertions(+)
create mode 100644 stdio-common/tst-ungetc-fflush.c
diff --git a/libio/fileops.c b/libio/fileops.c
index 775999deb3..27e1977eb6 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -799,6 +799,11 @@ _IO_new_file_sync (FILE *fp)
if (fp->_IO_write_ptr > fp->_IO_write_base)
if (_IO_do_flush(fp)) return EOF;
delta = fp->_IO_read_ptr - fp->_IO_read_end;
+ if (_IO_in_backup (fp))
+ {
+ _IO_switch_to_main_get_area (fp);
+ delta += fp->_IO_read_ptr - fp->_IO_read_end;
+ }
if (delta != 0)
{
off64_t new_pos = _IO_SYSSEEK (fp, delta, 1);
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index bbdc1ff709..589b786f45 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -302,6 +302,7 @@ tests := \
tst-swscanf \
tst-tmpnam \
tst-ungetc \
+ tst-ungetc-fflush \
tst-ungetc-leak \
tst-ungetc-nomem \
tst-unlockedio \
diff --git a/stdio-common/tst-ungetc-fflush.c b/stdio-common/tst-ungetc-fflush.c
new file mode 100644
index 0000000000..a86d1fdb7f
--- /dev/null
+++ b/stdio-common/tst-ungetc-fflush.c
@@ -0,0 +1,64 @@
+/* Test flushing input file after ungetc (bug 5994).
+ Copyright (C) 2025 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 <stdio.h>
+
+#include <support/check.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+
+int
+do_test (void)
+{
+ char *filename = NULL;
+ int fd = create_temp_file ("tst-ungetc-fflush", &filename);
+ TEST_VERIFY_EXIT (fd != -1);
+ xclose (fd);
+
+ /* Test as in bug 5994. */
+ FILE *fp = xfopen (filename, "w");
+ TEST_VERIFY_EXIT (fputs ("#include", fp) >= 0);
+ xfclose (fp);
+ fp = xfopen (filename, "r");
+ TEST_COMPARE (fgetc (fp), '#');
+ TEST_COMPARE (fgetc (fp), 'i');
+ TEST_COMPARE (ungetc ('@', fp), '@');
+ TEST_COMPARE (fflush (fp), 0);
+ TEST_COMPARE (lseek (fileno (fp), 0, SEEK_CUR), 1);
+ TEST_COMPARE (fgetc (fp), 'i');
+ TEST_COMPARE (fgetc (fp), 'n');
+ xfclose (fp);
+
+ /* Test as in bug 12799 (duplicate of 5994). */
+ fp = xfopen (filename, "w+");
+ TEST_VERIFY_EXIT (fputs ("hello world", fp) >= 0);
+ rewind (fp);
+ TEST_VERIFY (fileno (fp) >= 0);
+ char buffer[10];
+ TEST_COMPARE (fread (buffer, 1, 5, fp), 5);
+ TEST_COMPARE (fgetc (fp), ' ');
+ TEST_COMPARE (ungetc ('@', fp), '@');
+ TEST_COMPARE (fflush (fp), 0);
+ TEST_COMPARE (fgetc (fp), ' ');
+ xfclose (fp);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.43.0
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/6] Make fclose seek input file to right offset (bug 12724)
2025-01-14 2:00 [PATCH 0/6] Fix issues around flushing and file offsets for input files Joseph Myers
2025-01-14 2:02 ` [PATCH 1/6] Fix fflush after ungetc on input file (bug 5994) Joseph Myers
@ 2025-01-14 2:03 ` Joseph Myers
2025-01-15 5:33 ` DJ Delorie
2025-01-14 2:03 ` [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369) Joseph Myers
` (3 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Joseph Myers @ 2025-01-14 2:03 UTC (permalink / raw)
To: libc-alpha
As discussed in bug 12724 and required by POSIX, before an input file
(based on an underlying seekable file descriptor) is closed, fclose is
sometimes required to seek that file descriptor to the correct offset,
so that any other file descriptors sharing the underlying open file
description are left at that offset (as a motivating example, a script
could call a sequence of commands each of which processes some data
from (seekable) stdin using stdio; fclose needs to do this so that
each successive command can read exactly the data not handled by
previous commands), but glibc fails to do this.
The precise POSIX wording has changed a few times; in the 2024 edition
it's "If the file is not already at EOF, and the file is one capable
of seeking, the file offset of the underlying open file description
shall be set to the file position of the stream if the stream is the
active handle to the underlying file description.".
Add appropriate logic to _IO_new_file_close_it to handle this case. I
haven't made any attempt to test or change things in this area for the
"old" functions.
Note that there was a previous attempt to fix bug 12724, reverted in
commit eb6cbd249f4465b01f428057bf6ab61f5f0c07e3. The fix version here
addresses the original test in that bug report without breaking the
one given in a subsequent comment in that bug report (which works with
glibc before the patch, but maybe was broken by the original fix that
was reverted).
The logic here tries to take care not to seek the file, even to its
newly computed current offset, if at EOF / possibly not the active
handle; even seeking to the current offset would be problematic
because of a potential race (fclose computes the current offset,
another thread or process with the active handle does its own seek,
fclose does a seek (not permitted by POSIX in this case) that loses
the effect of the seek on the active handle in another thread or
process). There are tests included for various cases of being or not
being the active handle, though there aren't tests for the potential
race condition.
Tested for x86_64.
---
libio/fileops.c | 43 +++++-
stdio-common/Makefile | 1 +
stdio-common/tst-fclose-offset.c | 225 +++++++++++++++++++++++++++++++
3 files changed, 264 insertions(+), 5 deletions(-)
create mode 100644 stdio-common/tst-fclose-offset.c
diff --git a/libio/fileops.c b/libio/fileops.c
index 27e1977eb6..358378431f 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -127,15 +127,48 @@ _IO_new_file_init (struct _IO_FILE_plus *fp)
int
_IO_new_file_close_it (FILE *fp)
{
- int write_status;
+ int flush_status = 0;
if (!_IO_file_is_open (fp))
return EOF;
if ((fp->_flags & _IO_NO_WRITES) == 0
&& (fp->_flags & _IO_CURRENTLY_PUTTING) != 0)
- write_status = _IO_do_flush (fp);
- else
- write_status = 0;
+ flush_status = _IO_do_flush (fp);
+ else if (fp->_fileno >= 0
+ /* If this is the active handle, we must seek the
+ underlying open file description (possibly shared with
+ other file descriptors that remain open) to the correct
+ offset. But if this stream is in a state such that some
+ other handle might have become the active handle, then
+ (a) at the time it entered that state, the underlying
+ open file description had the correct offset, and (b)
+ seeking the underlying open file description, even to
+ its newly determined current offset, is not safe because
+ it can race with operations on a different active
+ handle. So check here for cases where it is necessary
+ to seek, while avoiding seeking in cases where it is
+ unsafe to do so. */
+ && (_IO_in_backup (fp)
+ || (fp->_mode <= 0 && fp->_IO_read_ptr < fp->_IO_read_end)
+ || (_IO_vtable_offset (fp) == 0
+ && fp->_mode > 0 && (fp->_wide_data->_IO_read_ptr
+ < fp->_wide_data->_IO_read_end))))
+ {
+ off64_t o = _IO_SEEKOFF (fp, 0, _IO_seek_cur, 0);
+ if (o == EOF)
+ {
+ if (errno != ESPIPE)
+ flush_status = EOF;
+ }
+ else
+ {
+ if (_IO_in_backup (fp))
+ o -= fp->_IO_save_end - fp->_IO_save_base;
+ flush_status = (_IO_SYSSEEK (fp, o, SEEK_SET) < 0 && errno != ESPIPE
+ ? EOF
+ : 0);
+ }
+ }
_IO_unsave_markers (fp);
@@ -160,7 +193,7 @@ _IO_new_file_close_it (FILE *fp)
fp->_fileno = -1;
fp->_offset = _IO_pos_BAD;
- return close_status ? close_status : write_status;
+ return close_status ? close_status : flush_status;
}
libc_hidden_ver (_IO_new_file_close_it, _IO_file_close_it)
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 589b786f45..b5f78c365b 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -234,6 +234,7 @@ tests := \
tst-bz11319-fortify2 \
tst-cookie \
tst-dprintf-length \
+ tst-fclose-offset \
tst-fdopen \
tst-fdopen2 \
tst-ferror \
diff --git a/stdio-common/tst-fclose-offset.c b/stdio-common/tst-fclose-offset.c
new file mode 100644
index 0000000000..a31de1117c
--- /dev/null
+++ b/stdio-common/tst-fclose-offset.c
@@ -0,0 +1,225 @@
+/* Test offset of input file descriptor after close (bug 12724).
+ Copyright (C) 2025 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 <errno.h>
+#include <stdio.h>
+#include <wchar.h>
+
+#include <support/check.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+
+int
+do_test (void)
+{
+ char *filename = NULL;
+ int fd = create_temp_file ("tst-fclose-offset", &filename);
+ TEST_VERIFY_EXIT (fd != -1);
+
+ /* Test offset of open file description for output and input streams
+ after fclose, case from bug 12724. */
+
+ const char buf[] = "hello world";
+ xwrite (fd, buf, sizeof buf);
+ TEST_COMPARE (lseek (fd, 1, SEEK_SET), 1);
+ int fd2 = xdup (fd);
+ FILE *f = fdopen (fd2, "w");
+ TEST_VERIFY_EXIT (f != NULL);
+ TEST_COMPARE (fputc (buf[1], f), buf[1]);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 2);
+
+ /* Likewise for an input stream. */
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "r");
+ TEST_VERIFY_EXIT (f != NULL);
+ TEST_COMPARE (fgetc (f), buf[2]);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 3);
+
+ /* Test offset of open file description for output and input streams
+ after fclose, case from comment on bug 12724 (failed after first
+ attempt at fixing that bug). This verifies that the offset is
+ not reset when there has been no input or output on the FILE* (in
+ that case, the FILE* might not be the active handle). */
+
+ TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
+ xwrite (fd, buf, sizeof buf);
+ TEST_COMPARE (lseek (fd, 1, SEEK_SET), 1);
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "w");
+ TEST_VERIFY_EXIT (f != NULL);
+ TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
+
+ /* Likewise for an input stream. */
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "r");
+ TEST_VERIFY_EXIT (f != NULL);
+ TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
+
+ /* Further cases without specific tests in bug 12724, to verify
+ proper operation of the rules about the offset only being set
+ when the stream is the active handle. */
+
+ /* Test offset set by fclose after fseek and fgetc. */
+ TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "r");
+ TEST_VERIFY_EXIT (f != NULL);
+ TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
+ TEST_COMPARE (fgetc (f), buf[1]);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 2);
+
+ /* Test offset not set by fclose after fseek and fgetc, if that
+ fgetc is at EOF (in which case the active handle might have
+ changed). */
+ TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "r");
+ TEST_VERIFY_EXIT (f != NULL);
+ TEST_COMPARE (fseek (f, sizeof buf, SEEK_SET), 0);
+ TEST_COMPARE (fgetc (f), EOF);
+ TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
+
+ /* Test offset not set by fclose after fseek and fgetc and fflush
+ (active handle might have changed after fflush). */
+ TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "r");
+ TEST_VERIFY_EXIT (f != NULL);
+ TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
+ TEST_COMPARE (fgetc (f), buf[1]);
+ TEST_COMPARE (fflush (f), 0);
+ TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
+
+ /* Test offset not set by fclose after fseek and fgetc, if the
+ stream is unbuffered (active handle might change at any
+ time). */
+ TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "r");
+ TEST_VERIFY_EXIT (f != NULL);
+ setbuf (f, NULL);
+ TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
+ TEST_COMPARE (fgetc (f), buf[1]);
+ TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
+
+ /* Also test such cases with the stream in wide mode. */
+
+ /* Test offset set by fclose after fseek and fgetwc. */
+ TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "r");
+ TEST_VERIFY_EXIT (f != NULL);
+ TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
+ TEST_COMPARE (fgetwc (f), (wint_t) buf[1]);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 2);
+
+ /* Test offset not set by fclose after fseek and fgetwc, if that
+ fgetwc is at EOF (in which case the active handle might have
+ changed). */
+ TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "r");
+ TEST_VERIFY_EXIT (f != NULL);
+ TEST_COMPARE (fseek (f, sizeof buf, SEEK_SET), 0);
+ TEST_COMPARE (fgetwc (f), WEOF);
+ TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
+
+ /* Test offset not set by fclose after fseek and fgetwc and fflush
+ (active handle might have changed after fflush). */
+ TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "r");
+ TEST_VERIFY_EXIT (f != NULL);
+ TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
+ TEST_COMPARE (fgetwc (f), (wint_t) buf[1]);
+ TEST_COMPARE (fflush (f), 0);
+ TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
+
+ /* Test offset not set by fclose after fseek and fgetwc, if the
+ stream is unbuffered (active handle might change at any
+ time). */
+ TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
+ fd2 = xdup (fd);
+ f = fdopen (fd2, "r");
+ TEST_VERIFY_EXIT (f != NULL);
+ setbuf (f, NULL);
+ TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
+ TEST_COMPARE (fgetwc (f), (wint_t) buf[1]);
+ TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
+ xfclose (f);
+ errno = 0;
+ TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
+ TEST_COMPARE (errno, EBADF);
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.43.0
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369)
2025-01-14 2:00 [PATCH 0/6] Fix issues around flushing and file offsets for input files Joseph Myers
2025-01-14 2:02 ` [PATCH 1/6] Fix fflush after ungetc on input file (bug 5994) Joseph Myers
2025-01-14 2:03 ` [PATCH 2/6] Make fclose seek input file to right offset (bug 12724) Joseph Myers
@ 2025-01-14 2:03 ` Joseph Myers
2025-01-15 6:01 ` DJ Delorie
2025-01-15 17:27 ` Florian Weimer
2025-01-14 2:03 ` [PATCH 4/6] Fix fseek handling for mmap files after ungetc or fflush (bug 32529) Joseph Myers
` (2 subsequent siblings)
5 siblings, 2 replies; 26+ messages in thread
From: Joseph Myers @ 2025-01-14 2:03 UTC (permalink / raw)
To: libc-alpha
As discussed in bug 32369 and required by POSIX, the POSIX feature
fflush (NULL) should flush input files, not just output files. The
POSIX requirement is that "fflush() shall perform this flushing action
on all streams for which the behavior is defined above", and the
definition for input files is for "a stream open for reading with an
underlying file description, if the file is not already at EOF, and
the file is one capable of seeking".
Implement this requirement in glibc. (The underlying flushing
implementation is what deals with avoiding errors for seeking on an
unseekable file.)
Tested for x86_64.
---
libio/genops.c | 7 ++++
stdio-common/Makefile | 1 +
stdio-common/tst-fflush-all-input.c | 53 +++++++++++++++++++++++++++++
3 files changed, 61 insertions(+)
create mode 100644 stdio-common/tst-fflush-all-input.c
diff --git a/libio/genops.c b/libio/genops.c
index 2197bfe7a1..e4378ca48f 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -730,6 +730,13 @@ _IO_flush_all (void)
)
&& _IO_OVERFLOW (fp, EOF) == EOF)
result = EOF;
+ if (_IO_fileno (fp) >= 0
+ && ((fp->_mode <= 0 && fp->_IO_read_ptr < fp->_IO_read_end)
+ || (_IO_vtable_offset (fp) == 0
+ && fp->_mode > 0 && (fp->_wide_data->_IO_read_ptr
+ < fp->_wide_data->_IO_read_end)))
+ && _IO_SYNC (fp) != 0)
+ result = EOF;
_IO_funlockfile (fp);
run_fp = NULL;
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index b5f78c365b..48fbf05a85 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -238,6 +238,7 @@ tests := \
tst-fdopen \
tst-fdopen2 \
tst-ferror \
+ tst-fflush-all-input \
tst-fgets \
tst-fgets2 \
tst-fileno \
diff --git a/stdio-common/tst-fflush-all-input.c b/stdio-common/tst-fflush-all-input.c
new file mode 100644
index 0000000000..e9df3a0c08
--- /dev/null
+++ b/stdio-common/tst-fflush-all-input.c
@@ -0,0 +1,53 @@
+/* Test fflush (NULL) flushes input files (bug 32369).
+ Copyright (C) 2025 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 <stdio.h>
+#include <unistd.h>
+#include <wchar.h>
+
+#include <support/check.h>
+#include <support/xstdio.h>
+
+int
+do_test (void)
+{
+ FILE *temp = tmpfile ();
+ TEST_VERIFY_EXIT (temp != NULL);
+ fprintf (temp, "abc");
+ TEST_COMPARE (fflush (temp), 0);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
+ TEST_COMPARE (fgetc (temp), 'a');
+ TEST_COMPARE (fflush (NULL), 0);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
+ xfclose (temp);
+
+ /* Likewise, but in wide mode. */
+ temp = tmpfile ();
+ TEST_VERIFY_EXIT (temp != NULL);
+ fwprintf (temp, L"abc");
+ TEST_COMPARE (fflush (temp), 0);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
+ TEST_COMPARE (fgetwc (temp), L'a');
+ TEST_COMPARE (fflush (NULL), 0);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
+ xfclose (temp);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.43.0
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/6] Fix fseek handling for mmap files after ungetc or fflush (bug 32529)
2025-01-14 2:00 [PATCH 0/6] Fix issues around flushing and file offsets for input files Joseph Myers
` (2 preceding siblings ...)
2025-01-14 2:03 ` [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369) Joseph Myers
@ 2025-01-14 2:03 ` Joseph Myers
2025-01-15 6:35 ` DJ Delorie
2025-01-14 2:04 ` [PATCH 5/6] Fix fflush handling for mmap files after ungetc (bug 32535) Joseph Myers
2025-01-14 2:04 ` [PATCH 6/6] Add test of input file flushing / offset issues Joseph Myers
5 siblings, 1 reply; 26+ messages in thread
From: Joseph Myers @ 2025-01-14 2:03 UTC (permalink / raw)
To: libc-alpha
As discussed in bug 32529, fseek fails on files opened for reading
using mmap after ungetc. The implementation of fseek for such files
has an offset computation that's also incorrect after fflush. A
combined fix addresses both problems (with tests for both included as
well) and it seems reasonable to consider them a single bug.
Tested for x86_64.
---
libio/fileops.c | 9 +++++-
stdio-common/Makefile | 1 +
stdio-common/tst-fseek-mmap.c | 59 +++++++++++++++++++++++++++++++++++
3 files changed, 68 insertions(+), 1 deletion(-)
create mode 100644 stdio-common/tst-fseek-mmap.c
diff --git a/libio/fileops.c b/libio/fileops.c
index 358378431f..97875d1eaf 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -1106,11 +1106,18 @@ _IO_file_seekoff_mmap (FILE *fp, off64_t offset, int dir, int mode)
if (mode == 0)
return fp->_offset - (fp->_IO_read_end - fp->_IO_read_ptr);
+ if (_IO_in_backup (fp))
+ {
+ if (dir == _IO_seek_cur)
+ offset += fp->_IO_read_ptr - fp->_IO_read_end;
+ _IO_switch_to_main_get_area (fp);
+ }
+
switch (dir)
{
case _IO_seek_cur:
/* Adjust for read-ahead (bytes is buffer). */
- offset += fp->_IO_read_ptr - fp->_IO_read_base;
+ offset += fp->_offset - (fp->_IO_read_end - fp->_IO_read_ptr);
break;
case _IO_seek_set:
break;
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 48fbf05a85..06c9eaf426 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -261,6 +261,7 @@ tests := \
tst-freopen64-7 \
tst-freopen7 \
tst-fseek \
+ tst-fseek-mmap \
tst-fwrite \
tst-fwrite-memstrm \
tst-fwrite-overflow \
diff --git a/stdio-common/tst-fseek-mmap.c b/stdio-common/tst-fseek-mmap.c
new file mode 100644
index 0000000000..86fa99a1a2
--- /dev/null
+++ b/stdio-common/tst-fseek-mmap.c
@@ -0,0 +1,59 @@
+/* Test fseek on files using mmap (bug 32529).
+ Copyright (C) 2025 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 <stdio.h>
+
+#include <support/check.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+
+int
+do_test (void)
+{
+ char *filename = NULL;
+ int fd = create_temp_file ("tst-fseek-mmap", &filename);
+ TEST_VERIFY_EXIT (fd != -1);
+ xclose (fd);
+
+ /* Test fseek after ungetc (bug 32529). */
+ FILE *fp = xfopen (filename, "w");
+ TEST_VERIFY (0 <= fputs ("test", fp));
+ xfclose (fp);
+
+ fp = xfopen (filename, "rm");
+ TEST_COMPARE (fgetc (fp), 't');
+ TEST_COMPARE (ungetc ('u', fp), 'u');
+ TEST_COMPARE (fseek (fp, 0, SEEK_CUR), 0);
+ xfclose (fp);
+
+ /* Test fseek positioning after fflush (another issue covered by the
+ same fix). */
+ fp = xfopen (filename, "rm");
+ TEST_COMPARE (fgetc (fp), 't');
+ TEST_COMPARE (fflush (fp), 0);
+ TEST_COMPARE (ftell (fp), 1);
+ TEST_COMPARE (fseek (fp, 0, SEEK_CUR), 0);
+ TEST_COMPARE (ftell (fp), 1);
+ TEST_COMPARE (fgetc (fp), 'e');
+ xfclose (fp);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.43.0
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/6] Fix fflush handling for mmap files after ungetc (bug 32535)
2025-01-14 2:00 [PATCH 0/6] Fix issues around flushing and file offsets for input files Joseph Myers
` (3 preceding siblings ...)
2025-01-14 2:03 ` [PATCH 4/6] Fix fseek handling for mmap files after ungetc or fflush (bug 32529) Joseph Myers
@ 2025-01-14 2:04 ` Joseph Myers
2025-01-15 7:17 ` DJ Delorie
2025-01-14 2:04 ` [PATCH 6/6] Add test of input file flushing / offset issues Joseph Myers
5 siblings, 1 reply; 26+ messages in thread
From: Joseph Myers @ 2025-01-14 2:04 UTC (permalink / raw)
To: libc-alpha
As discussed in bug 32535, fflush fails on files opened for reading
using mmap after ungetc. Fix the logic to handle this case and still
compute the file offset correctly.
Tested for x86_64.
---
libio/fileops.c | 12 ++++++---
stdio-common/Makefile | 1 +
stdio-common/tst-fflush-mmap.c | 48 ++++++++++++++++++++++++++++++++++
3 files changed, 57 insertions(+), 4 deletions(-)
create mode 100644 stdio-common/tst-fflush-mmap.c
diff --git a/libio/fileops.c b/libio/fileops.c
index 97875d1eaf..7e370d12b2 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -858,17 +858,21 @@ libc_hidden_ver (_IO_new_file_sync, _IO_file_sync)
int
_IO_file_sync_mmap (FILE *fp)
{
+ off64_t o = fp->_offset - (fp->_IO_read_end - fp->_IO_read_ptr);
if (fp->_IO_read_ptr != fp->_IO_read_end)
{
- if (__lseek64 (fp->_fileno, fp->_IO_read_ptr - fp->_IO_buf_base,
- SEEK_SET)
- != fp->_IO_read_ptr - fp->_IO_buf_base)
+ if (_IO_in_backup (fp))
+ {
+ _IO_switch_to_main_get_area (fp);
+ o -= fp->_IO_read_end - fp->_IO_read_base;
+ }
+ if (__lseek64 (fp->_fileno, o, SEEK_SET) != o)
{
fp->_flags |= _IO_ERR_SEEN;
return EOF;
}
}
- fp->_offset = fp->_IO_read_ptr - fp->_IO_buf_base;
+ fp->_offset = o;
fp->_IO_read_end = fp->_IO_read_ptr = fp->_IO_read_base;
return 0;
}
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 06c9eaf426..703e8af7e8 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -239,6 +239,7 @@ tests := \
tst-fdopen2 \
tst-ferror \
tst-fflush-all-input \
+ tst-fflush-mmap \
tst-fgets \
tst-fgets2 \
tst-fileno \
diff --git a/stdio-common/tst-fflush-mmap.c b/stdio-common/tst-fflush-mmap.c
new file mode 100644
index 0000000000..fd3671f198
--- /dev/null
+++ b/stdio-common/tst-fflush-mmap.c
@@ -0,0 +1,48 @@
+/* Test fflush after ungetc on files using mmap (bug 32535).
+ Copyright (C) 2025 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 <stdio.h>
+
+#include <support/check.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+
+int
+do_test (void)
+{
+ char *filename = NULL;
+ int fd = create_temp_file ("tst-fflush-mmap", &filename);
+ TEST_VERIFY_EXIT (fd != -1);
+ xclose (fd);
+
+ /* Test fflush after ungetc (bug 32535). */
+ FILE *fp = xfopen (filename, "w");
+ TEST_VERIFY (0 <= fputs ("test", fp));
+ xfclose (fp);
+
+ fp = xfopen (filename, "rm");
+ TEST_COMPARE (fgetc (fp), 't');
+ TEST_COMPARE (ungetc ('u', fp), 'u');
+ TEST_COMPARE (fflush (fp), 0);
+ xfclose (fp);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.43.0
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 6/6] Add test of input file flushing / offset issues
2025-01-14 2:00 [PATCH 0/6] Fix issues around flushing and file offsets for input files Joseph Myers
` (4 preceding siblings ...)
2025-01-14 2:04 ` [PATCH 5/6] Fix fflush handling for mmap files after ungetc (bug 32535) Joseph Myers
@ 2025-01-14 2:04 ` Joseph Myers
2025-01-15 21:39 ` DJ Delorie
5 siblings, 1 reply; 26+ messages in thread
From: Joseph Myers @ 2025-01-14 2:04 UTC (permalink / raw)
To: libc-alpha
Having fixed several bugs relating to flushing of FILE* streams (with
fflush and other operations) and their offsets (both the file position
indicator in the FILE*, and the offset in the underlying open file
description), especially after ungetc but not limited to that case,
add a test that more systematically covers different combinations of
cases for such issues, with 25664 separate scenarios tested (which
include examples of all the five separate fixed bugs), all of which
pass given the five previous bug fixes.
Tested for x86_64.
---
stdio-common/Makefile | 1 +
stdio-common/tst-read-offset.c | 544 +++++++++++++++++++++++++++++++++
2 files changed, 545 insertions(+)
create mode 100644 stdio-common/tst-read-offset.c
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 703e8af7e8..c14cf32776 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -285,6 +285,7 @@ tests := \
tst-printf-round \
tst-printfsz \
tst-put-error \
+ tst-read-offset \
tst-renameat2 \
tst-rndseek \
tst-scanf-binary-c11 \
diff --git a/stdio-common/tst-read-offset.c b/stdio-common/tst-read-offset.c
new file mode 100644
index 0000000000..20ffbf55a4
--- /dev/null
+++ b/stdio-common/tst-read-offset.c
@@ -0,0 +1,544 @@
+/* Test offsets in files being read, in particular with ungetc.
+ Copyright (C) 2025 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 <dlfcn.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <support/check.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+
+static volatile bool fail = false;
+
+/* Induce a malloc failure whenever FAIL is set. */
+void *
+malloc (size_t sz)
+{
+ if (fail)
+ return NULL;
+
+ static void *(*real_malloc) (size_t);
+
+ if (real_malloc == NULL)
+ real_malloc = dlsym (RTLD_NEXT, "malloc");
+
+ return real_malloc (sz);
+}
+
+/* The name of the temporary file used by all the tests. */
+static char *filename;
+
+/* st_blksize value for that file, or BUFSIZ if out of range. */
+static int blksize = BUFSIZ;
+
+/* Test data, both written to that file and used as an in-memory
+ stream. */
+char test_data[2 * BUFSIZ];
+
+/* Ways to open a test stream for reading (that may use different code
+ paths in libio). */
+enum test_open_case
+ {
+ test_open_fopen,
+ test_open_fopen_m,
+ test_open_fopen64,
+ test_open_fopen64_m,
+ test_open_fmemopen,
+ test_open_max
+ };
+
+static const char *const test_open_case_name[test_open_max] =
+ {
+ "fopen", "fopen(mmap)", "fopen64", "fopen64(mmap)", "fmemopen"
+ };
+
+static FILE *
+open_test_stream (enum test_open_case c)
+{
+ FILE *fp;
+ switch (c)
+ {
+ case test_open_fopen:
+ fp = fopen (filename, "r");
+ break;
+
+ case test_open_fopen_m:
+ fp = fopen (filename, "rm");
+ break;
+
+ case test_open_fopen64:
+ fp = fopen64 (filename, "r");
+ break;
+
+ case test_open_fopen64_m:
+ fp = fopen64 (filename, "rm");
+ break;
+
+ case test_open_fmemopen:
+ fp = fmemopen (test_data, 2 * BUFSIZ, "r");
+ break;
+
+ default:
+ abort ();
+ }
+ TEST_VERIFY_EXIT (fp != NULL);
+ return fp;
+}
+
+/* Base locations at which the main test (ungetc calls then doing
+ something that clears ungetc characters, then checking offset)
+ starts. */
+enum test_base_loc
+ {
+ base_loc_start,
+ base_loc_blksize,
+ base_loc_bufsiz,
+ base_loc_max
+ };
+
+static int
+base_loc_to_bytes (enum test_base_loc loc, int offset)
+{
+ switch (loc)
+ {
+ case base_loc_start:
+ return offset;
+
+ case base_loc_blksize:
+ return blksize + offset;
+
+ case base_loc_bufsiz:
+ return BUFSIZ + offset;
+
+ default:
+ abort ();
+ }
+}
+
+/* Ways to clear data from ungetc. */
+enum clear_ungetc_case
+ {
+ clear_fseek,
+ clear_fseekm1,
+ clear_fseekp1,
+ clear_fseeko,
+ clear_fseekom1,
+ clear_fseekop1,
+ clear_fseeko64,
+ clear_fseeko64m1,
+ clear_fseeko64p1,
+ clear_fsetpos,
+ clear_fsetposu,
+ clear_fsetpos64,
+ clear_fsetpos64u,
+ clear_fflush,
+ clear_fflush_null,
+ clear_fclose,
+ clear_max
+ };
+
+static const char *const clear_ungetc_case_name[clear_max] =
+ {
+ "fseek", "fseek(-1)", "fseek(1)", "fseeko", "fseeko(-1)", "fseeko(1)",
+ "fseeko64", "fseeko64(-1)", "fseeko64(1)", "fsetpos", "fsetpos(before)",
+ "fsetpos64", "fsetpos64(before)", "fflush", "fflush(NULL)", "fclose"
+ };
+
+static int
+clear_offset (enum clear_ungetc_case c, int num_ungetc)
+{
+ switch (c)
+ {
+ case clear_fseekm1:
+ case clear_fseekom1:
+ case clear_fseeko64m1:
+ return -1;
+
+ case clear_fseekp1:
+ case clear_fseekop1:
+ case clear_fseeko64p1:
+ return 1;
+
+ case clear_fsetposu:
+ case clear_fsetpos64u:
+ return num_ungetc;
+
+ default:
+ return 0;
+ }
+}
+
+/* The offsets used with fsetpos / fsetpos64. */
+static fpos_t pos;
+static fpos64_t pos64;
+
+static int
+do_clear_ungetc (FILE *fp, enum clear_ungetc_case c, int num_ungetc)
+{
+ int ret;
+ int offset = clear_offset (c, num_ungetc);
+ switch (c)
+ {
+ case clear_fseek:
+ case clear_fseekm1:
+ case clear_fseekp1:
+ ret = fseek (fp, offset, SEEK_CUR);
+ break;
+
+ case clear_fseeko:
+ case clear_fseekom1:
+ case clear_fseekop1:
+ ret = fseeko (fp, offset, SEEK_CUR);
+ break;
+
+ case clear_fseeko64:
+ case clear_fseeko64m1:
+ case clear_fseeko64p1:
+ ret = fseeko64 (fp, offset, SEEK_CUR);
+ break;
+
+ case clear_fsetpos:
+ case clear_fsetposu:
+ ret = fsetpos (fp, &pos);
+ break;
+
+ case clear_fsetpos64:
+ case clear_fsetpos64u:
+ ret = fsetpos64 (fp, &pos64);
+ break;
+
+ case clear_fflush:
+ ret = fflush (fp);
+ break;
+
+ case clear_fflush_null:
+ ret = fflush (NULL);
+ break;
+
+ case clear_fclose:
+ ret = fclose (fp);
+ break;
+
+ default:
+ abort();
+ }
+ TEST_COMPARE (ret, 0);
+ return offset;
+}
+
+static bool
+clear_valid (enum test_open_case c, enum clear_ungetc_case cl)
+{
+ switch (c)
+ {
+ case test_open_fmemopen:
+ /* fflush is not valid for input memory streams, and fclose is
+ useless for this test for such streams because there is no
+ underlying open file description for which an offset could be
+ checked after fclose. */
+ switch (cl)
+ {
+ case clear_fflush:
+ case clear_fflush_null:
+ case clear_fclose:
+ return false;
+
+ default:
+ return true;
+ }
+
+ default:
+ /* All ways of clearing ungetc state are valid for streams with
+ an underlying file. */
+ return true;
+ }
+}
+
+static bool
+clear_closes_file (enum clear_ungetc_case cl)
+{
+ switch (cl)
+ {
+ case clear_fclose:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+static void
+clear_getpos_before (FILE *fp, enum clear_ungetc_case c)
+{
+ switch (c)
+ {
+ case clear_fsetposu:
+ TEST_COMPARE (fgetpos (fp, &pos), 0);
+ break;
+
+ case clear_fsetpos64u:
+ TEST_COMPARE (fgetpos64 (fp, &pos64), 0);
+ break;
+
+ default:
+ break;
+ }
+}
+
+static void
+clear_getpos_after (FILE *fp, enum clear_ungetc_case c)
+{
+ switch (c)
+ {
+ case clear_fsetpos:
+ TEST_COMPARE (fgetpos (fp, &pos), 0);
+ break;
+
+ case clear_fsetpos64:
+ TEST_COMPARE (fgetpos64 (fp, &pos64), 0);
+ break;
+
+ default:
+ break;
+ }
+}
+
+/* Ways to verify results of clearing ungetc data. */
+enum verify_case
+ {
+ verify_read,
+ verify_ftell,
+ verify_ftello,
+ verify_ftello64,
+ verify_fd,
+ verify_max
+ };
+
+static const char *const verify_case_name[verify_max] =
+ {
+ "read", "ftell", "ftello", "ftello64", "fd"
+ };
+
+static bool
+valid_fd_offset (enum test_open_case c, enum clear_ungetc_case cl)
+{
+ switch (c)
+ {
+ case test_open_fmemopen:
+ /* No open file description. */
+ return false;
+
+ default:
+ /* fseek does not necessarily set the offset for the underlying
+ open file description ("If the most recent operation, other
+ than ftell(), on a given stream is fflush(), the file offset
+ in the underlying open file description shall be adjusted to
+ reflect the location specified by fseek()." in POSIX does not
+ include the case here where getc was the last operation).
+ Similarly, fsetpos does not necessarily set that offset
+ either. */
+ switch (cl)
+ {
+ case clear_fflush:
+ case clear_fflush_null:
+ case clear_fclose:
+ return true;
+
+ default:
+ return false;
+ }
+ }
+}
+
+static bool
+verify_valid (enum test_open_case c, enum clear_ungetc_case cl,
+ enum verify_case v)
+{
+ switch (v)
+ {
+ case verify_fd:
+ return valid_fd_offset (c, cl);
+
+ default:
+ switch (cl)
+ {
+ case clear_fclose:
+ return false;
+
+ default:
+ return true;
+ }
+ }
+}
+
+static bool
+verify_uses_fd (enum verify_case v)
+{
+ switch (v)
+ {
+ case verify_fd:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+static int
+read_to_test_loc (FILE *fp, enum test_base_loc loc, int offset)
+{
+ int to_read = base_loc_to_bytes (loc, offset);
+ for (int i = 0; i < to_read; i++)
+ TEST_COMPARE (getc (fp), (unsigned char) i);
+ return to_read;
+}
+
+static void
+setup (void)
+{
+ int fd = create_temp_file ("tst-read-offset", &filename);
+ TEST_VERIFY_EXIT (fd != -1);
+ struct stat64 st;
+ xfstat64 (fd, &st);
+ if (st.st_blksize > 0 && st.st_blksize < BUFSIZ)
+ blksize = st.st_blksize;
+ printf ("BUFSIZ = %d, blksize = %d\n", BUFSIZ, blksize);
+ xclose (fd);
+ FILE *fp = xfopen (filename, "w");
+ for (size_t i = 0; i < 2 * BUFSIZ; i++)
+ {
+ unsigned char c = i;
+ TEST_VERIFY_EXIT (fputc (c, fp) == c);
+ test_data[i] = c;
+ }
+ xfclose (fp);
+}
+
+static void
+test_one_case (enum test_open_case c, enum test_base_loc loc, int offset,
+ int num_ungetc, int num_ungetc_diff, bool ungetc_fallback,
+ enum clear_ungetc_case cl, enum verify_case v)
+{
+ int full_offset = base_loc_to_bytes (loc, offset);
+ printf ("Testing %s offset %d ungetc %d different %d %s%s %s\n",
+ test_open_case_name[c], full_offset, num_ungetc, num_ungetc_diff,
+ ungetc_fallback ? "fallback " : "", clear_ungetc_case_name[cl],
+ verify_case_name[v]);
+ FILE *fp = open_test_stream (c);
+ int cur_offset = read_to_test_loc (fp, loc, offset);
+ clear_getpos_before (fp, cl);
+ for (int i = 0; i < num_ungetc; i++)
+ {
+ unsigned char c = (i >= num_ungetc - num_ungetc_diff
+ ? cur_offset
+ : cur_offset - 1);
+ if (ungetc_fallback)
+ fail = true;
+ TEST_COMPARE (ungetc (c, fp), c);
+ fail = false;
+ cur_offset--;
+ }
+ clear_getpos_after (fp, cl);
+ int fd = -1;
+ bool done_dup = false;
+ if (verify_uses_fd (v))
+ {
+ fd = fileno (fp);
+ TEST_VERIFY (fd != -1);
+ if (clear_closes_file (cl))
+ {
+ fd = xdup (fd);
+ done_dup = true;
+ }
+ }
+ cur_offset += do_clear_ungetc (fp, cl, num_ungetc);
+ switch (v)
+ {
+ case verify_read:
+ for (; cur_offset <= full_offset + 1; cur_offset++)
+ TEST_COMPARE (getc (fp), (unsigned char) cur_offset);
+ break;
+
+ case verify_ftell:
+ TEST_COMPARE (ftell (fp), cur_offset);
+ break;
+
+ case verify_ftello:
+ TEST_COMPARE (ftello (fp), cur_offset);
+ break;
+
+ case verify_ftello64:
+ TEST_COMPARE (ftello64 (fp), cur_offset);
+ break;
+
+ case verify_fd:
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), cur_offset);
+ break;
+
+ default:
+ abort ();
+ }
+ if (! clear_closes_file (cl))
+ {
+ int ret = fclose (fp);
+ TEST_COMPARE (ret, 0);
+ }
+ if (done_dup)
+ xclose (fd);
+}
+
+int
+do_test (void)
+{
+ setup ();
+ for (enum test_open_case c = 0; c < test_open_max; c++)
+ for (enum test_base_loc loc = 0; loc < base_loc_max; loc++)
+ for (int offset = 0; offset <= 3; offset++)
+ for (int num_ungetc = 0;
+ num_ungetc <= 2 && num_ungetc <= base_loc_to_bytes (loc, offset);
+ num_ungetc++)
+ for (int num_ungetc_diff = 0;
+ num_ungetc_diff <= num_ungetc;
+ num_ungetc_diff++)
+ for (int ungetc_fallback = 0;
+ ungetc_fallback <= (num_ungetc == 1 ? 1 : 0);
+ ungetc_fallback++)
+ for (enum clear_ungetc_case cl = 0; cl < clear_max; cl++)
+ {
+ if (!clear_valid (c, cl))
+ continue;
+ if ((base_loc_to_bytes (loc, offset)
+ - num_ungetc
+ + clear_offset (cl, num_ungetc)) < 0)
+ continue;
+ for (enum verify_case v = 0; v < verify_max; v++)
+ {
+ if (!verify_valid (c, cl, v))
+ continue;
+ test_one_case (c, loc, offset, num_ungetc,
+ num_ungetc_diff, ungetc_fallback, cl, v);
+ }
+ }
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.43.0
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/6] Fix fflush after ungetc on input file (bug 5994)
2025-01-14 2:02 ` [PATCH 1/6] Fix fflush after ungetc on input file (bug 5994) Joseph Myers
@ 2025-01-15 4:20 ` DJ Delorie
0 siblings, 0 replies; 26+ messages in thread
From: DJ Delorie @ 2025-01-15 4:20 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>
Joseph Myers <josmyers@redhat.com> writes:
> diff --git a/libio/fileops.c b/libio/fileops.c
> index 775999deb3..27e1977eb6 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -799,6 +799,11 @@ _IO_new_file_sync (FILE *fp)
> if (fp->_IO_write_ptr > fp->_IO_write_base)
> if (_IO_do_flush(fp)) return EOF;
> delta = fp->_IO_read_ptr - fp->_IO_read_end;
> + if (_IO_in_backup (fp))
> + {
> + _IO_switch_to_main_get_area (fp);
> + delta += fp->_IO_read_ptr - fp->_IO_read_end;
> + }
The normal buffer fills on read, and the file pointer reflects the end
of the already-read data. If we've ungetc'd, we've switched to the
backup buffer and *start* at the end of the buffer, which corresponds to
wherever we were when we started ungetc'ing, and "fills" towards the
beginning.
The new code says "if we had been ungetc'ing, we need to seek backwards
by as much as we'd been ungetc'ing, then switch to the main buffer and
seek backwards for that too" unless we hadn't been ungetc'ing, in which
case it just does the main buffer seek.
Ok.
> if (delta != 0)
> {
> off64_t new_pos = _IO_SYSSEEK (fp, delta, 1);
Then we seek by pre-read data + any ungetc'd data.
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index bbdc1ff709..589b786f45 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -302,6 +302,7 @@ tests := \
> tst-swscanf \
> tst-tmpnam \
> tst-ungetc \
> + tst-ungetc-fflush \
> tst-ungetc-leak \
> tst-ungetc-nomem \
> tst-unlockedio \
Ok.
> diff --git a/stdio-common/tst-ungetc-fflush.c b/stdio-common/tst-ungetc-fflush.c
> +/* Test flushing input file after ungetc (bug 5994).
> + Copyright (C) 2025 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 <stdio.h>
> +
> +#include <support/check.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
Ok.
> +int
> +do_test (void)
> +{
> + char *filename = NULL;
> + int fd = create_temp_file ("tst-ungetc-fflush", &filename);
> + TEST_VERIFY_EXIT (fd != -1);
> + xclose (fd);
New file, ok.
> + /* Test as in bug 5994. */
> + FILE *fp = xfopen (filename, "w");
> + TEST_VERIFY_EXIT (fputs ("#include", fp) >= 0);
> + xfclose (fp);
File has [#include]. ok.
> + fp = xfopen (filename, "r");
> + TEST_COMPARE (fgetc (fp), '#');
> + TEST_COMPARE (fgetc (fp), 'i');
> + TEST_COMPARE (ungetc ('@', fp), '@');
back to pos 1, ok
> + TEST_COMPARE (fflush (fp), 0);
> + TEST_COMPARE (lseek (fileno (fp), 0, SEEK_CUR), 1);
Ok.
> + TEST_COMPARE (fgetc (fp), 'i');
This is the char in the file, not the char we ungetc'd, ok.
> + TEST_COMPARE (fgetc (fp), 'n');
> + xfclose (fp);
Ok.
> + /* Test as in bug 12799 (duplicate of 5994). */
> + fp = xfopen (filename, "w+");
> + TEST_VERIFY_EXIT (fputs ("hello world", fp) >= 0);
> + rewind (fp);
> + TEST_VERIFY (fileno (fp) >= 0);
Ok.
> + char buffer[10];
> + TEST_COMPARE (fread (buffer, 1, 5, fp), 5);
[hello] ok
> + TEST_COMPARE (fgetc (fp), ' ');
[ ] ok.
> + TEST_COMPARE (ungetc ('@', fp), '@');
ok.
> + TEST_COMPARE (fflush (fp), 0);
> + TEST_COMPARE (fgetc (fp), ' ');
Re-reads the ' ' from file, ok.
> + xfclose (fp);
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
Ok.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/6] Make fclose seek input file to right offset (bug 12724)
2025-01-14 2:03 ` [PATCH 2/6] Make fclose seek input file to right offset (bug 12724) Joseph Myers
@ 2025-01-15 5:33 ` DJ Delorie
2025-01-15 19:16 ` Joseph Myers
0 siblings, 1 reply; 26+ messages in thread
From: DJ Delorie @ 2025-01-15 5:33 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
One question below about why a fix is here and not in IO_SEEKOFF. The
fix looks correct but why is it here?
The rest looks OK aside from that one pondery.
Joseph Myers <josmyers@redhat.com> writes:
> As discussed in bug 12724 and required by POSIX, before an input file
> . . .
I.e. if a stream pre-reads a block of data, but doesn't use it all, it
has to put the file back at close so the next user can use it, unless it
shouldn't ;-)
> diff --git a/libio/fileops.c b/libio/fileops.c
> @@ -127,15 +127,48 @@ _IO_new_file_init (struct _IO_FILE_plus *fp)
> int
> _IO_new_file_close_it (FILE *fp)
> {
> - int write_status;
> + int flush_status = 0;
> if (!_IO_file_is_open (fp))
> return EOF;
>
> if ((fp->_flags & _IO_NO_WRITES) == 0
> && (fp->_flags & _IO_CURRENTLY_PUTTING) != 0)
> - write_status = _IO_do_flush (fp);
> - else
> - write_status = 0;
> + flush_status = _IO_do_flush (fp);
Same logic; if we are reading without ungetc'ing, we can do a basic
sync.
> + else if (fp->_fileno >= 0
> + /* If this is the active handle, we must seek the
> + underlying open file description (possibly shared with
> + other file descriptors that remain open) to the correct
> + offset. But if this stream is in a state such that some
> + other handle might have become the active handle, then
> + (a) at the time it entered that state, the underlying
> + open file description had the correct offset, and (b)
> + seeking the underlying open file description, even to
> + its newly determined current offset, is not safe because
> + it can race with operations on a different active
> + handle. So check here for cases where it is necessary
> + to seek, while avoiding seeking in cases where it is
> + unsafe to do so. */
> + && (_IO_in_backup (fp)
if we called ungetc, we need to report that.
> + || (fp->_mode <= 0 && fp->_IO_read_ptr < fp->_IO_read_end)
If we've read-ahead and not used the data...
> + || (_IO_vtable_offset (fp) == 0
> + && fp->_mode > 0 && (fp->_wide_data->_IO_read_ptr
> + < fp->_wide_data->_IO_read_end))))
For new format files, we also check for buffered wide data.
> + {
> + off64_t o = _IO_SEEKOFF (fp, 0, _IO_seek_cur, 0);
> + if (o == EOF)
> + {
> + if (errno != ESPIPE)
> + flush_status = EOF;
> + }
Ok.
> + else
> + {
> + if (_IO_in_backup (fp))
> + o -= fp->_IO_save_end - fp->_IO_save_base;
Must undo any unused read-ahead in the main buffer. IO_SEEKOFF doesn't
check for the dual buffer state so only undoes the ungetc buffer.
Why can't this fix be in the IO_SEEKOFF logic? Doesn't this imply that
IO_SEEKOFF is faulty?
> + flush_status = (_IO_SYSSEEK (fp, o, SEEK_SET) < 0 && errno != ESPIPE
> + ? EOF
> + : 0);
> + }
> + }
Ok.
> @@ -160,7 +193,7 @@ _IO_new_file_close_it (FILE *fp)
> fp->_fileno = -1;
> fp->_offset = _IO_pos_BAD;
>
> - return close_status ? close_status : write_status;
> + return close_status ? close_status : flush_status;
> }
> libc_hidden_ver (_IO_new_file_close_it, _IO_file_close_it)
Ok.
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 589b786f45..b5f78c365b 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -234,6 +234,7 @@ tests := \
> tst-bz11319-fortify2 \
> tst-cookie \
> tst-dprintf-length \
> + tst-fclose-offset \
> tst-fdopen \
> tst-fdopen2 \
> tst-ferror \
Ok.
> diff --git a/stdio-common/tst-fclose-offset.c b/stdio-common/tst-fclose-offset.c
> +/* Test offset of input file descriptor after close (bug 12724).
> + Copyright (C) 2025 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 <errno.h>
> +#include <stdio.h>
> +#include <wchar.h>
> +
> +#include <support/check.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
Ok.
> +int
> +do_test (void)
> +{
> + char *filename = NULL;
> + int fd = create_temp_file ("tst-fclose-offset", &filename);
> + TEST_VERIFY_EXIT (fd != -1);
> +
> + /* Test offset of open file description for output and input streams
> + after fclose, case from bug 12724. */
> +
> + const char buf[] = "hello world";
> + xwrite (fd, buf, sizeof buf);
> + TEST_COMPARE (lseek (fd, 1, SEEK_SET), 1);
fd now at "ello world"
> + int fd2 = xdup (fd);
> + FILE *f = fdopen (fd2, "w");
> + TEST_VERIFY_EXIT (f != NULL);
f is clone of fd
> + TEST_COMPARE (fputc (buf[1], f), buf[1]);
This writes to f / fd2 but not fd
bumps fd2 to pos 2
> + xfclose (f);
This should bump fd2 and thus fd
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
fd2 is closed, ok
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 2);
fd should be at... 2... ok.
> + /* Likewise for an input stream. */
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "r");
> + TEST_VERIFY_EXIT (f != NULL);
> + TEST_COMPARE (fgetc (f), buf[2]);
ok
> + xfclose (f);
fd2 at pos 3
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
Ok.
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 3);
Ok.
> + /* Test offset of open file description for output and input streams
> + after fclose, case from comment on bug 12724 (failed after first
> + attempt at fixing that bug). This verifies that the offset is
> + not reset when there has been no input or output on the FILE* (in
> + that case, the FILE* might not be the active handle). */
> +
> + TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
> + xwrite (fd, buf, sizeof buf);
> + TEST_COMPARE (lseek (fd, 1, SEEK_SET), 1);
reset ok
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "w");
> + TEST_VERIFY_EXIT (f != NULL);
> + TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
Ok.
> + xfclose (f);
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
fd is not changed, ok - no file ops done.
> + /* Likewise for an input stream. */
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "r");
> + TEST_VERIFY_EXIT (f != NULL);
> + TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
> + xfclose (f);
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
Ok.
> + /* Further cases without specific tests in bug 12724, to verify
> + proper operation of the rules about the offset only being set
> + when the stream is the active handle. */
> +
> + /* Test offset set by fclose after fseek and fgetc. */
> + TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "r");
> + TEST_VERIFY_EXIT (f != NULL);
> + TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
> + TEST_COMPARE (fgetc (f), buf[1]);
> + xfclose (f);
fd is at 2
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 2);
Ok.
> + /* Test offset not set by fclose after fseek and fgetc, if that
> + fgetc is at EOF (in which case the active handle might have
> + changed). */
> + TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "r");
> + TEST_VERIFY_EXIT (f != NULL);
> + TEST_COMPARE (fseek (f, sizeof buf, SEEK_SET), 0);
> + TEST_COMPARE (fgetc (f), EOF);
fd2 at EOF.
> + TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
> + xfclose (f);
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
no seek on fd. Ok.
> + /* Test offset not set by fclose after fseek and fgetc and fflush
> + (active handle might have changed after fflush). */
> + TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "r");
> + TEST_VERIFY_EXIT (f != NULL);
> + TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
> + TEST_COMPARE (fgetc (f), buf[1]);
> + TEST_COMPARE (fflush (f), 0);
> + TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
> + xfclose (f);
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
Ok.
> + /* Test offset not set by fclose after fseek and fgetc, if the
> + stream is unbuffered (active handle might change at any
> + time). */
> + TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "r");
> + TEST_VERIFY_EXIT (f != NULL);
> + setbuf (f, NULL);
> + TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
> + TEST_COMPARE (fgetc (f), buf[1]);
> + TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
> + xfclose (f);
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
Ok.
> + /* Also test such cases with the stream in wide mode. */
> +
> + /* Test offset set by fclose after fseek and fgetwc. */
> + TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "r");
> + TEST_VERIFY_EXIT (f != NULL);
> + TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
> + TEST_COMPARE (fgetwc (f), (wint_t) buf[1]);
> + xfclose (f);
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 2);
Ok.
> + /* Test offset not set by fclose after fseek and fgetwc, if that
> + fgetwc is at EOF (in which case the active handle might have
> + changed). */
> + TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "r");
> + TEST_VERIFY_EXIT (f != NULL);
> + TEST_COMPARE (fseek (f, sizeof buf, SEEK_SET), 0);
> + TEST_COMPARE (fgetwc (f), WEOF);
> + TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
> + xfclose (f);
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
Ok.
> + /* Test offset not set by fclose after fseek and fgetwc and fflush
> + (active handle might have changed after fflush). */
> + TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "r");
> + TEST_VERIFY_EXIT (f != NULL);
> + TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
> + TEST_COMPARE (fgetwc (f), (wint_t) buf[1]);
> + TEST_COMPARE (fflush (f), 0);
> + TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
> + xfclose (f);
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
Ok.
> + /* Test offset not set by fclose after fseek and fgetwc, if the
> + stream is unbuffered (active handle might change at any
> + time). */
> + TEST_COMPARE (lseek (fd, 0, SEEK_SET), 0);
> + fd2 = xdup (fd);
> + f = fdopen (fd2, "r");
> + TEST_VERIFY_EXIT (f != NULL);
> + setbuf (f, NULL);
> + TEST_COMPARE (fseek (f, 1, SEEK_SET), 0);
> + TEST_COMPARE (fgetwc (f), (wint_t) buf[1]);
> + TEST_COMPARE (lseek (fd, 4, SEEK_SET), 4);
> + xfclose (f);
> + errno = 0;
> + TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), -1);
> + TEST_COMPARE (errno, EBADF);
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), 4);
Ok.
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
Ok.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369)
2025-01-14 2:03 ` [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369) Joseph Myers
@ 2025-01-15 6:01 ` DJ Delorie
2025-01-15 19:24 ` Joseph Myers
2025-01-15 17:27 ` Florian Weimer
1 sibling, 1 reply; 26+ messages in thread
From: DJ Delorie @ 2025-01-15 6:01 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
I was surprised at the way fwprintf worked, perhaps a comment explaining
that we expect the wide result to be the same as the narrow result
because we expect the wide chars to be converted to multibyte chars?
I'm used to seeing 16-bit unicode in files, and expected to see that
here too...
We don't have a separate test case for when ungetc() triggers this logic
but I don't think that's needed.
LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>
Joseph Myers <josmyers@redhat.com> writes:
> diff --git a/libio/genops.c b/libio/genops.c
> @@ -730,6 +730,13 @@ _IO_flush_all (void)
> )
> && _IO_OVERFLOW (fp, EOF) == EOF)
> result = EOF;
> + if (_IO_fileno (fp) >= 0
> + && ((fp->_mode <= 0 && fp->_IO_read_ptr < fp->_IO_read_end)
> + || (_IO_vtable_offset (fp) == 0
> + && fp->_mode > 0 && (fp->_wide_data->_IO_read_ptr
> + < fp->_wide_data->_IO_read_end)))
> + && _IO_SYNC (fp) != 0)
> + result = EOF;
This checks for read or wide read buffers having data; if we ungetc'd
we'd still evaluate true here. Ok.
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index b5f78c365b..48fbf05a85 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -238,6 +238,7 @@ tests := \
> tst-fdopen \
> tst-fdopen2 \
> tst-ferror \
> + tst-fflush-all-input \
> tst-fgets \
> tst-fgets2 \
> tst-fileno \
Ok.
> diff --git a/stdio-common/tst-fflush-all-input.c b/stdio-common/tst-fflush-all-input.c
> new file mode 100644
> index 0000000000..e9df3a0c08
> --- /dev/null
> +++ b/stdio-common/tst-fflush-all-input.c
> @@ -0,0 +1,53 @@
> +/* Test fflush (NULL) flushes input files (bug 32369).
> + Copyright (C) 2025 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 <stdio.h>
> +#include <unistd.h>
> +#include <wchar.h>
> +
> +#include <support/check.h>
> +#include <support/xstdio.h>
Ok.
> +int
> +do_test (void)
> +{
> + FILE *temp = tmpfile ();
> + TEST_VERIFY_EXIT (temp != NULL);
> + fprintf (temp, "abc");
> + TEST_COMPARE (fflush (temp), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
> + TEST_COMPARE (fgetc (temp), 'a');
> + TEST_COMPARE (fflush (NULL), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
> + xfclose (temp);
Ok.
> + /* Likewise, but in wide mode. */
> + temp = tmpfile ();
> + TEST_VERIFY_EXIT (temp != NULL);
> + fwprintf (temp, L"abc");
> + TEST_COMPARE (fflush (temp), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
> + TEST_COMPARE (fgetwc (temp), L'a');
> + TEST_COMPARE (fflush (NULL), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
Shouldn't this be sizeof(wchar_t), not 1 ? /me tests... no,
fwprintf(L"abc") writes three narrow chars to the file! Needs comment?
Needs specific locale? Are there any locales where we write this out as
a wide char and not a multi-byte char?
> + xfclose (temp);
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
Ok.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/6] Fix fseek handling for mmap files after ungetc or fflush (bug 32529)
2025-01-14 2:03 ` [PATCH 4/6] Fix fseek handling for mmap files after ungetc or fflush (bug 32529) Joseph Myers
@ 2025-01-15 6:35 ` DJ Delorie
0 siblings, 0 replies; 26+ messages in thread
From: DJ Delorie @ 2025-01-15 6:35 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>
Joseph Myers <josmyers@redhat.com> writes:
> diff --git a/libio/fileops.c b/libio/fileops.c
> @@ -1106,11 +1106,18 @@ _IO_file_seekoff_mmap (FILE *fp, off64_t offset, int dir, int mode)
> if (mode == 0)
> return fp->_offset - (fp->_IO_read_end - fp->_IO_read_ptr);
>
> + if (_IO_in_backup (fp))
> + {
> + if (dir == _IO_seek_cur)
> + offset += fp->_IO_read_ptr - fp->_IO_read_end;
> + _IO_switch_to_main_get_area (fp);
> + }
> +
Ok.
> switch (dir)
> {
> case _IO_seek_cur:
> /* Adjust for read-ahead (bytes is buffer). */
> - offset += fp->_IO_read_ptr - fp->_IO_read_base;
> + offset += fp->_offset - (fp->_IO_read_end - fp->_IO_read_ptr);
> break;
Ok.
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 48fbf05a85..06c9eaf426 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -261,6 +261,7 @@ tests := \
> tst-freopen64-7 \
> tst-freopen7 \
> tst-fseek \
> + tst-fseek-mmap \
> tst-fwrite \
> tst-fwrite-memstrm \
> tst-fwrite-overflow \
Ok.
> diff --git a/stdio-common/tst-fseek-mmap.c b/stdio-common/tst-fseek-mmap.c
> +/* Test fseek on files using mmap (bug 32529).
> + Copyright (C) 2025 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 <stdio.h>
> +
> +#include <support/check.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
Ok.
> +int
> +do_test (void)
> +{
> + char *filename = NULL;
> + int fd = create_temp_file ("tst-fseek-mmap", &filename);
> + TEST_VERIFY_EXIT (fd != -1);
> + xclose (fd);
Ok.
> + /* Test fseek after ungetc (bug 32529). */
> + FILE *fp = xfopen (filename, "w");
> + TEST_VERIFY (0 <= fputs ("test", fp));
> + xfclose (fp);
Ok.
> + fp = xfopen (filename, "rm");
> + TEST_COMPARE (fgetc (fp), 't');
> + TEST_COMPARE (ungetc ('u', fp), 'u');
> + TEST_COMPARE (fseek (fp, 0, SEEK_CUR), 0);
> + xfclose (fp);
Ok.
> + /* Test fseek positioning after fflush (another issue covered by the
> + same fix). */
> + fp = xfopen (filename, "rm");
> + TEST_COMPARE (fgetc (fp), 't');
> + TEST_COMPARE (fflush (fp), 0);
at 1
> + TEST_COMPARE (ftell (fp), 1);
Ok.
> + TEST_COMPARE (fseek (fp, 0, SEEK_CUR), 0);
> + TEST_COMPARE (ftell (fp), 1);
Ok.
> + TEST_COMPARE (fgetc (fp), 'e');
> + xfclose (fp);
Ok.
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
Ok.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] Fix fflush handling for mmap files after ungetc (bug 32535)
2025-01-14 2:04 ` [PATCH 5/6] Fix fflush handling for mmap files after ungetc (bug 32535) Joseph Myers
@ 2025-01-15 7:17 ` DJ Delorie
2025-01-20 22:56 ` Joseph Myers
0 siblings, 1 reply; 26+ messages in thread
From: DJ Delorie @ 2025-01-15 7:17 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
Some of the logic seems a little off to me, but it might be my brand new
understanding of how _IO_* works ;-)
Also, the test seems weak somehow.
Joseph Myers <josmyers@redhat.com> writes:
> diff --git a/libio/fileops.c b/libio/fileops.c
> int
> _IO_file_sync_mmap (FILE *fp)
> {
> + off64_t o = fp->_offset - (fp->_IO_read_end - fp->_IO_read_ptr);
To be consistent with logic elsewhere, this could have been:
> off64_t o = fp->_offset + (fp->_IO_read_ptr - fp->_IO_read_end);
But this logic doesn't reflect the true meaning of the underlying
pointers when _IO_in_backup() is true. It ends up calculating the same
value, but it's applying fp->_offset to the wrong buffer in that case.
The effective offset of the backup buffer's end is the main buffer's
current read pointer.
This is how it's done elsewhere, though, so ok.
> if (fp->_IO_read_ptr != fp->_IO_read_end)
> {
> - if (__lseek64 (fp->_fileno, fp->_IO_read_ptr - fp->_IO_buf_base,
> - SEEK_SET)
> - != fp->_IO_read_ptr - fp->_IO_buf_base)
> + if (_IO_in_backup (fp))
> + {
> + _IO_switch_to_main_get_area (fp);
> + o -= fp->_IO_read_end - fp->_IO_read_base;
Shouldn't one of these be _IO_read_ptr ?
> + }
> + if (__lseek64 (fp->_fileno, o, SEEK_SET) != o)
Ok.
> {
> fp->_flags |= _IO_ERR_SEEN;
> return EOF;
> }
> }
> - fp->_offset = fp->_IO_read_ptr - fp->_IO_buf_base;
> + fp->_offset = o;
> fp->_IO_read_end = fp->_IO_read_ptr = fp->_IO_read_base;
> return 0;
> }
Ok.
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 06c9eaf426..703e8af7e8 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -239,6 +239,7 @@ tests := \
> tst-fdopen2 \
> tst-ferror \
> tst-fflush-all-input \
> + tst-fflush-mmap \
> tst-fgets \
> tst-fgets2 \
> tst-fileno \
Ok.
> diff --git a/stdio-common/tst-fflush-mmap.c b/stdio-common/tst-fflush-mmap.c
> +/* Test fflush after ungetc on files using mmap (bug 32535).
> + Copyright (C) 2025 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 <stdio.h>
> +
> +#include <support/check.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
Ok.
> +int
> +do_test (void)
> +{
> + char *filename = NULL;
> + int fd = create_temp_file ("tst-fflush-mmap", &filename);
> + TEST_VERIFY_EXIT (fd != -1);
> + xclose (fd);
Ok.
> + /* Test fflush after ungetc (bug 32535). */
> + FILE *fp = xfopen (filename, "w");
> + TEST_VERIFY (0 <= fputs ("test", fp));
> + xfclose (fp);
Ok.
> + fp = xfopen (filename, "rm");
> + TEST_COMPARE (fgetc (fp), 't');
> + TEST_COMPARE (ungetc ('u', fp), 'u');
> + TEST_COMPARE (fflush (fp), 0);
Should we test that fflush() did the right thing, somehow?
Can we, for mmap'd files?
> + xfclose (fp);
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
Ok.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369)
2025-01-14 2:03 ` [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369) Joseph Myers
2025-01-15 6:01 ` DJ Delorie
@ 2025-01-15 17:27 ` Florian Weimer
2025-01-20 22:51 ` Joseph Myers
1 sibling, 1 reply; 26+ messages in thread
From: Florian Weimer @ 2025-01-15 17:27 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
* Joseph Myers:
> diff --git a/libio/genops.c b/libio/genops.c
> index 2197bfe7a1..e4378ca48f 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -730,6 +730,13 @@ _IO_flush_all (void)
> )
> && _IO_OVERFLOW (fp, EOF) == EOF)
> result = EOF;
> + if (_IO_fileno (fp) >= 0
> + && ((fp->_mode <= 0 && fp->_IO_read_ptr < fp->_IO_read_end)
> + || (_IO_vtable_offset (fp) == 0
> + && fp->_mode > 0 && (fp->_wide_data->_IO_read_ptr
> + < fp->_wide_data->_IO_read_end)))
> + && _IO_SYNC (fp) != 0)
> + result = EOF;
>
> _IO_funlockfile (fp);
> run_fp = NULL;
I was confused for a bit why exit would block indefinitely with a locked
input stream prior this change, but then I realized that we
unconditionally lock the stream before this check.
> + FILE *temp = tmpfile ();
> + TEST_VERIFY_EXIT (temp != NULL);
> + fprintf (temp, "abc");
> + TEST_COMPARE (fflush (temp), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
> + TEST_COMPARE (fgetc (temp), 'a');
> + TEST_COMPARE (fflush (NULL), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
> + xfclose (temp);
> +
> + /* Likewise, but in wide mode. */
> + temp = tmpfile ();
> + TEST_VERIFY_EXIT (temp != NULL);
> + fwprintf (temp, L"abc");
> + TEST_COMPARE (fflush (temp), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
> + TEST_COMPARE (fgetwc (temp), L'a');
> + TEST_COMPARE (fflush (NULL), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
> + xfclose (temp);
> +
> + return 0;
> +}
Could you add tests that do the flush via fork and exit?
Thanks,
Florian
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/6] Make fclose seek input file to right offset (bug 12724)
2025-01-15 5:33 ` DJ Delorie
@ 2025-01-15 19:16 ` Joseph Myers
2025-01-15 20:01 ` DJ Delorie
0 siblings, 1 reply; 26+ messages in thread
From: Joseph Myers @ 2025-01-15 19:16 UTC (permalink / raw)
To: DJ Delorie; +Cc: libc-alpha
On Wed, 15 Jan 2025, DJ Delorie wrote:
> Must undo any unused read-ahead in the main buffer. IO_SEEKOFF doesn't
> check for the dual buffer state so only undoes the ungetc buffer.
>
> Why can't this fix be in the IO_SEEKOFF logic? Doesn't this imply that
> IO_SEEKOFF is faulty?
I don't know why the interface is designed the way it is, but all of
ftell, ftello, ftello64, fgetpos and fgetpos64 work with the IO_SEEKOFF
interface as it is (i.e., making this adjustment in the caller).
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369)
2025-01-15 6:01 ` DJ Delorie
@ 2025-01-15 19:24 ` Joseph Myers
2025-01-15 20:06 ` DJ Delorie
2025-01-15 20:07 ` DJ Delorie
0 siblings, 2 replies; 26+ messages in thread
From: Joseph Myers @ 2025-01-15 19:24 UTC (permalink / raw)
To: DJ Delorie; +Cc: libc-alpha
On Wed, 15 Jan 2025, DJ Delorie wrote:
> I was surprised at the way fwprintf worked, perhaps a comment explaining
> that we expect the wide result to be the same as the narrow result
> because we expect the wide chars to be converted to multibyte chars?
That's simply how all the wide I/O functions work.
As remarked for patch 1, the file position indicator is unspecified after
ungetwc (hence no tests using ungetwc - tests in this series only use wide
wide I/O in cases where no pushback is involved - and no special checks
needed in general for the combination of wide I/O and pushback).
> Shouldn't this be sizeof(wchar_t), not 1 ? /me tests... no,
> fwprintf(L"abc") writes three narrow chars to the file! Needs comment?
> Needs specific locale? Are there any locales where we write this out as
> a wide char and not a multi-byte char?
This test is using the C locale; since it's only using ASCII characters,
no specific locale is needed. A null character can't be part of another
multibyte character, which means that UTF-16 and UTF-32 are not possible
locale multibyte encodings to write out to the file.
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/6] Make fclose seek input file to right offset (bug 12724)
2025-01-15 19:16 ` Joseph Myers
@ 2025-01-15 20:01 ` DJ Delorie
0 siblings, 0 replies; 26+ messages in thread
From: DJ Delorie @ 2025-01-15 20:01 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
Joseph Myers <josmyers@redhat.com> writes:
>> Must undo any unused read-ahead in the main buffer. IO_SEEKOFF doesn't
>> check for the dual buffer state so only undoes the ungetc buffer.
>>
>> Why can't this fix be in the IO_SEEKOFF logic? Doesn't this imply that
>> IO_SEEKOFF is faulty?
>
> I don't know why the interface is designed the way it is, but all of
> ftell, ftello, ftello64, fgetpos and fgetpos64 work with the IO_SEEKOFF
> interface as it is (i.e., making this adjustment in the caller).
Odd. Perhaps someone "fixed" it the easy way once, and that propogated
until it became a standard. Sigh.
LGTM then.
Reviewed-by: DJ Delorie <dj@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369)
2025-01-15 19:24 ` Joseph Myers
@ 2025-01-15 20:06 ` DJ Delorie
2025-01-16 0:28 ` Joseph Myers
2025-01-15 20:07 ` DJ Delorie
1 sibling, 1 reply; 26+ messages in thread
From: DJ Delorie @ 2025-01-15 20:06 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
Joseph Myers <josmyers@redhat.com> writes:
> That's simply how all the wide I/O functions work.
I'm not doubting the test's correctness or now the functions run, I'm
just saying I was surprised that it did a wide->multi conversion. And
IMHO when someone reading code is surprised, that's a good spot for a
comment ;-)
> This test is using the C locale; since it's only using ASCII characters,
> no specific locale is needed. A null character can't be part of another
> multibyte character, which means that UTF-16 and UTF-32 are not possible
> locale multibyte encodings to write out to the file.
Do we have an API that can write UTF-16 or UTF-32 encodings to files?
Or do they devolve to using the regular fwrite et al?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369)
2025-01-15 19:24 ` Joseph Myers
2025-01-15 20:06 ` DJ Delorie
@ 2025-01-15 20:07 ` DJ Delorie
1 sibling, 0 replies; 26+ messages in thread
From: DJ Delorie @ 2025-01-15 20:07 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
Also, I read the POSIX spec for those functions and it wasn't obvious
from those that they did a wide->multi conversion either! I mean, you
can figure it out eventually if you know they're doing it, but a new
reader would miss it.
Sigh.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] Add test of input file flushing / offset issues
2025-01-14 2:04 ` [PATCH 6/6] Add test of input file flushing / offset issues Joseph Myers
@ 2025-01-15 21:39 ` DJ Delorie
2025-01-20 23:07 ` Joseph Myers
0 siblings, 1 reply; 26+ messages in thread
From: DJ Delorie @ 2025-01-15 21:39 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
LGTM although there's a few places where the test could be improved if
you want. Mostly I wonder if there are more "edge" cases wrt block
boundaries than we actually test here; we mostly test after each
boundary, less so before each boundary. The only time I think this is
needed, though, is testing near EOF.
Reviewed-by: DJ Delorie <dj@redhat.com>
Joseph Myers <josmyers@redhat.com> writes:
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> tst-printf-round \
> tst-printfsz \
> tst-put-error \
> + tst-read-offset \
> tst-renameat2 \
> tst-rndseek \
> tst-scanf-binary-c11 \
Ok.
> diff --git a/stdio-common/tst-read-offset.c b/stdio-common/tst-read-offset.c
> +/* Test offsets in files being read, in particular with ungetc.
> + Copyright (C) 2025 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 <dlfcn.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +#include <support/check.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
Ok.
> +static volatile bool fail = false;
> +
> +/* Induce a malloc failure whenever FAIL is set. */
> +void *
> +malloc (size_t sz)
> +{
> + if (fail)
> + return NULL;
> +
> + static void *(*real_malloc) (size_t);
> +
> + if (real_malloc == NULL)
> + real_malloc = dlsym (RTLD_NEXT, "malloc");
> +
> + return real_malloc (sz);
> +}
Ok, but checking for real_malloc == NULL after dlsym would be more
paranoid. It shouldn't fail; isn't part of this test, and a core dump
is sufficient notice that something unusual went wrong ;-)
> +/* The name of the temporary file used by all the tests. */
> +static char *filename;
> +
> +/* st_blksize value for that file, or BUFSIZ if out of range. */
> +static int blksize = BUFSIZ;
> +
> +/* Test data, both written to that file and used as an in-memory
> + stream. */
> +char test_data[2 * BUFSIZ];
Ok.
> +/* Ways to open a test stream for reading (that may use different code
> + paths in libio). */
> +enum test_open_case
> + {
> + test_open_fopen,
> + test_open_fopen_m,
> + test_open_fopen64,
> + test_open_fopen64_m,
> + test_open_fmemopen,
> + test_open_max
> + };
> +
> +static const char *const test_open_case_name[test_open_max] =
> + {
> + "fopen", "fopen(mmap)", "fopen64", "fopen64(mmap)", "fmemopen"
> + };
Ok.
> +static FILE *
> +open_test_stream (enum test_open_case c)
> +{
> + FILE *fp;
> + switch (c)
> + {
> + case test_open_fopen:
> + fp = fopen (filename, "r");
> + break;
> +
> + case test_open_fopen_m:
> + fp = fopen (filename, "rm");
> + break;
> +
> + case test_open_fopen64:
> + fp = fopen64 (filename, "r");
> + break;
> +
> + case test_open_fopen64_m:
> + fp = fopen64 (filename, "rm");
> + break;
> +
> + case test_open_fmemopen:
> + fp = fmemopen (test_data, 2 * BUFSIZ, "r");
> + break;
> +
> + default:
> + abort ();
> + }
> + TEST_VERIFY_EXIT (fp != NULL);
> + return fp;
> +}
Ok.
> +/* Base locations at which the main test (ungetc calls then doing
> + something that clears ungetc characters, then checking offset)
> + starts. */
> +enum test_base_loc
> + {
> + base_loc_start,
> + base_loc_blksize,
> + base_loc_bufsiz,
> + base_loc_max
> + };
Should include one pseudo-random location (mid-buffer), and one near the
end (i.e. read to EOF then ungetc).
I see we add an offset of 0..3 in the main loop, but that doesn't insert
tests *before* block breaks, just *after* them.
> +static int
> +base_loc_to_bytes (enum test_base_loc loc, int offset)
> +{
> + switch (loc)
> + {
> + case base_loc_start:
> + return offset;
> +
> + case base_loc_blksize:
> + return blksize + offset;
> +
> + case base_loc_bufsiz:
> + return BUFSIZ + offset;
> +
> + default:
> + abort ();
> + }
> +}
Ok.
> +/* Ways to clear data from ungetc. */
> +enum clear_ungetc_case
> + {
> + clear_fseek,
> + clear_fseekm1,
> + clear_fseekp1,
> + clear_fseeko,
> + clear_fseekom1,
> + clear_fseekop1,
> + clear_fseeko64,
> + clear_fseeko64m1,
> + clear_fseeko64p1,
> + clear_fsetpos,
> + clear_fsetposu,
> + clear_fsetpos64,
> + clear_fsetpos64u,
> + clear_fflush,
> + clear_fflush_null,
> + clear_fclose,
> + clear_max
> + };
Ok.
> +static const char *const clear_ungetc_case_name[clear_max] =
> + {
> + "fseek", "fseek(-1)", "fseek(1)", "fseeko", "fseeko(-1)", "fseeko(1)",
> + "fseeko64", "fseeko64(-1)", "fseeko64(1)", "fsetpos", "fsetpos(before)",
> + "fsetpos64", "fsetpos64(before)", "fflush", "fflush(NULL)", "fclose"
> + };
Ok.
> +static int
> +clear_offset (enum clear_ungetc_case c, int num_ungetc)
> +{
> + switch (c)
> + {
> + case clear_fseekm1:
> + case clear_fseekom1:
> + case clear_fseeko64m1:
> + return -1;
> +
> + case clear_fseekp1:
> + case clear_fseekop1:
> + case clear_fseeko64p1:
> + return 1;
> +
> + case clear_fsetposu:
> + case clear_fsetpos64u:
> + return num_ungetc;
> +
> + default:
> + return 0;
> + }
> +}
Ok. Could have used a comment saying what the return value meant, but I
figured it out ;-)
> +/* The offsets used with fsetpos / fsetpos64. */
> +static fpos_t pos;
> +static fpos64_t pos64;
> +
> +static int
> +do_clear_ungetc (FILE *fp, enum clear_ungetc_case c, int num_ungetc)
> +{
> + int ret;
> + int offset = clear_offset (c, num_ungetc);
> + switch (c)
> + {
> + case clear_fseek:
> + case clear_fseekm1:
> + case clear_fseekp1:
> + ret = fseek (fp, offset, SEEK_CUR);
> + break;
> +
> + case clear_fseeko:
> + case clear_fseekom1:
> + case clear_fseekop1:
> + ret = fseeko (fp, offset, SEEK_CUR);
> + break;
> +
> + case clear_fseeko64:
> + case clear_fseeko64m1:
> + case clear_fseeko64p1:
> + ret = fseeko64 (fp, offset, SEEK_CUR);
> + break;
> +
> + case clear_fsetpos:
> + case clear_fsetposu:
> + ret = fsetpos (fp, &pos);
> + break;
> +
> + case clear_fsetpos64:
> + case clear_fsetpos64u:
> + ret = fsetpos64 (fp, &pos64);
> + break;
> +
> + case clear_fflush:
> + ret = fflush (fp);
> + break;
> +
> + case clear_fflush_null:
> + ret = fflush (NULL);
> + break;
> +
> + case clear_fclose:
> + ret = fclose (fp);
> + break;
> +
> + default:
> + abort();
> + }
> + TEST_COMPARE (ret, 0);
> + return offset;
> +}
Ok.
> +static bool
> +clear_valid (enum test_open_case c, enum clear_ungetc_case cl)
> +{
> + switch (c)
> + {
> + case test_open_fmemopen:
> + /* fflush is not valid for input memory streams, and fclose is
> + useless for this test for such streams because there is no
> + underlying open file description for which an offset could be
> + checked after fclose. */
> + switch (cl)
> + {
> + case clear_fflush:
> + case clear_fflush_null:
> + case clear_fclose:
> + return false;
> +
> + default:
> + return true;
> + }
> +
> + default:
> + /* All ways of clearing ungetc state are valid for streams with
> + an underlying file. */
> + return true;
> + }
> +}
Ok.
> +static bool
> +clear_closes_file (enum clear_ungetc_case cl)
> +{
> + switch (cl)
> + {
> + case clear_fclose:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
Ok.
> +static void
> +clear_getpos_before (FILE *fp, enum clear_ungetc_case c)
> +{
> + switch (c)
> + {
> + case clear_fsetposu:
> + TEST_COMPARE (fgetpos (fp, &pos), 0);
> + break;
> +
> + case clear_fsetpos64u:
> + TEST_COMPARE (fgetpos64 (fp, &pos64), 0);
> + break;
> +
> + default:
> + break;
> + }
> +}
Ok.
> +static void
> +clear_getpos_after (FILE *fp, enum clear_ungetc_case c)
> +{
> + switch (c)
> + {
> + case clear_fsetpos:
> + TEST_COMPARE (fgetpos (fp, &pos), 0);
> + break;
> +
> + case clear_fsetpos64:
> + TEST_COMPARE (fgetpos64 (fp, &pos64), 0);
> + break;
> +
> + default:
> + break;
> + }
> +}
Ok.
> +/* Ways to verify results of clearing ungetc data. */
> +enum verify_case
> + {
> + verify_read,
> + verify_ftell,
> + verify_ftello,
> + verify_ftello64,
> + verify_fd,
> + verify_max
> + };
Ok.
> +static const char *const verify_case_name[verify_max] =
> + {
> + "read", "ftell", "ftello", "ftello64", "fd"
> + };
Ok.
> +static bool
> +valid_fd_offset (enum test_open_case c, enum clear_ungetc_case cl)
> +{
> + switch (c)
> + {
> + case test_open_fmemopen:
> + /* No open file description. */
> + return false;
> +
> + default:
> + /* fseek does not necessarily set the offset for the underlying
> + open file description ("If the most recent operation, other
> + than ftell(), on a given stream is fflush(), the file offset
> + in the underlying open file description shall be adjusted to
> + reflect the location specified by fseek()." in POSIX does not
> + include the case here where getc was the last operation).
> + Similarly, fsetpos does not necessarily set that offset
> + either. */
> + switch (cl)
> + {
> + case clear_fflush:
> + case clear_fflush_null:
> + case clear_fclose:
> + return true;
> +
> + default:
> + return false;
> + }
> + }
> +}
Ok.
> +static bool
> +verify_valid (enum test_open_case c, enum clear_ungetc_case cl,
> + enum verify_case v)
> +{
> + switch (v)
> + {
> + case verify_fd:
> + return valid_fd_offset (c, cl);
> +
> + default:
> + switch (cl)
> + {
> + case clear_fclose:
> + return false;
> +
> + default:
> + return true;
> + }
> + }
> +}
Ok.
> +static bool
> +verify_uses_fd (enum verify_case v)
> +{
> + switch (v)
> + {
> + case verify_fd:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
Ok.
> +static int
> +read_to_test_loc (FILE *fp, enum test_base_loc loc, int offset)
> +{
> + int to_read = base_loc_to_bytes (loc, offset);
> + for (int i = 0; i < to_read; i++)
> + TEST_COMPARE (getc (fp), (unsigned char) i);
> + return to_read;
> +}
Ok.
> +static void
> +setup (void)
> +{
> + int fd = create_temp_file ("tst-read-offset", &filename);
> + TEST_VERIFY_EXIT (fd != -1);
> + struct stat64 st;
> + xfstat64 (fd, &st);
> + if (st.st_blksize > 0 && st.st_blksize < BUFSIZ)
> + blksize = st.st_blksize;
> + printf ("BUFSIZ = %d, blksize = %d\n", BUFSIZ, blksize);
> + xclose (fd);
> + FILE *fp = xfopen (filename, "w");
> + for (size_t i = 0; i < 2 * BUFSIZ; i++)
> + {
> + unsigned char c = i;
> + TEST_VERIFY_EXIT (fputc (c, fp) == c);
> + test_data[i] = c;
> + }
> + xfclose (fp);
> +}
Ok.
> +static void
> +test_one_case (enum test_open_case c, enum test_base_loc loc, int offset,
> + int num_ungetc, int num_ungetc_diff, bool ungetc_fallback,
> + enum clear_ungetc_case cl, enum verify_case v)
> +{
> + int full_offset = base_loc_to_bytes (loc, offset);
> + printf ("Testing %s offset %d ungetc %d different %d %s%s %s\n",
> + test_open_case_name[c], full_offset, num_ungetc, num_ungetc_diff,
> + ungetc_fallback ? "fallback " : "", clear_ungetc_case_name[cl],
> + verify_case_name[v]);
> + FILE *fp = open_test_stream (c);
> + int cur_offset = read_to_test_loc (fp, loc, offset);
> + clear_getpos_before (fp, cl);
> + for (int i = 0; i < num_ungetc; i++)
> + {
> + unsigned char c = (i >= num_ungetc - num_ungetc_diff
> + ? cur_offset
> + : cur_offset - 1);
> + if (ungetc_fallback)
> + fail = true;
> + TEST_COMPARE (ungetc (c, fp), c);
> + fail = false;
> + cur_offset--;
> + }
Ok.
> + clear_getpos_after (fp, cl);
> + int fd = -1;
> + bool done_dup = false;
> + if (verify_uses_fd (v))
> + {
> + fd = fileno (fp);
> + TEST_VERIFY (fd != -1);
> + if (clear_closes_file (cl))
> + {
> + fd = xdup (fd);
> + done_dup = true;
> + }
> + }
Ok.
> + cur_offset += do_clear_ungetc (fp, cl, num_ungetc);
> + switch (v)
> + {
> + case verify_read:
> + for (; cur_offset <= full_offset + 1; cur_offset++)
> + TEST_COMPARE (getc (fp), (unsigned char) cur_offset);
> + break;
> +
> + case verify_ftell:
> + TEST_COMPARE (ftell (fp), cur_offset);
> + break;
> +
> + case verify_ftello:
> + TEST_COMPARE (ftello (fp), cur_offset);
> + break;
> +
> + case verify_ftello64:
> + TEST_COMPARE (ftello64 (fp), cur_offset);
> + break;
> +
> + case verify_fd:
> + TEST_COMPARE (lseek (fd, 0, SEEK_CUR), cur_offset);
> + break;
> +
> + default:
> + abort ();
> + }
Ok.
> + if (! clear_closes_file (cl))
> + {
> + int ret = fclose (fp);
> + TEST_COMPARE (ret, 0);
> + }
> + if (done_dup)
> + xclose (fd);
> +}
Ok.
> +int
> +do_test (void)
> +{
> + setup ();
> + for (enum test_open_case c = 0; c < test_open_max; c++)
> + for (enum test_base_loc loc = 0; loc < base_loc_max; loc++)
> + for (int offset = 0; offset <= 3; offset++)
> + for (int num_ungetc = 0;
> + num_ungetc <= 2 && num_ungetc <= base_loc_to_bytes (loc, offset);
> + num_ungetc++)
> + for (int num_ungetc_diff = 0;
> + num_ungetc_diff <= num_ungetc;
> + num_ungetc_diff++)
> + for (int ungetc_fallback = 0;
> + ungetc_fallback <= (num_ungetc == 1 ? 1 : 0);
> + ungetc_fallback++)
> + for (enum clear_ungetc_case cl = 0; cl < clear_max; cl++)
> + {
> + if (!clear_valid (c, cl))
> + continue;
> + if ((base_loc_to_bytes (loc, offset)
> + - num_ungetc
> + + clear_offset (cl, num_ungetc)) < 0)
> + continue;
> + for (enum verify_case v = 0; v < verify_max; v++)
> + {
> + if (!verify_valid (c, cl, v))
> + continue;
> + test_one_case (c, loc, offset, num_ungetc,
> + num_ungetc_diff, ungetc_fallback, cl, v);
> + }
> + }
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
Ok.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369)
2025-01-15 20:06 ` DJ Delorie
@ 2025-01-16 0:28 ` Joseph Myers
0 siblings, 0 replies; 26+ messages in thread
From: Joseph Myers @ 2025-01-16 0:28 UTC (permalink / raw)
To: DJ Delorie; +Cc: libc-alpha
On Wed, 15 Jan 2025, DJ Delorie wrote:
> Joseph Myers <josmyers@redhat.com> writes:
> > That's simply how all the wide I/O functions work.
>
> I'm not doubting the test's correctness or now the functions run, I'm
> just saying I was surprised that it did a wide->multi conversion. And
> IMHO when someone reading code is surprised, that's a good spot for a
> comment ;-)
I'm thinking of this as something basic from the description of files in
the C standard (C23 7.23.3; two paragraphs "The wide character input
functions read multibyte characters from the stream and convert them to
wide characters as if ..." and "The wide character output functions
convert wide characters to multibyte characters and write them to the
stream as if ..."), so not worthy of remark in the context of an
individual test.
> > This test is using the C locale; since it's only using ASCII characters,
> > no specific locale is needed. A null character can't be part of another
> > multibyte character, which means that UTF-16 and UTF-32 are not possible
> > locale multibyte encodings to write out to the file.
>
> Do we have an API that can write UTF-16 or UTF-32 encodings to files?
> Or do they devolve to using the regular fwrite et al?
You'd need to use fwrite. Text files are made up of multibyte characters.
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369)
2025-01-15 17:27 ` Florian Weimer
@ 2025-01-20 22:51 ` Joseph Myers
2025-01-28 20:23 ` DJ Delorie
0 siblings, 1 reply; 26+ messages in thread
From: Joseph Myers @ 2025-01-20 22:51 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
On Wed, 15 Jan 2025, Florian Weimer wrote:
> > + /* Likewise, but in wide mode. */
> > + temp = tmpfile ();
> > + TEST_VERIFY_EXIT (temp != NULL);
> > + fwprintf (temp, L"abc");
> > + TEST_COMPARE (fflush (temp), 0);
> > + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
> > + TEST_COMPARE (fgetwc (temp), L'a');
> > + TEST_COMPARE (fflush (NULL), 0);
> > + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
> > + xfclose (temp);
> > +
> > + return 0;
> > +}
>
> Could you add tests that do the flush via fork and exit?
I've added those in v2 of the patch.
Make fflush (NULL) flush input files (bug 32369)
As discussed in bug 32369 and required by POSIX, the POSIX feature
fflush (NULL) should flush input files, not just output files. The
POSIX requirement is that "fflush() shall perform this flushing action
on all streams for which the behavior is defined above", and the
definition for input files is for "a stream open for reading with an
underlying file description, if the file is not already at EOF, and
the file is one capable of seeking".
Implement this requirement in glibc. (The underlying flushing
implementation is what deals with avoiding errors for seeking on an
unseekable file.)
Tested for x86_64.
---
Changed in v2: also test the case where flushing of all input files is
achieved by a call to exit after forking rather than an explicit call
to fflush (NULL).
---
libio/genops.c | 7 +++
stdio-common/Makefile | 1 +
stdio-common/tst-fflush-all-input.c | 94 +++++++++++++++++++++++++++++
3 files changed, 102 insertions(+)
create mode 100644 stdio-common/tst-fflush-all-input.c
diff --git a/libio/genops.c b/libio/genops.c
index 2197bfe7a1..e4378ca48f 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -730,6 +730,13 @@ _IO_flush_all (void)
)
&& _IO_OVERFLOW (fp, EOF) == EOF)
result = EOF;
+ if (_IO_fileno (fp) >= 0
+ && ((fp->_mode <= 0 && fp->_IO_read_ptr < fp->_IO_read_end)
+ || (_IO_vtable_offset (fp) == 0
+ && fp->_mode > 0 && (fp->_wide_data->_IO_read_ptr
+ < fp->_wide_data->_IO_read_end)))
+ && _IO_SYNC (fp) != 0)
+ result = EOF;
_IO_funlockfile (fp);
run_fp = NULL;
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index b5f78c365b..48fbf05a85 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -238,6 +238,7 @@ tests := \
tst-fdopen \
tst-fdopen2 \
tst-ferror \
+ tst-fflush-all-input \
tst-fgets \
tst-fgets2 \
tst-fileno \
diff --git a/stdio-common/tst-fflush-all-input.c b/stdio-common/tst-fflush-all-input.c
new file mode 100644
index 0000000000..8e3fca3a08
--- /dev/null
+++ b/stdio-common/tst-fflush-all-input.c
@@ -0,0 +1,94 @@
+/* Test fflush (NULL) flushes input files (bug 32369).
+ Copyright (C) 2025 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 <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <wchar.h>
+
+#include <support/check.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+
+int
+do_test (void)
+{
+ FILE *temp = tmpfile ();
+ TEST_VERIFY_EXIT (temp != NULL);
+ fprintf (temp, "abc");
+ TEST_COMPARE (fflush (temp), 0);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
+ TEST_COMPARE (fgetc (temp), 'a');
+ TEST_COMPARE (fflush (NULL), 0);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
+ xfclose (temp);
+
+ /* Likewise, but in wide mode. */
+ temp = tmpfile ();
+ TEST_VERIFY_EXIT (temp != NULL);
+ fwprintf (temp, L"abc");
+ TEST_COMPARE (fflush (temp), 0);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
+ TEST_COMPARE (fgetwc (temp), L'a');
+ TEST_COMPARE (fflush (NULL), 0);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
+ xfclose (temp);
+
+ /* Similar tests, but with the flush implicitly occurring on exit
+ (in a forked subprocess). */
+
+ temp = tmpfile ();
+ TEST_VERIFY_EXIT (temp != NULL);
+ pid_t pid = xfork ();
+ if (pid == 0)
+ {
+ fprintf (temp, "abc");
+ TEST_COMPARE (fflush (temp), 0);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
+ TEST_COMPARE (fgetc (temp), 'a');
+ exit (EXIT_SUCCESS);
+ }
+ else
+ {
+ TEST_COMPARE (xwaitpid (pid, NULL, 0), pid);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
+ xfclose (temp);
+ }
+
+ temp = tmpfile ();
+ TEST_VERIFY_EXIT (temp != NULL);
+ pid = xfork ();
+ if (pid == 0)
+ {
+ fwprintf (temp, L"abc");
+ TEST_COMPARE (fflush (temp), 0);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
+ TEST_COMPARE (fgetwc (temp), L'a');
+ exit (EXIT_SUCCESS);
+ }
+ else
+ {
+ TEST_COMPARE (xwaitpid (pid, NULL, 0), pid);
+ TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
+ xfclose (temp);
+ }
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.43.0
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] Fix fflush handling for mmap files after ungetc (bug 32535)
2025-01-15 7:17 ` DJ Delorie
@ 2025-01-20 22:56 ` Joseph Myers
2025-01-21 2:28 ` DJ Delorie
0 siblings, 1 reply; 26+ messages in thread
From: Joseph Myers @ 2025-01-20 22:56 UTC (permalink / raw)
To: DJ Delorie; +Cc: libc-alpha
On Wed, 15 Jan 2025, DJ Delorie wrote:
> Some of the logic seems a little off to me, but it might be my brand new
> understanding of how _IO_* works ;-)
>
> Also, the test seems weak somehow.
>
> Joseph Myers <josmyers@redhat.com> writes:
> > diff --git a/libio/fileops.c b/libio/fileops.c
> > int
> > _IO_file_sync_mmap (FILE *fp)
> > {
> > + off64_t o = fp->_offset - (fp->_IO_read_end - fp->_IO_read_ptr);
>
> To be consistent with logic elsewhere, this could have been:
>
> > off64_t o = fp->_offset + (fp->_IO_read_ptr - fp->_IO_read_end);
This is following _IO_file_seekoff_mmap.
> > if (fp->_IO_read_ptr != fp->_IO_read_end)
> > {
> > - if (__lseek64 (fp->_fileno, fp->_IO_read_ptr - fp->_IO_buf_base,
> > - SEEK_SET)
> > - != fp->_IO_read_ptr - fp->_IO_buf_base)
> > + if (_IO_in_backup (fp))
> > + {
> > + _IO_switch_to_main_get_area (fp);
> > + o -= fp->_IO_read_end - fp->_IO_read_base;
>
> Shouldn't one of these be _IO_read_ptr ?
_IO_switch_to_main_get_area includes "fp->_IO_read_ptr =
fp->_IO_read_base;". (This logic is actually emulating the use of
"fp->_IO_save_end - fp->_IO_save_base" in ftell and related functions.)
> > + fp = xfopen (filename, "rm");
> > + TEST_COMPARE (fgetc (fp), 't');
> > + TEST_COMPARE (ungetc ('u', fp), 'u');
> > + TEST_COMPARE (fflush (fp), 0);
>
> Should we test that fflush() did the right thing, somehow?
> Can we, for mmap'd files?
I've added tests of the next characters read from the file, which is more
or less what it means for fflush to do the right thing here (discard the
character pushed with ungetc while preserving the offset).
Fix fflush handling for mmap files after ungetc (bug 32535)
As discussed in bug 32535, fflush fails on files opened for reading
using mmap after ungetc. Fix the logic to handle this case and still
compute the file offset correctly.
Tested for x86_64.
---
Changed in v2: also verify characters read after fflush.
---
libio/fileops.c | 12 +++++---
stdio-common/Makefile | 1 +
stdio-common/tst-fflush-mmap.c | 50 ++++++++++++++++++++++++++++++++++
3 files changed, 59 insertions(+), 4 deletions(-)
create mode 100644 stdio-common/tst-fflush-mmap.c
diff --git a/libio/fileops.c b/libio/fileops.c
index 97875d1eaf..7e370d12b2 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -858,17 +858,21 @@ libc_hidden_ver (_IO_new_file_sync, _IO_file_sync)
int
_IO_file_sync_mmap (FILE *fp)
{
+ off64_t o = fp->_offset - (fp->_IO_read_end - fp->_IO_read_ptr);
if (fp->_IO_read_ptr != fp->_IO_read_end)
{
- if (__lseek64 (fp->_fileno, fp->_IO_read_ptr - fp->_IO_buf_base,
- SEEK_SET)
- != fp->_IO_read_ptr - fp->_IO_buf_base)
+ if (_IO_in_backup (fp))
+ {
+ _IO_switch_to_main_get_area (fp);
+ o -= fp->_IO_read_end - fp->_IO_read_base;
+ }
+ if (__lseek64 (fp->_fileno, o, SEEK_SET) != o)
{
fp->_flags |= _IO_ERR_SEEN;
return EOF;
}
}
- fp->_offset = fp->_IO_read_ptr - fp->_IO_buf_base;
+ fp->_offset = o;
fp->_IO_read_end = fp->_IO_read_ptr = fp->_IO_read_base;
return 0;
}
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 06c9eaf426..703e8af7e8 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -239,6 +239,7 @@ tests := \
tst-fdopen2 \
tst-ferror \
tst-fflush-all-input \
+ tst-fflush-mmap \
tst-fgets \
tst-fgets2 \
tst-fileno \
diff --git a/stdio-common/tst-fflush-mmap.c b/stdio-common/tst-fflush-mmap.c
new file mode 100644
index 0000000000..3bddb909e0
--- /dev/null
+++ b/stdio-common/tst-fflush-mmap.c
@@ -0,0 +1,50 @@
+/* Test fflush after ungetc on files using mmap (bug 32535).
+ Copyright (C) 2025 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 <stdio.h>
+
+#include <support/check.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+
+int
+do_test (void)
+{
+ char *filename = NULL;
+ int fd = create_temp_file ("tst-fflush-mmap", &filename);
+ TEST_VERIFY_EXIT (fd != -1);
+ xclose (fd);
+
+ /* Test fflush after ungetc (bug 32535). */
+ FILE *fp = xfopen (filename, "w");
+ TEST_VERIFY (0 <= fputs ("test", fp));
+ xfclose (fp);
+
+ fp = xfopen (filename, "rm");
+ TEST_COMPARE (fgetc (fp), 't');
+ TEST_COMPARE (ungetc ('u', fp), 'u');
+ TEST_COMPARE (fflush (fp), 0);
+ TEST_COMPARE (fgetc (fp), 't');
+ TEST_COMPARE (fgetc (fp), 'e');
+ xfclose (fp);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.43.0
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] Add test of input file flushing / offset issues
2025-01-15 21:39 ` DJ Delorie
@ 2025-01-20 23:07 ` Joseph Myers
2025-01-21 2:30 ` DJ Delorie
0 siblings, 1 reply; 26+ messages in thread
From: Joseph Myers @ 2025-01-20 23:07 UTC (permalink / raw)
To: DJ Delorie; +Cc: libc-alpha
On Wed, 15 Jan 2025, DJ Delorie wrote:
> > +/* Base locations at which the main test (ungetc calls then doing
> > + something that clears ungetc characters, then checking offset)
> > + starts. */
> > +enum test_base_loc
> > + {
> > + base_loc_start,
> > + base_loc_blksize,
> > + base_loc_bufsiz,
> > + base_loc_max
> > + };
>
> Should include one pseudo-random location (mid-buffer), and one near the
> end (i.e. read to EOF then ungetc).
Added offsets BUFSIZ / 2 and 2 * BUFSIZ.
> I see we add an offset of 0..3 in the main loop, but that doesn't insert
> tests *before* block breaks, just *after* them.
Changed to cover offsets -2 to 3.
Add test of input file flushing / offset issues
Having fixed several bugs relating to flushing of FILE* streams (with
fflush and other operations) and their offsets (both the file position
indicator in the FILE*, and the offset in the underlying open file
description), especially after ungetc but not limited to that case,
add a test that more systematically covers different combinations of
cases for such issues, with 57220 separate scenarios tested (which
include examples of all the five separate fixed bugs), all of which
pass given the five previous bug fixes.
Tested for x86_64.
---
Changed in v2: also checking at locations BUFSIZ / 2 and 2 * BUFSIZ
(the latter being at the end of the file, but without actually reading
a further character unsuccessfully to put the stream into EOF state);
adding offsets from -2 to 3 (not just 0 to 3) in main loop.
---
stdio-common/Makefile | 1 +
stdio-common/tst-read-offset.c | 560 +++++++++++++++++++++++++++++++++
2 files changed, 561 insertions(+)
create mode 100644 stdio-common/tst-read-offset.c
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 703e8af7e8..c14cf32776 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -285,6 +285,7 @@ tests := \
tst-printf-round \
tst-printfsz \
tst-put-error \
+ tst-read-offset \
tst-renameat2 \
tst-rndseek \
tst-scanf-binary-c11 \
diff --git a/stdio-common/tst-read-offset.c b/stdio-common/tst-read-offset.c
new file mode 100644
index 0000000000..b8706607fc
--- /dev/null
+++ b/stdio-common/tst-read-offset.c
@@ -0,0 +1,560 @@
+/* Test offsets in files being read, in particular with ungetc.
+ Copyright (C) 2025 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 <dlfcn.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <support/check.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+
+static volatile bool fail = false;
+
+/* Induce a malloc failure whenever FAIL is set. */
+void *
+malloc (size_t sz)
+{
+ if (fail)
+ return NULL;
+
+ static void *(*real_malloc) (size_t);
+
+ if (real_malloc == NULL)
+ real_malloc = dlsym (RTLD_NEXT, "malloc");
+
+ return real_malloc (sz);
+}
+
+/* The name of the temporary file used by all the tests. */
+static char *filename;
+
+/* st_blksize value for that file, or BUFSIZ if out of range. */
+static int blksize = BUFSIZ;
+
+/* Test data, both written to that file and used as an in-memory
+ stream. */
+char test_data[2 * BUFSIZ];
+
+/* Ways to open a test stream for reading (that may use different code
+ paths in libio). */
+enum test_open_case
+ {
+ test_open_fopen,
+ test_open_fopen_m,
+ test_open_fopen64,
+ test_open_fopen64_m,
+ test_open_fmemopen,
+ test_open_max
+ };
+
+static const char *const test_open_case_name[test_open_max] =
+ {
+ "fopen", "fopen(mmap)", "fopen64", "fopen64(mmap)", "fmemopen"
+ };
+
+static FILE *
+open_test_stream (enum test_open_case c)
+{
+ FILE *fp;
+ switch (c)
+ {
+ case test_open_fopen:
+ fp = fopen (filename, "r");
+ break;
+
+ case test_open_fopen_m:
+ fp = fopen (filename, "rm");
+ break;
+
+ case test_open_fopen64:
+ fp = fopen64 (filename, "r");
+ break;
+
+ case test_open_fopen64_m:
+ fp = fopen64 (filename, "rm");
+ break;
+
+ case test_open_fmemopen:
+ fp = fmemopen (test_data, 2 * BUFSIZ, "r");
+ break;
+
+ default:
+ abort ();
+ }
+ TEST_VERIFY_EXIT (fp != NULL);
+ return fp;
+}
+
+/* Base locations at which the main test (ungetc calls then doing
+ something that clears ungetc characters, then checking offset)
+ starts. */
+enum test_base_loc
+ {
+ base_loc_start,
+ base_loc_blksize,
+ base_loc_half,
+ base_loc_bufsiz,
+ base_loc_eof,
+ base_loc_max
+ };
+
+static int
+base_loc_to_bytes (enum test_base_loc loc, int offset)
+{
+ switch (loc)
+ {
+ case base_loc_start:
+ return offset;
+
+ case base_loc_blksize:
+ return blksize + offset;
+
+ case base_loc_half:
+ return BUFSIZ / 2 + offset;
+
+ case base_loc_bufsiz:
+ return BUFSIZ + offset;
+
+ case base_loc_eof:
+ return 2 * BUFSIZ + offset;
+
+ default:
+ abort ();
+ }
+}
+
+/* Ways to clear data from ungetc. */
+enum clear_ungetc_case
+ {
+ clear_fseek,
+ clear_fseekm1,
+ clear_fseekp1,
+ clear_fseeko,
+ clear_fseekom1,
+ clear_fseekop1,
+ clear_fseeko64,
+ clear_fseeko64m1,
+ clear_fseeko64p1,
+ clear_fsetpos,
+ clear_fsetposu,
+ clear_fsetpos64,
+ clear_fsetpos64u,
+ clear_fflush,
+ clear_fflush_null,
+ clear_fclose,
+ clear_max
+ };
+
+static const char *const clear_ungetc_case_name[clear_max] =
+ {
+ "fseek", "fseek(-1)", "fseek(1)", "fseeko", "fseeko(-1)", "fseeko(1)",
+ "fseeko64", "fseeko64(-1)", "fseeko64(1)", "fsetpos", "fsetpos(before)",
+ "fsetpos64", "fsetpos64(before)", "fflush", "fflush(NULL)", "fclose"
+ };
+
+static int
+clear_offset (enum clear_ungetc_case c, int num_ungetc)
+{
+ switch (c)
+ {
+ case clear_fseekm1:
+ case clear_fseekom1:
+ case clear_fseeko64m1:
+ return -1;
+
+ case clear_fseekp1:
+ case clear_fseekop1:
+ case clear_fseeko64p1:
+ return 1;
+
+ case clear_fsetposu:
+ case clear_fsetpos64u:
+ return num_ungetc;
+
+ default:
+ return 0;
+ }
+}
+
+/* The offsets used with fsetpos / fsetpos64. */
+static fpos_t pos;
+static fpos64_t pos64;
+
+static int
+do_clear_ungetc (FILE *fp, enum clear_ungetc_case c, int num_ungetc)
+{
+ int ret;
+ int offset = clear_offset (c, num_ungetc);
+ switch (c)
+ {
+ case clear_fseek:
+ case clear_fseekm1:
+ case clear_fseekp1:
+ ret = fseek (fp, offset, SEEK_CUR);
+ break;
+
+ case clear_fseeko:
+ case clear_fseekom1:
+ case clear_fseekop1:
+ ret = fseeko (fp, offset, SEEK_CUR);
+ break;
+
+ case clear_fseeko64:
+ case clear_fseeko64m1:
+ case clear_fseeko64p1:
+ ret = fseeko64 (fp, offset, SEEK_CUR);
+ break;
+
+ case clear_fsetpos:
+ case clear_fsetposu:
+ ret = fsetpos (fp, &pos);
+ break;
+
+ case clear_fsetpos64:
+ case clear_fsetpos64u:
+ ret = fsetpos64 (fp, &pos64);
+ break;
+
+ case clear_fflush:
+ ret = fflush (fp);
+ break;
+
+ case clear_fflush_null:
+ ret = fflush (NULL);
+ break;
+
+ case clear_fclose:
+ ret = fclose (fp);
+ break;
+
+ default:
+ abort();
+ }
+ TEST_COMPARE (ret, 0);
+ return offset;
+}
+
+static bool
+clear_valid (enum test_open_case c, enum clear_ungetc_case cl)
+{
+ switch (c)
+ {
+ case test_open_fmemopen:
+ /* fflush is not valid for input memory streams, and fclose is
+ useless for this test for such streams because there is no
+ underlying open file description for which an offset could be
+ checked after fclose. */
+ switch (cl)
+ {
+ case clear_fflush:
+ case clear_fflush_null:
+ case clear_fclose:
+ return false;
+
+ default:
+ return true;
+ }
+
+ default:
+ /* All ways of clearing ungetc state are valid for streams with
+ an underlying file. */
+ return true;
+ }
+}
+
+static bool
+clear_closes_file (enum clear_ungetc_case cl)
+{
+ switch (cl)
+ {
+ case clear_fclose:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+static void
+clear_getpos_before (FILE *fp, enum clear_ungetc_case c)
+{
+ switch (c)
+ {
+ case clear_fsetposu:
+ TEST_COMPARE (fgetpos (fp, &pos), 0);
+ break;
+
+ case clear_fsetpos64u:
+ TEST_COMPARE (fgetpos64 (fp, &pos64), 0);
+ break;
+
+ default:
+ break;
+ }
+}
+
+static void
+clear_getpos_after (FILE *fp, enum clear_ungetc_case c)
+{
+ switch (c)
+ {
+ case clear_fsetpos:
+ TEST_COMPARE (fgetpos (fp, &pos), 0);
+ break;
+
+ case clear_fsetpos64:
+ TEST_COMPARE (fgetpos64 (fp, &pos64), 0);
+ break;
+
+ default:
+ break;
+ }
+}
+
+/* Ways to verify results of clearing ungetc data. */
+enum verify_case
+ {
+ verify_read,
+ verify_ftell,
+ verify_ftello,
+ verify_ftello64,
+ verify_fd,
+ verify_max
+ };
+
+static const char *const verify_case_name[verify_max] =
+ {
+ "read", "ftell", "ftello", "ftello64", "fd"
+ };
+
+static bool
+valid_fd_offset (enum test_open_case c, enum clear_ungetc_case cl)
+{
+ switch (c)
+ {
+ case test_open_fmemopen:
+ /* No open file description. */
+ return false;
+
+ default:
+ /* fseek does not necessarily set the offset for the underlying
+ open file description ("If the most recent operation, other
+ than ftell(), on a given stream is fflush(), the file offset
+ in the underlying open file description shall be adjusted to
+ reflect the location specified by fseek()." in POSIX does not
+ include the case here where getc was the last operation).
+ Similarly, fsetpos does not necessarily set that offset
+ either. */
+ switch (cl)
+ {
+ case clear_fflush:
+ case clear_fflush_null:
+ case clear_fclose:
+ return true;
+
+ default:
+ return false;
+ }
+ }
+}
+
+static bool
+verify_valid (enum test_open_case c, enum clear_ungetc_case cl,
+ enum verify_case v)
+{
+ switch (v)
+ {
+ case verify_fd:
+ return valid_fd_offset (c, cl);
+
+ default:
+ switch (cl)
+ {
+ case clear_fclose:
+ return false;
+
+ default:
+ return true;
+ }
+ }
+}
+
+static bool
+verify_uses_fd (enum verify_case v)
+{
+ switch (v)
+ {
+ case verify_fd:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+static int
+read_to_test_loc (FILE *fp, enum test_base_loc loc, int offset)
+{
+ int to_read = base_loc_to_bytes (loc, offset);
+ for (int i = 0; i < to_read; i++)
+ TEST_COMPARE (getc (fp), (unsigned char) i);
+ return to_read;
+}
+
+static void
+setup (void)
+{
+ int fd = create_temp_file ("tst-read-offset", &filename);
+ TEST_VERIFY_EXIT (fd != -1);
+ struct stat64 st;
+ xfstat64 (fd, &st);
+ if (st.st_blksize > 0 && st.st_blksize < BUFSIZ)
+ blksize = st.st_blksize;
+ printf ("BUFSIZ = %d, blksize = %d\n", BUFSIZ, blksize);
+ xclose (fd);
+ FILE *fp = xfopen (filename, "w");
+ for (size_t i = 0; i < 2 * BUFSIZ; i++)
+ {
+ unsigned char c = i;
+ TEST_VERIFY_EXIT (fputc (c, fp) == c);
+ test_data[i] = c;
+ }
+ xfclose (fp);
+}
+
+static void
+test_one_case (enum test_open_case c, enum test_base_loc loc, int offset,
+ int num_ungetc, int num_ungetc_diff, bool ungetc_fallback,
+ enum clear_ungetc_case cl, enum verify_case v)
+{
+ int full_offset = base_loc_to_bytes (loc, offset);
+ printf ("Testing %s offset %d ungetc %d different %d %s%s %s\n",
+ test_open_case_name[c], full_offset, num_ungetc, num_ungetc_diff,
+ ungetc_fallback ? "fallback " : "", clear_ungetc_case_name[cl],
+ verify_case_name[v]);
+ FILE *fp = open_test_stream (c);
+ int cur_offset = read_to_test_loc (fp, loc, offset);
+ clear_getpos_before (fp, cl);
+ for (int i = 0; i < num_ungetc; i++)
+ {
+ unsigned char c = (i >= num_ungetc - num_ungetc_diff
+ ? cur_offset
+ : cur_offset - 1);
+ if (ungetc_fallback)
+ fail = true;
+ TEST_COMPARE (ungetc (c, fp), c);
+ fail = false;
+ cur_offset--;
+ }
+ clear_getpos_after (fp, cl);
+ int fd = -1;
+ bool done_dup = false;
+ if (verify_uses_fd (v))
+ {
+ fd = fileno (fp);
+ TEST_VERIFY (fd != -1);
+ if (clear_closes_file (cl))
+ {
+ fd = xdup (fd);
+ done_dup = true;
+ }
+ }
+ cur_offset += do_clear_ungetc (fp, cl, num_ungetc);
+ switch (v)
+ {
+ case verify_read:
+ for (;
+ cur_offset <= full_offset + 1 && cur_offset < 2 * BUFSIZ;
+ cur_offset++)
+ TEST_COMPARE (getc (fp), (unsigned char) cur_offset);
+ break;
+
+ case verify_ftell:
+ TEST_COMPARE (ftell (fp), cur_offset);
+ break;
+
+ case verify_ftello:
+ TEST_COMPARE (ftello (fp), cur_offset);
+ break;
+
+ case verify_ftello64:
+ TEST_COMPARE (ftello64 (fp), cur_offset);
+ break;
+
+ case verify_fd:
+ TEST_COMPARE (lseek (fd, 0, SEEK_CUR), cur_offset);
+ break;
+
+ default:
+ abort ();
+ }
+ if (! clear_closes_file (cl))
+ {
+ int ret = fclose (fp);
+ TEST_COMPARE (ret, 0);
+ }
+ if (done_dup)
+ xclose (fd);
+}
+
+int
+do_test (void)
+{
+ setup ();
+ for (enum test_open_case c = 0; c < test_open_max; c++)
+ for (enum test_base_loc loc = 0; loc < base_loc_max; loc++)
+ for (int offset = -2; offset <= 3; offset++)
+ for (int num_ungetc = 0;
+ num_ungetc <= 2 && num_ungetc <= base_loc_to_bytes (loc, offset);
+ num_ungetc++)
+ for (int num_ungetc_diff = 0;
+ num_ungetc_diff <= num_ungetc;
+ num_ungetc_diff++)
+ for (int ungetc_fallback = 0;
+ ungetc_fallback <= (num_ungetc == 1 ? 1 : 0);
+ ungetc_fallback++)
+ for (enum clear_ungetc_case cl = 0; cl < clear_max; cl++)
+ {
+ if (!clear_valid (c, cl))
+ continue;
+ if (base_loc_to_bytes (loc, offset) > 2 * BUFSIZ)
+ continue;
+ if ((base_loc_to_bytes (loc, offset)
+ - num_ungetc
+ + clear_offset (cl, num_ungetc)) < 0)
+ continue;
+ if ((base_loc_to_bytes (loc, offset)
+ - num_ungetc
+ + clear_offset (cl, num_ungetc)) > 2 * BUFSIZ)
+ continue;
+ for (enum verify_case v = 0; v < verify_max; v++)
+ {
+ if (!verify_valid (c, cl, v))
+ continue;
+ test_one_case (c, loc, offset, num_ungetc,
+ num_ungetc_diff, ungetc_fallback, cl, v);
+ }
+ }
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.43.0
--
Joseph S. Myers
josmyers@redhat.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] Fix fflush handling for mmap files after ungetc (bug 32535)
2025-01-20 22:56 ` Joseph Myers
@ 2025-01-21 2:28 ` DJ Delorie
0 siblings, 0 replies; 26+ messages in thread
From: DJ Delorie @ 2025-01-21 2:28 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
Joseph Myers <josmyers@redhat.com> writes:
>> > + _IO_switch_to_main_get_area (fp);
>> > + o -= fp->_IO_read_end - fp->_IO_read_base;
>>
>> Shouldn't one of these be _IO_read_ptr ?
>
> _IO_switch_to_main_get_area includes "fp->_IO_read_ptr =
> fp->_IO_read_base;". (This logic is actually emulating the use of
> "fp->_IO_save_end - fp->_IO_save_base" in ftell and related functions.)
Hmmm... that sounds like it's propogating a bad idea, in case
_IO_switch_to_main_get_area stops making that promise. But that's a
problem for another day...
Reviewed-by: DJ Delorie <dj@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] Add test of input file flushing / offset issues
2025-01-20 23:07 ` Joseph Myers
@ 2025-01-21 2:30 ` DJ Delorie
0 siblings, 0 replies; 26+ messages in thread
From: DJ Delorie @ 2025-01-21 2:30 UTC (permalink / raw)
To: Joseph Myers; +Cc: libc-alpha
Joseph Myers <josmyers@redhat.com> writes:
> On Wed, 15 Jan 2025, DJ Delorie wrote:
>> Should include one pseudo-random location (mid-buffer), and one near the
>> end (i.e. read to EOF then ungetc).
>
> Added offsets BUFSIZ / 2 and 2 * BUFSIZ.
>
>> I see we add an offset of 0..3 in the main loop, but that doesn't insert
>> tests *before* block breaks, just *after* them.
>
> Changed to cover offsets -2 to 3.
LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369)
2025-01-20 22:51 ` Joseph Myers
@ 2025-01-28 20:23 ` DJ Delorie
0 siblings, 0 replies; 26+ messages in thread
From: DJ Delorie @ 2025-01-28 20:23 UTC (permalink / raw)
To: Joseph Myers; +Cc: fweimer, libc-alpha
I suspect trying to be more clever about sync-after-fork would lead to
lots of problems, like, if we ungetc, fork, exit in the child, does the
parent lose the ungetc? We would need to fflush(NULL) in fork() before
the actual fork syscall, and that might not catch everything anyway.
LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>
FYI patchwork really wants "v2" type patches to be sent without the Re:
text and with a "v2" in the subject, in case you were looking for this
patch in patchwork ;-)
Joseph Myers <josmyers@redhat.com> writes:
> As discussed in bug 32369 and required by POSIX, the POSIX feature
> fflush (NULL) should flush input files, not just output files. The
> POSIX requirement is that "fflush() shall perform this flushing action
> on all streams for which the behavior is defined above", and the
> definition for input files is for "a stream open for reading with an
> underlying file description, if the file is not already at EOF, and
> the file is one capable of seeking".
>
> Implement this requirement in glibc. (The underlying flushing
> implementation is what deals with avoiding errors for seeking on an
> unseekable file.)
>
> Tested for x86_64.
>
> ---
>
> Changed in v2: also test the case where flushing of all input files is
> achieved by a call to exit after forking rather than an explicit call
> to fflush (NULL).
> diff --git a/libio/genops.c b/libio/genops.c
> index 2197bfe7a1..e4378ca48f 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -730,6 +730,13 @@ _IO_flush_all (void)
> )
> && _IO_OVERFLOW (fp, EOF) == EOF)
> result = EOF;
> + if (_IO_fileno (fp) >= 0
> + && ((fp->_mode <= 0 && fp->_IO_read_ptr < fp->_IO_read_end)
> + || (_IO_vtable_offset (fp) == 0
> + && fp->_mode > 0 && (fp->_wide_data->_IO_read_ptr
> + < fp->_wide_data->_IO_read_end)))
> + && _IO_SYNC (fp) != 0)
> + result = EOF;
>
> _IO_funlockfile (fp);
> run_fp = NULL;
same as v1, ok.
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index b5f78c365b..48fbf05a85 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -238,6 +238,7 @@ tests := \
> tst-fdopen \
> tst-fdopen2 \
> tst-ferror \
> + tst-fflush-all-input \
> tst-fgets \
> tst-fgets2 \
> tst-fileno \
same as v1, ok.
> diff --git a/stdio-common/tst-fflush-all-input.c b/stdio-common/tst-fflush-all-input.c
> new file mode 100644
> index 0000000000..8e3fca3a08
> --- /dev/null
> +++ b/stdio-common/tst-fflush-all-input.c
> @@ -0,0 +1,94 @@
> +/* Test fflush (NULL) flushes input files (bug 32369).
> + Copyright (C) 2025 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 <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <wchar.h>
> +
> +#include <support/check.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
Ok.
> +int
> +do_test (void)
> +{
> + FILE *temp = tmpfile ();
> + TEST_VERIFY_EXIT (temp != NULL);
> + fprintf (temp, "abc");
> + TEST_COMPARE (fflush (temp), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
> + TEST_COMPARE (fgetc (temp), 'a');
> + TEST_COMPARE (fflush (NULL), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
> + xfclose (temp);
> +
> + /* Likewise, but in wide mode. */
> + temp = tmpfile ();
> + TEST_VERIFY_EXIT (temp != NULL);
> + fwprintf (temp, L"abc");
> + TEST_COMPARE (fflush (temp), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
> + TEST_COMPARE (fgetwc (temp), L'a');
> + TEST_COMPARE (fflush (NULL), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
> + xfclose (temp);
Same as v1, ok.
> + /* Similar tests, but with the flush implicitly occurring on exit
> + (in a forked subprocess). */
> +
> + temp = tmpfile ();
> + TEST_VERIFY_EXIT (temp != NULL);
> + pid_t pid = xfork ();
> + if (pid == 0)
> + {
child, ok
> + fprintf (temp, "abc");
> + TEST_COMPARE (fflush (temp), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
> + TEST_COMPARE (fgetc (temp), 'a');
> + exit (EXIT_SUCCESS);
Ok; file left at position 1.
> + }
> + else
> + {
> + TEST_COMPARE (xwaitpid (pid, NULL, 0), pid);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
> + xfclose (temp);
Ok.
> + }
> +
> + temp = tmpfile ();
> + TEST_VERIFY_EXIT (temp != NULL);
> + pid = xfork ();
> + if (pid == 0)
> + {
> + fwprintf (temp, L"abc");
> + TEST_COMPARE (fflush (temp), 0);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_SET), 0);
> + TEST_COMPARE (fgetwc (temp), L'a');
> + exit (EXIT_SUCCESS);
> + }
> + else
> + {
> + TEST_COMPARE (xwaitpid (pid, NULL, 0), pid);
> + TEST_COMPARE (lseek (fileno (temp), 0, SEEK_CUR), 1);
> + xfclose (temp);
> + }
Ok.
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
Ok.
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2025-01-28 20:23 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-14 2:00 [PATCH 0/6] Fix issues around flushing and file offsets for input files Joseph Myers
2025-01-14 2:02 ` [PATCH 1/6] Fix fflush after ungetc on input file (bug 5994) Joseph Myers
2025-01-15 4:20 ` DJ Delorie
2025-01-14 2:03 ` [PATCH 2/6] Make fclose seek input file to right offset (bug 12724) Joseph Myers
2025-01-15 5:33 ` DJ Delorie
2025-01-15 19:16 ` Joseph Myers
2025-01-15 20:01 ` DJ Delorie
2025-01-14 2:03 ` [PATCH 3/6] Make fflush (NULL) flush input files (bug 32369) Joseph Myers
2025-01-15 6:01 ` DJ Delorie
2025-01-15 19:24 ` Joseph Myers
2025-01-15 20:06 ` DJ Delorie
2025-01-16 0:28 ` Joseph Myers
2025-01-15 20:07 ` DJ Delorie
2025-01-15 17:27 ` Florian Weimer
2025-01-20 22:51 ` Joseph Myers
2025-01-28 20:23 ` DJ Delorie
2025-01-14 2:03 ` [PATCH 4/6] Fix fseek handling for mmap files after ungetc or fflush (bug 32529) Joseph Myers
2025-01-15 6:35 ` DJ Delorie
2025-01-14 2:04 ` [PATCH 5/6] Fix fflush handling for mmap files after ungetc (bug 32535) Joseph Myers
2025-01-15 7:17 ` DJ Delorie
2025-01-20 22:56 ` Joseph Myers
2025-01-21 2:28 ` DJ Delorie
2025-01-14 2:04 ` [PATCH 6/6] Add test of input file flushing / offset issues Joseph Myers
2025-01-15 21:39 ` DJ Delorie
2025-01-20 23:07 ` Joseph Myers
2025-01-21 2:30 ` DJ Delorie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).