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

["Jan Djärv" <jan.h.d@swipnet.se>]

Paul Eggert skrev 2011-08-05 04:33:

> === modified file 'src/xrdb.c'
> --- src/xrdb.c	2011-07-10 08:20:10 +0000
> +++ src/xrdb.c	2011-08-05 02:15:35 +0000
> @@ -426,24 +426,22 @@
>   {
>     XrmDatabase db;
>     char *p;
> -  char *path = 0, *home = 0;
> -  const char *host;
> +  char *path = 0;
>
>     if ((p = getenv ("XENVIRONMENT")) == NULL)
>       {
> -      home = gethomedir ();
> -      host = get_system_name ();
> -      path = (char *) xmalloc (strlen (home)
> -			      + sizeof (".Xdefaults-")
> -			      + strlen (host));
> -      sprintf (path, "%s%s%s", home, ".Xdefaults-", host);
> +      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.

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


> === modified file 'src/xselect.c'
> --- src/xselect.c	2011-07-13 03:45:56 +0000
> +++ src/xselect.c	2011-08-05 02:15:35 +0000
> @@ -66,22 +66,15 @@
>   static void wait_for_property_change (struct prop_location *);
>   static Lisp_Object x_get_foreign_selection (Lisp_Object, Lisp_Object,
>   					    Lisp_Object, Lisp_Object);
> -static void x_get_window_property (Display *, Window, Atom,
> -                                   unsigned char **, int *,
> -                                   Atom *, int *, unsigned long *, int);
> -static void receive_incremental_selection (Display *, Window, Atom,
> -                                           Lisp_Object, unsigned,
> -                                           unsigned char **, int *,
> -                                           Atom *, int *, unsigned long *);
>   static Lisp_Object x_get_window_property_as_lisp_data (Display *,
>                                                          Window, Atom,
>                                                          Lisp_Object, Atom);
>   static Lisp_Object selection_data_to_lisp_data (Display *,
>   						const unsigned char *,
> -                                                int, Atom, int);
> +						ptrdiff_t, Atom, int);
>   static void lisp_data_to_selection_data (Display *, Lisp_Object,
>                                            unsigned char **, Atom *,
> -                                         unsigned *, int *, int *);
> +					 ptrdiff_t *, int *, int *);
>   static Lisp_Object clean_local_selection_data (Lisp_Object);
>
>   /* Printing traces to stderr.  */
> @@ -114,15 +107,37 @@
>   static Lisp_Object Qforeign_selection;
>   static Lisp_Object Qx_lost_selection_functions, Qx_sent_selection_functions;
>
> +/* Bytes needed to represent 'long' data.  This is as per libX11; it
> +   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
> +
>   /* 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
>      than this.  The max-request-size is usually around 64k, so if you want
>      emacs to use incremental selection transfers when the selection is
>      smaller than that, set this.  I added this mostly for debugging the
> -   incremental transfer stuff, but it might improve server performance.  */
> -#define MAX_SELECTION_QUANTUM 0xFFFFFF
> -
> -#define SELECTION_QUANTUM(dpy) ((XMaxRequestSize(dpy)<<  2) - 100)
> +   incremental transfer stuff, but it might improve server performance.
> +
> +   This value cannot exceed INT_MAX / max (X_LONG_SIZE, sizeof (long))
> +   because it is multiplied by X_LONG_SIZE and by sizeof (long) in
> +   subscript calculations.  Similarly for PTRDIFF_MAX - 1 or SIZE_MAX
> +   - 1 in place of INT_MAX.  */
> +#define MAX_SELECTION_QUANTUM						\
> +  ((int) min (0xFFFFFF, (min (INT_MAX, min (PTRDIFF_MAX, SIZE_MAX) - 1)	\
> +			 / max (X_LONG_SIZE, sizeof (long)))))
> +
> +static int
> +selection_quantum (Display *display)
> +{
> +  long mrs = XMaxRequestSize (display);
> +  return (mrs<  MAX_SELECTION_QUANTUM / X_LONG_SIZE + 25
> +	  ? (mrs - 25) * X_LONG_SIZE
> +	  : MAX_SELECTION_QUANTUM);
> +}
>
>   #define LOCAL_SELECTION(selection_symbol,dpyinfo)			\
>     assq_no_quit (selection_symbol, dpyinfo->terminal->Vselection_alist)
> @@ -477,7 +492,7 @@
>   struct selection_data
>   {
>     unsigned char *data;
> -  unsigned int size;
> +  ptrdiff_t size;
>     int format;
>     Atom type;
>     int nofree;
> @@ -581,14 +596,11 @@
>     XSelectionEvent *reply =&(reply_base.xselection);
>     Display *display = SELECTION_EVENT_DISPLAY (event);
>     Window window = SELECTION_EVENT_REQUESTOR (event);
> -  int bytes_remaining;
> -  int max_bytes = SELECTION_QUANTUM (display);
> +  ptrdiff_t bytes_remaining;
> +  int max_bytes = selection_quantum (display);
>     int count = SPECPDL_INDEX ();
>     struct selection_data *cs;
>
> -  if (max_bytes>  MAX_SELECTION_QUANTUM)
> -    max_bytes = MAX_SELECTION_QUANTUM;
> -
>     reply->type = SelectionNotify;
>     reply->display = display;
>     reply->requestor = window;
> @@ -616,11 +628,12 @@
>         if (cs->property == None)
>   	continue;
>
> -      bytes_remaining = cs->size * (cs->format / 8);
> +      bytes_remaining = cs->size;
> +      bytes_remaining *= cs->format>>  3;
>         if (bytes_remaining<= max_bytes)
>   	{
>   	  /* Send all the data at once, with minimal handshaking.  */
> -	  TRACE1 ("Sending all %d bytes", bytes_remaining);
> +	  TRACE1 ("Sending all %"pD"d bytes", bytes_remaining);
>   	  XChangeProperty (display, window, cs->property,
>   			   cs->type, cs->format, PropModeReplace,
>   			   cs->data, cs->size);
> @@ -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.

>
> -	  TRACE2 ("Start sending %d bytes incrementally (%s)",
> +	  TRACE2 ("Start sending %"pD"d bytes incrementally (%s)",
>   		  bytes_remaining, XGetAtomName (display, cs->property));
>   	  cs->wait_object
>   	    = expect_property_change (display, window, cs->property,
> @@ -638,7 +651,7 @@
>
>   	  /* XChangeProperty expects an array of long even if long is
>   	     more than 32 bits.  */
> -	  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.


> @@ -1269,19 +1283,28 @@
>
>   static void
>   x_get_window_property (Display *display, Window window, Atom property,
> -		       unsigned char **data_ret, int *bytes_ret,
> +		       unsigned char **data_ret, ptrdiff_t *bytes_ret,
>   		       Atom *actual_type_ret, int *actual_format_ret,
>   		       unsigned long *actual_size_ret, int delete_p)
>   {
> -  int total_size;
> +  ptrdiff_t total_size;
>     unsigned long bytes_remaining;
> -  int offset = 0;
> +  ptrdiff_t offset = 0;
> +  unsigned char *data = 0;
>     unsigned char *tmp_data = 0;
>     int result;
> -  int buffer_size = SELECTION_QUANTUM (display);
> -
> -  if (buffer_size>  MAX_SELECTION_QUANTUM)
> -    buffer_size = MAX_SELECTION_QUANTUM;
> +  int buffer_size = selection_quantum (display);
> +
> +  /* Wide enough to avoid overflow in expressions using it.  */
> +  ptrdiff_t x_long_size = X_LONG_SIZE;
> +
> +  /* Maximum value for TOTAL_SIZE.  It cannot exceed PTRDIFF_MAX - 1
> +     and SIZE_MAX - 1, for an extra byte at the end.  And it cannot
> +     exceed LONG_MAX * X_LONG_SIZE, for XGetWindowProperty.  */
> +  ptrdiff_t total_size_max =
> +    ((min (PTRDIFF_MAX, SIZE_MAX) - 1) / x_long_size<  LONG_MAX
> +     ? min (PTRDIFF_MAX, SIZE_MAX) - 1
> +     : LONG_MAX * x_long_size);
>
>     BLOCK_INPUT;
>
> @@ -1292,49 +1315,44 @@
>   			       actual_size_ret,
>   			&bytes_remaining,&tmp_data);
>     if (result != Success)
> -    {
> -      UNBLOCK_INPUT;
> -      *data_ret = 0;
> -      *bytes_ret = 0;
> -      return;
> -    }
> +    goto done;
>
>     /* This was allocated by Xlib, so use XFree.  */
>     XFree ((char *) tmp_data);
>
>     if (*actual_type_ret == None || *actual_format_ret == 0)
> -    {
> -      UNBLOCK_INPUT;
> -      return;
> -    }
> +    goto done;
>
> -  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?

> +  if (! data)
> +    goto memory_exhausted;
>
>     /* Now read, until we've gotten it all.  */
>     while (bytes_remaining)
>       {
> -#ifdef TRACE_SELECTION
> -      unsigned long last = bytes_remaining;
> -#endif
> +      ptrdiff_t bytes_gotten;
> +      int bytes_per_item;
>         result
>   	= XGetWindowProperty (display, window, property,
> -			      (long)offset/4, (long)buffer_size/4,
> +			      offset / X_LONG_SIZE,
> +			      buffer_size / X_LONG_SIZE,
>   			      False,
>   			      AnyPropertyType,
>   			      actual_type_ret, actual_format_ret,
>   			      actual_size_ret,&bytes_remaining,&tmp_data);
>
> -      TRACE2 ("Read %lu bytes from property %s",
> -	      last - bytes_remaining,
> -	      XGetAtomName (display, property));
> -
>         /* If this doesn't return Success at this point, it means that
>   	 some clod deleted the selection while we were in the midst of
>   	 reading it.  Deal with that, I guess.... */
>         if (result != Success)
>   	break;
>
> +      bytes_per_item = *actual_format_ret>>  3;
> +      xassert (*actual_size_ret<= buffer_size / bytes_per_item);
> +
>         /* The man page for XGetWindowProperty says:
>            "If the returned format is 32, the returned data is represented
>             as a long array and should be cast to that type to obtain the
> @@ -1348,32 +1366,61 @@
>            The bytes and offsets passed to XGetWindowProperty refers to the
>            property and those are indeed in 32 bit quantities if format is 32.  */
>
> +      bytes_gotten = *actual_size_ret;
> +      bytes_gotten *= bytes_per_item;
> +
> +      TRACE2 ("Read %"pD"d bytes from property %s",
> +	      bytes_gotten, XGetAtomName (display, property));
> +
> +      if (total_size - offset<  bytes_gotten)
> +	{
> +	  unsigned char *data1;
> +	  ptrdiff_t remaining_lim = total_size_max - offset - bytes_gotten;
> +	  if (remaining_lim<  0 || remaining_lim<  bytes_remaining)
> +	    goto size_overflow;
> +	  total_size = offset + bytes_gotten + bytes_remaining;
> +	  data1 = realloc (data, total_size + 1);
> +	  if (! data1)
> +	    goto memory_exhausted;
> +	  data = data1;
> +	}
> +
>         if (32<  BITS_PER_LONG&&  *actual_format_ret == 32)
>           {
>             unsigned long i;
> -          int  *idata = (int *) ((*data_ret) + offset);
> +	  int  *idata = (int *) (data + offset);
>             long *ldata = (long *) tmp_data;
>
>             for (i = 0; i<  *actual_size_ret; ++i)
> -            {
> -              idata[i]= (int) ldata[i];
> -              offset += 4;
> -            }
> +	    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).


> @@ -2426,7 +2479,7 @@
>     unsigned long size = 160/event->format;
>     int x, y;
>     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?

>     ptrdiff_t i;
>
>     for (i = 0; i<  dpyinfo->x_dnd_atoms_length; ++i)
> @@ -2444,7 +2497,7 @@
>     if (32<  BITS_PER_LONG&&  event->format == 32)
>       {
>         for (i = 0; i<  5; ++i) /* There are only 5 longs in a ClientMessage. */
> -        idata[i] = (int) event->data.l[i];
> +	idata[i] = event->data.l[i];
>         data = (unsigned char *) idata;
>       }
>
>

 > # Begin bundle
 > IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWXkU8k0BAh7/gH/wAht7////

Why send this?  What is it?  It seems to just take up bandwidth.

	Jan D.