From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Jan =?UTF-8?Q?Dj=C3=A4rv?= Newsgroups: gmane.emacs.bugs Subject: bug#9196: integer and memory overflow issues (e.g., cut-and-paste crashes Emacs) Date: Fri, 05 Aug 2011 11:26:59 +0200 Message-ID: <4E3BB763.8040902@swipnet.se> References: <4E3256E9.3020208@cs.ucla.edu> <4E3284EB.1010308@swipnet.se> <4E32DE0E.5050208@cs.ucla.edu> <4E32E490.3050002@swipnet.se> <4E332009.3090909@cs.ucla.edu> <4E339C30.9090708@swipnet.se> <4E345892.8010200@cs.ucla.edu> <4E3518F0.4040002@swipnet.se> <4E3B5671.1040704@cs.ucla.edu> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Trace: dough.gmane.org 1312536448 18736 80.91.229.12 (5 Aug 2011 09:27:28 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Fri, 5 Aug 2011 09:27:28 +0000 (UTC) Cc: 9196@debbugs.gnu.org To: Paul Eggert Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Fri Aug 05 11:27:24 2011 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([140.186.70.17]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1QpGge-0005j4-44 for geb-bug-gnu-emacs@m.gmane.org; Fri, 05 Aug 2011 11:27:24 +0200 Original-Received: from localhost ([::1]:38139 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpGgd-0006le-BE for geb-bug-gnu-emacs@m.gmane.org; Fri, 05 Aug 2011 05:27:23 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:43071) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpGga-0006lK-4g for bug-gnu-emacs@gnu.org; Fri, 05 Aug 2011 05:27:21 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QpGgY-0001rL-Kw for bug-gnu-emacs@gnu.org; Fri, 05 Aug 2011 05:27:20 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:58478) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpGgY-0001rH-Fc for bug-gnu-emacs@gnu.org; Fri, 05 Aug 2011 05:27:18 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.69) (envelope-from ) id 1QpGhG-0007ZF-7j; Fri, 05 Aug 2011 05:28:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Jan =?UTF-8?Q?Dj=C3=A4rv?= Original-Sender: debbugs-submit-bounces@debbugs.gnu.org Resent-To: owner@debbugs.gnu.org Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 05 Aug 2011 09:28:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 9196 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 9196-submit@debbugs.gnu.org id=B9196.131253646829069 (code B ref 9196); Fri, 05 Aug 2011 09:28:02 +0000 Original-Received: (at 9196) by debbugs.gnu.org; 5 Aug 2011 09:27:48 +0000 Original-Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1QpGh1-0007Yo-UC for submit@debbugs.gnu.org; Fri, 05 Aug 2011 05:27:48 -0400 Original-Received: from smtprelay-b11.telenor.se ([62.127.194.20]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1QpGgz-0007Yg-R1 for 9196@debbugs.gnu.org; Fri, 05 Aug 2011 05:27:47 -0400 Original-Received: from ipb1.telenor.se (ipb1.telenor.se [195.54.127.164]) by smtprelay-b11.telenor.se (Postfix) with ESMTP id 9541EEAA9A for <9196@debbugs.gnu.org>; Fri, 5 Aug 2011 11:27:00 +0200 (CEST) X-SENDER-IP: [85.225.45.26] X-LISTENER: [smtp.bredband.net] X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AhBxAJu2O05V4S0aPGdsb2JhbABCDoQ5hEeeUAsBAQEBHhkNJYFAAQEFIwQRHiIBBQsLGgIFFgsCAgkDAgECARsMCgMRBgoDAQcBAYdrrnSRT4ErhAuBEASYB4soOQ X-IronPort-AV: E=Sophos;i="4.67,322,1309730400"; d="scan'208";a="211613072" Original-Received: from c-1a2de155.25-1-64736c10.cust.bredbandsbolaget.se (HELO coolsville.localdomain) ([85.225.45.26]) by ipb1.telenor.se with ESMTP; 05 Aug 2011 11:27:00 +0200 Original-Received: from [172.20.199.13] (zeplin [172.20.199.13]) by coolsville.localdomain (Postfix) with ESMTPSA id 75C697FA059; Fri, 5 Aug 2011 11:26:59 +0200 (CEST) User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:5.0) Gecko/20110624 Thunderbird/5.0 In-Reply-To: <4E3B5671.1040704@cs.ucla.edu> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list Resent-Date: Fri, 05 Aug 2011 05:28:02 -0400 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 1) X-Received-From: 140.186.70.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:49913 Archived-At: 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.