GNU bug report logs - #9397
lib-src integer and memory overflow issues

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9397 in the body.
You can then email your comments to 9397 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Subject: lib-src integer and memory overflow issues
Date: Sun, 28 Aug 2011 17:18:59 -0700

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 <at> 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 <at> 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 <at> 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;

Message #8 received at 9397 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 9397 <at> debbugs.gnu.org
Subject: Re: bug#9397: lib-src integer and memory overflow issues
Date: Mon, 29 Aug 2011 11:59:26 -0400

> 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

Message #11 received at 9397 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 9397 <at> debbugs.gnu.org
Subject: Re: bug#9397: lib-src integer and memory overflow issues
Date: Mon, 29 Aug 2011 10:27:41 -0700

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.

Message #16 received at 9397-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: 9397-done <at> debbugs.gnu.org, 9412-done <at> debbugs.gnu.org
Subject: patch committed
Date: Sun, 04 Sep 2011 14:56:09 -0700

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.