Subject: bug#9397: closed (patch committed)

From: [email-masked] (GNU bug Tracking System)
Date: Sun, 04 Sep 2011 22:00:03 +0000

Your bug report

#9397: lib-src integer and memory overflow issues

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 [email-masked].

-- 
9397: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=9397
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: patch committed
From: Paul Eggert <[email-masked]>
To: [email-masked], [email-masked]
Date: Sun, 04 Sep 2011 14:56:09 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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

Subject: lib-src integer and memory overflow issues
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Sun, 28 Aug 2011 17:18:59 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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  <[email-masked]>
+
+	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  <[email-masked]>
 
 	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 <[email-masked]>. */
 	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;