coreutils readlink buffer overflow on GNU/Hurd, plus speedup

[Paul Eggert]

While auditing coreutils I noticed a potential buffer overrun in
copy.c on hosts like GNU/Hurd where symbolic links have potentially
unlimited length.  Here's a patch.  As a nice side effect, this patch
improves the performance of programs on 'ls' on more-ordinary hosts
when they are given long symlinks, as it causes readlink to be invoked
once, rather than twice, when the symlink is long.

2004-05-25  Paul Eggert  <address@hidden>

        Improve the efficiency (and in one case, correctness) of code
        that reads symlinks.

        * lib/xreadlink.c: Include xreadlink.h first, to catch .h file
        dependency problems.
        (xreadlink): Accept new arg SIZE, for efficiency.
        All decls and uses changed.
        * lib/xreadlink.h: Include <stddef.h>, for size_t.
        * src/copy.c (copy_internal): Don't use alloca, as it can mess up
        royally if the link length is long (e.g., GNU/Hurd).  Use
        xreadlink instead, it's safer.  Don't bother to read the link if
        it's the wrong size.  Add a FIXME because this area is a bit murky
        and undocumented.

Index: lib/canonicalize.c
===================================================================
RCS file: /home/meyering/coreutils/cu/lib/canonicalize.c,v
retrieving revision 1.10
diff -p -u -r1.10 canonicalize.c
--- lib/canonicalize.c  29 Mar 2004 07:29:06 -0000      1.10
+++ lib/canonicalize.c  25 May 2004 22:05:46 -0000
@@ -1,5 +1,5 @@
 /* Return the canonical absolute name of a given file.
-   Copyright (C) 1996-2001, 2002, 2003 Free Software Foundation, Inc.
+   Copyright (C) 1996-2001, 2002, 2003, 2004 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -260,7 +260,7 @@ canonicalize_file_name (const char *name
                }
 #  endif /* MAXSYMLINKS */
 
-             buf = xreadlink (rpath);
+             buf = xreadlink (rpath, st.st_size);
              if (!buf)
                goto error;
 
Index: lib/xreadlink.c
===================================================================
RCS file: /home/meyering/coreutils/cu/lib/xreadlink.c,v
retrieving revision 1.12
diff -p -u -r1.12 xreadlink.c
--- lib/xreadlink.c     21 Nov 2003 08:21:23 -0000      1.12
+++ lib/xreadlink.c     25 May 2004 21:57:36 -0000
@@ -1,6 +1,6 @@
 /* xreadlink.c -- readlink wrapper to return the link name in malloc'd storage
 
-   Copyright (C) 2001, 2003 Free Software Foundation, Inc.
+   Copyright (C) 2001, 2003, 2004 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -23,6 +23,8 @@
 # include <config.h>
 #endif
 
+#include "xreadlink.h"
+
 #include <stdio.h>
 #include <errno.h>
 #ifndef errno
@@ -44,20 +46,21 @@ extern int errno;
 #endif
 
 #include "xalloc.h"
-#include "xreadlink.h"
 
 /* Call readlink to get the symbolic link value of FILENAME.
+   SIZE is a hint as to how long the link is expected to be;
+   typically it is taken from st_size.  It need not be correct.
    Return a pointer to that NUL-terminated string in malloc'd storage.
    If readlink fails, return NULL (caller may use errno to diagnose).
    If malloc fails, or if the link value is longer than SSIZE_MAX :-),
    give a diagnostic and exit.  */
 
 char *
-xreadlink (char const *filename)
+xreadlink (char const *filename, size_t size)
 {
   /* The initial buffer size for the link value.  A power of 2
      detects arithmetic overflow earlier, but is not required.  */
-  size_t buf_size = 128;
+  size_t buf_size = size + 1;
 
   while (1)
     {
@@ -80,7 +83,7 @@ xreadlink (char const *filename)
 
       free (buffer);
       buf_size *= 2;
-      if (SSIZE_MAX < buf_size || (SIZE_MAX / 2 < SSIZE_MAX && buf_size == 0))
+      if (! (0 < buf_size && buf_size <= SSIZE_MAX))
        xalloc_die ();
     }
 }
Index: lib/xreadlink.h
===================================================================
RCS file: /home/meyering/coreutils/cu/lib/xreadlink.h,v
retrieving revision 1.4
diff -p -u -r1.4 xreadlink.h
--- lib/xreadlink.h     18 Jun 2003 08:03:45 -0000      1.4
+++ lib/xreadlink.h     25 May 2004 21:57:29 -0000
@@ -1,6 +1,6 @@
 /* readlink wrapper to return the link name in malloc'd storage
 
-   Copyright (C) 2001, 2003 Free Software Foundation, Inc.
+   Copyright (C) 2001, 2003, 2004 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -19,4 +19,5 @@
 
 /* Written by Jim Meyering <address@hidden>  */
 
