Re: [Bug-gnulib] invalid use of errno after ferror

[Paul Eggert]

Bruno Haible <address@hidden> writes:

> In lib/exclude.c one assumes that ferror() doesn't modify errno;
> this assumption is not valid in theory (strictly POSIX), but probably
> still valid in practice (since ferror() is just a bit test in most stdio
> implementations).

Isn't this also true for lib/mountlist.c?

I think it's a reasonable assumption.  Perhaps it's not entirely
portable in theory, but as you say it is valid in practice, and
perhaps this is more a bug in the standard rather than a portability
bug in the applications.

I looked at all instances of ferror in coreutils and have the
following proposed patch to fix the practical problems that I found.
This patch is not ideal (in some cases it "forgets" what's in errno,
and merely says "read error" or "write error") but at least it fixes
the bugs.

I installed the gnulib part of this patch into gnulib.

2003-09-15  Paul Eggert  <address@hidden>

        * lib/getndelim2.c (getndelim2): Don't trash errno when a read
        fails, so that the caller gets the proper errno.
        * lib/readutmp.c (read_utmp): Likewise.
        Check for fstat error.  Close stream and free storage
        when failing.

        Don't assume ferror sets errno.  Bug reported by Bruno Haible.
        * src/cksum.c (cksum): Don't assume ferror sets errno.
        * src/fold.c (fold_file): Likewise.
        * src/nl.c (nl_file): Likewise.
        * src/od.c (check_and_close): Likewise.
        * src/paste.c (paste_serial): Likewise.
        * src/tac.c (tac_file, save_stdin): Likewise.
        * src/tee.c (tee): Likewise.
        * src/unexpand.c (unexpand): Likewise.
        * src/uniq.c (check_file): Likewise.

        * src/tac.c (tac_seekable): Check for lseek error.
        (tac_mem): Don't return a value; nobody uses it.
        * src/yes.c (main): Exit immediately when write failure is
        detected.

