bug#9397: lib-src integer and memory overflow issues

[Paul Eggert]

Package: Emacs
Version: 24.0.50
Tags: patch

Here's a patch to the Emacs trunk to fix some integer and memory
overflow issues in lib-src programs.  Currently, these programs can
mess up on 64-bit hosts when given long strings, or uids greater than
INT_MAX.  In some cases the messups can occur on 32-bit hosts too.  I
plan to install this patch after a bit more testing.

Here's an example bug fixed by the patch: "ctags -u -o FOO *.c"
crashes on Fedora 14 x86-64 if the file name FOO is longer than 8192
bytes.

=== modified file 'lib-src/ChangeLog'
--- lib-src/ChangeLog   2011-07-28 00:48:01 +0000
+++ lib-src/ChangeLog   2011-08-28 23:59:14 +0000
@@ -1,3 +1,38 @@
+2011-08-28  Paul Eggert  <eggert@cs.ucla.edu>
+
+       Integer and memory overflow issues.
+
+       * emacsclient.c (xmalloc): Accept size_t, not unsigned int, to
+       avoid potential buffer overflow issues on typical 64-bit hosts.
+       Return void *, not long *.
+       (get_current_dir_name): Report a failure, instead of looping
+       forever, if buffer size calculation overflows.  Treat malloc
+       failures like realloc failures, as that has better behavior and is
+       more consistent.  Do not check whether xmalloc returns NULL, as
+       that's not possible.
+       (message): Do not arbitrarily truncate message to 2048 bytes when
+       sending it to stderr; use vfprintf instead.
+       (get_server_config, set_local_socket)
+       (start_daemon_and_retry_set_socket): Do not alloca
+       arbitrarily-large buffers; that's not safe.
+       (get_server_config, set_local_socket): Do not use sprintf when its
+       result might not fit in 'int'.
+       (set_local_socket): Do not assume uid fits in 'int'.
+
+       * etags.c (xmalloc, xrealloc): Accept size_t, not unsigned int,
+       to avoid potential buffer overflow issues on typical 64-bit hosts.
+       (whatlen_max): New static var.
+       (main): Avoid buffer overflow if subsidiary command length is
+       greater than BUFSIZ or 2*BUFSIZ + 20. Do not use sprintf when its
+       result might not fit in 'int'.
+
+       * movemail.c (main): Do not use sprintf when its result might not fit
+       in 'int'.  Instead, put the possibly-long file name into the
+       output of pfatal_with_name.
+
+       * update-game-score.c: Include <limits.h>
+       (get_user_id): Do not assume uid fits in 'int'.  Simplify.
+
 2011-07-28  Paul Eggert  <eggert@cs.ucla.edu>
 
        Assume freestanding C89 headers, string.h, stdlib.h.

=== modified file 'lib-src/emacsclient.c'
--- lib-src/emacsclient.c       2011-07-02 15:07:57 +0000
+++ lib-src/emacsclient.c       2011-08-28 23:52:34 +0000
@@ -194,10 +194,10 @@
 
 /* Like malloc but get fatal error if memory is exhausted.  */
 
