[PATCH] Also flush write when reading large amount of data (BZ #18659)

[PATCH] Also flush write when reading large amount of data (BZ #18659)

[Siddhesh Poyarekar]

When a program calls fread on a file immediately after writing a small
amount of data, it may fail to flush the written data if the read size
is greater than or equal to the FILE buffer size.  Trouble is that
when we do fread directly after fwrite (where the former has not
flushed), we don't bother to switch to get mode right away and we
depend on __underflow to do that for us.  Fix is to explicitly switch
to get mode if we're currently putting.

Verified on x86_64 that there are no regressions due to this fix.

Siddhesh

	[BZ #18659]
	* libio/fileops.c (_IO_file_xsgetn): Switch to get mode if
	we're in put mode.
	* libio/tst-big-read-after-write.c: New test case.
	* libio/Makefile (tests): Use it.
---
 libio/Makefile                   |  2 +-
 libio/fileops.c                  |  7 ++++
 libio/tst-big-read-after-write.c | 91 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 libio/tst-big-read-after-write.c

diff --git a/libio/Makefile b/libio/Makefile
index 7b3bcf9..60a2341 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -61,7 +61,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	bug-memstream1 bug-wmemstream1 \
 	tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \
 	tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \
-	tst-ftell-append tst-fputws
+	tst-ftell-append tst-fputws tst-big-read-after-write
 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 cbcd6f5..3064f44 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -1375,6 +1375,13 @@ _IO_file_xsgetn (_IO_FILE *fp, void *data, _IO_size_t n)
       _IO_doallocbuf (fp);
     }
 
+  /* If we were in put mode, switch to get mode.  __underflow does that for us,
+     but only if the request sizes fit into the buffer.  For larger requests,
+     unbuffered requests go unnoticed.  */
+  if (_IO_in_put_mode (fp))
+    if (_IO_switch_to_get_mode (fp) == EOF)
+      return EOF;
+
   while (want > 0)
     {
       have = fp->_IO_read_end - fp->_IO_read_ptr;
diff --git a/libio/tst-big-read-after-write.c b/libio/tst-big-read-after-write.c
new file mode 100644
index 0000000..506b955
--- /dev/null
+++ b/libio/tst-big-read-after-write.c
@@ -0,0 +1,91 @@
+/* Verify that reads on large block sizes immediately after writes don't fail
+   to flush buffer and switch correctly to read mode.
+   Copyright (C) 2015 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 <locale.h>
+
+/* It is very unlikely that the block size (and hence buffer size) would be
+   equal to or greater than 64MB.  */
+#define BUF_LEN (64 * 1024 * 1024)
+char foo[BUF_LEN];
+static const char *data = "abcdefghijklmnopqrstuvwxyz";
+
+static int do_test (void);
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
+
+static int
+do_one_test (const char *filename, const char *mode)
+{
+  FILE *f;
+  size_t n;
+
+  f = fopen (filename, mode);
+  if (f == NULL)
+    {
+      printf ("Failed to open %s in %s mode\n", filename, mode);
+      return 1;
+    }
+
+  fwrite(data, 1, strlen (data), f);
+
+  n = fread(foo, 1, BUF_LEN, f);
+  if (n != 0)
+    {
+      printf ("fread before seek: expected %d but got %zu\n", 0, n);
+      return 1;
+    }
+
+  fseek(f, 0, SEEK_SET);
+  n = fread(foo, 1, BUF_LEN, f);
+  if (n != strlen (data))
+    {
+      printf ("fread after seek: expected %zu but got %zu\n", strlen (data), n);
+      return 1;
+    }
+
+  return 0;
+}
+
+static int
+do_test (void)
+{
+  int ret = 0;
+  char *filename;
+  int fd = create_temp_file ("tst-big-read-after-write-tmp.", &filename);
+
+  if (fd == -1)
+    {
+      printf ("create_temp_file: %m\n");
+      return 1;
+    }
+
+  close (fd);
+
+  ret |= do_one_test (filename, "a+");
+  ret |= do_one_test (filename, "r+");
+  ret |= do_one_test (filename, "w+");
+
+  return ret;
+}
-- 
2.4.3

Re: [PATCH] Also flush write when reading large amount of data (BZ #18659)

[Ondřej Bílka]

On Sat, Jul 11, 2015 at 01:26:20PM +0530, Siddhesh Poyarekar wrote:
> When a program calls fread on a file immediately after writing a small
> amount of data, it may fail to flush the written data if the read size
> is greater than or equal to the FILE buffer size.  Trouble is that
> when we do fread directly after fwrite (where the former has not
> flushed), we don't bother to switch to get mode right away and we
> depend on __underflow to do that for us.  Fix is to explicitly switch
> to get mode if we're currently putting.
> 
> Verified on x86_64 that there are no regressions due to this fix.
>
Patch looks, but you need to trivial style fix in testcase.
 +
> +  fwrite(data, 1, strlen (data), f);
> +
> +  n = fread(foo, 1, BUF_LEN, f);
space before ( here
> +  if (n != 0)
> +    {
> +      printf ("fread before seek: expected %d but got %zu\n", 0, n);
> +      return 1;
> +    }
> +
> +  fseek(f, 0, SEEK_SET);
> +  n = fread(foo, 1, BUF_LEN, f);
and here.

Re: [PATCH] Also flush write when reading large amount of data (BZ #18659)

[Andreas Schwab]

"Siddhesh Poyarekar" <siddhesh@redhat.com> writes:

> +  fwrite(data, 1, strlen (data), f);
> +
> +  n = fread(foo, 1, BUF_LEN, f);

That testcase is INVALID.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html

However, the application shall ensure that output is not directly
followed by input without an intervening call to fflush() or to a file
positioning function (fseek(), fsetpos(), or rewind()),

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Also flush write when reading large amount of data (BZ #18659)

[Siddhesh Poyarekar]

On 11 July 2015 at 13:47, Andreas Schwab <schwab@linux-m68k.org> wrote:
> That testcase is INVALID.
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html
>
> However, the application shall ensure that output is not directly
> followed by input without an intervening call to fflush() or to a file
> positioning function (fseek(), fsetpos(), or rewind()),

Ouch, of course, I don't know how I missed that.  I withdraw my patch
because an intervening fflush fixes the problem.

Thanks,
Siddhesh
-- 
http://siddhesh.in