From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.bugs Subject: bug#9196: integer and memory overflow issues (e.g., cut-and-paste crashes Emacs) Date: Fri, 05 Aug 2011 18:24:36 -0700 Message-ID: <4E3C97D4.5020408@cs.ucla.edu> 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> <4E3BB763.8040902@swipnet.se> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Trace: dough.gmane.org 1312593926 3610 80.91.229.12 (6 Aug 2011 01:25:26 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Sat, 6 Aug 2011 01:25:26 +0000 (UTC) Cc: 9196@debbugs.gnu.org To: Jan =?UTF-8?Q?Dj=C3=A4rv?= Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sat Aug 06 03:25:21 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 1QpVdf-00039k-IO for geb-bug-gnu-emacs@m.gmane.org; Sat, 06 Aug 2011 03:25:19 +0200 Original-Received: from localhost ([::1]:52575 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpVdf-0006Bl-1H for geb-bug-gnu-emacs@m.gmane.org; Fri, 05 Aug 2011 21:25:19 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:33264) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpVdb-0006BV-QI for bug-gnu-emacs@gnu.org; Fri, 05 Aug 2011 21:25:17 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QpVda-00086r-Ij for bug-gnu-emacs@gnu.org; Fri, 05 Aug 2011 21:25:15 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:39509) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpVda-00086n-Gj for bug-gnu-emacs@gnu.org; Fri, 05 Aug 2011 21:25:14 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.69) (envelope-from ) id 1QpVeM-0004sA-9g; Fri, 05 Aug 2011 21:26:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Paul Eggert Original-Sender: debbugs-submit-bounces@debbugs.gnu.org Resent-To: owner@debbugs.gnu.org Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 06 Aug 2011 01:26: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.131259393318696 (code B ref 9196); Sat, 06 Aug 2011 01:26:02 +0000 Original-Received: (at 9196) by debbugs.gnu.org; 6 Aug 2011 01:25:33 +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 1QpVdt-0004rV-Gk for submit@debbugs.gnu.org; Fri, 05 Aug 2011 21:25:33 -0400 Original-Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1QpVdr-0004rM-5z for 9196@debbugs.gnu.org; Fri, 05 Aug 2011 21:25:32 -0400 Original-Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id DBFDB39E80F2; Fri, 5 Aug 2011 18:24:41 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Original-Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JsY1dh-eQJJn; Fri, 5 Aug 2011 18:24:40 -0700 (PDT) Original-Received: from [10.1.205.231] (206-169-234-26.static.twtelecom.net [206.169.234.26]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id 2CB1D39E80D2; Fri, 5 Aug 2011 18:24:40 -0700 (PDT) User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.18) Gecko/20110617 Thunderbird/3.1.11 In-Reply-To: <4E3BB763.8040902@swipnet.se> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list Resent-Date: Fri, 05 Aug 2011 21:26: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:49921 Archived-At: On 08/05/2011 02:26 AM, Jan Dj=C3=A4rv wrote: >> + static char const xdefaults[] =3D ".Xdefaults-"; >=20 > I think there might be problems with dumping and static variables.=20 > 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 =3D gethomedir (); >> + char const *host =3D get_system_name (); >> + ptrdiff_t pathsize =3D strlen (home) + sizeof xdefaults + strle= n (host); >> + path =3D (char *) xrealloc (home, pathsize); >> + strcat (strcat (path, xdefaults), host); >> p =3D path; >> } >> >> db =3D XrmGetFileDatabase (p); >> >> xfree (path); >> - xfree (home); >=20 > 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]; >=20 > 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] =3D bytes_remaining; >> + value[0] =3D min (bytes_remaining, X_ULONG_MAX); >=20 > 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 =3D bytes_remaining + 1; >> - *data_ret =3D (unsigned char *) xmalloc (total_size); >> + if (total_size_max< bytes_remaining) >> + goto size_overflow; >> + total_size =3D bytes_remaining; >> + data =3D malloc (total_size + 1); >=20 > 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] =3D ldata[i]; >=20 > 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 =3D (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 =3D (unsigned char *) event->data.b; >> - int idata[5]; >> + unsigned int idata[5]; >=20 > idata can be signed, so why the change to unsigned? Sorry, I misunderstood that. I'll change it back to 'signed'. > IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWXkU8k0BAh7/gH/wAht= 7//// >=20 > 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 . 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. =3D=3D=3D 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 =20 -/* 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) =20 /* 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]; =20 TRACE2 ("Start sending %"pD"d bytes incrementally (%s)", bytes_remaining, XGetAtomName (display, cs->property)); @@ -651,7 +653,7 @@ x_reply_selection_request (struct input_ =20 /* XChangeProperty expects an array of long even if long is more than 32 bits. */ - value[0] =3D min (bytes_remaining, X_ULONG_MAX); + value[0] =3D 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] =3D symbol_to_x_atom (dpyinfo, obj); if (NILP (type)) type =3D QATOM; } - else if (RANGED_INTEGERP (0, obj, X_USHRT_MAX)) + else if (RANGED_INTEGERP (X_SHRT_MIN, obj, X_SHRT_MAX)) { *data_ret =3D (unsigned char *) xmalloc (sizeof (short) + 1); *format_ret =3D 16; *size_ret =3D 1; (*data_ret) [sizeof (short)] =3D 0; - (*(unsigned short **) data_ret) [0] =3D XINT (obj); + (*(short **) data_ret) [0] =3D XINT (obj); if (NILP (type)) type =3D QINTEGER; } else if (INTEGERP (obj) @@ -1783,7 +1785,7 @@ lisp_data_to_selection_data (Display *di *format_ret =3D 32; *size_ret =3D 1; (*data_ret) [sizeof (long)] =3D 0; - (*(unsigned long **) data_ret) [0] =3D cons_to_unsigned (obj, X_UL= ONG_MAX); + (*(long **) data_ret) [0] =3D cons_to_signed (obj, X_LONG_MIN, X_L= ONG_MAX); if (NILP (type)) type =3D QINTEGER; } else if (VECTORP (obj)) @@ -1817,25 +1819,30 @@ lisp_data_to_selection_data (Display *di int data_size =3D sizeof (short); if (NILP (type)) type =3D QINTEGER; for (i =3D 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 =3D sizeof (long); - format =3D 32; - } + { + intmax_t v =3D cons_to_signed (XVECTOR (obj)->contents[i], + X_LONG_MIN, X_LONG_MAX); + if (X_SHRT_MIN <=3D v && v <=3D 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 =3D sizeof (long); + format =3D 32; + } + } *data_ret =3D xnmalloc (size, data_size); *format_ret =3D format; *size_ret =3D size; for (i =3D 0; i < size; i++) - if (format =3D=3D 32) - (*((unsigned long **) data_ret)) [i] =3D - cons_to_unsigned (XVECTOR (obj)->contents[i], X_ULONG_MAX); - else - (*((unsigned short **) data_ret)) [i] =3D - cons_to_unsigned (XVECTOR (obj)->contents[i], X_USHRT_MAX); + { + long v =3D cons_to_signed (XVECTOR (obj)->contents[i], + X_LONG_MIN, X_LONG_MAX); + if (format =3D=3D 32) + (*((long **) data_ret)) [i] =3D v; + else + (*((short **) data_ret)) [i] =3D v; + } } } else @@ -2479,7 +2486,7 @@ x_handle_dnd_message (struct frame *f, X unsigned long size =3D 160/event->format; int x, y; unsigned char *data =3D (unsigned char *) event->data.b; - unsigned int idata[5]; + int idata[5]; ptrdiff_t i; =20 for (i =3D 0; i < dpyinfo->x_dnd_atoms_length; ++i)