[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#9196: integer and memory overflow issues (e.g., cut-and-paste crashe
|
From: |
Jan Djärv |
|
Subject: |
bug#9196: integer and memory overflow issues (e.g., cut-and-paste crashes Emacs) |
|
Date: |
Fri, 05 Aug 2011 11:26:59 +0200 |
|
User-agent: |
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:5.0) Gecko/20110624 Thunderbird/5.0 |
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.