Package: emacs;
Reported by: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Fri, 1 Apr 2011 06:48:02 UTC
Severity: normal
Done: Paul Eggert <eggert <at> cs.ucla.edu>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: help-debbugs <at> gnu.org (GNU bug Tracking System) To: Paul Eggert <eggert <at> cs.ucla.edu> Subject: bug#8401: closed (fix installed in trunk) Date: Wed, 06 Apr 2011 05:48:02 +0000
[Message part 1 (text/plain, inline)]
Your bug report #8401: removing duplication and improving the readlink code which was filed against the emacs package, has been closed. The explanation is attached below, along with your original report. If you require more details, please reply to 8401 <at> debbugs.gnu.org. -- 8401: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8401 GNU Bug Tracking System Contact help-debbugs <at> gnu.org with problems
[Message part 2 (message/rfc822, inline)]
From: Paul Eggert <eggert <at> cs.ucla.edu> To: 8401-done <at> debbugs.gnu.org, 8410-done <at> debbugs.gnu.org Subject: fix installed in trunk Date: Tue, 05 Apr 2011 22:47:16 -0700I committed the previously-mentioned fix as part of the merge to the trunk in bzr 103841.
[Message part 3 (message/rfc822, inline)]
From: Paul Eggert <eggert <at> cs.ucla.edu> To: bug-gnu-emacs <at> gnu.org Cc: Eli Zaretskii <eliz <at> gnu.org> Subject: removing duplication and improving the readlink code Date: Thu, 31 Mar 2011 23:47:14 -0700[Message part 4 (text/plain, inline)]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 <at> 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 <at> 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 <at> 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 <at> 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); +} #ifdef USG /*[patch.txt.gz (application/x-gzip, attachment)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.