[PATCH 2/6] Make fclose seek input file to right offset (bug 12724)
DJ Delorie
dj@redhat.com
Wed Jan 15 05:33:43 GMT 2025
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.
More information about the Libc-alpha
mailing list