bug#9196: integer and memory overflow issues (e.g., cut-and-paste crashes Emacs)

[Paul Eggert <eggert@cs.ucla.edu>]

On 08/05/2011 02:26 AM, Jan Djärv wrote:
>> +      static char const xdefaults[] = ".Xdefaults-";
> 
> I think there might be problems with dumping and static variables. 
> There is a reason for initializing static variables in init-functions
> rather in an initializer.  I don't remember the details.

In the old days, Emacs sometimes did '#define static /* empty */' as
part of its undumping scheme, which meant that static variables inside
functions didn't preserve their values from call to call.  Emacs no
longer does that, so we're OK here.  (And even if Emacs still did
that, this particular code would be safe, as this particular variable
would be reinitialized to the correct value on every call.)

>> +      char *home = gethomedir ();
>> +      char const *host = get_system_name ();
>> +      ptrdiff_t pathsize = strlen (home) + sizeof xdefaults + strlen (host);
>> +      path = (char *) xrealloc (home, pathsize);
>> +      strcat (strcat (path, xdefaults), host);
>>         p = path;
>>       }
>>
>>     db = XrmGetFileDatabase (p);
>>
>>     xfree (path);
>> -  xfree (home);
> 
> Since home isn't free:d, you have introduced a memory leak.

No, we should be OK here -- the realloc frees 'home'.

