[PATCH v2] Fix ftell with fdopen for all cases and fopen with a+ mode (#16532)

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

Hi,

So here's take two at fixing the problem with fdopen/fopen and ftell
in append modes.

When fdopen attaches to a file descriptor, it sets the offset at that
point, which may be deceptive.  It gives later ftell() calls the idea
that we know the offset since the handle was active, which is not true
since one could have changed the underlying file offset before the
handle became active, i.e. had an operation performed on it.

Additionally, with a+ mode, fopen cannot tell the correct file
position if it is initially in write mode and the buffer has not been
flushed, since even an lseek(SEEK_CUR) at this stage will give the
wrong information.  In fact, this is true for all append mode
operations, but fopen gets away with it with "a" because it seeks to
SEEK_END when it opens the file.

So the fix here is to avoid doing any seek at fdopen time to allow
ftell() to get the real offset when needed, and for append modes to
always SEEK_END when the stream is being written to.  All this is
necessary only the first time, i.e. just after the file has been
open/attached and the buffer not yet flushed.  Subsequent queries
should always be correct since applications are required to flush or
seek to the desired position when switching between reading or
writing, both of which sync up the offset.

There is also a test case that verifies that both append modes work
correctly with fdopen and fopen and that both write modes and the r+
mode work correctly with fdopen.  The test case verifies I/O in both
regular and wide modes.

Siddhesh

	[BZ #16532]
	* libio/tst-fopen-append-update.c: New test case.
	* libio/Makefile (tests): Add it.
	* libio/fileops.c (_IO_new_file_seekoff): Seek to end in ftell
	for streams in append mode.
	* libio/wfileops.c (_IO_wfile_seekoff): Likewise.
	* libio/iofdopen.c (_IO_new_fdopen): Don't use _IO_fd_attach.

---
 libio/Makefile                  |   2 +-
 libio/fileops.c                 |  16 ++-
 libio/iofdopen.c                |  18 ++-
 libio/tst-fopen-append-update.c | 275 ++++++++++++++++++++++++++++++++++++++++
 libio/wfileops.c                |  11 ++
 5 files changed, 310 insertions(+), 12 deletions(-)
 create mode 100644 libio/tst-fopen-append-update.c

diff --git a/libio/Makefile b/libio/Makefile
index 747a779..640b475 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -60,7 +60,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst-wmemstream1 tst-wmemstream2 \
 	bug-memstream1 bug-wmemstream1 \
 	tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \
-	tst-fwrite-error tst-ftell-partial-wide
+	tst-fwrite-error tst-ftell-partial-wide tst-fopen-append-update
 ifeq (yes,$(build-shared))
 # Add test-fopenloc only if shared library is enabled since it depends on
 # shared localedata objects.
diff --git a/libio/fileops.c b/libio/fileops.c
index a3499be..829634f 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -949,7 +949,21 @@ _IO_new_file_seekoff (fp, offset, dir, mode)
 		      || _IO_in_put_mode (fp));
 
   if (mode == 0)
-    dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */
+    {
+      dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */
+
+      /* When we write in append mode for the first time, the offset may still
+	 be set to the beginning of the file because we have not flushed the
+	 buffer.  Send the offset to the end of the file, where it belongs, so
+	 that we return the correct offset through ftell.  */
+      if (was_writing && (fp->_flags & _IO_IS_APPENDING) == _IO_IS_APPENDING)
+	{
+	  off_t ret = _IO_SYSSEEK (fp, 0, _IO_seek_end);
+	  if (ret == EOF)
+	    return ret;
+	  fp->_offset = ret;
+	}
+    }
 
   /* Flush unwritten characters.
      (This may do an unneeded write if we seek within the buffer.
diff --git a/libio/iofdopen.c b/libio/iofdopen.c
index 066ff19..6dbdcb5 100644
--- a/libio/iofdopen.c
+++ b/libio/iofdopen.c
@@ -141,9 +141,6 @@ _IO_new_fdopen (fd, mode)
 #ifdef _IO_MTSAFE_IO
   new_f->fp.file._lock = &new_f->lock;
 #endif
-  /* Set up initially to use the `maybe_mmap' jump tables rather than using
-     __fopen_maybe_mmap to do it, because we need them in place before we
-     call _IO_file_attach or else it will allocate a buffer immediately.  */
   _IO_no_init (&new_f->fp.file, 0, 0, &new_f->wd,
 #ifdef _G_HAVE_MMAP
 	       (use_mmap && (read_write & _IO_NO_WRITES))
@@ -155,17 +152,18 @@ _IO_new_fdopen (fd, mode)
     (use_mmap && (read_write & _IO_NO_WRITES)) ? &_IO_file_jumps_maybe_mmap :
 #endif
       &_IO_file_jumps;
+
+  /* _IO_file_init unsets the file position.  This is convenient because it can
+     then be queried when we do an ftell.  We don't bother with positioning the
+     offset in the file or even querying what it is right now since we don't
+     want to interfere with any reads or writes that may be happening on the
+     file descriptor before it becomes active, i.e. has its first
+     operation.  */
   _IO_file_init (&new_f->fp);
 #if  !_IO_UNIFIED_JUMPTABLES
   new_f->fp.vtable = NULL;
 #endif
-  if (_IO_file_attach ((_IO_FILE *) &new_f->fp, fd) == NULL)
-    {
-      _IO_setb (&new_f->fp.file, NULL, NULL, 0);
-      _IO_un_link (&new_f->fp);
-      free (new_f);
-      return NULL;
-    }
+  new_f->fp.file._fileno = fd;
   new_f->fp.file._flags &= ~_IO_DELETE_DONT_CLOSE;
 
   _IO_mask_flags (&new_f->fp.file, read_write,
diff --git a/libio/tst-fopen-append-update.c b/libio/tst-fopen-append-update.c
new file mode 100644
index 0000000..f1b7a10
--- /dev/null
+++ b/libio/tst-fopen-append-update.c
@@ -0,0 +1,275 @@
+/* Verify that ftell returns the correct value when a file is opened in
+   append (a) and append-update (a+) mode.
+   Copyright (C) 2014 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <locale.h>
+#include <wchar.h>
+
+static int do_test (void);
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
+
+static const void *data;
+static const char *char_data = "abcdef";
+static const wchar_t *wide_data = L"abcdef";
+size_t data_len;
+
+typedef int (*fputs_func_t) (const void *data, FILE *fp);
+fputs_func_t fputs_func;
+
+static int
+do_write_test (const char *filename, int count, const char *mode)
+{
+  FILE *fp = NULL;
+  int ret = 0;
+  int m;
+
+  printf ("fdopen (file, \"%s\"): ", mode);
+
+  if (strchr (mode, '+') != NULL)
+    m = O_RDWR;
+  else if (strchr (mode, 'w') != NULL)
+    m = O_WRONLY;
+  else
+    {
+      printf ("Invalid mode %s\n", mode);
+      ret = 1;
+      goto out;
+    }
+
+  int fd = open (filename, m, 0);
+
+  if (fd == -1)
+    {
+      printf ("Failed to open temp file: %m\n");
+      ret = 1;
+      goto out;
+    }
+
+  fp = fdopen (fd, mode);
+  if (fp == NULL)
+    {
+      printf ("fdopen[%d]: %m\n", count);
+      ret = 1;
+      close (fd);
+      goto out;
+    }
+
+  /* Move offset to COUNT from the beginning.  */
+  lseek (fd, count, SEEK_SET);
+
+  /* Write some data.  */
+  size_t written = fputs_func (data, fp);
+
+  if (written == EOF)
+    {
+      printf ("fputs[1] failed to write data\n");
+      ret = 1;
+      goto out;
+    }
+
+  /* Verify that the offset points to the end of the file.  */
+  long offset = ftell (fp);
+
+  if (offset != count + data_len)
+    {
+      printf ("Incorrect offset.  Expected %zu, but got %ld\n",
+	      (count + data_len), offset);
+
+      ret = 1;
+      goto out;
+    }
+
+  printf ("offset = %ld\n", offset);
+
+out:
+  if (fp)
+    fclose (fp);
+  return ret;
+}
+
+static int
+do_append_test (const char *filename, int count, bool do_fopen, bool update)
+{
+  FILE *fp = NULL;
+  int ret = 0;
+
+  const char *mode = update ? "a+" : "a";
+
+  printf ("%s (file, \"%s\"): ", do_fopen ? "fopen" : "fdopen", mode);
+
+  if (do_fopen)
+    {
+      fp = fopen (filename, mode);
+      if (fp == NULL)
+	{
+	  printf ("fopen[%d]: %m\n", count);
+	  ret = 1;
+	  goto out;
+	}
+    }
+  else
+    {
+      int fd = open (filename, update ? O_RDWR : O_WRONLY, 0);
+
+      if (fd == -1)
+	{
+	  printf ("Failed to open temp file: %m\n");
+	  ret = 1;
+	  goto out;
+	}
+
+      fp = fdopen (fd, mode);
+      if (fp == NULL)
+	{
+	  printf ("fdopen[%d]: %m\n", count);
+	  ret = 1;
+	  close (fd);
+	  goto out;
+	}
+    }
+
+  /* Write some data.  */
+  size_t written = fputs_func (data, fp);
+
+  if (written == EOF)
+    {
+      printf ("fputs[1] failed to write all data\n");
+      ret = 1;
+      goto out;
+    }
+
+  /* Verify that the offset points to the end of the file.  */
+  long offset = ftell (fp);
+
+  if (offset != count * data_len)
+    {
+      printf ("Incorrect offset.  Expected %zu, but got %ld\n",
+	      (count * data_len), offset);
+
+      ret = 1;
+      goto out;
+    }
+
+  printf ("offset = %ld\n", offset);
+
+out:
+  if (fp)
+    fclose (fp);
+  return ret;
+}
+
+static int
+do_test (void)
+{
+  int ret = 0;
+  FILE *fp = NULL;
+  char *filename;
+  size_t written;
+  int fd = create_temp_file ("tst-fseek-wide-partial.out", &filename);
+
+  if (fd == -1)
+    {
+      printf ("create_temp_file: %m\n");
+      return 1;
+    }
+
+  fp = fdopen (fd, "w");
+  if (fp == NULL)
+    {
+      printf ("fdopen[0]: %m\n");
+      close (fd);
+      return 1;
+    }
+
+  data = char_data;
+  data_len = strlen (char_data);
+  written = fputs (data, fp);
+
+  if (written == EOF)
+    {
+      printf ("fputs[1] failed to write data\n");
+      ret = 1;
+    }
+
+  fclose (fp);
+  if (ret)
+    return ret;
+
+  /* Counter to keep track of what offset to expect in the file and also to
+     enumerate the tests.  */
+  int i = 1;
+
+  /* Tests for regular files.  */
+  fputs_func = (fputs_func_t) fputs;
+  data = char_data;
+  data_len = strlen (char_data);
+
+  /* In append mode (a), fopen and fdopen should move its offset to the end of
+     the file.  */
+  ret |= do_append_test (filename, ++i, false, false);
+  ret |= do_append_test (filename, ++i, true, false);
+
+  /* In append-update mode (a+), fopen and fdopen should move its offset to the
+     end of the file.  */
+  ret |= do_append_test (filename, ++i, false, true);
+  ret |= do_append_test (filename, ++i, true, true);
+
+  /* The write modes and the r+ mode should adjust correctly to the underlying
+     change in file offset.  */
+  ret |= do_write_test (filename, ++i, "w");
+  ret |= do_write_test (filename, ++i, "w+");
+  ret |= do_write_test (filename, ++i, "r+");
+
+
+  /* Tests for wide files.  */
+  if (setlocale (LC_ALL, "en_US.UTF-8") == NULL)
+    {
+      printf ("Cannot set en_US.UTF-8 locale.\n");
+      return 1;
+    }
+  i -= 3;
+  fputs_func = (fputs_func_t) fputws;
+  data = wide_data;
+  data_len = wcslen (wide_data);
+
+  /* In append mode (a), fopen and fdopen should move its offset to the end of
+     the file.  */
+  ret |= do_append_test (filename, ++i, false, false);
+  ret |= do_append_test (filename, ++i, true, false);
+
+  /* In append-update mode (a+), fopen and fdopen should move its offset to the
+     end of the file.  */
+  ret |= do_append_test (filename, ++i, false, true);
+  ret |= do_append_test (filename, ++i, true, true);
+
+  /* The write modes and the r+ mode should adjust correctly to the underlying
+     change in file offset.  */
+  ret |= do_write_test (filename, ++i, "w");
+  ret |= do_write_test (filename, ++i, "w+");
+  ret |= do_write_test (filename, ++i, "r+");
+
+  return ret;
+}
diff --git a/libio/wfileops.c b/libio/wfileops.c
index 9cebe77..142a6c9 100644
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -639,6 +639,17 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
 	}
 
       dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */
+      /* When we write in append mode for the first time, the offset may still
+	 be set to the beginning of the file because we have not flushed the
+	 buffer.  Send the offset to the end of the file, where it belongs, so
+	 that we return the correct offset through ftell.  */
+      if (was_writing && (fp->_flags & _IO_IS_APPENDING) == _IO_IS_APPENDING)
+	{
+	  off_t ret = _IO_SYSSEEK (fp, 0, _IO_seek_end);
+	  if (ret == EOF)
+	    return ret;
+	  fp->_offset = ret;
+	}
     }
 
   /* Flush unwritten characters.
-- 
1.8.3.1

Attachment: pgpft2GbjhEyt.pgp
Description: PGP signature