-char *xreadlink (char const *);
+#include <stddef.h>
+char *xreadlink (char const *, size_t);
Index: src/copy.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/copy.c,v
retrieving revision 1.162
diff -p -u -r1.162 copy.c
--- src/copy.c  27 Apr 2004 15:01:31 -0000      1.162
+++ src/copy.c  25 May 2004 22:04:43 -0000
@@ -1475,7 +1475,7 @@ copy_internal (const char *src_path, con
 #ifdef S_ISLNK
   if (S_ISLNK (src_type))
     {
-      char *src_link_val = xreadlink (src_path);
+      char *src_link_val = xreadlink (src_path, src_sb.st_size);
       if (src_link_val == NULL)
        {
          error (0, errno, _("cannot read symbolic link %s"), quote (src_path));
@@ -1487,17 +1487,18 @@ copy_internal (const char *src_path, con
       else
        {
          int saved_errno = errno;
-         int same_link = 0;
-         if (x->update && !new_dst && S_ISLNK (dst_sb.st_mode))
+         bool same_link = false;
+         if (x->update && !new_dst && S_ISLNK (dst_sb.st_mode)
+             && dst_sb.st_size == strlen (src_link_val))
            {
-             /* See if the destination is already the desired symlink.  */
-             size_t src_link_len = strlen (src_link_val);
-             char *dest_link_val = alloca (src_link_len + 1);
-             int dest_link_len = readlink (dst_path, dest_link_val,
-                                           src_link_len + 1);
-             if ((size_t) dest_link_len == src_link_len
-                 && strncmp (dest_link_val, src_link_val, src_link_len) == 0)
-               same_link = 1;
+             /* See if the destination is already the desired symlink.
+                FIXME: This behavior isn't documented, and seems wrong
+                in some cases, e.g., if the destination symlink has the
+                wrong ownership, permissions, or time stamps.  */
+             char *dest_link_val = xreadlink (dst_path, dst_sb.st_size);
+             if (strcmp (dest_link_val, src_link_val) == 0)
+               same_link = true;
+             free (dest_link_val);
            }
          free (src_link_val);
 
Index: src/ls.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/ls.c,v
retrieving revision 1.355
diff -p -u -r1.355 ls.c
--- src/ls.c    24 Apr 2004 08:03:15 -0000      1.355
+++ src/ls.c    25 May 2004 21:45:58 -0000
@@ -2641,7 +2641,7 @@ gobble_file (const char *name, enum file
 static void
 get_link_name (const char *filename, struct fileinfo *f)
 {
-  f->linkname = xreadlink (filename);
+  f->linkname = xreadlink (filename, f->stat.st_size);
   if (f->linkname == NULL)
     {
       error (0, errno, _("cannot read symbolic link %s"),
Index: src/readlink.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/readlink.c,v
retrieving revision 1.9
diff -p -u -r1.9 readlink.c
--- src/readlink.c      18 Oct 2003 10:05:47 -0000      1.9
+++ src/readlink.c      25 May 2004 21:58:00 -0000
@@ -1,5 +1,5 @@
 /* readlink -- display value of a symbolic link.
-   Copyright (C) 2002, 2003 Free Software Foundation, Inc.
+   Copyright (C) 2002, 2003, 2004 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -137,7 +137,9 @@ main (int argc, char *const argv[])
       usage (EXIT_FAILURE);
     }
 
-  value = (canonicalize ? canonicalize_file_name : xreadlink) (fname);
+  value = (canonicalize
+          ? canonicalize_file_name (fname)
+          : xreadlink (fname, 1024));
   if (value)
     {
       printf ("%s%s", value, (no_newline ? "" : "\n"));
Index: src/stat.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/stat.c,v
retrieving revision 1.69
diff -p -u -r1.69 stat.c
--- src/stat.c  9 Apr 2004 12:03:02 -0000       1.69
+++ src/stat.c  25 May 2004 21:48:09 -0000
@@ -433,7 +433,7 @@ print_stat (char *pformat, char m, char 
       strcat (pformat, "s");
       if (S_ISLNK (statbuf->st_mode))
        {
-         char *linkname = xreadlink (filename);
+         char *linkname = xreadlink (filename, statbuf->st_size);
          if (linkname == NULL)
            {
              error (0, errno, _("cannot read symbolic link %s"),