Re: a memory bug in stat.c

[Jim Meyering]

Guochun Shi <address@hidden> wrote:
> I ran "stat -c %s <filename>" and get segmentation fault. I found there is a 
> bug, here is a patch to fix it.
>
> diff -u stat.c.old stat.c
> --- stat.c.old  2003-03-22 16:32:02.000000000 -0600
> +++ stat.c      2005-03-11 16:51:37.000000000 -0600
> @@ -554,7 +554,7 @@
>    /* create a working copy of the format string */
>    char *format = xstrdup (masterformat);
>  
> -  char *dest = xmalloc (strlen (format) + 1);
> +  char *dest = xmalloc (strlen (format) + 3);
>  
>  
>    b = format;

Thank you for the report and patch.
I've applied it, along with the following separate patch that
should make it harder to introduce such bugs in the future.

2005-03-12  Jim Meyering  <address@hidden>

        Add a little infrastructure to help prevent future bugs like the
        one fixed below.
        * src/stat.c (xstrcat): New function.
        (print_statfs, print_stat): Add buf_len parameter and convert all
        uses of strcat to xstrcat.  Update callers.
        (print_it): Call print_func with buf_len parameter.

        Invoking stat -c FMT with a lone format directive of %s, %f, %h, %s,
        etc. could cause a buffer overrun error.
        * src/stat.c (print_it): Allocate 2 more bytes, to accommodate our
        conversion of the stat %s format string to the longer printf %llu one.
        Patch from Guochun Shi.

Index: src/stat.c
===================================================================
RCS file: /fetish/cu/src/stat.c,v
retrieving revision 1.81
retrieving revision 1.82
diff -u -p -u -r1.81 -r1.82
--- src/stat.c  12 Mar 2005 10:54:20 -0000      1.81
+++ src/stat.c  12 Mar 2005 10:59:23 -0000      1.82
@@ -295,9 +295,22 @@ human_time (time_t t, int t_ns)
   return str;
 }
 
+/* Like strcat, but don't return anything and do check that
+   DEST_BUFSIZE is at least a long as strlen (DEST) + strlen (SRC) + 1.
+   The signature is deliberately different from that of strncat.  */
+static void
+xstrcat (char *dest, size_t dest_bufsize, char const *src)
+{
+  size_t dest_len = strlen (dest);
+  size_t src_len = strlen (src);
+  if (dest_bufsize < dest_len + src_len + 1)
+    abort ();
+  memcpy (dest + dest_len, src, src_len + 1);
+}
+
 /* print statfs info */
 static void
