[PATCH 2/2] Fix file offsets when in append update mode (a+) (#16532)

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

Hi,

adb26fae introduced an optimization in ftell that allowed it to skip a
flush when it would compute the file offset based on the position in
the buffer and the offset in the file.  This breaks for files opened
in "a+" mode because these streams have the special requirement of
being able to read from the beginning, but append at the end.  So if
we were reading and then writing without flushing the buffer, the
offsets recorded in the stream would be incorrect.  Other update modes
(r+ or w+) don't have this caveat since they explicitly need to have a
flush or fseek between the read/write switch.

The fix here is to add an exception to ftell where we do the switch to
read mode (and hence flush the buffer).  A more involved fix ought to
record the correct offset using seek whenever the write is attempted,
so that one has the correct offset to the end of file in append mode.
That would be an intrusive fix, because of which I have left it for
later.

This change also adds to the tst-fopen-append-update.c test case.
Verified on x6_64 that this does not break any existing tests either.

OK to commit?

Siddhesh

	[BZ #16532]
	* libio/fileops.c (_IO_new_file_seekoff): Flush if file is in
	append-update mode.
	* libio/wfileops.c (_IO_wfile_seekoff): Likewise.
	* libio/tst-fopen-append-update.c (do_one_test): Add UPDATE
	argument.
	(do_test): Add tests for append-update.


---
 libio/fileops.c                 | 15 ++++++++++++++-
 libio/tst-fopen-append-update.c | 24 ++++++++++++++++--------
 libio/wfileops.c                | 13 +++++++++++++
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/libio/fileops.c b/libio/fileops.c
index a3499be..030796a 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -949,7 +949,20 @@ _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. */
+
+      /* Writing in a+ mode.  The offset may be unreliable since it could have
+	 been set during the last read, so flush and switch to get mode.
+
+	 TODO: A good way to avoid the flush here would be to do an lseek when
+	 we switch over to write mode.  */
+      bool append_update = ((fp->_flags & (_IO_NO_READS | _IO_IS_APPENDING))
+			    == _IO_IS_APPENDING);
+
+      if (was_writing && append_update && _IO_switch_to_get_mode (fp))
+	return EOF;
+    }
 
   /* Flush unwritten characters.
      (This may do an unneeded write if we seek within the buffer.
diff --git a/libio/tst-fopen-append-update.c b/libio/tst-fopen-append-update.c
index 83d7744..49e77c5 100644
--- a/libio/tst-fopen-append-update.c
+++ b/libio/tst-fopen-append-update.c
@@ -32,14 +32,18 @@ static int do_test (void);
 static const char *data = "abcdef";
 
 static int
-do_one_test (const char *filename, int count, bool do_fopen)
+do_one_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, "a");
+      fp = fopen (filename, mode);
       if (fp == NULL)
 	{
 	  printf ("fopen[%d]: %m\n", count);
@@ -49,7 +53,7 @@ do_one_test (const char *filename, int count, bool do_fopen)
     }
   else
     {
-      int fd = open (filename, O_WRONLY, 0);
+      int fd = open (filename, update ? O_RDWR : O_WRONLY, 0);
 
       if (fd == -1)
 	{
@@ -58,7 +62,7 @@ do_one_test (const char *filename, int count, bool do_fopen)
 	  goto out;
 	}
 
-      fp = fdopen (fd, "a");
+      fp = fdopen (fd, mode);
       if (fp == NULL)
 	{
 	  printf ("fdopen[%d]: %m\n", count);
@@ -90,8 +94,7 @@ do_one_test (const char *filename, int count, bool do_fopen)
       goto out;
     }
 
-  printf ("Test 1 succeeded for %s: offset = %ld\n",
-	  do_fopen ? "fopen" : "fdopen", offset);
+  printf ("offset = %ld\n", offset);
 
 out:
   if (fp)
@@ -140,8 +143,13 @@ do_test (void)
 
   /* In append mode (a), fopen and fdopen should move its offset to the end of
      the file.  */
-  ret |= do_one_test (filename, ++i, false);
-  ret |= do_one_test (filename, ++i, true);
+  ret |= do_one_test (filename, ++i, false, false);
+  ret |= do_one_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_one_test (filename, ++i, false, true);
+  ret |= do_one_test (filename, ++i, true, true);
 
   return ret;
 }
diff --git a/libio/wfileops.c b/libio/wfileops.c
index 9cebe77..094e091 100644
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -636,6 +636,19 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
 	  /* There is no more data in the backup buffer.  We can
 	     switch back.  */
 	  _IO_switch_to_main_wget_area (fp);
+
+	  /* Writing in a+ mode.  The offset may be unreliable since it could
+	     have been set during the last read, so flush and switch to get
+	     mode.
+
+	     TODO: A good way to avoid the flush here would be to do an lseek
+	     when we switch over to write mode.  */
+	  bool append_update = ((fp->_flags
+				 & (_IO_NO_READS | _IO_IS_APPENDING))
+				== _IO_IS_APPENDING);
+
+	  if (was_writing && append_update && _IO_switch_to_get_mode (fp))
+	    return EOF;
 	}
 
       dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */
-- 
1.8.3.1