Re: [PATCH] libio: Implement internal function __libc_readline_unlocked

[Florian Weimer <fweimer at redhat dot com>]

On 09/01/2017 08:00 PM, Florian Weimer wrote:
> This should use __underflow instead of __uflow, so that the need for the
> special handling for '\n' goes away.

Here is the version based on __underflow.  I changed the way the
__ftello64 and __fseeko64 aliases are handled because the other patch
needs to call __fseko64 from a DSO, so the hidden symbol no longer works.

Thanks,
Florian

libio: Implement internal function __libc_readline_unlocked

This is a variant of fgets which fails with ERANGE if the
buffer is too small, and the buffer length is given as an
argument of type size_t.

This function will be useful for implementing NSS file reading
operations.  Compared to a direct implementation using the public API,
it avoids an lseek system call in case the line terminator can be
found in the internal read buffer.

2017-09-02  Florian Weimer  <fweimer@redhat.com>

	* include/stdio.h (__libc_readline_unlocked): Declare.
	(__ftello64, __fseeko64): Declare aliases.
	* libio/readline.c: New file.
	* libio/tst-readline.c: Likewise.
	(routines): Add readline.
	(tests-internal): Add tst-readlime.
	* libio/Versions (GLIBC_PRIVATE): Export __fseeko64, __ftello64,
	__libc_readline_unlocked.
	* libio/fseeko.c (__fseeko): Rename from fseeko.
	(fseeko): Add alias.
	[__OFF_T_MATCHES_OFF64_T] (fseeko64, __fseeko64): Likewise.
	* libio/fseeko64.c (__fseeko64): Rename from fseeko64.
	(fseeko64): Add alias.
	* libio/ftello.c [__OFF_T_MATCHES_OFF64_T] (__ftello64): Add alias.
	* libio/ftello64.c (__ftello64): Rename from ftello64.
	(ftello64): Add alias.

diff --git a/include/stdio.h b/include/stdio.h
index 509447c528..50b4191279 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -122,6 +122,18 @@ extern int __fxprintf (FILE *__fp, const char *__fmt, ...)
 extern int __fxprintf_nocancel (FILE *__fp, const char *__fmt, ...)
      __attribute__ ((__format__ (__printf__, 2, 3)));
 
+/* Read the next line from FP into BUFFER, of LENGTH bytes.  LINE will
+   include the line terminator and a NUL terminator.  On success,
+   return the length of the line, including the line terminator, but
+   excluding the NUL termintor.  On EOF, return zero and write a NUL
+   terminator.  On error, return -1 and set errno.  If the total byte
+   count (line and both terminators) exceeds LENGTH, return -1 and set
+   errno to ERANGE (but do not mark the stream as failed).
+
+   The behavior is undefined if FP is not seekable.  */
+ssize_t __libc_readline_unlocked (_IO_FILE *fp, char *buffer, size_t length);
+libc_hidden_proto (__libc_readline_unlocked);
+
 extern const char *const _sys_errlist_internal[] attribute_hidden;
 extern int _sys_nerr_internal attribute_hidden;
 
@@ -161,6 +173,10 @@ libc_hidden_proto (fwrite)
 libc_hidden_proto (fseek)
 extern __typeof (ftello) __ftello;
 libc_hidden_proto (__ftello)
+extern __typeof (fseeko64) __fseeko64;
+libc_hidden_proto (__fseeko64)
+extern __typeof (ftello64) __ftello64;
+libc_hidden_proto (__ftello64)
 libc_hidden_proto (fflush)
 libc_hidden_proto (fflush_unlocked)
 extern __typeof (fflush_unlocked) __fflush_unlocked;
diff --git a/libio/Makefile b/libio/Makefile
index 9d09bd8b6a..85da7dc42f 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -47,7 +47,7 @@ routines	:=							      \
 	__fbufsize __freading __fwriting __freadable __fwritable __flbf	      \
 	__fpurge __fpending __fsetlocking				      \
 									      \
-	libc_fatal fmemopen oldfmemopen vtables
+	libc_fatal fmemopen oldfmemopen vtables readline
 
 tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst_wprintf2 tst-widetext test-fmemopen tst-ext tst-ext2 \
