Package: emacs;
Reported by: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Fri, 29 Jul 2011 06:47:02 UTC
Severity: normal
Tags: patch
Found in version 24.0.50
Done: Paul Eggert <eggert <at> cs.ucla.edu>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Jan Djärv <jan.h.d <at> swipnet.se> To: Paul Eggert <eggert <at> cs.ucla.edu> Cc: 9196 <at> debbugs.gnu.org Subject: bug#9196: integer and memory overflow issues (e.g., cut-and-paste crashes Emacs) Date: Fri, 05 Aug 2011 11:26:59 +0200
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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.