From debbugs-submit-bounces@debbugs.gnu.org Fri Apr 01 02:47:41 2011 Received: (at submit) by debbugs.gnu.org; 1 Apr 2011 06:47:41 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5Y8y-0007qh-O0 for submit@debbugs.gnu.org; Fri, 01 Apr 2011 02:47:41 -0400 Received: from eggs.gnu.org ([140.186.70.92]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5Y8v-0007qU-5O for submit@debbugs.gnu.org; Fri, 01 Apr 2011 02:47:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q5Y8o-0006YS-BE for submit@debbugs.gnu.org; Fri, 01 Apr 2011 02:47:31 -0400 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on eggs.gnu.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,T_RP_MATCHES_RCVD autolearn=unavailable version=3.3.1 Received: from lists.gnu.org ([199.232.76.165]:57121) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q5Y8n-0006YK-Va for submit@debbugs.gnu.org; Fri, 01 Apr 2011 02:47:30 -0400 Received: from [140.186.70.92] (port=47526 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q5Y8m-0006QT-HX for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 02:47:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q5Y8k-0006Xs-AA for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 02:47:28 -0400 Received: from smtp.cs.ucla.edu ([131.179.128.62]:60240) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q5Y8j-0006XW-Hu; Fri, 01 Apr 2011 02:47:26 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id 4A92839E80F8; Thu, 31 Mar 2011 23:47:23 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id SMvM-wRs7fv7; Thu, 31 Mar 2011 23:47:21 -0700 (PDT) Received: from [192.168.1.10] (pool-71-189-109-235.lsanca.fios.verizon.net [71.189.109.235]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id BAC1839E80F7; Thu, 31 Mar 2011 23:47:21 -0700 (PDT) Message-ID: <4D9574F2.20108@cs.ucla.edu> Date: Thu, 31 Mar 2011 23:47:14 -0700 From: Paul Eggert Organization: UCLA Computer Science Department User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.14) Gecko/20110223 Thunderbird/3.1.8 MIME-Version: 1.0 To: bug-gnu-emacs@gnu.org Subject: removing duplication and improving the readlink code Content-Type: multipart/mixed; boundary="------------020501070200050907060101" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 199.232.76.165 X-Spam-Score: -4.7 (----) X-Debbugs-Envelope-To: submit Cc: Eli Zaretskii X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -4.7 (----) This is a multi-part message in MIME format. --------------020501070200050907060101 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 + + 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 * 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 + + 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 * 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 +#include +#include #include #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 /* --------------020501070200050907060101 Content-Type: application/x-gzip; name="patch.txt.gz" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="patch.txt.gz" H4sIABdzlU0CA+w8aXfbyJGfrV9RHs+LSPEQKVGWRZke0xIlM6GOiNR6vI4eFgJAEiscDACa Yhz/962q7gYaICl7Ji/Zl107GQnoo7q67q4uqNPpgB/a7th1bBi7ngPbJ1MzmDiDcLK9VavV IH19ttdoNmuN/dreK2g024399sEhVBr4b6tSqayMa9UaTWi8bO+9ardeyXFv30KtWd2HSrPa bMDbt1sVbSxcm3MPepOJEyUArx1+eGvF9bnlmXXHnr/ZqmxVnt04M8+0HEgWIVjhzHViCMcQ OabtucEDNtkOLNxkCrEbTHA/k2Duuffw2YliNwzqCGEHLswHhzZbdwMonV/eDvrvjIur09tB b1huQ9e2wTIVRDMRcxDIrul5oWUmYVSfVrkhN8xa0zZt82S/tRvH7t8cI6n7rTZcOgsmdlwF c56Evpm4FsJewsQJnMhMiBdR6Evccf0t0IgP554TBHARRpEbI6Wiif8WR9bDaPJmC7YA1yOo CGt3PrMRmiFf20jiOIbaGJKQh0SOFQbj+tZWZ1UMNBoJQdAaMlE4gMZhu9lq7zc1UVgduUkY 9vebVRQi8QvlAV7AzyWxayOOLNuNylCKHA8J9NkhrH8uqWYzsCGehnPPhqmJnfdzFx+TMPRi AmPGMHOiFWi7p73r3uVp7/Kk3xvWtyDPfOjAX7ZqAFa0nCXhrm8fgJ2EZpxETBSfZGviJF5o 2ubnCT2Gs6SGK+C0CuSE5nfBAAB3EiBbap9Nb+6AGySzKJzF4MUJgvQfEtd3MlnnCXFoPaA8 QJzYbog/ozEPipc+j4mXsUGT062Orq4Gxtmgey52y6w3bTvle0HOBe8Ljc+aR4cNZClztdHm /2v8L45+WgYa1QYZhNYBG4Rd1E4kUrSEFEIM8dyaEkd9bquMI8chpUCSw0k4W0buZJpA6aTM WgJn2A3DcJwskB9wFs4DVALU/Sr0A0vNG01Rd5C4k8j0AR8JJtJSTGrDMpwjOwOkte0iTd37 eYLsSEjqdsNI6MqSAWEjLoCilkxRQp3IZ3tEL0hwOGeF9uB6fu+5Fgxcywlih6WTWuIp0v1e AKIpm1A/BgdNGi4izRjsq0UkxCqEEUMpoZwg8hGgVOFA0pMloP5kczdRINuojYLHwKfhDPc0 RZC4y4XreXDvwDx2xnOvyjBwNHzoj95f3Y6ge/kRPnRvbrqXo4/HbIJD7HU+OwKW6888si64 s8gMkiVugEFc9G5O3uOc7rv+oD/6iPuAs/7osjccwtnVDXThunsz6p/cDro3cH17c3017NUB hg4h5jCEJ+g8Zl4hKW0nMV0vVnv/iOzVjQcaQgctjA0m+ZTltznIUEwvDCbC2SQaMRG9/hiC MKlCjGi+nibJrL27u1gs6tJM73oCSrz7Bgfv7BJWKPgfIjdJkFz3S90XpiNeuGMUtDEY5wOj OxhcnXRHVzfGe9EVWN4cLctrNAM4pj5lZ4kMnVtJpklblS+EOC51gm1wwVDYGYgRDvise+TI HtAWCH3bRgyuAhgjAedISjGLCQCKiJGTzKMA0MIMqkCMn0yJABAgXePYjFz0bbGD0hBFQYjg PkxxmxOkeCDhmPA3J0LzhT6SZM03lzpMIUXumAwBAUT5k1QB+By6NuyUdgSu6C6Eny0fC1bj XpkZAboBrwrkZuGml24cben6rcuO7bpEUCOAmv7b9i7hrFLgO/euCL6ZAhJlJIFoqcIKKZjt Zze9Hu2dTV5+19S0nQdc2qHGFOhTdD3t94hJUqxQ9RSlxqx7wAME1eRuiE5ir8VFbVetSSt+ 5VVfOAGa3fUuKx+JZV4r3/4djqsw4bt8V/PwSDmvG5xMzvc+JGtBYGJy46hiaCzHYzTDyjIy vz3XdxPpnfQ4Z2xvcnCNZpV+YuzVaLT48ZB/HtWedn4M7IcD/OEA/+0cYBXeRfMghPeme+8h oyny/6PrY5y4dCI85GkOUnlBOta4E+kF0+af8kezn/KdWsSa73ktbDYBy9pYceNCIzpfOq8V GxnJfOM8QGGzJYK489uY2UnheuL4MJ4HFkkvHhDJRFKPPMeGKMSRa9PR8d6xzDnPc2NmByrl HO2hHeKZmKbZzgwtJqC2EB9RElB2+r8iT3yUP9eKJeFezDmsEA40fZXeROJ3ige6QCyFuiEA O0FCri0UCkLnjwCpSppEi5soDU7k/HXuxm7iqKVkCDPs/2fPuOj+ii2IJYNWTVBSHhxqzXJq 9LX4Z7hutj5dzS+ljbuwVy7CgufwvvsfPQOd1Omgf/mn7oh32ucDGJydIsrnTiK5otlzEGcz 1JCz/qB32b3osUTO5mwkyNYzM97dnp31bqoqF4G2XrQYhFRdWh/FZzCtJBY+WEloal6maCsz sd3G0xCeEU30mYqocr9bFV28rQV6UUQGPQl656kZoV4HcQI75DID03dk647wSlXpkPV/Aqx0 Wwa9lWUAKfwygj6mNxmspMfSUraEBJ6DQe5cZwXSvBvHcx+VhIltzaMIRQvNMRqlhE6CSKiz 0yodlTdxg7HPcSRGAaTQpEYeyQ04pyI0scit1DOLUA8t19kpLdkdGWenJx9w4RQqCz8CRMkm eLrD1jBnMIsweqDF0l1UCSd0K+hRAs4pqdVoJu8EocekhOAEHERSGKBwFLKj0CZP6NQ1MWOp oPAx1qVMmEo5lZw5Oi+iiXDbY30ozaQodANepNCMW7VAOXuJjJbJKxVJ21J0OdpBU2NO2McD x4LkGxICyQTDKU60TfIdz1A83XvXc9EzyuA0jQqEhCEUmRYZMwaxCj95mxJLsSdcpZttVYbr CQ6L2SOyFNGrGdnZ6SiLCxy4zqxCpqTxzLEoRRZjXLCQpwdbxHga3bIIvapUg9aT9MC4FTcb H0NITFy4sZML9hEnhqMfloRzE9qaV/JvaHhRp3MKv0a9VyYUz49qBW5YHS0hlnZmGYplxjGP oDI96QFF2ZUUQbYrGYKM3bo2wzcfoSMQyVzA68yZ/JKeB7X+CjShnY45TldGkbAeDIT9qdnY a93Jk446Xs2KJ0zoSJ95nBu28RiG42XfsXbQmeWPVziIGnIjtLMQdpOccDeqQUksJQjwRTFE Yopj+XftjYYm90s0sgE6XjyCTwNpd4oR9yE6WRe+yJ6vglyE1fOC08hhR/Zenbk5Lg7cxMX4 UipIGCj1tB6Um1yYaD5FSK4JnRX6Po62zJi9sZnmPPFFGFIWalhwwhVMhCK8sakBkbcEYryg U6mMa2EYhrrNYO/dSdYzDvFhIQxaDs4UfQtbfEnLUjk90kJmDjuZkB3nu5hW1I+/aAOFYZK6 2IBjxIxj2aBmanCk6NrhGtpjhO3PktR8ZWY+yLsx5RX1XSgVpwkGWpkJUrMDM90ikTXKBQDV FMdyumUdTKrbILxSSQf+GhrljMpfdJODe7kKoIsB7cEAPh/U99kNvL+u3f4KzaYLn/eg0dpt HFWzyERY2hgDy4LtYrFgewudDvTwCHfOqQzNcJPLCkMhKDmaMNpBosVohoQkIB7nBo7pPqMw 8LlaspxH60vRwtJskoDnitnl1bDty5pITuozzS0fr+tXCBdRWzP4a7FJc1+F4dpQKb70L2U6 rqex+jgbkQoBj3qdyc8GSSCLnY6vVO4Q8PZfGtsaRJ14HU0Hv0Vw6ZAQYEk8llPjmmG4SlJp A9dw5x4p/LAy3nd8Cw/dpXupME+AlgZgM6WRmV7sbKQh/OEPkBMhakh91jfIgSo3ZDtHYRZG k2M6KQn+k+VzkxXF2ERE5YFK39ovc20dITfRQW+Qoqkb29yYp/QprzKFGZKqnXwgQqfMDEA6 bKcDe+n6KXMyKDkg6wDkl8mB0kbnJEuQZ1Voi6aYBWcxpTSqvlVCkMMOMYpdfknMUaaid3l1 0bvQD4DSAnz9jgTtdEOCdvpbE7Tfebn48vBflp/9cQH5I//6/zb/ml1QnnSzE3TxjnJsBYn3 VEK0eO48/pEn+pEn+qfkif4NUj7/QMLnd6Z9fk/y57emgNbP3/SvmC9SBmE4R++MRkkwXCT3 dHptk0VgtuvcNaPJ3EcFjAUWC7qKt0NWeZh5ppvlsrNri9W7gvTuoZh2z94ozc1BWnaDoqxF /halml6jCHHXOK5d6jC2+YsdGs7mtDaVI+tCmoUGpFdBvomGnbkfWEJfiliLwjM0PUmVverC AZ/DGjRHCweVZ66uqFDSNGuK3RIBRSyQbkBtlZoUrdLtl2r7jVbz6OUBXcuAvAxQ4vPbLjKe EKVviT4JknYpxE90yln1YLS1dWWSFJHKMk3/IYtq06asVBLj04P2fiuLT1VAWxy7KZY9olLJ o7RQMuVFGjeRwRIBoBlLe4iE4lACBUkFBziXpp+npaZocgUKNaqerG/VXmCUjKPtuYU2bNnW e6FWwzgqjBJ8QJfVqeNv7Ovgf1TCWKvFGPpZTu3ejB1qxBa/Jd78Fs0JLfGGD/iaIIKxaOBH bDLnjylkXxaS1ojPnZRO2BOEtKzEyDetKKyhxRm7j52JJ7o/WzUusf0dhZi/pQbze8svK//H iPqPlbr+MygM9L+Lq+Fo8PFk0OteUjQ2hEoH7QXG3zt1TvrYc3/GyvTyqPoSKviz2WJ9QpVg M4RGQ960S8NrVSpou2IcUcER984E3UNhhKlb/K2KYJthGsOr25sTiUOuqohG9X4d3XSN0/5w RP1a8UF+7JQN0wbU8gvDJvxS9jCN1lkxv7U78WpW6M/qfktYsVzTN6zYurGbrNjeKyI8/hR2 DKB7gm71z7f9m17pEz5f31ydG3iGG/Tf3ZWp/wUesVGQuCBe7ggdeE3GdO0NQyTb2uQc1nVr pNsIIqXbphF2GJMSbewWytDmfbde0r5bh+m+10zw517impE13QRRIb2pX+rKpl1L/7pxdmI+ 0Wffo2F4ohvJLXZ6yKr1qll9JXc68QxhwQw2SuQzt/9FnK2wfJ287538yTi7vTwZGleXJyho 2ZiNQlZgP27i4vTgW4LAX7g0D4gAzeaRToHbS1T1U/mlgdG/PO1z/W6GykZEUqYKQKOP1z00 LSd/wsOmMfoWp7F7ksg5fOM5+hbzcQmilDEcdUfyffhxyK9rsKdpiLnQ7ENW7aP96pHcN3+U Qd5nV2Ms2rRcl2RosXlhRkEtDGoYd5IVhOJnBaqpaFrXtDJkapV80hqEiuI82aAcF7aID1SY l/utVqq3aOxSPUVzJ5vSE0PaItlGDRXIfwGkRiSG7Ru8mtZm5kaw1lHDSlI1BzG12VnTN1Kp +bHflUXd2+cs6gvIJuKpNaLL2wMooXtPnMek1qg3X9UpEW4H3prqVapb3eeK1Zf0s9mofVfO lICJkinafDFfevxk8lLMpmLrGOYBp3SRijNK+MSczExCkXWTydVcxrUqZnOSLcyyw8KBWqas 0sOAm3NxHHiLFINrMZ4YQSGNPjsiYUygzkjr9JSa2h5F63gY5TyrOgpRXpSPTmI+GrPTHqpn 6VNBq+8Qz0/K3HXR4AmjV/pER2IJ7K4KNM36bKQN4uxEfvfk6uK6T7p91hsMe6X0UEV9g+7l OTvmm+5FST9uffqk1Tku491kOXOoKPLurpofRge4x+x2W9Xo0d1EpdhYvASSyZLnj8d3d2UN cH4vnaUTr2ywE4Q4pSyvNCgahp9zAxClIGTpkV8iCAL3L9FHaFRD9O9Wzpmf5JGec4hUlIdC RQRQyVs+nHG6zYzl2bMucBm7WxV8WheJxZG1W/gCM9eURmL7TWgetVt77X09Els3Vmn1/qv2 wd6aLzEP/9e+xBxNi61gUplLzBk/SsjIeyt5HTCP52hrqOADmfHMFCmQl3gUchOYhnTUoYRH sJ2IpHoc+hRRYG9kRkvY3+OBsnKXv8skmrth3YLSGT0a8lBhzMptLsl18BQUG6ltT+cgTg80 S2ZyDXo3wgWeqHHiwH1wKMunvhuNZxjUl1QywXh3eyaSrnngZflZqMw8ismoU7Yzq1ttsoKs ZrlPUPNOjqaUBNAgNOQNq5FOkAuQg3EtkUQxg0Sb9CQmiEZMp8SSRCnEUadpGqpe3y14ZxDc wbmybwXX7JtWFGX449wMfBPe8eXMX+f0QQy89pyHh2T+duKbrlfH44X6vPXRJqJaVEpBT565 NDjfHpgeonXmPlKyIwlJGesbtUzuKNMx2ZBqWPMI9vbarZftg0ZBw/IjN+qXiIfol4odCC/K FCN9CHF0l4g0vlDWZhqGD7F8pryVfMQjM7WiNnEVufjoE70fhzFg0lmYhi7cwA4X+ODHGOQQ /DAeB/Tw4CzvQzOyCYiPP6RACjgTL7w3PRr2c0kUzRvTMlVWzUIu256SCMaO51hU2o26mvBg /tx1HdfXMluuJfvUTY9613MBFJjRputhW/y2iuTQqSRJIze0fitiad64ohlRPjHvC6TBJUwr YVrSc+wkOtUty+N9bBImZUgyaVIt+rfSzcP2/mG7eVAQp8LQjSfnAzxZHWGATb/FyeIL7W6A +zGu7v8beYR2L7A9Kg2DXDVlTZQo4bOotpLvSHPxXoCC7VlVpOIkF0YWrdjdMakkgDhhDUc3 /cvzrAC8zJDVG/raM+dxhhgabGy5TSsW/3PgejhDbPWoVd2nw8QBniaaBzJFk4PVuzy5Ou0Z lObJr0i7kzvFYQeNY9mQ1k7WuDKuJkpcamnBhFasUVtbRfGYr1qRU8rpcN/x6RJH9DZyA9QQ VT3RSCdJHmjFV1AaDk+7o662q6yQLr8i1WmkADr8wUTtGe7phTumDLyoKsMmLt0RpXGRQ5nG WCty21jgRvNohUJhXJk7dMwzuXomy1JqMqsuRn6RvwEe9aoa1SgDPOK/avyKv7+KXfIvWaKi lnzTKRIXe7ge9wGduLi+FWyQM8rHWfVk3tutpbYu8+XjXFmrLInJ4SzqhBkDPr/qKAgdkBU/ nxp3RMft3W0qu8JR1jSSiG63t8ucAVCQ0IahVd2jbh3kT7vtn8q8LyEGj4XSPq2mKbcLgXZu dLrUaU9TJQE52yIbA/j6pOkT8VDe+Im2zJse0F8NwZB1xZuuDN741yKOXrJRoN/Nl2wTZDSD xmwLVoMxKPGzG4xDgyPzHW5W9efemNm9JawAGUQUAL7BFZTkos5ErzvHfsFOGotOz5PrsEKn JtdMqrBjU/nFjhXisZD5pJYkZFIDoBnlzFSpKiUVubressaHS5SIhK4tucBZRKpUiYLxrMfX Yjbl0qVp4Ay/HFSbUQCtvIzU7e+1gSnGa8yg6Ftjl1bNHGXwdRMnaF+FNSAKxistPdxkv54s 0tUMJaNARpJ0b41NEyhmtkyZsKIRonE5A/S9nrKyIgVFU6SoUrA/7PrSL66dRwygHCGBD8zW Kh0LaPN0UavVGVRhQgUZKER4IKQtY4wmiZIjSLkgCNJKCDQzrkqDkJHu8qp3OYJfoAFthHKc kkpK8UCUleNOGeWYb7n/C1fd5goZ+uKNS3+RU5dUnpGW4KSsExh8wmF3QpqUJZao5WzxZrwU /XryzwhkNSXpgXGBZyFuFwothHJBaVGqOuPjZBJuKTqgdrpcQm87AmlHs5Zzhzea/mEBVRbL twAt/oM3rb0sO46Ggj8LiIQzUAqxXd/OXMdzM4G//x2e41jpIzbySq+hFmK26gmeFSflMy1M MsHKVHMRgZpkx7G8zzjknbRepTth0tXe0ElcY5ki/gdnm+rgUH5RcEMuepL1Dp6Sk5Tvazb1 5IbWEUKtexWkpWKE2S+8hqSQCFiv8fgvPryVMWl5c25GHDYyTyfesyAfz4z77f2D/2nkaloa BoLoPfgjBg+idQ9tKFV7EDwUCTQKJj1L1RQEbaW1qBT/uzszO/uZBU+BNJnd7OzO7ry+N9Px VbTLBQ/m6yGNh4Q345WHlXNE2ik4Wjy/bXYdUSLwI5OfaXJ6hJuBgv3aoFO5d762OteTl4hd kXn1vFvv3+EAcXDTnkbJFGCdBmOdLcQBLrRO1zRSYh+FS0i0m3pWN7PWdpylUnLaFpUV9d4i iOAXi+jxoyArzpNyx/nyAobltNQJ2yjyZfRo1psj8qUsdUcvoY+aV3XVNswsgT4iJPj8SC/J DniTYaZ97VsKE2vCTpzanafjsQEqJhNGKiaXqiwFq+Dlb2LC6dPPZ8czRe89Z3QiJKYmnb8y XDEBoxIsijV7B/hmfE+Z6im0hJWpSfK4wj3hV6he/9Ri6xDywD1ewseGUCEqcEMxQreS8jyZ LWunKNUqUTFDz1ZwEYaeIT5y+yvcy/QRq7qr2upmjhPZWvDuMRPz3u3OUe0Zamjb03vi7/Az 1DSr/YMyWYidmq6/vC6155Hkrc3hPfunhZVqOCs+dXGHFeE4IrP6SoDsYMiJ0TnABatH3HC+ RJyaLPe88NzI/vrPSiIINYMRUiAd4daZ9KypJDohWGxSTTjJzkkV085YqQ5HFIwwFi2a20Iv 36L4A5dznx82UQAA --------------020501070200050907060101-- From debbugs-submit-bounces@debbugs.gnu.org Fri Apr 01 04:34:56 2011 Received: (at submit) by debbugs.gnu.org; 1 Apr 2011 08:34:56 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5Zol-0001iA-PS for submit@debbugs.gnu.org; Fri, 01 Apr 2011 04:34:56 -0400 Received: from eggs.gnu.org ([140.186.70.92]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5Zok-0001hw-2Q for submit@debbugs.gnu.org; Fri, 01 Apr 2011 04:34:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q5Zod-0003kg-VZ for submit@debbugs.gnu.org; Fri, 01 Apr 2011 04:34:48 -0400 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on eggs.gnu.org X-Spam-Level: X-Spam-Status: No, score=0.8 required=5.0 tests=BAYES_00,RCVD_IN_PSBL autolearn=no version=3.3.1 Received: from lists.gnu.org ([199.232.76.165]:43233) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q5Zod-0003ka-No for submit@debbugs.gnu.org; Fri, 01 Apr 2011 04:34:47 -0400 Received: from [140.186.70.92] (port=42034 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q5Zoc-0008BC-Kl for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 04:34:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q5Zob-0003kB-30 for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 04:34:46 -0400 Received: from mtaout20.012.net.il ([80.179.55.166]:54539) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q5Zoa-0003k1-QT for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 04:34:45 -0400 Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0LIY00800T3KES00@a-mtaout20.012.net.il> for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 11:33:36 +0300 (IDT) Received: from HOME-C4E4A596F7 ([77.126.47.180]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0LIY007JJT3YPL90@a-mtaout20.012.net.il>; Fri, 01 Apr 2011 11:33:35 +0300 (IDT) Date: Fri, 01 Apr 2011 11:33:37 +0300 From: Eli Zaretskii Subject: Re: removing duplication and improving the readlink code In-reply-to: <4D9574F2.20108@cs.ucla.edu> X-012-Sender: halo1@inter.net.il To: Paul Eggert Message-id: <83vcyypdzy.fsf@gnu.org> References: <4D9574F2.20108@cs.ucla.edu> X-detected-operating-system: by eggs.gnu.org: Solaris 10 (beta) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 199.232.76.165 X-Spam-Score: -4.4 (----) X-Debbugs-Envelope-To: submit Cc: bug-gnu-emacs@gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list Reply-To: Eli Zaretskii List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -4.4 (----) > Date: Thu, 31 Mar 2011 23:47:14 -0700 > From: Paul Eggert > CC: Eli Zaretskii > > 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. Isn't much easier and much more elegant to use ssize_t instead of an int for the buffer sizes in both cases? > This patch adds a couple of dependencies so it may affect the > Windows build. If this patch is accepted, the new emacs_readlink function will be a trivial "fail" stub on Windows. I don't see a need to compile in all this gnulib code just to return NULL because readlink always fails. From debbugs-submit-bounces@debbugs.gnu.org Fri Apr 01 15:00:38 2011 Received: (at 8401) by debbugs.gnu.org; 1 Apr 2011 19:00:38 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5jaH-00083A-Tm for submit@debbugs.gnu.org; Fri, 01 Apr 2011 15:00:38 -0400 Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5jaE-00082s-NZ for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 15:00:35 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id 1C0B939E80F8; Fri, 1 Apr 2011 12:00:29 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8T0hIQ9opOjR; Fri, 1 Apr 2011 12:00:28 -0700 (PDT) Received: from [131.179.64.200] (Penguin.CS.UCLA.EDU [131.179.64.200]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id A828D39E80B1; Fri, 1 Apr 2011 12:00:28 -0700 (PDT) Message-ID: <4D9620CC.4000806@cs.ucla.edu> Date: Fri, 01 Apr 2011 12:00:28 -0700 From: Paul Eggert Organization: UCLA Computer Science Department User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.15) Gecko/20110307 Fedora/3.1.9-0.39.b3pre.fc14 Thunderbird/3.1.9 MIME-Version: 1.0 To: Eli Zaretskii Subject: Re: removing duplication and improving the readlink code References: <4D9574F2.20108@cs.ucla.edu> <83vcyypdzy.fsf@gnu.org> In-Reply-To: <83vcyypdzy.fsf@gnu.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Spam-Score: -3.2 (---) X-Debbugs-Envelope-To: 8401 Cc: 8401@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -3.2 (---) On 04/01/2011 01:33 AM, Eli Zaretskii wrote: > Isn't much easier and much more elegant to use ssize_t instead of an > int for the buffer sizes in both cases? That doesn't suffice; the code should not only use ssize_t for readlink's returned value, but it should also use size_t for the buffer size, and it should check that neither type overflows. We could modify both copies of Emacs's readlink-using code to fix these problems, but when there's duplication like this, it's typically better to have just one copy of the code, and make any necessary fixes in that copy. On 04/01/2011 01:33 AM, Eli Zaretskii wrote: > If this patch is accepted, the new emacs_readlink function will be a > trivial "fail" stub on Windows. That would introduce an unnecessary "#ifdef DOS_NT" into the mainline code. We should strive to keep the mainline code free of porting #ifdefs when it is easy, as it is in this case. The proposed code should run just fine on Windows, using the already-existing stubs. We shouldn't need to clutter up up the mainline code with unnecessary Windows-specific microoptimizations. From debbugs-submit-bounces@debbugs.gnu.org Fri Apr 01 15:39:58 2011 Received: (at 8401) by debbugs.gnu.org; 1 Apr 2011 19:39:58 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5kCM-0000Sv-ER for submit@debbugs.gnu.org; Fri, 01 Apr 2011 15:39:58 -0400 Received: from mtaout20.012.net.il ([80.179.55.166]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5kCJ-0000Sh-UU for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 15:39:57 -0400 Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0LIZ00G00NT61100@a-mtaout20.012.net.il> for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 22:38:57 +0300 (IDT) Received: from HOME-C4E4A596F7 ([77.126.47.180]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0LIZ00GQ1NWW1300@a-mtaout20.012.net.il>; Fri, 01 Apr 2011 22:38:57 +0300 (IDT) Date: Fri, 01 Apr 2011 22:38:59 +0300 From: Eli Zaretskii Subject: Re: removing duplication and improving the readlink code In-reply-to: <4D9620CC.4000806@cs.ucla.edu> X-012-Sender: halo1@inter.net.il To: Paul Eggert Message-id: <83hbahyd64.fsf@gnu.org> References: <4D9574F2.20108@cs.ucla.edu> <83vcyypdzy.fsf@gnu.org> <4D9620CC.4000806@cs.ucla.edu> X-Spam-Score: -2.1 (--) X-Debbugs-Envelope-To: 8401 Cc: 8401@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list Reply-To: Eli Zaretskii List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.1 (--) > Date: Fri, 01 Apr 2011 12:00:28 -0700 > From: Paul Eggert > CC: 8401@debbugs.gnu.org > > On 04/01/2011 01:33 AM, Eli Zaretskii wrote: > > Isn't much easier and much more elegant to use ssize_t instead of an > > int for the buffer sizes in both cases? > > That doesn't suffice; the code should not only use ssize_t for > readlink's returned value, but it should also use size_t for the > buffer size Well, let's use that as well, then. > and it should check that neither type overflows. Are we really going to consider seriously the case of a link name overflowing a 64-bit ssize_t data type? On what platform can this happen? I could perhaps be sympathetic to defensive programming in this area if it were a simple enough test. But why do we need to introduce the allocator and careadlinkat modules, and all the nested function calls needed for them, just to protect a simple code fragment from overflowing? > We could modify both copies of Emacs's readlink-using > code to fix these problems, but when there's duplication like > this, it's typically better to have just one copy of the code, > and make any necessary fixes in that copy. We could refactor the duplicated code into a short function, and use that. > On 04/01/2011 01:33 AM, Eli Zaretskii wrote: > > If this patch is accepted, the new emacs_readlink function will be a > > trivial "fail" stub on Windows. > > That would introduce an unnecessary "#ifdef DOS_NT" into the mainline > code. We should strive to keep the mainline code free of > porting #ifdefs when it is easy, as it is in this case. > The proposed code should run just fine on Windows, using > the already-existing stubs. We shouldn't need to clutter up > up the mainline code with unnecessary Windows-specific > microoptimizations. I'm all for that, but if you want to help, please restructure the code so that neither allocator.h nor careadlinkat are not used on platforms whose readlink is an always-fail stub. Sorry, but there are limits to what I can stand in code inelegance and gratuitous complexity without having my stomach spilled out. I have no authority to reject your patch, but I _can_ leave the parts of code I'm responsible for as unaffected by it as possible. On top of all that, the functions you introduce use malloc/realloc, which I think will prevent their callers from being safe for asynchronous calls triggered by external events (mouse etc.). The original code used xmalloc/xrealloc instead, which have Emacs-specific implementations to avoid that limitation. From debbugs-submit-bounces@debbugs.gnu.org Fri Apr 01 16:09:32 2011 Received: (at 8401) by debbugs.gnu.org; 1 Apr 2011 20:09:32 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5kex-00017x-T4 for submit@debbugs.gnu.org; Fri, 01 Apr 2011 16:09:32 -0400 Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5kev-00017i-4c for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 16:09:30 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id 59A0639E80F8; Fri, 1 Apr 2011 13:09:23 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 6KFUfj6K7hqR; Fri, 1 Apr 2011 13:09:22 -0700 (PDT) Received: from [131.179.64.200] (Penguin.CS.UCLA.EDU [131.179.64.200]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id A2F3739E80B1; Fri, 1 Apr 2011 13:09:22 -0700 (PDT) Message-ID: <4D9630F2.1010806@cs.ucla.edu> Date: Fri, 01 Apr 2011 13:09:22 -0700 From: Paul Eggert Organization: UCLA Computer Science Department User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.15) Gecko/20110307 Fedora/3.1.9-0.39.b3pre.fc14 Thunderbird/3.1.9 MIME-Version: 1.0 To: Eli Zaretskii Subject: Re: removing duplication and improving the readlink code References: <4D9574F2.20108@cs.ucla.edu> <83vcyypdzy.fsf@gnu.org> <4D9620CC.4000806@cs.ucla.edu> <83hbahyd64.fsf@gnu.org> In-Reply-To: <83hbahyd64.fsf@gnu.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Spam-Score: -3.2 (---) X-Debbugs-Envelope-To: 8401 Cc: 8401@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -3.2 (---) On 04/01/2011 12:38 PM, Eli Zaretskii wrote: > the functions you introduce use malloc/realloc, > which I think will prevent their callers from being safe for > asynchronous calls triggered by external events (mouse etc.). No, because the functions always use Emacs xmalloc / xrealloc when Emacs invokes them. This is because emacs_readlink tells the lower-level primitives to use xmalloc and xrealloc. >> That doesn't suffice; the code should not only use ssize_t for >> readlink's returned value, but it should also use size_t for the >> buffer size > > Well, let's use that as well, then. What I sense that you're suggesting, is that we take all the fixes in the gnulib code, and port them all into the two duplicates of similar code that's in Emacs. That would be an error-prone process and would leave us with three copies of the code to maintain. It's better to have just one copy of the code. > Are we really going to consider seriously the case of a link name > overflowing a 64-bit ssize_t data type? As a general rule, GNU code shouldn't have arbitrary limits, and should defend against limits in underyling systems. Emacs is not as good as it should be about this, and I'm trying to make it better. This is just one example of many, found by static checking, and we might as well fix it while we're fixing all the others. > But why do we need to introduce the allocator and careadlinkat > modules, and all the nested function calls needed for them, just to > protect a simple code fragment from overflowing? It's more reliable to put the overflow checks in one place, where they can be carefully checked, than to duplicate the code in multiple places where it's easy for programmers to get it wrong. > We could refactor the duplicated code into a short function, and use > that. That's what's being done here. The function does even more than that, though, as it avoids the need to call malloc and free entirely, in the usual case. This is a performance win in the typical case. > please restructure the code so that neither allocator.h nor > careadlinkat are not used on platforms whose readlink is an > always-fail stub. That's not possible to do, unless we add more "#ifdef DOS_NT"s to the mainstream code. But that would be undesirable. The mainstream code should be written for the usual case, and platforms that lack the necessary primitives should supply their own stubs (which may always fail, but that's OK). That is standard porting technology, and it's an improvement over sprinking #ifdefs throughout the mainstream code. From debbugs-submit-bounces@debbugs.gnu.org Fri Apr 01 16:58:14 2011 Received: (at 8401) by debbugs.gnu.org; 1 Apr 2011 20:58:14 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5lQ5-0002D3-S7 for submit@debbugs.gnu.org; Fri, 01 Apr 2011 16:58:14 -0400 Received: from mtaout20.012.net.il ([80.179.55.166]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5lQ3-0002Cn-8x for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 16:58:12 -0400 Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0LIZ00G00QAVDY00@a-mtaout20.012.net.il> for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 23:57:55 +0300 (IDT) Received: from HOME-C4E4A596F7 ([77.126.47.180]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0LIZ00GTTRKFNN30@a-mtaout20.012.net.il>; Fri, 01 Apr 2011 23:57:53 +0300 (IDT) Date: Fri, 01 Apr 2011 23:57:54 +0300 From: Eli Zaretskii Subject: Re: removing duplication and improving the readlink code In-reply-to: <4D9630F2.1010806@cs.ucla.edu> X-012-Sender: halo1@inter.net.il To: Paul Eggert Message-id: <838vvty9il.fsf@gnu.org> References: <4D9574F2.20108@cs.ucla.edu> <83vcyypdzy.fsf@gnu.org> <4D9620CC.4000806@cs.ucla.edu> <83hbahyd64.fsf@gnu.org> <4D9630F2.1010806@cs.ucla.edu> X-Spam-Score: -2.1 (--) X-Debbugs-Envelope-To: 8401 Cc: 8401@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list Reply-To: Eli Zaretskii List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.1 (--) > Date: Fri, 01 Apr 2011 13:09:22 -0700 > From: Paul Eggert > CC: 8401@debbugs.gnu.org > > On 04/01/2011 12:38 PM, Eli Zaretskii wrote: > > > the functions you introduce use malloc/realloc, > > which I think will prevent their callers from being safe for > > asynchronous calls triggered by external events (mouse etc.). > > No, because the functions always use Emacs xmalloc / xrealloc when > Emacs invokes them. This is because emacs_readlink tells the > lower-level primitives to use xmalloc and xrealloc. You are right, I stand corrected. However, this just underlines the difficulty of reading the convoluted arrangement that this patch introduces. Any such difficulty is an impediment to maintenance. > >> That doesn't suffice; the code should not only use ssize_t for > >> readlink's returned value, but it should also use size_t for the > >> buffer size > > > > Well, let's use that as well, then. > > What I sense that you're suggesting, is that we take all the fixes in > the gnulib code, and port them all into the two duplicates of similar > code that's in Emacs. No. Gnulib code that you suggest to add does much more than just avoid overflow. See below. > > Are we really going to consider seriously the case of a link name > > overflowing a 64-bit ssize_t data type? > > As a general rule, GNU code shouldn't have arbitrary limits, and > should defend against limits in underyling systems. Sorry, that doesn't answer the question. The issue at hand is the price (in complexity and gratuitous added code) we should pay for a simple overflow test. I submit that the price you are asking for is way too high in this case. Where a single comparison will suffice to resolve a very remote possibility of overflow, you suggest to add 2 non-trivial gnulib modules. > Emacs is not as good as it should be about this, and I'm trying to > make it better. Your efforts are appreciated. But the issue at hand is not whether Emacs code can and should be made better. The issue is _how_ to do that. I respectfully submit that in this particular case, the technique selected for improving the code in this particular area is not the best alternative. > This is just one example of many, found by static checking, and we > might as well fix it while we're fixing all the others. I support fixing possible overflows, just not with sledgehammer style techniques such as this one. > > We could refactor the duplicated code into a short function, and use > > that. > > That's what's being done here. The function does even more than that, > though Exactly! > it avoids the need to call malloc and free entirely, in the usual > case. This is a performance win in the typical case. There should be a good reason for introducing this additional code; a remote possibility of an overflow is not such a good reason. As for performance win, the callers of this code are not performance critical, AFAICT. So this is an example of premature optimization, IMO. What I'm suggesting is to solve the overflow, and nothing else. Again, I can hardly believe that doing so would need more than a simple comparison, and I agree that a function which allocates a buffer and calls readlink with that buffer, while avoiding overflow, could be used in both places, to avoid code replication. From debbugs-submit-bounces@debbugs.gnu.org Fri Apr 01 21:57:41 2011 Received: (at 8401) by debbugs.gnu.org; 2 Apr 2011 01:57:41 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5q5t-0000Pk-1I for submit@debbugs.gnu.org; Fri, 01 Apr 2011 21:57:41 -0400 Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5q5q-0000PZ-Q2 for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 21:57:39 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id 1DD6C39E80FA; Fri, 1 Apr 2011 18:57:33 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id pji42Phw5BjV; Fri, 1 Apr 2011 18:57:32 -0700 (PDT) Received: from [192.168.1.10] (pool-71-189-109-235.lsanca.fios.verizon.net [71.189.109.235]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id 7A64F39E80F0; Fri, 1 Apr 2011 18:57:32 -0700 (PDT) Message-ID: <4D968284.1000009@cs.ucla.edu> Date: Fri, 01 Apr 2011 18:57:24 -0700 From: Paul Eggert Organization: UCLA Computer Science Department User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.14) Gecko/20110223 Thunderbird/3.1.8 MIME-Version: 1.0 To: Eli Zaretskii Subject: Re: removing duplication and improving the readlink code References: <4D9574F2.20108@cs.ucla.edu> <83vcyypdzy.fsf@gnu.org> <4D9620CC.4000806@cs.ucla.edu> <83hbahyd64.fsf@gnu.org> <4D9630F2.1010806@cs.ucla.edu> <838vvty9il.fsf@gnu.org> In-Reply-To: <838vvty9il.fsf@gnu.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Score: -3.0 (---) X-Debbugs-Envelope-To: 8401 Cc: 8401@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -3.0 (---) On 04/01/2011 01:57 PM, Eli Zaretskii wrote: > this just underlines the difficulty of reading the convoluted > arrangement that this patch introduces. Actually, the patch reduces the complexity of Emacs proper; it removes 58 lines and adds 39 lines. The patch's core is simple: a function emacs_readlink, whose body is two lines long, replaces duplicated code elsewhere in Emacs. There's nothing that hard about this. There is some indirection (which you're calling a "convoluted arrangement"), but that is normal when replacing inline code with a call to a parameterizable library. It's not rocket science. > this just underlines the difficulty of reading the convoluted > arrangement that this patch introduces I would think that only a quick and careless read of the code would lead one to believe that it calls malloc from Emacs without blocking interrupts. However, I wrote the code and perhaps am not the best to judge its clarity. A specific suggestion to make the code clearer (without introducing bugs or reducing flexibility) would be welcome. > Sorry, that doesn't answer the question. OK, then the answer is yes, I'm going to seriously consider the case of Emacs going bad when int or size_t or ssize_t overflows. This can occur when the underlying file system is corrupted and reports an incorrect file size. I've had this happen personally. In such cases Emacs should not crash. > There should be a good reason for introducing this There are at least four good reasons. The change simplifies Emacs's source code. It makes Emacs smaller and faster; for example, it typically reduces the number of system calls (on my RHEL 5.6 host a patched Emacs typically uses four fewer system calls to lock a file). The change improves the reliability of Emacs slightly, in unusual overflow cases. And the code is written and works. From debbugs-submit-bounces@debbugs.gnu.org Sun Apr 03 12:41:33 2011 Received: (at 8401) by debbugs.gnu.org; 3 Apr 2011 16:41:33 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q6QMn-0005Ma-5j for submit@debbugs.gnu.org; Sun, 03 Apr 2011 12:41:33 -0400 Received: from ironport2-out.teksavvy.com ([206.248.154.181] helo=ironport2-out.pppoe.ca) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q6QMk-0005MM-4e for 8401@debbugs.gnu.org; Sun, 03 Apr 2011 12:41:30 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AvsEAD+imE1Ld/gU/2dsb2JhbAClXHiIebddhWsElj+DTQ X-IronPort-AV: E=Sophos;i="4.63,292,1299474000"; d="scan'208";a="102933715" Received: from 75-119-248-20.dsl.teksavvy.com (HELO pastel.home) ([75.119.248.20]) by ironport2-out.pppoe.ca with ESMTP/TLS/ADH-AES256-SHA; 03 Apr 2011 12:41:22 -0400 Received: by pastel.home (Postfix, from userid 20848) id 3211558E7E; Sun, 3 Apr 2011 12:41:22 -0400 (EDT) From: Stefan Monnier To: Paul Eggert Subject: Re: bug#8401: removing duplication and improving the readlink code Message-ID: References: <4D9574F2.20108@cs.ucla.edu> <83vcyypdzy.fsf@gnu.org> <4D9620CC.4000806@cs.ucla.edu> <83hbahyd64.fsf@gnu.org> <4D9630F2.1010806@cs.ucla.edu> <838vvty9il.fsf@gnu.org> <4D968284.1000009@cs.ucla.edu> Date: Sun, 03 Apr 2011 12:41:22 -0400 In-Reply-To: <4D968284.1000009@cs.ucla.edu> (Paul Eggert's message of "Fri, 01 Apr 2011 18:57:24 -0700") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.1 (--) X-Debbugs-Envelope-To: 8401 Cc: Eli Zaretskii , 8401@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.1 (--) > source code. It makes Emacs smaller and faster; for example, it In which way does it make it smaller? Do you mean the source code exclusing the gnulib imported modues, or do you really mean the executable? Stefan From debbugs-submit-bounces@debbugs.gnu.org Mon Apr 04 00:38:30 2011 Received: (at 8401) by debbugs.gnu.org; 4 Apr 2011 04:38:30 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q6bYc-0005Xu-16 for submit@debbugs.gnu.org; Mon, 04 Apr 2011 00:38:30 -0400 Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q6bYY-0005Xg-SI for 8401@debbugs.gnu.org; Mon, 04 Apr 2011 00:38:27 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id F10DA39E80F9; Sun, 3 Apr 2011 21:38:20 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XTeoPjzikmFX; Sun, 3 Apr 2011 21:38:20 -0700 (PDT) Received: from [192.168.1.10] (pool-71-189-109-235.lsanca.fios.verizon.net [71.189.109.235]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id 72D8539E80B1; Sun, 3 Apr 2011 21:38:20 -0700 (PDT) Message-ID: <4D994B36.6050909@cs.ucla.edu> Date: Sun, 03 Apr 2011 21:38:14 -0700 From: Paul Eggert Organization: UCLA Computer Science Department User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.14) Gecko/20110223 Thunderbird/3.1.8 MIME-Version: 1.0 To: Stefan Monnier Subject: Re: bug#8401: removing duplication and improving the readlink code References: <4D9574F2.20108@cs.ucla.edu> <83vcyypdzy.fsf@gnu.org> <4D9620CC.4000806@cs.ucla.edu> <83hbahyd64.fsf@gnu.org> <4D9630F2.1010806@cs.ucla.edu> <838vvty9il.fsf@gnu.org> <4D968284.1000009@cs.ucla.edu> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Score: -3.0 (---) X-Debbugs-Envelope-To: 8401 Cc: Eli Zaretskii , 8401@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -3.0 (---) On 04/03/2011 09:41 AM, Stefan Monnier wrote: > In which way does it make it smaller? Do you mean the source code > exclusing the gnulib imported modues, or do you really mean > the executable? I meant less memory used at runtime, because a patched Emacs typically skips a malloc when it reads a link. (The non-gnulib source code also shrinks.) These effects are both minor, of course. From debbugs-submit-bounces@debbugs.gnu.org Wed Apr 06 01:47:46 2011 Received: (at 8401-done) by debbugs.gnu.org; 6 Apr 2011 05:47:46 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q7Laj-0006RT-GW for submit@debbugs.gnu.org; Wed, 06 Apr 2011 01:47:45 -0400 Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q7Lah-0006RF-DP; Wed, 06 Apr 2011 01:47:44 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id 71E7339E80F0; Tue, 5 Apr 2011 22:47:37 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id bpiraXhtX56i; Tue, 5 Apr 2011 22:47:37 -0700 (PDT) Received: from [192.168.1.10] (pool-71-189-109-235.lsanca.fios.verizon.net [71.189.109.235]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id 2BF8039E80B1; Tue, 5 Apr 2011 22:47:37 -0700 (PDT) Message-ID: <4D9BFE64.8090502@cs.ucla.edu> Date: Tue, 05 Apr 2011 22:47:16 -0700 From: Paul Eggert Organization: UCLA Computer Science Department User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.14) Gecko/20110223 Thunderbird/3.1.8 MIME-Version: 1.0 To: 8401-done@debbugs.gnu.org, 8410-done@debbugs.gnu.org Subject: fix installed in trunk Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Score: -3.0 (---) X-Debbugs-Envelope-To: 8401-done X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -3.0 (---) I committed the previously-mentioned fix as part of the merge to the trunk in bzr 103841. From unknown Mon Jun 23 11:28:11 2025 Received: (at fakecontrol) by fakecontrolmessage; To: internal_control@debbugs.gnu.org From: Debbugs Internal Request Subject: Internal Control Message-Id: bug archived. Date: Wed, 04 May 2011 11:24:05 +0000 User-Agent: Fakemail v42.6.9 # This is a fake control message. # # The action: # bug archived. thanks # This fakemail brought to you by your local debbugs # administrator