@@ -63,6 +63,9 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	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
+
+tests-internal = tst-readline
+
 ifeq (yes,$(build-shared))
 # Add test-fopenloc only if shared library is enabled since it depends on
 # shared localedata objects.
diff --git a/libio/Versions b/libio/Versions
index 77123347e3..acb896afa9 100644
--- a/libio/Versions
+++ b/libio/Versions
@@ -158,5 +158,9 @@ libc {
 
     # Used by NPTL
     _IO_enable_locks;
+
+    __fseeko64;
+    __ftello64;
+    __libc_readline_unlocked;
   }
 }
diff --git a/libio/fseeko.c b/libio/fseeko.c
index b77d4be18f..606a30366f 100644
--- a/libio/fseeko.c
+++ b/libio/fseeko.c
@@ -28,7 +28,7 @@
 #include "stdio.h"
 
 int
-fseeko (_IO_FILE *fp, off_t offset, int whence)
+__fseeko (_IO_FILE *fp, off_t offset, int whence)
 {
   int result;
   CHECK_FILE (fp, -1);
@@ -38,6 +38,10 @@ fseeko (_IO_FILE *fp, off_t offset, int whence)
   return result;
 }
 
+weak_alias (__fseeko, fseeko)
+
 #ifdef __OFF_T_MATCHES_OFF64_T
-weak_alias (fseeko, fseeko64)
+weak_alias (__fseeko, fseeko64)
+strong_alias (__fseeko, __fseeko64)
+libc_hidden_ver (__fseeko, __fseeko64)
 #endif
diff --git a/libio/fseeko64.c b/libio/fseeko64.c
index 12221d585e..fd612f4ed8 100644
--- a/libio/fseeko64.c
+++ b/libio/fseeko64.c
@@ -32,7 +32,7 @@
 #ifndef __OFF_T_MATCHES_OFF64_T
 
 int
-fseeko64 (_IO_FILE *fp, __off64_t offset, int whence)
+__fseeko64 (_IO_FILE *fp, __off64_t offset, int whence)
 {
   int result;
   CHECK_FILE (fp, -1);
@@ -42,4 +42,8 @@ fseeko64 (_IO_FILE *fp, __off64_t offset, int whence)
   return result;
 }
 
+libc_hidden_def (__fseeko64)
+
+weak_alias (__fseeko64, fseeko64)
+
 #endif
diff --git a/libio/ftello.c b/libio/ftello.c
index 9791e835c6..a11c54d63c 100644
--- a/libio/ftello.c
+++ b/libio/ftello.c
@@ -61,4 +61,6 @@ weak_alias (__ftello, ftello)
 
 #ifdef __OFF_T_MATCHES_OFF64_T
 weak_alias (__ftello, ftello64)
+strong_alias (__ftello, __ftello64)
+libc_hidden_ver (__ftello, __ftello64)
 #endif
diff --git a/libio/ftello64.c b/libio/ftello64.c
index 15518af7a6..afa0703f2e 100644
--- a/libio/ftello64.c
+++ b/libio/ftello64.c
@@ -32,7 +32,7 @@
 #ifndef __OFF_T_MATCHES_OFF64_T
 
 off64_t
-ftello64 (_IO_FILE *fp)
+__ftello64 (_IO_FILE *fp)
 {
   _IO_off64_t pos;
   CHECK_FILE (fp, -1L);
@@ -53,4 +53,8 @@ ftello64 (_IO_FILE *fp)
   return pos;
 }
 
+libc_hidden_def (__ftello64)
+
+weak_alias (__ftello64, ftello64)
+
 #endif
