bug#8401: removing duplication and improving the readlink code

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 7913 bytes --]

In two places Emacs calls readlink with similar code to reallocate
buffers until there's enough room to store the symbolic link's value.
And in both places there are minor problems with overflow, since Emacs
uses 32-bit int where modern 64-bit systems use 64-bit ssize_t, and it
doesn't check for overflow in buffer size calculations.  These
problems cause GCC to complain, if warnings are enabled.  I plan to
fix the problems with the following patch, which substitutes a gnulib
implementation of the same basic readlink idea; this implementation
does more-careful buffer size checking, and makes it possible to
avoid the malloc+free in the usual case.

This patch adds a couple of dependencies so it may affect the
Windows build.

The full patch (including autogenerated files) is attached as
a compressed file.

=== modified file 'ChangeLog'
--- ChangeLog	2011-03-28 01:03:57 +0000
+++ ChangeLog	2011-04-01 06:28:48 +0000
@@ -1,3 +1,10 @@
+2011-04-01  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Replace two copies of readlink code with single gnulib version.
+	* Makefile.in (GNULIB_MODULES): Add careadlinkat.
+	* lib/allocator.h, lib/careadlinkat.c, lib/careadlinkat.h:
+	* m4/ssize_t.m4: New files, automatically generated from gnulib.
+
 2011-03-28  Glenn Morris  <rgm@gnu.org>
 
 	* autogen/update_autogen: Pass -f to autoreconf.

=== modified file 'Makefile.in'
--- Makefile.in	2011-03-25 07:14:31 +0000
+++ Makefile.in	2011-04-01 06:28:48 +0000
@@ -331,7 +331,7 @@
 # $(gnulib_srcdir) (relative to $(srcdir) and should have build tools
 # as per $(gnulib_srcdir)/DEPENDENCIES.
 GNULIB_MODULES = \
-  crypto/md5 dtoastr filemode getloadavg getopt-gnu \
+  careadlinkat crypto/md5 dtoastr filemode getloadavg getopt-gnu \
   ignore-value intprops lstat mktime readlink \
   socklen stdio strftime symlink sys_stat
 GNULIB_TOOL_FLAGS = \

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2011-03-31 19:42:38 +0000
+++ src/ChangeLog	2011-04-01 06:38:52 +0000
@@ -1,3 +1,17 @@
+2011-04-01  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Replace two copies of readlink code with single gnulib version.
+	The gnulib version avoids calling malloc in the usual case,
+	and on 64-bit hosts doesn't have some arbitrary 32-bit limits.
+	* fileio.c (Ffile_symlink_p): Use emacs_readlink.
+	* filelock.c (current_lock_owner): Likewise.
+	* lisp.h (READLINK_BUFSIZE, emacs_readlink): New function.
+	* sysdep.c: Include allocator.h, careadlinkat.h.
+	(emacs_no_realloc_allocator): New static constant.
+	(emacs_readlink): New function.
+	* deps.mk (sysdep.o): Depend on ../lib/allocator.h and on
+	../lib/careadlinkat.h.
+
 2011-03-31  Juanma Barranquero  <lekktu@gmail.com>
 
 	* xdisp.c (redisplay_internal): Fix prototype.

=== modified file 'src/deps.mk'
--- src/deps.mk	2011-03-19 22:46:50 +0000
+++ src/deps.mk	2011-04-01 06:38:52 +0000
@@ -187,6 +187,7 @@
    process.h dispextern.h termhooks.h termchar.h termopts.h coding.h \
    frame.h atimer.h window.h msdos.h dosfns.h keyboard.h cm.h lisp.h \
    globals.h $(config_h) composite.h sysselect.h gnutls.h \
+   ../lib/allocator.h ../lib/careadlinkat.h \
    ../lib/unistd.h ../lib/ignore-value.h
 term.o: term.c termchar.h termhooks.h termopts.h lisp.h globals.h $(config_h) \
    cm.h frame.h disptab.h keyboard.h character.h charset.h coding.h ccl.h \

=== modified file 'src/fileio.c'
--- src/fileio.c	2011-03-25 17:37:15 +0000
+++ src/fileio.c	2011-04-01 06:28:48 +0000
@@ -2579,9 +2579,8 @@
 {
   Lisp_Object handler;
   char *buf;
-  int bufsize;
-  int valsize;
   Lisp_Object val;
+  char readlink_buf[READLINK_BUFSIZE];
 
   CHECK_STRING (filename);
   filename = Fexpand_file_name (filename, Qnil);
@@ -2594,36 +2593,15 @@
 
   filename = ENCODE_FILE (filename);
 
-  bufsize = 50;
-  buf = NULL;
-  do
-    {
-      bufsize *= 2;
-      buf = (char *) xrealloc (buf, bufsize);
-      memset (buf, 0, bufsize);
-
-      errno = 0;
-      valsize = readlink (SSDATA (filename), buf, bufsize);
-      if (valsize == -1)
-	{
-#ifdef ERANGE
-	  /* HP-UX reports ERANGE if buffer is too small.  */
-	  if (errno == ERANGE)
-	    valsize = bufsize;
-	  else
-#endif
-	    {
-	      xfree (buf);
-	      return Qnil;
-	    }
-	}
-    }
-  while (valsize >= bufsize);
-
-  val = make_string (buf, valsize);
+  buf = emacs_readlink (SSDATA (filename), readlink_buf);
+  if (! buf)
+    return Qnil;
+
+  val = build_string (buf);
   if (buf[0] == '/' && strchr (buf, ':'))
     val = concat2 (build_string ("/:"), val);
-  xfree (buf);
+  if (buf != readlink_buf)
+    xfree (buf);
   val = DECODE_FILE (val);
   return val;
 }

