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: [Emacs-diffs] /srv/bzr/emacs/trunk r105581: Integer and memory overflow issues (Bug#9196). Date: Sat, 27 Aug 2011 00:17:51 -0700 Organization: UCLA Computer Science Department Message-ID: <4E589A1F.3020902__3542.62151890925$1314429508$gmane$org@cs.ucla.edu> References: NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Trace: dough.gmane.org 1314429508 18084 80.91.229.12 (27 Aug 2011 07:18:28 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Sat, 27 Aug 2011 07:18:28 +0000 (UTC) Cc: 9196@debbugs.gnu.org, emacs-devel@gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sat Aug 27 09:18: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 1QxD9r-000754-6D for geb-bug-gnu-emacs@m.gmane.org; Sat, 27 Aug 2011 09:18:23 +0200 Original-Received: from localhost ([::1]:47774 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QxD9q-0000St-Ek for geb-bug-gnu-emacs@m.gmane.org; Sat, 27 Aug 2011 03:18:22 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:53759) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QxD9l-0000NM-49 for bug-gnu-emacs@gnu.org; Sat, 27 Aug 2011 03:18:19 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QxD9h-00059H-M8 for bug-gnu-emacs@gnu.org; Sat, 27 Aug 2011 03:18:16 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:59009) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QxD9h-00059B-H5 for bug-gnu-emacs@gnu.org; Sat, 27 Aug 2011 03:18:13 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.69) (envelope-from ) id 1QxDCP-0004ws-RG; Sat, 27 Aug 2011 03:21:01 -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, 27 Aug 2011 07:21:01 +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.131442965219007 (code B ref 9196); Sat, 27 Aug 2011 07:21:01 +0000 Original-Received: (at 9196) by debbugs.gnu.org; 27 Aug 2011 07:20:52 +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 1QxDCG-0004wV-Gl for submit@debbugs.gnu.org; Sat, 27 Aug 2011 03:20:52 -0400 Original-Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1QxDCC-0004wM-VB for 9196@debbugs.gnu.org; Sat, 27 Aug 2011 03:20:50 -0400 Original-Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id C9CB439E80F2; Sat, 27 Aug 2011 00:17:58 -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 EN6wI5B4cnaV; Sat, 27 Aug 2011 00:17:58 -0700 (PDT) Original-Received: from [192.168.1.10] (pool-71-189-109-235.lsanca.fios.verizon.net [71.189.109.235]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id E013139E80D2; Sat, 27 Aug 2011 00:17:57 -0700 (PDT) User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.20) Gecko/20110805 Thunderbird/3.1.12 In-Reply-To: X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list Resent-Date: Sat, 27 Aug 2011 03:21:01 -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:50378 Archived-At: In Stefan Monnier wrote: > I don't want to sprinkle memory_full calls all over the code like your > patch does. I'd rather avoid them too. That is, I'd like to revamp the code to avoid the need for most of the recently-introduced memory_full calls. (Counting the further patch discussed below, 11 memory_full calls were recently removed and 27 added, for a net gain of 16.) However, I don't see an easy way to do that. Perhaps we can revisit this after Emacs 24 is out, when it'll be possible to do some more-serious code reorganization. I looked again at the part of the patch that you quoted, and it turned out to be overly conservative: it introduced an integer overflow check that wasn't needed, regardless of whether --with-wide-int is used. (This is because CCL_EXECUTE_BUF_SIZE is less than INT_MAX / 5; I hadn't noticed that earlier, and thanks for catching that problem.) I just now removed that overflow check from the trunk, as part of bzr 105585. > many of the overflow risks it fixes can only happen in > the case where we use 64bit Lisp_Objects on a 32bit system, Sorry, I don't follow this remark. As far as I know, the recently-introduced overflow checks are needed even if configure's --with-wide-int option is not used. > right now, those overflows don't worry me as much as the code > cleanliness which seems to suffer from your patch. Thanks to your comments, src/ccl.c is now shorter than it was before that patch was installed, so that part of the change should be OK now. I'll be happy to try to address any other parts of the change that need similar improvements. (It is a 6000-line patch, after all, and a patch that large can always be improved....) That being said, I'm afraid that Emacs's buffer and integer overflow bugs cannot be fixed without adding *some* complications -- unless we're willing to do a pretty drastic rewrite of the internals. >> > +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! This was to avoid duplication of that long expression. The duplication was already present in the old version, but the patch duplicated the expression one more time, which was a minor annoyance. If you prefer the old way of doing things, where the expression is duplicated, I can easily get rid of bidi_shelve_header_size entirely. Or, if you prefer a #define to the enum, I can easily change the code to do that. Enums often work better with debuggers, though.