[2/6] Make fclose seek input file to right offset (bug 12724)

  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
  

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.
  

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 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>