[PATCH v3] libio: null terminate the buffer upon initial allocation in getdelim

[Collin Funk]

Commit 33eff78c8b28adc4963987880e10d96761f2a167 caused issues in nbdkit
which had code similar to this to get the last line of the file:

    while (getline (&line, &len, fp) != -1)
      ;
    /* Process LINE.  */

After that commit, line[0] would be equal to '\0' instead of containing
the last line of the file like before that commit. A recent POSIX issue
clarified that the behavior before and after that commit are allowed,
since the contents of LINE are unspecified after -1 is returned
[1]. However, some programs rely on the previous behavior.

This patch null terminates the buffer upon getdelim/getline's initial
allocation. This is compatible with previous glibc versions, while also
protecting the caller from reading uninitialized memory if the file is
empty, as long as getline/getdelim does the initial allocation.

[1] https://www.austingroupbugs.net/bug_view_page.php?bug_id=1953

Suggested-by: Eric Blake <eblake@redhat.com>
---
 libio/iogetdelim.c   |  7 +++++--
 libio/tst-getdelim.c | 47 +++++++++++++++++++++++++++++++++++++-------
 manual/stdio.texi    |  7 ++++++-
 3 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/libio/iogetdelim.c b/libio/iogetdelim.c
index 1d89757352..8528e2a01e 100644
--- a/libio/iogetdelim.c
+++ b/libio/iogetdelim.c
@@ -63,7 +63,11 @@ __getdelim (char **lineptr, size_t *n, int delimiter, FILE *fp)
     {
       *n = 120;
       *lineptr = (char *) malloc (*n);
-      if (*lineptr == NULL)
+      /* Null terminate the buffer upon allocation otherwise it will not be
+         null-terminated upon reading from an empty file.  */
+      if (*lineptr != NULL)
+        (*lineptr)[0] = '\0';
+      else
 	{
 	  fseterr_unlocked (fp);
 	  result = -1;
@@ -77,7 +81,6 @@ __getdelim (char **lineptr, size_t *n, int delimiter, FILE *fp)
       if (__underflow (fp) == EOF)
 	{
 	  result = -1;
-	  (*lineptr)[0] = '\0';
 	  goto unlock_return;
 	}
       len = fp->_IO_read_end - fp->_IO_read_ptr;
diff --git a/libio/tst-getdelim.c b/libio/tst-getdelim.c
index 556697453c..4d642a90eb 100644
--- a/libio/tst-getdelim.c
+++ b/libio/tst-getdelim.c
@@ -22,6 +22,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <errno.h>
+#include <string.h>
 
 #include <support/check.h>
 #include <support/support.h>
@@ -51,20 +52,52 @@ do_test (void)
   xfclose (memstream);
   free (lineptr);
 
-  /* Test that getdelim NUL terminates upon reading an EOF from an empty
-     file (BZ #28038).  This test fails on glibc 2.42 and earlier.  */
-  lineptr = xmalloc (1);
-  lineptr[0] = 'A';
-  linelen = 1;
+  /* Test that we null-terminate the buffer upon allocating it (BZ #28038).  */
+  lineptr = NULL;
+  linelen = 0;
   char *file_name;
   TEST_VERIFY_EXIT (create_temp_file ("tst-getdelim.", &file_name) != -1);
-  FILE *fp = fopen (file_name, "r");
+  FILE *fp = fopen (file_name, "w+");
+  free (file_name);
   TEST_VERIFY_EXIT (fp != NULL);
   TEST_VERIFY (getdelim (&lineptr, &linelen, '\n', fp) == -1);
+  TEST_VERIFY (feof (fp));
   TEST_VERIFY (linelen > 0);
   TEST_VERIFY (lineptr[0] == '\0');
+  free (lineptr);
+
+  /* Test that we can read until -1 is returned and then access the last line
+     of the file.  This behavior was broken by commit
+     33eff78c8b28adc4963987880e10d96761f2a167 and later fixed.  */
+  lineptr = NULL;
+  linelen = 0;
+  TEST_VERIFY_EXIT (fwrite ("a\nb\nc\n", 1, sizeof "a\nb\nc\n" - 1, fp)
+                    == sizeof "a\nb\nc\n" - 1);
+  TEST_VERIFY_EXIT (0 <= fseeko (fp, 0, SEEK_SET));
+  char expect[] = { 'a' - 1, '\n', '\0' };
+  for (int i = 0; i < 3; ++i)
+    {
+      ++expect[0];
+      TEST_VERIFY (getdelim (&lineptr, &linelen, '\n', fp) == 2);
+      TEST_VERIFY (linelen > 0);
+      TEST_VERIFY (strcmp (lineptr, expect) == 0);
+    }
+  TEST_VERIFY (getdelim (&lineptr, &linelen, '\n', fp) == -1);
+  TEST_VERIFY (feof (fp));
+  TEST_VERIFY (linelen > 0);
+  TEST_VERIFY (strcmp (lineptr, expect) == 0);
+
+  /* Test the same thing without a newline.  */
+  TEST_VERIFY_EXIT (fwrite ("d", 1, 1, fp) == 1);
+  TEST_VERIFY_EXIT (0 <= fseeko (fp, -1, SEEK_CUR));
+  TEST_VERIFY (getdelim (&lineptr, &linelen, '\n', fp) == 1);
+  TEST_VERIFY (linelen > 0);
+  TEST_VERIFY (strcmp (lineptr, "d") == 0);
+  TEST_VERIFY (getdelim (&lineptr, &linelen, '\n', fp) == -1);
+  TEST_VERIFY (feof (fp));
+  TEST_VERIFY (linelen > 0);
+  TEST_VERIFY (strcmp (lineptr, "d") == 0);
   fclose (fp);
-  free (file_name);
   free (lineptr);
 
   return 0;
diff --git a/manual/stdio.texi b/manual/stdio.texi
index e8f60b09c1..c01feaed7d 100644
--- a/manual/stdio.texi
+++ b/manual/stdio.texi
@@ -1279,7 +1279,12 @@ @node Line Input
 POSIX.1-2008.
 
 If an error occurs or end of file is reached without any bytes read,
-@code{getline} returns @code{-1}.
+@code{getline} returns @code{-1}.  POSIX leaves the contents of
+@code{*@var{lineptr}} undefined when @code{getline} returns @code{-1}.
+If the @glibcadj{} implementation of @code{getline} allocates the
+initial buffer it will null terminate it to prevent the caller from
+reading uninitialized memory if no characters can be read from
+@code{stream}.
 @end deftypefun
 
 @deftypefun ssize_t getdelim (char **restrict @var{lineptr}, size_t *restrict @var{n}, int @var{delimiter}, FILE *restrict @var{stream})
-- 
2.52.0