-print_statfs (char *pformat, char m, char const *filename,
+print_statfs (char *pformat, size_t buf_len, char m, char const *filename,
              void const *data)
 {
   STRUCT_STATVFS const *statfsbuf = data;
@@ -305,28 +318,28 @@ print_statfs (char *pformat, char m, cha
   switch (m)
     {
     case 'n':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       printf (pformat, filename);
       break;
 
     case 'i':
 #if HAVE_STRUCT_STATXFS_F_FSID___VAL
-      strcat (pformat, "x %-8x");
+      xstrcat (pformat, buf_len, "x %-8x");
       printf (pformat, statfsbuf->f_fsid.__val[0], /* u_long */
              statfsbuf->f_fsid.__val[1]);
 #else
-      strcat (pformat, "Lx");
+      xstrcat (pformat, buf_len, "Lx");
       printf (pformat, statfsbuf->f_fsid);
 #endif
       break;
 
     case 'l':
-      strcat (pformat, NAMEMAX_FORMAT);
+      xstrcat (pformat, buf_len, NAMEMAX_FORMAT);
       printf (pformat, SB_F_NAMEMAX (statfsbuf));
       break;
     case 't':
 #if HAVE_STRUCT_STATXFS_F_TYPE
-      strcat (pformat, "lx");
+      xstrcat (pformat, buf_len, "lx");
       printf (pformat,
              (unsigned long int) (statfsbuf->f_type));  /* no equiv. */
 #else
@@ -334,23 +347,23 @@ print_statfs (char *pformat, char m, cha
 #endif
       break;
     case 'T':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       printf (pformat, human_fstype (statfsbuf));
       break;
     case 'b':
-      strcat (pformat, PRIdMAX);
+      xstrcat (pformat, buf_len, PRIdMAX);
       printf (pformat, (intmax_t) (statfsbuf->f_blocks));
       break;
     case 'f':
-      strcat (pformat, PRIdMAX);
+      xstrcat (pformat, buf_len, PRIdMAX);
       printf (pformat, (intmax_t) (statfsbuf->f_bfree));
       break;
     case 'a':
-      strcat (pformat, PRIdMAX);
+      xstrcat (pformat, buf_len, PRIdMAX);
       printf (pformat, (intmax_t) (statfsbuf->f_bavail));
       break;
     case 's':
-      strcat (pformat, "lu");
+      xstrcat (pformat, buf_len, "lu");
       printf (pformat, (unsigned long int) (statfsbuf->f_bsize));
       break;
     case 'S':
@@ -358,21 +371,21 @@ print_statfs (char *pformat, char m, cha
        unsigned long int frsize = STATFS_FRSIZE (statfsbuf);
        if (! frsize)
          frsize = statfsbuf->f_bsize;
-       strcat (pformat, "lu");
+       xstrcat (pformat, buf_len, "lu");
        printf (pformat, frsize);
       }
       break;
     case 'c':
-      strcat (pformat, PRIdMAX);
+      xstrcat (pformat, buf_len, PRIdMAX);
       printf (pformat, (intmax_t) (statfsbuf->f_files));
       break;
     case 'd':
-      strcat (pformat, PRIdMAX);
+      xstrcat (pformat, buf_len, PRIdMAX);
       printf (pformat, (intmax_t) (statfsbuf->f_ffree));
       break;
 
     default:
-      strcat (pformat, "c");
+      xstrcat (pformat, buf_len, "c");
       printf (pformat, m);
       break;
     }
@@ -380,7 +393,8 @@ print_statfs (char *pformat, char m, cha
 
 /* print stat info */
 static void
-print_stat (char *pformat, char m, char const *filename, void const *data)
+print_stat (char *pformat, size_t buf_len, char m,
+           char const *filename, void const *data)
 {
   struct stat *statbuf = (struct stat *) data;
   struct passwd *pw_ent;
@@ -389,11 +403,11 @@ print_stat (char *pformat, char m, char 
   switch (m)
     {
     case 'n':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       printf (pformat, filename);
       break;
     case 'N':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       if (S_ISLNK (statbuf->st_mode))
        {
          char *linkname = xreadlink (filename, statbuf->st_size);
@@ -414,111 +428,111 @@ print_stat (char *pformat, char m, char 
        }
       break;
     case 'd':
-      strcat (pformat, PRIuMAX);
+      xstrcat (pformat, buf_len, PRIuMAX);
       printf (pformat, (uintmax_t) statbuf->st_dev);
       break;
     case 'D':
-      strcat (pformat, PRIxMAX);
+      xstrcat (pformat, buf_len, PRIxMAX);
       printf (pformat, (uintmax_t) statbuf->st_dev);
       break;
     case 'i':
-      strcat (pformat, PRIuMAX);
+      xstrcat (pformat, buf_len, PRIuMAX);
       printf (pformat, (uintmax_t) statbuf->st_ino);
       break;
     case 'a':
-      strcat (pformat, "lo");
+      xstrcat (pformat, buf_len, "lo");
       printf (pformat,
              (unsigned long int) (statbuf->st_mode & CHMOD_MODE_BITS));
       break;
     case 'A':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       printf (pformat, human_access (statbuf));
       break;
     case 'f':
-      strcat (pformat, "lx");
+      xstrcat (pformat, buf_len, "lx");
       printf (pformat, (unsigned long int) statbuf->st_mode);
       break;
     case 'F':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       printf (pformat, file_type (statbuf));
       break;
     case 'h':
-      strcat (pformat, "lu");
+      xstrcat (pformat, buf_len, "lu");
       printf (pformat, (unsigned long int) statbuf->st_nlink);
       break;
     case 'u':
-      strcat (pformat, "lu");
+      xstrcat (pformat, buf_len, "lu");
       printf (pformat, (unsigned long int) statbuf->st_uid);
       break;
     case 'U':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       setpwent ();
       pw_ent = getpwuid (statbuf->st_uid);
       printf (pformat, (pw_ent != 0L) ? pw_ent->pw_name : "UNKNOWN");
       break;
     case 'g':
-      strcat (pformat, "lu");
+      xstrcat (pformat, buf_len, "lu");
       printf (pformat, (unsigned long int) statbuf->st_gid);
       break;
     case 'G':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       setgrent ();
       gw_ent = getgrgid (statbuf->st_gid);
       printf (pformat, (gw_ent != 0L) ? gw_ent->gr_name : "UNKNOWN");
       break;
     case 't':
-      strcat (pformat, "lx");
+      xstrcat (pformat, buf_len, "lx");
       printf (pformat, (unsigned long int) major (statbuf->st_rdev));
       break;
     case 'T':
-      strcat (pformat, "lx");
+      xstrcat (pformat, buf_len, "lx");
       printf (pformat, (unsigned long int) minor (statbuf->st_rdev));
       break;
     case 's':
-      strcat (pformat, PRIuMAX);
+      xstrcat (pformat, buf_len, PRIuMAX);
       printf (pformat, (uintmax_t) (statbuf->st_size));
       break;
     case 'B':
-      strcat (pformat, "lu");
+      xstrcat (pformat, buf_len, "lu");
       printf (pformat, (unsigned long int) ST_NBLOCKSIZE);
       break;
     case 'b':
-      strcat (pformat, PRIuMAX);
+      xstrcat (pformat, buf_len, PRIuMAX);
       printf (pformat, (uintmax_t) ST_NBLOCKS (*statbuf));
       break;
     case 'o':
-      strcat (pformat, "lu");
+      xstrcat (pformat, buf_len, "lu");
       printf (pformat, (unsigned long int) statbuf->st_blksize);
       break;
     case 'x':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       printf (pformat, human_time (statbuf->st_atime,
                                   TIMESPEC_NS (statbuf->st_atim)));
       break;
     case 'X':
-      strcat (pformat, TYPE_SIGNED (time_t) ? "ld" : "lu");
+      xstrcat (pformat, buf_len, TYPE_SIGNED (time_t) ? "ld" : "lu");
       printf (pformat, (unsigned long int) statbuf->st_atime);
       break;
     case 'y':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       printf (pformat, human_time (statbuf->st_mtime,
                                   TIMESPEC_NS (statbuf->st_mtim)));
       break;
     case 'Y':
-      strcat (pformat, TYPE_SIGNED (time_t) ? "ld" : "lu");
+      xstrcat (pformat, buf_len, TYPE_SIGNED (time_t) ? "ld" : "lu");
       printf (pformat, (unsigned long int) statbuf->st_mtime);
       break;
     case 'z':
-      strcat (pformat, "s");
+      xstrcat (pformat, buf_len, "s");
       printf (pformat, human_time (statbuf->st_ctime,
                                   TIMESPEC_NS (statbuf->st_ctim)));
       break;
     case 'Z':
-      strcat (pformat, TYPE_SIGNED (time_t) ? "ld" : "lu");
+      xstrcat (pformat, buf_len, TYPE_SIGNED (time_t) ? "ld" : "lu");
       printf (pformat, (unsigned long int) statbuf->st_ctime);
       break;
     default:
-      strcat (pformat, "c");
+      xstrcat (pformat, buf_len, "c");
       printf (pformat, m);
       break;
     }
@@ -526,7 +540,7 @@ print_stat (char *pformat, char m, char 
 
 static void
 print_it (char const *masterformat, char const *filename,
-         void (*print_func) (char *, char, char const *, void const *),
+         void (*print_func) (char *, size_t, char, char const *, void const *),
          void const *data)
 {
   char *b;
@@ -534,7 +548,10 @@ print_it (char const *masterformat, char
   /* create a working copy of the format string */
   char *format = xstrdup (masterformat);
 
-  char *dest = xmalloc (strlen (format) + 2 + 1);
+  /* Add 2 to accommodate our conversion of the stat `%s' format string
+     to the printf `%llu' one.  */
+  size_t n_alloc = strlen (format) + 2 + 1;
+  char *dest = xmalloc (n_alloc);
 
   b = format;
   while (b)
@@ -562,7 +579,7 @@ print_it (char const *masterformat, char
              putchar ('%');
              break;
            default:
-             print_func (dest, *p, filename, data);
+             print_func (dest, n_alloc, *p, filename, data);
              break;
            }
        }