Re: Coreutils 5.0.91: Fix crash in head/tail

[Paul Eggert]

Andreas Schwab <address@hidden> writes:

> tail crashes on read failures, like trying to read from a directory.
> head has the same bug, but it is harder to trigger.

Thanks for catching that.  Here's a proposed improvement on your
patch, which avoids an unnecessary assignment to tmp->nbytes.

I noticed that the utilities are inconsistent about using BUFSIZ, and
that the use of 'unsigned int' and 'int' for sizes is a bit confusing,
so this patch fixes those neighboring problems too.

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

        Fix a bug reported by Andreas Schwab in
        <http://mail.gnu.org/archive/html/bug-coreutils/2003-09/msg00009.html>.
        * src/head.c (elide_tail_lines_pipe): Don't assign 0 or
        SAFE_READ_ERROR to tmp->nbytes.
        * src/tail.c (pipe_lines, pipe_bytes): Likewise.

        * src/head.c (struct linebuffer): Change nbytes and nlines
        from unsigned int to size_t.  unsigned int is safe (after the
        changes noted above) but size_t is cleaner.
        * src/tail.c (struct linebuffer, struct charbuffer): Likewise.
        (pipe_bytes): Likewise for local variable 'i', which was 'int'.

        Standardize on BUFSIZ as opposed to other macro names and values.
        * src/head.c (BUFSIZE): Remove.  All uses changed to BUFSIZ.
        * src/tail.c (BUFSIZ) [!defined BUFSIZ]: Remove.
        stdio.h has always defined it,
        and other code already assumes it's defined.
        * src/tr.c (BUFSIZ) [!defined BUFSIZ]: Likewise.
        (IO_BUF_SIZE): Remove; replace all uses with sizeof io_buf.
        (io_buf): IO_BUF_SIZE -> BUFSIZ.

Index: src/head.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/head.c,v
retrieving revision 1.86
diff -p -u -r1.86 head.c
--- src/head.c  23 Jul 2003 07:29:54 -0000      1.86
+++ src/head.c  4 Sep 2003 19:58:24 -0000
@@ -49,9 +49,6 @@
 /* Number of lines/chars/blocks to head. */
 #define DEFAULT_NUMBER 10
 
-/* Size of atomic reads. */
-#define BUFSIZE (512 * 8)
-
 /* Useful only when eliding tail bytes or lines.
    If nonzero, skip the is-regular-file test used to determine whether
    to use the lseek optimization.  Instead, use the more general (and
@@ -183,7 +180,7 @@ write_header (const char *filename)
 static enum Copy_fd_status
 copy_fd (int src_fd, FILE *o_stream, uintmax_t n_bytes)
 {
-  char buf[BUFSIZE];
+  char buf[BUFSIZ];
   const size_t buf_size = sizeof (buf);
 
   /* Copy the file contents.  */