Index: lib/getndelim2.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/lib/getndelim2.c,v
retrieving revision 1.2
diff -p -u -r1.2 getndelim2.c
--- lib/getndelim2.c    10 Sep 2003 08:42:04 -0000      1.2
+++ lib/getndelim2.c    15 Sep 2003 22:12:54 -0000
@@ -70,7 +70,7 @@ getndelim2 (char **lineptr, size_t *line
     {
       /* Here always *lineptr + *linesize == read_pos + nbytes_avail.  */
 
-      register int c = getc (stream);
+      register int c;
 
       /* We always want at least one char left in the buffer, since we
         always (unless we get an error while reading the first char)
@@ -95,7 +95,8 @@ getndelim2 (char **lineptr, size_t *line
            }
        }
 
-      if (c == EOF || ferror (stream))
+      c = getc (stream);
+      if (c == EOF)
        {
          /* Return partial line, if any.  */
          if (read_pos == *lineptr)
Index: lib/readutmp.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/lib/readutmp.c,v
retrieving revision 1.16
diff -p -u -r1.16 readutmp.c
--- lib/readutmp.c      11 Apr 2003 12:21:59 -0000      1.16
+++ lib/readutmp.c      15 Sep 2003 22:12:55 -0000
@@ -104,23 +104,33 @@ read_utmp (const char *filename, int *n_
   if (utmp == NULL)
     return 1;
 
-  fstat (fileno (utmp), &file_stats);
+  if (fstat (fileno (utmp), &file_stats) != 0)
+    {
+      int e = errno;
+      fclose (utmp);
+      errno = e;
+      return 1;
+    }
   size = file_stats.st_size;
-  if (size > 0)
-    buf = xmalloc (size);
-  else
+  buf = xmalloc (size);
+  n_read = fread (buf, sizeof *buf, size / sizeof *buf, utmp);
+  if (ferror (utmp))
     {
+      int e = errno;
+      free (buf);
       fclose (utmp);
+      errno = e;
+      return 1;
+    }
+  if (fclose (utmp) != 0)
+    {
+      int e = errno;
+      free (buf);
+      errno = e;
       return 1;
     }
 
-  /* Use < instead of != in case the utmp just grew.  */
-  n_read = fread (buf, 1, size, utmp);
-  if (ferror (utmp) || fclose (utmp) == EOF
-      || n_read < size)
-    return 1;
-
-  *n_entries = size / sizeof (STRUCT_UTMP);
+  *n_entries = n_read;
   *utmp_buf = buf;
 
   return 0;
Index: src/cksum.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/cksum.c,v
retrieving revision 1.63
diff -p -u -r1.63 cksum.c
--- src/cksum.c 23 Jul 2003 07:29:54 -0000      1.63
+++ src/cksum.c 15 Sep 2003 22:17:00 -0000
@@ -265,7 +265,7 @@ cksum (const char *file, int print_name)
     printf ("%u %s\n", (unsigned) crc, hp);
 
   if (ferror (stdout))
-    error (EXIT_FAILURE, errno, "-: %s", _("write error"));
+    error (EXIT_FAILURE, 0, "-: %s", _("write error"));
 
   return 0;
 }
Index: src/comm.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/comm.c,v
retrieving revision 1.67
diff -p -u -r1.67 comm.c
--- src/comm.c  23 Jul 2003 07:29:54 -0000      1.67
+++ src/comm.c  15 Sep 2003 22:17:00 -0000
@@ -209,7 +209,7 @@ compare_files (char **infiles)
       free (lb1[i].buffer);
       if (ferror (streams[i]) || fclose (streams[i]) == EOF)
        {
-         error (0, errno, "%s", infiles[i]);
+         error (0, 0, _("%s: read error"), infiles[i]);
          ret = 1;
        }
     }
Index: src/fold.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/fold.c,v
retrieving revision 1.71
diff -p -u -r1.71 fold.c
--- src/fold.c  11 Aug 2003 14:35:52 -0000      1.71
+++ src/fold.c  15 Sep 2003 22:17:00 -0000
@@ -126,6 +126,7 @@ fold_file (char *filename, int width)
   int offset_out = 0;  /* Index in `line_out' for next char. */
   static char *line_out = NULL;
   static int allocated_out = 0;
+  int saved_errno;
 
   if (STREQ (filename, "-"))
     {
@@ -209,12 +210,14 @@ fold_file (char *filename, int width)
       line_out[offset_out++] = c;
     }
 
+  saved_errno = errno;
+
   if (offset_out)
     fwrite (line_out, sizeof (char), (size_t) offset_out, stdout);
 
   if (ferror (istream))
     {
-      error (0, errno, "%s", filename);
+      error (0, saved_errno, "%s", filename);
       if (!STREQ (filename, "-"))
        fclose (istream);
       return 1;
Index: src/nl.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/nl.c,v
retrieving revision 1.74
diff -p -u -r1.74 nl.c
--- src/nl.c    23 Jul 2003 07:29:54 -0000      1.74
+++ src/nl.c    15 Sep 2003 22:17:00 -0000
@@ -447,7 +447,7 @@ nl_file (const char *file)
 
   if (ferror (stream))
     {
-      error (0, errno, "%s", file);
+      error (0, 0, _("%s: read error"), file);
       return 1;
     }
   if (STREQ (file, "-"))
Index: src/od.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/od.c,v
retrieving revision 1.137
diff -p -u -r1.137 od.c
--- src/od.c    23 Jul 2003 07:29:54 -0000      1.137
+++ src/od.c    15 Sep 2003 22:17:01 -0000
@@ -993,7 +993,7 @@ check_and_close (void)
     {
       if (ferror (in_stream))
        {
-         error (0, errno, "%s", input_filename);
+         error (0, 0, _("%s: read error"), input_filename);
          if (in_stream != stdin)
            fclose (in_stream);
          err = 1;
@@ -1009,7 +1009,7 @@ check_and_close (void)
 
   if (ferror (stdout))
     {
-      error (0, errno, _("standard output"));
+      error (0, 0, _("write error"));
       err = 1;
     }
 
Index: src/paste.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/paste.c,v
retrieving revision 1.62
diff -p -u -r1.62 paste.c
--- src/paste.c 27 Aug 2003 11:41:49 -0000      1.62
+++ src/paste.c 15 Sep 2003 22:17:01 -0000
@@ -325,6 +325,7 @@ paste_serial (int nfiles, char **fnamptr
   register int charnew, charold; /* Current and previous char read. */
   register char *delimptr;     /* Current delimiter char. */
   register FILE *fileptr;      /* Open for reading current file. */
+  int saved_errno;
 
   for (; nfiles; nfiles--, fnamptr++)
     {
@@ -347,6 +348,7 @@ paste_serial (int nfiles, char **fnamptr
       delimptr = delims;       /* Set up for delimiter string. */
 
       charold = getc (fileptr);
+      errno = saved_errno;
       if (charold != EOF)
        {
          /* `charold' is set up.  Hit it!
@@ -381,7 +383,7 @@ paste_serial (int nfiles, char **fnamptr
 
       if (ferror (fileptr))
        {
-         error (0, errno, "%s", *fnamptr);
+         error (0, saved_errno, "%s", *fnamptr);
          errors = 1;
        }
       if (fileptr == stdin)
Index: src/tac.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/tac.c,v
retrieving revision 1.102
diff -p -u -r1.102 tac.c
--- src/tac.c   23 Jul 2003 07:29:55 -0000      1.102
+++ src/tac.c   15 Sep 2003 22:17:01 -0000
@@ -314,7 +314,8 @@ tac_seekable (int input_fd, const char *
              read_size = file_pos;
              file_pos = 0;
            }
-         lseek (input_fd, file_pos, SEEK_SET);
+         if (lseek (input_fd, file_pos, SEEK_SET) < 0)
+           error (0, errno, _("%s: seek failed"), file);
 
          /* Shift the pending record data right to make room for the new.
             The source and destination regions probably overlap.  */
@@ -376,7 +377,7 @@ tac_file (const char *file)
     }
   SET_BINARY (fileno (in));
   errors = tac_seekable (fileno (in), file);
-  if (ferror (in) || fclose (in) == EOF)
+  if (fclose (in) == EOF)
     {
       error (0, errno, "%s", file);
       return 1;
@@ -452,10 +453,10 @@ save_stdin (FILE **g_tmp, char **g_tempf
        error (EXIT_FAILURE, errno, _("stdin: read error"));
 
       if (fwrite (G_buffer, 1, bytes_read, tmp) != bytes_read)
-       break;
+       error (EXIT_FAILURE, errno, "%s", tempfile);
     }
 
-  if (ferror (tmp) || fflush (tmp) == EOF)
+  if (fflush (tmp) == EOF)
     error (EXIT_FAILURE, errno, "%s", tempfile);
 
   SET_BINARY (fileno (tmp));
@@ -515,14 +516,14 @@ memrchr (const char *buf_start, const ch
 
 /* FIXME: describe */
 
-static int
+static void
 tac_mem (const char *buf, size_t n_bytes, FILE *out)
 {
   const char *nl;
   const char *bol;
 
   if (n_bytes == 0)
-    return 0;
+    return;
 
   nl = memrchr (buf, buf + n_bytes, '\n');
   bol = (nl == NULL ? buf : nl + 1);
@@ -555,7 +556,6 @@ tac_mem (const char *buf, size_t n_bytes
     fwrite (buf, 1, bol - buf, out);
 
   /* FIXME: this is work in progress.... */
-  return ferror (out);
 }
 
 /* FIXME: describe */
Index: src/tee.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/tee.c,v
retrieving revision 1.63
diff -p -u -r1.63 tee.c
--- src/tee.c   23 Jul 2003 07:29:55 -0000      1.63
+++ src/tee.c   15 Sep 2003 22:17:01 -0000
@@ -226,7 +226,7 @@ tee (int nfiles, const char **files)
     if (descriptors[i] != NULL
        && (ferror (descriptors[i]) || fclose (descriptors[i]) == EOF))
       {
-       error (0, errno, "%s", files[i]);
+       error (0, 0, _("%s: write error"), files[i]);
        ret = 1;
       }
 
Index: src/unexpand.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/unexpand.c,v
retrieving revision 1.75
diff -p -u -r1.75 unexpand.c
--- src/unexpand.c      23 Jul 2003 07:29:55 -0000      1.75
+++ src/unexpand.c      15 Sep 2003 22:17:02 -0000
@@ -235,6 +235,7 @@ unexpand (void)
   int next_tab_column;         /* Column the next tab stop is on. */
   int convert = 1;             /* If nonzero, perform translations. */
   unsigned int pending = 0;    /* Pending columns of blanks. */
+  int saved_errno;
 
   fp = next_file ((FILE *) NULL);
   if (fp == NULL)
@@ -246,6 +247,7 @@ unexpand (void)
   for (;;)
     {
       c = getc (fp);
+      saved_errno = errno;
 
       if (c == ' ' && convert && column < TAB_STOP_SENTINEL)
        {
@@ -325,6 +327,7 @@ unexpand (void)
 
          if (c == EOF)
            {
+             errno = saved_errno;
              fp = next_file (fp);
              if (fp == NULL)
                break;          /* No more files. */
Index: src/uniq.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/uniq.c,v
retrieving revision 1.104
diff -p -u -r1.104 uniq.c
--- src/uniq.c  23 Jul 2003 07:29:55 -0000      1.104
+++ src/uniq.c  15 Sep 2003 22:17:03 -0000
@@ -375,12 +375,12 @@ check_file (const char *infile, const ch
 
  closefiles:
   if (ferror (istream) || fclose (istream) == EOF)
-    error (EXIT_FAILURE, errno, _("error reading %s"), infile);
+    error (EXIT_FAILURE, 0, _("error reading %s"), infile);
 
   /* Close ostream only if it's not stdout -- the latter is closed
      via the atexit-invoked close_stdout.  */
   if (ostream != stdout && (ferror (ostream) || fclose (ostream) == EOF))
-    error (EXIT_FAILURE, errno, _("error writing %s"), outfile);
+    error (EXIT_FAILURE, 0, _("error writing %s"), outfile);
 
   free (lb1.buffer);
   free (lb2.buffer);
Index: src/yes.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/yes.c,v
retrieving revision 1.46
diff -p -u -r1.46 yes.c
--- src/yes.c   23 Jul 2003 07:29:55 -0000      1.46
+++ src/yes.c   15 Sep 2003 22:17:03 -0000
@@ -85,9 +85,8 @@ main (int argc, char **argv)
        {
          int i;
          for (i = 0; i < UNROLL; i++)
-           puts ("y");
-         if (ferror (stdout))
-           break;
+           if (puts ("y") == EOF)
+             goto write_error;
        }
     }
   else
@@ -100,15 +99,16 @@ main (int argc, char **argv)
              int j;
              for (j = 1; j < argc; j++)
                {
-                 fputs (argv[j], stdout);
-                 putchar (j == argc - 1 ? '\n' : ' ');
+                 if (fputs (argv[j], stdout) == EOF)
+                   goto write_error;
+                 if (putchar (j == argc - 1 ? '\n' : ' ') == EOF)
+                   goto write_error;
                }
            }
-         if (ferror (stdout))
-           break;
        }
     }
 
+ write_error:
   error (0, errno, _("standard output"));
   exit (EXIT_FAILURE);
 }