From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: [Emacs-diffs] /srv/bzr/emacs/trunk r105581: Integer and memory overflow issues (Bug#9196). Date: Fri, 26 Aug 2011 23:28:49 -0400 Message-ID: References: NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: dough.gmane.org 1314415750 24224 80.91.229.12 (27 Aug 2011 03:29:10 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Sat, 27 Aug 2011 03:29:10 +0000 (UTC) Cc: emacs-devel@gnu.org To: Paul Eggert Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Aug 27 05:29:07 2011 Return-path: Envelope-to: ged-emacs-devel@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 1Qx9Zy-0003w8-NS for ged-emacs-devel@m.gmane.org; Sat, 27 Aug 2011 05:29:06 +0200 Original-Received: from localhost ([::1]:41659 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qx9Zy-0003RF-80 for ged-emacs-devel@m.gmane.org; Fri, 26 Aug 2011 23:29:06 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:57798) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qx9Zu-0003R9-Sm for emacs-devel@gnu.org; Fri, 26 Aug 2011 23:29:04 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qx9Zt-0004Dd-VY for emacs-devel@gnu.org; Fri, 26 Aug 2011 23:29:02 -0400 Original-Received: from pruche.dit.umontreal.ca ([132.204.246.22]:41373) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qx9Zt-0004DW-PN for emacs-devel@gnu.org; Fri, 26 Aug 2011 23:29:01 -0400 Original-Received: from ceviche.home (lechon.iro.umontreal.ca [132.204.27.242]) by pruche.dit.umontreal.ca (8.14.1/8.14.1) with ESMTP id p7R3SnOr012916; Fri, 26 Aug 2011 23:28:49 -0400 Original-Received: by ceviche.home (Postfix, from userid 20848) id 784E1660CF; Fri, 26 Aug 2011 23:28:49 -0400 (EDT) In-Reply-To: (Paul Eggert's message of "Fri, 26 Aug 2011 09:36:17 -0700") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) X-NAI-Spam-Flag: NO X-NAI-Spam-Threshold: 5 X-NAI-Spam-Score: 0 X-NAI-Spam-Rules: 1 Rules triggered RV3961=0 X-NAI-Spam-Version: 2.2.0.9286 : core <3961> : streams <675506> : uri <944421> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 132.204.246.22 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:143600 Archived-At: > +/* Size of header used by bidi_shelve_cache. */ > +enum > + { > + bidi_shelve_header_size = > + (sizeof (bidi_cache_idx) + sizeof (bidi_cache_start_stack) > + + sizeof (bidi_cache_sp) + sizeof (bidi_cache_start) > + + sizeof (bidi_cache_last_idx)) > + }; Why an enum? Sounds really ugly! > - if (outp - outbuf + MAX_MULTIBYTE_LENGTH * ccl.produced > - > outbufsize) > + /* FIXME: Surely this should be buf_magnification instead. > + MAX_MULTIBYTE_LENGTH overestimates the storage needed. */ > + int magnification = MAX_MULTIBYTE_LENGTH; > + > + ptrdiff_t offset = outp - outbuf; > + ptrdiff_t shortfall; > + if (INT_MULTIPLY_OVERFLOW (ccl.produced, magnification)) > + memory_full (SIZE_MAX); > + shortfall = ccl.produced * magnification - (outbufsize - offset); > + if (0 < shortfall) I don't want to sprinkle memory_full calls all over the code like your patch does. Especially since many of the overflow risks it fixes can only happen in the case where we use 64bit Lisp_Objects on a 32bit system, which is not currently the default and may end up never being the default (tho it may also end up being the default, of course). I.e. right now, those overflows don't worry me as much as the code cleanliness which seems to suffer from your patch. Stefan