@@ -215,7 +212,7 @@ elide_tail_bytes_pipe (const char *filen
   size_t n_elide = n_elide_0;
 
 #ifndef HEAD_TAIL_PIPE_READ_BUFSIZE
-# define HEAD_TAIL_PIPE_READ_BUFSIZE BUFSIZE
+# define HEAD_TAIL_PIPE_READ_BUFSIZE BUFSIZ
 #endif
 #define READ_BUFSIZE HEAD_TAIL_PIPE_READ_BUFSIZE
 
@@ -476,8 +473,9 @@ elide_tail_lines_pipe (const char *filen
 {
   struct linebuffer
   {
-    unsigned int nbytes, nlines;
     char buffer[BUFSIZ];
+    size_t nbytes;
+    size_t nlines;
     struct linebuffer *next;
   };
   typedef struct linebuffer LBUFFER;
@@ -496,9 +494,10 @@ elide_tail_lines_pipe (const char *filen
      n_elide newlines, or until EOF, whichever comes first.  */
   while (1)
     {
-      n_read = tmp->nbytes = safe_read (fd, tmp->buffer, BUFSIZ);
+      n_read = safe_read (fd, tmp->buffer, BUFSIZ);
       if (n_read == 0 || n_read == SAFE_READ_ERROR)
        break;
+      tmp->nbytes = n_read;
       tmp->nlines = 0;
       tmp->next = NULL;
 
@@ -752,8 +751,8 @@ elide_tail_lines_file (const char *filen
 static int
 head_bytes (const char *filename, int fd, uintmax_t bytes_to_write)
 {
-  char buffer[BUFSIZE];
-  size_t bytes_to_read = BUFSIZE;
+  char buffer[BUFSIZ];
+  size_t bytes_to_read = BUFSIZ;
 
   /* Need BINARY I/O for the byte counts to be accurate.  */
   SET_BINARY2 (fd, fileno (stdout));
@@ -781,7 +780,7 @@ head_bytes (const char *filename, int fd
 static int
 head_lines (const char *filename, int fd, uintmax_t lines_to_write)
 {
-  char buffer[BUFSIZE];
+  char buffer[BUFSIZ];
 
   /* Need BINARY I/O for the byte counts to be accurate.  */
   /* FIXME: do we really need this when counting *lines*?  */
@@ -789,7 +788,7 @@ head_lines (const char *filename, int fd
 
   while (lines_to_write)
     {
-      size_t bytes_read = safe_read (fd, buffer, BUFSIZE);
+      size_t bytes_read = safe_read (fd, buffer, BUFSIZ);
       size_t bytes_to_write = 0;
 
       if (bytes_read == SAFE_READ_ERROR)
Index: src/tail.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/tail.c,v
retrieving revision 1.209
diff -p -u -r1.209 tail.c
--- src/tail.c  16 Aug 2003 17:34:41 -0000      1.209
+++ src/tail.c  4 Sep 2003 19:58:25 -0000
@@ -57,11 +57,6 @@
 /* Number of items to tail.  */
 #define DEFAULT_N_LINES 10
 
-/* Size of atomic reads.  */
-#ifndef BUFSIZ
-# define BUFSIZ (512 * 8)
-#endif
-
 /* A special value for dump_remainder's N_BYTES parameter.  */
 #define COPY_TO_EOF OFF_T_MAX
 
@@ -518,8 +513,9 @@ pipe_lines (const char *pretty_filename,
 {
   struct linebuffer
   {
-    unsigned int nbytes, nlines;
     char buffer[BUFSIZ];
+    size_t nbytes;
+    size_t nlines;
     struct linebuffer *next;
   };
   typedef struct linebuffer LBUFFER;
@@ -536,10 +532,11 @@ pipe_lines (const char *pretty_filename,
   /* Input is always read into a fresh buffer.  */
   while (1)
     {
-      n_read = tmp->nbytes = safe_read (fd, tmp->buffer, BUFSIZ);
+      n_read = safe_read (fd, tmp->buffer, BUFSIZ);
       if (n_read == 0 || n_read == SAFE_READ_ERROR)
        break;
       *read_pos += n_read;
+      tmp->nbytes = n_read;
       tmp->nlines = 0;
       tmp->next = NULL;
 
@@ -655,13 +652,13 @@ pipe_bytes (const char *pretty_filename,
 {
   struct charbuffer
   {
-    unsigned int nbytes;
     char buffer[BUFSIZ];
+    size_t nbytes;
     struct charbuffer *next;
   };
   typedef struct charbuffer CBUFFER;
   CBUFFER *first, *last, *tmp;
-  int i;                       /* Index into buffers.  */
+  size_t i;                    /* Index into buffers.  */
   size_t total_bytes = 0;      /* Total characters in all buffers.  */
   int errors = 0;
   size_t n_read;
@@ -675,11 +672,11 @@ pipe_bytes (const char *pretty_filename,
   while (1)
     {
       n_read = safe_read (fd, tmp->buffer, BUFSIZ);
-      tmp->nbytes = n_read;
-      tmp->next = NULL;
       if (n_read == 0 || n_read == SAFE_READ_ERROR)
        break;
       *read_pos += n_read;
+      tmp->nbytes = n_read;
+      tmp->next = NULL;
 
       total_bytes += tmp->nbytes;
       /* If there is enough room in the last buffer read, just append the new
Index: src/tr.c
===================================================================
RCS file: /cvsroot/coreutils/coreutils/src/tr.c,v
retrieving revision 1.120
diff -p -u -r1.120 tr.c
--- src/tr.c    23 Jul 2003 07:29:55 -0000      1.120
+++ src/tr.c    4 Sep 2003 19:58:25 -0000
@@ -270,12 +270,7 @@ static int truncate_set1 = 0;
    It is set in main and used there and in validate().  */
 static int translating;
 
-#ifndef BUFSIZ
-# define BUFSIZ 8192
-#endif
-
-#define IO_BUF_SIZE BUFSIZ
-static unsigned char io_buf[IO_BUF_SIZE];
+static unsigned char io_buf[BUFSIZ];
 
 static char const *const char_class_name[] =
 {
@@ -1897,7 +1892,7 @@ without squeezing repeats"));
   if (squeeze_repeats && non_option_args == 1)
     {
       set_initialize (s1, complement, in_squeeze_set);
-      squeeze_filter (io_buf, IO_BUF_SIZE, NULL);
+      squeeze_filter (io_buf, sizeof io_buf, NULL);
     }
   else if (delete && non_option_args == 1)
     {
@@ -1906,7 +1901,7 @@ without squeezing repeats"));
       set_initialize (s1, complement, in_delete_set);
       do
        {
-         nr = read_and_delete (io_buf, IO_BUF_SIZE, NULL);
+         nr = read_and_delete (io_buf, sizeof io_buf, NULL);
          if (nr > 0 && fwrite ((char *) io_buf, 1, nr, stdout) == 0)
            error (EXIT_FAILURE, errno, _("write error"));
        }
@@ -1916,7 +1911,7 @@ without squeezing repeats"));
     {
       set_initialize (s1, complement, in_delete_set);
       set_initialize (s2, 0, in_squeeze_set);
-      squeeze_filter (io_buf, IO_BUF_SIZE, read_and_delete);
+      squeeze_filter (io_buf, sizeof io_buf, read_and_delete);
     }
   else if (translating)
     {
@@ -2005,7 +2000,7 @@ construct in string1 must be aligned wit
       if (squeeze_repeats)
        {
          set_initialize (s2, 0, in_squeeze_set);
-         squeeze_filter (io_buf, IO_BUF_SIZE, read_and_xlate);
+         squeeze_filter (io_buf, sizeof io_buf, read_and_xlate);
        }
       else
        {
@@ -2013,7 +2008,7 @@ construct in string1 must be aligned wit
 
          do
            {
-             bytes_read = read_and_xlate (io_buf, IO_BUF_SIZE, NULL);
+             bytes_read = read_and_xlate (io_buf, sizeof io_buf, NULL);
              if (bytes_read > 0
                  && fwrite ((char *) io_buf, 1, bytes_read, stdout) == 0)
                error (EXIT_FAILURE, errno, _("write error"));