-static long *
-xmalloc (unsigned int size)
+static void *
+xmalloc (size_t size)
 {
-  long *result = (long *) malloc (size);
+  void *result = malloc (size);
   if (result == NULL)
     {
       perror ("malloc");
@@ -250,32 +250,33 @@
       )
     {
       buf = (char *) xmalloc (strlen (pwd) + 1);
-      if (!buf)
-        return NULL;
       strcpy (buf, pwd);
     }
 #ifdef HAVE_GETCWD
   else
     {
       size_t buf_size = 1024;
-      buf = (char *) xmalloc (buf_size);
-      if (!buf)
-        return NULL;
       for (;;)
         {
+         int tmp_errno;
+         buf = malloc (buf_size);
+         if (! buf)
+           break;
           if (getcwd (buf, buf_size) == buf)
             break;
-          if (errno != ERANGE)
+         tmp_errno = errno;
+         free (buf);
+         if (tmp_errno != ERANGE)
             {
-              int tmp_errno = errno;
-              free (buf);
               errno = tmp_errno;
               return NULL;
             }
           buf_size *= 2;
-          buf = (char *) realloc (buf, buf_size);
-          if (!buf)
-            return NULL;
+         if (! buf_size)
+           {
+             errno = ENOMEM;
+             return NULL;
+           }
         }
     }
 #else
@@ -283,8 +284,6 @@
     {
       /* We need MAXPATHLEN here.  */
       buf = (char *) xmalloc (MAXPATHLEN + 1);
-      if (!buf)
-        return NULL;
       if (getwd (buf) == NULL)
         {
           int tmp_errno = errno;
@@ -494,16 +493,17 @@
 static void
 message (int is_error, const char *format, ...)
 {
-  char msg[2048];
   va_list args;
 
   va_start (args, format);
-  vsprintf (msg, format, args);
-  va_end (args);
 
 #ifdef WINDOWSNT
   if (w32_window_app ())
     {
+      char msg[2048];
+      vsnprintf (msg, sizeof msg, format, args);
+      msg[sizeof msg - 1] = '\0';
+
       if (is_error)
        MessageBox (NULL, msg, "Emacsclient ERROR", MB_ICONERROR);
       else
@@ -514,9 +514,11 @@
     {
       FILE *f = is_error ? stderr : stdout;
 
-      fputs (msg, f);
+      vfprintf (f, format, args);
       fflush (f);
     }
+
+  va_end (args);
 }
 
 /* Decode the options from argv and argc.
@@ -959,18 +961,24 @@
 
       if (home)
         {
-          char *path = alloca (strlen (home) + strlen (server_file)
-                              + EXTRA_SPACE);
-          sprintf (path, "%s/.emacs.d/server/%s", home, server_file);
+         char *path = xmalloc (strlen (home) + strlen (server_file)
+                               + EXTRA_SPACE);
+         strcpy (path, home);
+         strcat (path, "/.emacs.d/server/");
+         strcat (path, server_file);
           config = fopen (path, "rb");
+         free (path);
         }
 #ifdef WINDOWSNT
       if (!config && (home = egetenv ("APPDATA")))
         {
-          char *path = alloca (strlen (home) + strlen (server_file)
-                              + EXTRA_SPACE);
-          sprintf (path, "%s/.emacs.d/server/%s", home, server_file);
+         char *path = xmalloc (strlen (home) + strlen (server_file)
+                               + EXTRA_SPACE);
+         strcpy (path, home);
+         strcat (path, "/.emacs.d/server/");
+         strcat (path, server_file);
           config = fopen (path, "rb");
+         free (path);
         }
 #endif
     }
@@ -1243,6 +1251,8 @@
     int saved_errno = 0;
     const char *server_name = "server";
     const char *tmpdir IF_LINT ( = NULL);
+    char *tmpdir_storage = NULL;
+    char *socket_name_storage = NULL;
 
     if (socket_name && !strchr (socket_name, '/')
        && !strchr (socket_name, '\\'))
@@ -1255,6 +1265,8 @@
 
     if (default_sock)
       {
+       long uid = geteuid ();
+       ptrdiff_t tmpdirlen;
        tmpdir = egetenv ("TMPDIR");
        if (!tmpdir)
           {
@@ -1265,17 +1277,19 @@
             size_t n = confstr (_CS_DARWIN_USER_TEMP_DIR, NULL, (size_t) 0);
             if (n > 0)
               {
-                tmpdir = alloca (n);
+               tmpdir = tmpdir_storage = xmalloc (n);
                 confstr (_CS_DARWIN_USER_TEMP_DIR, tmpdir, n);
               }
             else
 #endif
               tmpdir = "/tmp";
           }
-       socket_name = alloca (strlen (tmpdir) + strlen (server_name)
-                             + EXTRA_SPACE);
-       sprintf (socket_name, "%s/emacs%d/%s",
-                tmpdir, (int) geteuid (), server_name);
+       tmpdirlen = strlen (tmpdir);
+       socket_name = socket_name_storage =
+         xmalloc (tmpdirlen + strlen (server_name) + EXTRA_SPACE);
+       strcpy (socket_name, tmpdir);
+       sprintf (socket_name + tmpdirlen, "/emacs%ld/", uid);
+       strcat (socket_name + tmpdirlen, server_name);
       }
 
     if (strlen (socket_name) < sizeof (server.sun_path))
@@ -1309,10 +1323,13 @@
            if (pw && (pw->pw_uid != geteuid ()))
              {
                /* We're running under su, apparently. */
-               socket_name = alloca (strlen (tmpdir) + strlen (server_name)
-                                     + EXTRA_SPACE);
-               sprintf (socket_name, "%s/emacs%d/%s",
-                        tmpdir, (int) pw->pw_uid, server_name);
+               long uid = pw->pw_uid;
+               ptrdiff_t tmpdirlen = strlen (tmpdir);
+               socket_name = xmalloc (tmpdirlen + strlen (server_name)
+                                      + EXTRA_SPACE);
+               strcpy (socket_name, tmpdir);
+               sprintf (socket_name + tmpdirlen, "/emacs%ld/", uid);
+               strcat (socket_name + tmpdirlen, server_name);
 
                if (strlen (socket_name) < sizeof (server.sun_path))
                  strcpy (server.sun_path, socket_name);
@@ -1322,6 +1339,7 @@
                             progname, socket_name);
                    exit (EXIT_FAILURE);
                  }
+               free (socket_name);
 
                sock_status = socket_status (server.sun_path);
                 saved_errno = errno;
@@ -1331,6 +1349,9 @@
          }
       }
 