=== modified file 'src/filelock.c'
--- src/filelock.c	2011-03-15 01:19:50 +0000
+++ src/filelock.c	2011-04-01 06:28:48 +0000
@@ -396,36 +396,16 @@
 static int
 current_lock_owner (lock_info_type *owner, char *lfname)
 {
-  int len, ret;
+  int ret;
+  size_t len;
   int local_owner = 0;
   char *at, *dot, *colon;
-  char *lfinfo = 0;
-  int bufsize = 50;
-  /* Read arbitrarily-long contents of symlink.  Similar code in
-     file-symlink-p in fileio.c.  */
-  do
-    {
-      bufsize *= 2;
-      lfinfo = (char *) xrealloc (lfinfo, bufsize);
-      errno = 0;
-      len = readlink (lfname, lfinfo, bufsize);
-#ifdef ERANGE
-      /* HP-UX reports ERANGE if the buffer is too small.  */
-      if (len == -1 && errno == ERANGE)
-	len = bufsize;
-#endif
-    }
-  while (len >= bufsize);
+  char readlink_buf[READLINK_BUFSIZE];
+  char *lfinfo = emacs_readlink (lfname, readlink_buf);
 
   /* If nonexistent lock file, all is well; otherwise, got strange error. */
-  if (len == -1)
-    {
-      xfree (lfinfo);
-      return errno == ENOENT ? 0 : -1;
-    }
-
-  /* Link info exists, so `len' is its length.  Null terminate.  */
-  lfinfo[len] = 0;
+  if (!lfinfo)
+    return errno == ENOENT ? 0 : -1;
 
   /* Even if the caller doesn't want the owner info, we still have to
      read it to determine return value, so allocate it.  */
@@ -441,7 +421,8 @@
   dot = strrchr (lfinfo, '.');
   if (!at || !dot)
     {
-      xfree (lfinfo);
+      if (lfinfo != readlink_buf)
+	xfree (lfinfo);
       return -1;
     }
   len = at - lfinfo;
@@ -467,7 +448,8 @@
   owner->host[len] = 0;
 
   /* We're done looking at the link info.  */
-  xfree (lfinfo);
+  if (lfinfo != readlink_buf)
+    xfree (lfinfo);
 
   /* On current host?  */
   if (STRINGP (Fsystem_name ())

=== modified file 'src/lisp.h'
--- src/lisp.h	2011-03-29 23:35:49 +0000
+++ src/lisp.h	2011-04-01 06:28:48 +0000
@@ -3340,6 +3340,8 @@
 extern int emacs_close (int);
 extern int emacs_read (int, char *, unsigned int);
 extern int emacs_write (int, const char *, unsigned int);
+enum { READLINK_BUFSIZE = 1024 };
+extern char *emacs_readlink (const char *, char [READLINK_BUFSIZE]);
 #ifndef HAVE_MEMSET
 extern void *memset (void *, int, size_t);
 #endif

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2011-03-27 02:27:11 +0000
+++ src/sysdep.c	2011-04-01 06:28:48 +0000
@@ -31,6 +31,8 @@
 #endif /* HAVE_LIMITS_H */
 #include <unistd.h>
 
+#include <allocator.h>
+#include <careadlinkat.h>
 #include <ignore-value.h>
 
 #include "lisp.h"
@@ -1866,6 +1868,22 @@
     }
   return (bytes_written);
 }
+
+static struct allocator const emacs_norealloc_allocator =
+  { xmalloc, NULL, xfree, memory_full };
+
+/* Get the symbolic link value of FILENAME.  Return a pointer to a
+   NUL-terminated string.  If readlink fails, return NULL and set
+   errno.  If the value fits in INITIAL_BUF, return INITIAL_BUF.
+   Otherwise, allocate memory and return a pointer to that memory.  If
+   memory allocation fails, diagnose and fail without returning.  If
+   successful, store the length of the symbolic link into *LINKLEN.  */
+char *
+emacs_readlink (char const *filename, char initial_buf[READLINK_BUFSIZE])
+{
+  return careadlinkat (AT_FDCWD, filename, initial_buf, READLINK_BUFSIZE,
+		       &emacs_norealloc_allocator, careadlinkatcwd);
+}
 \f
 #ifdef USG
 /*


[-- Attachment #2: patch.txt.gz --]
[-- Type: application/x-gzip, Size: 6330 bytes --]