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

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 @@
 \f
 /* 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);
 
 \f
 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;

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

[Stefan Monnier]

> 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.

At least on my GNU/Linux system, "grep PATH.\*MAX /usr/include/**/*.h"
seems to suggest that file names longer than 4096 are not supported ;-)


        Stefan

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

[Paul Eggert]

On 08/29/11 08:59, Stefan Monnier wrote:
>> 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.
> 
> At least on my GNU/Linux system, "grep PATH.\*MAX /usr/include/**/*.h"
> seems to suggest that file names longer than 4096 are not supported ;-)

Yes, point taken.  However, ctags cannot assume that BUFSIZ is greater
than PATH_MAX on all systems, as that assumption is not true everywhere.
And even on your GNU/Linux system, ctags should not dump core
merely because FOO's name is so long that FOO can't be opened.

bug#9397: patch committed

[Paul Eggert]

I committed the patch for this bug into the trunk
as bzr 105655 and am marking the bug as done.