+    free (socket_name_storage);
+    free (tmpdir_storage);
+
     switch (sock_status)
       {
       case 1:
@@ -1526,8 +1547,8 @@
        {
          /* Pass  --daemon=socket_name as argument.  */
          const char *deq = "--daemon=";
-         char *daemon_arg = alloca (strlen (deq)
-                                    + strlen (socket_name) + 1);
+         char *daemon_arg = xmalloc (strlen (deq)
+                                     + strlen (socket_name) + 1);
          strcpy (daemon_arg, deq);
          strcat (daemon_arg, socket_name);
          d_argv[1] = daemon_arg;

=== modified file 'lib-src/etags.c'
--- lib-src/etags.c     2011-07-07 01:32:56 +0000
+++ lib-src/etags.c     2011-08-28 23:55:41 +0000
@@ -414,8 +414,8 @@
 static void canonicalize_filename (char *);
 static void linebuffer_init (linebuffer *);
 static void linebuffer_setlen (linebuffer *, int);
-static PTR xmalloc (unsigned int);
-static PTR xrealloc (char *, unsigned int);
+static PTR xmalloc (size_t);
+static PTR xrealloc (char *, size_t);
 
 
 static char searchar = '/';    /* use /.../ searches */
@@ -425,6 +425,7 @@
 static char *cwd;              /* current working directory */
 static char *tagfiledir;       /* directory of tagfile */
 static FILE *tagf;             /* ioptr for tags file */
+static ptrdiff_t whatlen_max;  /* maximum length of any 'what' member */
 
 static fdesc *fdhead;          /* head of file description list */
 static fdesc *curfdp;          /* current file description */
@@ -1066,6 +1067,7 @@
   int current_arg, file_count;
   linebuffer filename_lb;
   bool help_asked = FALSE;
+  ptrdiff_t len;
  char *optstring;
  int opt;
 
@@ -1110,6 +1112,9 @@
        /* This means that a file name has been seen.  Record it. */
        argbuffer[current_arg].arg_type = at_filename;
        argbuffer[current_arg].what     = optarg;
+       len = strlen (optarg);
+       if (whatlen_max < len)
+         whatlen_max = len;
        ++current_arg;
        ++file_count;
        break;
@@ -1118,6 +1123,9 @@
        /* Parse standard input.  Idea by Vivek <vivek@etla.org>. */
        argbuffer[current_arg].arg_type = at_stdin;
        argbuffer[current_arg].what     = optarg;
+       len = strlen (optarg);
+       if (whatlen_max < len)
+         whatlen_max = len;
        ++current_arg;
        ++file_count;
        if (parsing_stdin)
@@ -1160,6 +1168,9 @@
       case 'r':
        argbuffer[current_arg].arg_type = at_regexp;
        argbuffer[current_arg].what = optarg;
+       len = strlen (optarg);
+       if (whatlen_max < len)
+         whatlen_max = len;
        ++current_arg;
        break;
       case 'R':
@@ -1198,6 +1209,9 @@
     {
       argbuffer[current_arg].arg_type = at_filename;
       argbuffer[current_arg].what = argv[optind];
+      len = strlen (argv[optind]);
+      if (whatlen_max < len)
+       whatlen_max = len;
       ++current_arg;
       ++file_count;
     }
@@ -1331,7 +1345,9 @@
   /* From here on, we are in (CTAGS && !cxref_style) */
   if (update)
     {
-      char cmd[BUFSIZ];
+      char *cmd =
+       xmalloc (strlen (tagfile) + whatlen_max +
+                sizeof "mv..OTAGS;fgrep -v '\t\t' OTAGS >;rm OTAGS");
       for (i = 0; i < current_arg; ++i)
        {
          switch (argbuffer[i].arg_type)
@@ -1342,12 +1358,17 @@
            default:
              continue;         /* the for loop */
            }
-         sprintf (cmd,
-                  "mv %s OTAGS;fgrep -v '\t%s\t' OTAGS >%s;rm OTAGS",
-                  tagfile, argbuffer[i].what, tagfile);
+         strcpy (cmd, "mv ");
+         strcat (cmd, tagfile);
+         strcat (cmd, " OTAGS;fgrep -v '\t");
+         strcat (cmd, argbuffer[i].what);
+         strcat (cmd, "\t' OTAGS >");
+         strcat (cmd, tagfile);
+         strcat (cmd, ";rm OTAGS");
          if (system (cmd) != EXIT_SUCCESS)
            fatal ("failed to execute shell command", (char *)NULL);
        }
+      free (cmd);
       append_to_tagfile = TRUE;
     }
 
@@ -1363,11 +1384,14 @@
   if (CTAGS)
     if (append_to_tagfile || update)
       {
-       char cmd[2*BUFSIZ+20];
+       char *cmd = xmalloc (2 * strlen (tagfile) + sizeof "sort -u -o..");
        /* Maybe these should be used:
           setenv ("LC_COLLATE", "C", 1);
           setenv ("LC_ALL", "C", 1); */
-       sprintf (cmd, "sort -u -o %.*s %.*s", BUFSIZ, tagfile, BUFSIZ, tagfile);
+       strcpy (cmd, "sort -u -o ");
+       strcat (cmd, tagfile);
+       strcat (cmd, " ");
+       strcat (cmd, tagfile);
        exit (system (cmd));
       }
   return EXIT_SUCCESS;
@@ -6656,7 +6680,7 @@
 
 /* Like malloc but get fatal error if memory is exhausted. */
 static PTR
-xmalloc (unsigned int size)
+xmalloc (size_t size)
 {
   PTR result = (PTR) malloc (size);
   if (result == NULL)
@@ -6665,7 +6689,7 @@
 }
 
 static PTR
-xrealloc (char *ptr, unsigned int size)
+xrealloc (char *ptr, size_t size)
 {
   PTR result = (PTR) realloc (ptr, size);
   if (result == NULL)

=== modified file 'lib-src/movemail.c'
--- lib-src/movemail.c  2011-07-07 01:32:56 +0000
+++ lib-src/movemail.c  2011-08-28 23:57:19 +0000
@@ -325,11 +325,10 @@
          if (desc < 0)
            {
              int mkstemp_errno = errno;
-             char *message = (char *) xmalloc (strlen (tempname) + 50);
-             sprintf (message, "creating %s, which would become the lock file",
-                      tempname);
+             error ("error while creating what would become the lock file",
+                    0, 0);
              errno = mkstemp_errno;
-             pfatal_with_name (message);
+             pfatal_with_name (tempname);
            }
          close (desc);
 

=== modified file 'lib-src/update-game-score.c'
--- lib-src/update-game-score.c 2011-07-11 06:05:57 +0000
+++ lib-src/update-game-score.c 2011-08-28 23:59:14 +0000
@@ -35,6 +35,7 @@
 
 #include <unistd.h>
 #include <errno.h>
+#include <limits.h>
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
@@ -128,19 +129,13 @@
 static char *
 get_user_id (void)
 {
-  char *name;
   struct passwd *buf = getpwuid (getuid ());
   if (!buf)
     {
-      int count = 1;
-      int uid = (int) getuid ();
-      int tuid = uid;
-      while (tuid /= 10)
-       count++;
-      name = malloc (count+1);
-      if (!name)
-       return NULL;
-      sprintf (name, "%d", uid);
+      long uid = getuid ();
+      char *name = malloc (sizeof uid * CHAR_BIT / 3 + 1);
+      if (name)
+       sprintf (name, "%ld", uid);
       return name;
     }
   return buf->pw_name;