>> @@ -628,9 +641,9 @@
>>         else
>>       {
>>         /* Send an INCR tag to initiate incremental transfer.  */
>> -      long value[1];
>> +      unsigned long value[1];
> 
> This is wrong, X11 expects long, not unsigned long.  Even if the value
> here can't be negative, it can be in other situations, so stick with
> long for consistency.

Thanks, will do.

>> -      value[0] = bytes_remaining;
>> +      value[0] = min (bytes_remaining, X_ULONG_MAX);
> 
> X_ULONG_MAX is wrong, max value is LONG_MAX as X11 can accept negative
> values.

Thanks again, I'll fix that too.

>> -  total_size = bytes_remaining + 1;
>> -  *data_ret = (unsigned char *) xmalloc (total_size);
>> +  if (total_size_max<  bytes_remaining)
>> +    goto size_overflow;
>> +  total_size = bytes_remaining;
>> +  data = malloc (total_size + 1);
> 
> Why isn't xmalloc used here?

This code is already protected against interrupts and it already
checks for memory exhaustion.  There's no need to call xmalloc, and
there are potential problems if the nested interrupt-blocking goes
awry.

>> +        idata[i] = ldata[i];
> 
> You must have the cast to int, otherwise there will be a warning on 64
> bit hosts (possible loss of precision).

That sort of thing used to be a concern, but it isn't any more.  The
existing Emacs code must have hundreds of assignments of 64-bit values
to 32-bit variables without a cast (on 64-bit hosts).  Here's one
example, taken from the trunk's xselect.c:

      usecs = (x_selection_timeout % 1000) * 1000;

It is possible to cause GCC to warn about these assignments, but
nobody does that any more because it cries wolf far too often.  Since
the rest of Emacs doesn't worry about this issue, we needn't worry
about it here.

>>     unsigned char *data = (unsigned char *) event->data.b;
>> -  int idata[5];
>> +  unsigned int idata[5];
> 
> idata can be signed, so why the change to unsigned?

Sorry, I misunderstood that.  I'll change it back to 'signed'.

> IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWXkU8k0BAh7/gH/wAht7////
> 
> Why send this?  What is it?  It seems to just take up bandwidth.

It records all the details of the patch, including intervening
changes, in a format suitable for bzr.  It's generated by "bzr send",
as suggested in <http://www.emacswiki.org/emacs/BzrForEmacsDevs>.  But
I agree it's pretty useless here, so I'll omit it from now on.

Here's a further patch that I hope addresses all the comments
satisfactorily.  And thanks again for the review.


=== modified file 'src/xselect.c'
--- src/xselect.c	2011-08-05 02:15:35 +0000
+++ src/xselect.c	2011-08-06 01:10:24 +0000
@@ -111,9 +111,11 @@ static Lisp_Object Qx_lost_selection_fun
    is not necessarily sizeof (long).  */
 #define X_LONG_SIZE 4
 
-/* Maximum unsigned 'short' and 'long' values suitable for libX11.  */
-#define X_USHRT_MAX 0xffff
-#define X_ULONG_MAX 0xffffffff
+/* Extreme 'short' and 'long' values suitable for libX11.  */
+#define X_SHRT_MAX 0x7fff
+#define X_SHRT_MIN (-1 - X_SHRT_MAX)
+#define X_LONG_MAX 0x7fffffff
+#define X_LONG_MIN (-1 - X_LONG_MAX)
 
 /* If this is a smaller number than the max-request-size of the display,
    emacs will use INCR selection transfer when the selection is larger
@@ -641,7 +643,7 @@ x_reply_selection_request (struct input_
       else
 	{
 	  /* Send an INCR tag to initiate incremental transfer.  */
-	  unsigned long value[1];
+	  long value[1];
 
 	  TRACE2 ("Start sending %"pD"d bytes incrementally (%s)",
 		  bytes_remaining, XGetAtomName (display, cs->property));
@@ -651,7 +653,7 @@ x_reply_selection_request (struct input_
 
 	  /* XChangeProperty expects an array of long even if long is
 	     more than 32 bits.  */
-	  value[0] = min (bytes_remaining, X_ULONG_MAX);
+	  value[0] = min (bytes_remaining, X_LONG_MAX);
 	  XChangeProperty (display, window, cs->property,
 			   dpyinfo->Xatom_INCR, 32, PropModeReplace,
 			   (unsigned char *) value, 1);
@@ -1764,13 +1766,13 @@ lisp_data_to_selection_data (Display *di
       (*(Atom **) data_ret) [0] = symbol_to_x_atom (dpyinfo, obj);
       if (NILP (type)) type = QATOM;
     }
-  else if (RANGED_INTEGERP (0, obj, X_USHRT_MAX))
+  else if (RANGED_INTEGERP (X_SHRT_MIN, obj, X_SHRT_MAX))
     {
       *data_ret = (unsigned char *) xmalloc (sizeof (short) + 1);
       *format_ret = 16;
       *size_ret = 1;
       (*data_ret) [sizeof (short)] = 0;
-      (*(unsigned short **) data_ret) [0] = XINT (obj);
+      (*(short **) data_ret) [0] = XINT (obj);
       if (NILP (type)) type = QINTEGER;
     }
   else if (INTEGERP (obj)
@@ -1783,7 +1785,7 @@ lisp_data_to_selection_data (Display *di
       *format_ret = 32;
       *size_ret = 1;
       (*data_ret) [sizeof (long)] = 0;
-      (*(unsigned long **) data_ret) [0] = cons_to_unsigned (obj, X_ULONG_MAX);
+      (*(long **) data_ret) [0] = cons_to_signed (obj, X_LONG_MIN, X_LONG_MAX);
       if (NILP (type)) type = QINTEGER;
     }
   else if (VECTORP (obj))
@@ -1817,25 +1819,30 @@ lisp_data_to_selection_data (Display *di
 	  int data_size = sizeof (short);
 	  if (NILP (type)) type = QINTEGER;
 	  for (i = 0; i < size; i++)
-	    if (X_USHRT_MAX
-		< cons_to_unsigned (XVECTOR (obj)->contents[i], X_ULONG_MAX))
-	      {
-		/* Use sizeof (long) even if it is more than 32 bits.
-		   See comment in x_get_window_property and
-		   x_fill_property_data.  */
-		data_size = sizeof (long);
-		format = 32;
-	      }
+	    {
+	      intmax_t v = cons_to_signed (XVECTOR (obj)->contents[i],
+					   X_LONG_MIN, X_LONG_MAX);
+	      if (X_SHRT_MIN <= v && v <= X_SHRT_MAX)
+		{
+		  /* Use sizeof (long) even if it is more than 32 bits.
+		     See comment in x_get_window_property and
+		     x_fill_property_data.  */
+		  data_size = sizeof (long);
+		  format = 32;
+		}
+	    }
 	  *data_ret = xnmalloc (size, data_size);
 	  *format_ret = format;
 	  *size_ret = size;
 	  for (i = 0; i < size; i++)
-	    if (format == 32)
-	      (*((unsigned long **) data_ret)) [i] =
-		cons_to_unsigned (XVECTOR (obj)->contents[i], X_ULONG_MAX);
-	    else
-	      (*((unsigned short **) data_ret)) [i] =
-		cons_to_unsigned (XVECTOR (obj)->contents[i], X_USHRT_MAX);
+	    {
+	      long v = cons_to_signed (XVECTOR (obj)->contents[i],
+				       X_LONG_MIN, X_LONG_MAX);
+	      if (format == 32)
+		(*((long **) data_ret)) [i] = v;
+	      else
+		(*((short **) data_ret)) [i] = v;
+	    }
 	}
     }
   else
@@ -2479,7 +2486,7 @@ x_handle_dnd_message (struct frame *f, X
   unsigned long size = 160/event->format;
   int x, y;
   unsigned char *data = (unsigned char *) event->data.b;
-  unsigned int idata[5];
+  int idata[5];
   ptrdiff_t i;
 
   for (i = 0; i < dpyinfo->x_dnd_atoms_length; ++i)