diff --git a/libio/readline.c b/libio/readline.c
new file mode 100644
index 0000000000..0d46844c1a
--- /dev/null
+++ b/libio/readline.c
@@ -0,0 +1,169 @@
+/* fgets with ERANGE error reporting and size_t buffer length.
+   Copyright (C) 2017 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 <assert.h>
+#include <errno.h>
+#include <stdio.h>
+#include <string.h>
+
+#include "libioP.h"
+
+/* Return -1 and set errno to EINVAL if it is ERANGE.  */
+static ssize_t
+fail_no_erange (void)
+{
+  if (errno == ERANGE)
+    __set_errno (EINVAL);
+  return -1;
+}
+
+/* Slow path for reading the line.  Called with no data in the stream
+   read buffer.  Write data to [BUFFER, BUFFER_END).  */
+static ssize_t
+readline_slow (_IO_FILE *fp, char *buffer, char *buffer_end)
+{
+  char *start = buffer;
+
+  while (buffer < buffer_end)
+    {
+      if (__underflow (fp) == EOF)
+        {
+          if (_IO_ferror_unlocked (fp))
+            /* If the EOF was caused by a read error, report it.  */
+            return fail_no_erange ();
+          *buffer = '\0';
+          /* Do not include the NULL terminator.  */
+          return buffer - start;
+        }
+
+      /* __underflow has filled the buffer.  */
+      char *readptr = fp->_IO_read_ptr;
+      ssize_t readlen = fp->_IO_read_end - readptr;
+      assert (readlen > 0);
+      char *pnl = memchr (readptr, '\n', readlen);
+      if (pnl != NULL)
+        {
+          /* We found the terminator.  */
+          size_t line_length = pnl - readptr;
+          if (line_length + 2 > buffer_end - buffer)
+            /* Not enough room in the caller-supplied buffer.  */
+            break;
+          memcpy (buffer, readptr, line_length + 1);
+          buffer[line_length + 1] = '\0';
+          fp->_IO_read_ptr = pnl + 1;
+          /* Do not include the NULL terminator.  */
+          return buffer - start + line_length + 1;
+        }
+
+      if (readlen >= buffer_end - buffer)
+        /* Not enough room in the caller-supplied buffer.  */
+        break;
+
+      /* Save and consume the stream buffer.  */
+      memcpy (buffer, readptr, readlen);
+      fp->_IO_read_ptr = fp->_IO_read_end;
+      buffer += readlen;
+    }
+
+  /* The line does not fit into the buffer.  */
+  __set_errno (ERANGE);
+  return -1;
+}
+
+ssize_t
+__libc_readline_unlocked (_IO_FILE *fp, char *buffer, size_t buffer_length)
+{
+  char *buffer_end = buffer + buffer_length;
+
+  /* Orient the stream.  */
+  if (__builtin_expect (fp->_mode, -1) == 0)
+    _IO_fwide (fp, -1);
+
+  /* Fast path: The line terminator is found in the buffer.  */
+  char *readptr = fp->_IO_read_ptr;
+  ssize_t readlen = fp->_IO_read_end - readptr;
+  off64_t start_offset;         /* File offset before reading anything.  */
+  if (readlen > 0)
+    {
+      char *pnl = memchr (readptr, '\n', readlen);
+      if (pnl != NULL)
+        {
+          size_t line_length = pnl - readptr;
+          /* Account for line and NULL terminators.  */
+          if (line_length + 2 > buffer_length)
+            {
+              __set_errno (ERANGE);
+              return -1;
+            }
+          memcpy (buffer, readptr, line_length + 1);
+          buffer[line_length + 1] = '\0';
+          /* Consume the entire line.  */
+          fp->_IO_read_ptr = pnl + 1;
+          return line_length + 1;
+        }
+
+      /* If the buffer does not have enough space for what is pending
+         in the stream (plus a NUL terminator), the buffer is too
+         small.  */
+      if (readlen + 1 > buffer_length)
+        {
+          __set_errno (ERANGE);
+          return -1;
+        }
+
+      /* End of line not found.  We need all the buffered data.  Fall
+         through to the slow path.  */
+      memcpy (buffer, readptr, readlen);
+      buffer += readlen;
+      /* The original length is invalid after this point.  Use
+         buffer_end instead.  */
+#pragma GCC poison buffer_length
+      /* Read the old offset before updating the read pointer.  */
+      start_offset = __ftello64 (fp);
+      fp->_IO_read_ptr = fp->_IO_read_end;
+    }
+  else
+    {
+      readlen = 0;
+      start_offset = __ftello64 (fp);
+    }
+
+  /* Slow path: Read more data from the underlying file.  We need to
+     restore the file pointer if the buffer is too small.  First,
+     check if the __ftello64 call above failed.  */
+  if (start_offset < 0)
+    return fail_no_erange ();
+
+  ssize_t result = readline_slow (fp, buffer, buffer_end);
+  if (result < 0)
+    {
+      if (errno == ERANGE)
+        {
+          /* Restore the file pointer so that the caller may read the
+             same line again.  */
+          if (__fseeko64 (fp, start_offset, SEEK_SET) < 0)
+            return fail_no_erange ();
+          __set_errno (ERANGE);
+        }
+      /* Do not restore the file position on other errors; it is
+         likely that the __fseeko64 call would fail, too.  */
+      return -1;
+    }
+  return readlen + result;
+}
+libc_hidden_def (__libc_readline_unlocked)
diff --git a/libio/tst-readline.c b/libio/tst-readline.c
new file mode 100644
index 0000000000..79ed6ea699
--- /dev/null
+++ b/libio/tst-readline.c
@@ -0,0 +1,235 @@
+/* Test the __libc_readline_unlocked function.
+   Copyright (C) 2017 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/>.  */
+
+/* Exercise __libc_readline_unlocked with various combinations of line
+   lengths, stdio buffer sizes, and line read buffer sizes.  */
+
+#include <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <support/check.h>
+#include <support/test-driver.h>
+#include <support/support.h>
+#include <support/xstdio.h>
+#include <support/xmemstream.h>
+#include <support/temp_file.h>
+#include <support/xunistd.h>
+
+enum
+  {
+    maximum_line_length = 7,
+    number_of_lines = 3,
+  };
+
+/* -1: Do not set buffer size.  0: unbuffered.  Otherwise, use this as
+   the size of the buffer.  */
+static int buffer_size;
+
+/* These size of the buffer used for reading.  Must be at least 2.  */
+static int read_size;
+
+/* If a read files with ERANGE, increase the buffer size by this
+   amount.  Must be positive.  */
+static int read_size_increment;
+
+/* If non-zero, do not reset the read size after an ERANGE error.  */
+static int read_size_preserve;
+
+/* If non-zero, no '\n' at the end of the file.  */
+static int no_newline_at_eof;
+
+/* Length of the line, or -1 if the line is not present.  */
+static int line_lengths[number_of_lines];
+
+/* The name of the test file.  */
+static char *test_file_path;
+
+/* The contents of the test file.  */
+static char expected_contents[(maximum_line_length + 2) * number_of_lines + 1];
+static size_t expected_length;
+
+/* Returns a random byte which is not zero or the line terminator.  */
+static char
+random_char (void)
+{
+  static unsigned int rand_state = 1;
+  while (true)
+    {
+      char result = rand_r (&rand_state) >> 16;
+      if (result != 0 && result != '\n')
+        return result;
+    }
+}
+
+/* Create the test file.  */
+static void
+prepare (int argc, char **argv)
+{
+  int fd = create_temp_file ("tst-readline-", &test_file_path);
+  TEST_VERIFY_EXIT (fd >= 0);
+  xclose (fd);
+}
+
+/* Prepare the test file.  Return false if the test parameters are
+   incongruent and the test should be skipped.  */
+static bool
+write_test_file (void)
+{
+  expected_length = 0;
+  char *p = expected_contents;
+  for (int lineno = 0; lineno < number_of_lines; ++lineno)
+    for (int i = 0; i < line_lengths[lineno]; ++i)
+      *p++ = random_char ();
+  expected_length = p - &expected_contents[0];
+  if (no_newline_at_eof)
+    {
+      if (expected_length == 0)
+        return false;
+      --expected_length;
+      --p;
+    }
+  if (test_verbose > 0)
+    {
+      printf ("info: writing test file of %zu bytes:\n", expected_length);
+      for (int i = 0; i < number_of_lines; ++i)
+        printf (" line %d: %d\n", i, line_lengths[i]);
+      if (no_newline_at_eof)
+        puts ("  (no newline at EOF)");
+    }
+  TEST_VERIFY_EXIT (expected_length < sizeof (expected_contents));
+  *p++ = '\0';
+  support_write_file_string (test_file_path, expected_contents);
+  return true;
+}
+
+/* Run a single test (a combination of a test file and read
+   parameters).  */
+static void
+run_test (void)
+{
+  TEST_VERIFY_EXIT (read_size_increment > 0);
+  if (test_verbose > 0)
+    {
+      printf ("info: running test: buffer_size=%d read_size=%d\n"
+              "  read_size_increment=%d read_size_preserve=%d\n",
+              buffer_size, read_size, read_size_increment, read_size_preserve);
+    }
+
+  struct xmemstream result;
+  xopen_memstream (&result);
+
+  FILE *fp = xfopen (test_file_path, "rce");
+  char *fp_buffer = NULL;
+  if (buffer_size == 0)
+    TEST_VERIFY_EXIT (setvbuf (fp, NULL, _IONBF, 0) == 0);
+  if (buffer_size > 0)
+    {
+      fp_buffer = xmalloc (buffer_size);
+      TEST_VERIFY_EXIT (setvbuf (fp, fp_buffer, _IOFBF, buffer_size) == 0);
+    }
+
+  char *line_buffer = xmalloc (read_size);
+  size_t line_buffer_size = read_size;
+
+  while (true)
+    {
+      ssize_t ret = __libc_readline_unlocked
+        (fp, line_buffer, line_buffer_size);
+      if (ret < 0)
+        {
+          TEST_VERIFY (ret == -1);
+          if (errno != ERANGE)
+            FAIL_EXIT1 ("__libc_readline_unlocked: %m");
+          line_buffer_size += read_size_increment;
+          free (line_buffer);
+          line_buffer = xmalloc (line_buffer_size);
+          /* Try reading this line again.  */
+        }
+      else if (ret == 0)
+        break;
+      else
+        {
+          /* A line has been read.  Save it.  */
+          TEST_VERIFY (ret == strlen (line_buffer));
+          const char *pnl = strchr (line_buffer, '\n');
+          /* If there is a \n, it must be at the end.  */
+          TEST_VERIFY (pnl == NULL || pnl == line_buffer + ret - 1);
+          fputs (line_buffer, result.out);
+
+          /* Restore the original read size if required.  */
+          if (line_buffer_size > read_size && !read_size_preserve)
+            {
+              line_buffer_size = read_size;
+              free (line_buffer);
+              line_buffer = xmalloc (line_buffer_size);
+            }
+        }
+    }
+
+  xfclose (fp);
+  free (fp_buffer);
+  free (line_buffer);
+
+  xfclose_memstream (&result);
+  TEST_VERIFY (result.length == expected_length);
+  TEST_VERIFY (strcmp (result.buffer, expected_contents) == 0);
+  if (test_verbose > 0)
+    {
+      printf ("info: expected (%zu): [[%s]]\n",
+              expected_length, expected_contents);
+      printf ("info:   actual (%zu): [[%s]]\n", result.length, result.buffer);
+    }
+  free (result.buffer);
+}
+
+/* Test one test file with multiple read parameters.  */
+static void
+test_one_file (void)
+{
+  for (buffer_size = -1; buffer_size <= maximum_line_length + 1; ++buffer_size)
+    for (read_size = 2; read_size <= maximum_line_length + 2; ++read_size)
+      for (read_size_increment = 1; read_size_increment <= 4;
+           ++read_size_increment)
+        for (read_size_preserve = 0; read_size_preserve < 2;
+             ++read_size_preserve)
+          run_test ();
+}
+
+
+static int
+do_test (void)
+{
+  /* Set up the test file contents.  */
+  for (line_lengths[0] = -1; line_lengths[0] <= maximum_line_length;
+       ++line_lengths[0])
+    for (line_lengths[1] = -1; line_lengths[1] <= maximum_line_length;
+         ++line_lengths[1])
+      for (line_lengths[2] = -1; line_lengths[2] <= maximum_line_length;
+           ++line_lengths[2])
+        for (no_newline_at_eof = 0; no_newline_at_eof < 2; ++no_newline_at_eof)
+          {
+            if (!write_test_file ())
+              continue;
+            test_one_file ();
+          }
+  free (test_file_path);
+  return 0;
+}
+
+#define PREPARE prepare
+#include <support/test-driver.c>