From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#8668: * editfns.c (Fformat): Fix several integer overflow problems Date: Mon, 16 May 2011 12:27:29 +0300 Message-ID: <83liy7dmgu.fsf@gnu.org> References: <4DCC9F20.7020001@cs.ucla.edu> <4DD0CABB.1050606@cs.ucla.edu> Reply-To: Eli Zaretskii NNTP-Posting-Host: lo.gmane.org X-Trace: dough.gmane.org 1305538093 18085 80.91.229.12 (16 May 2011 09:28:13 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Mon, 16 May 2011 09:28:13 +0000 (UTC) Cc: 8668@debbugs.gnu.org To: Paul Eggert Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon May 16 11:28:08 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 1QLu5v-00083q-Gq for geb-bug-gnu-emacs@m.gmane.org; Mon, 16 May 2011 11:28:07 +0200 Original-Received: from localhost ([::1]:51294 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QLu5u-0004Kj-M6 for geb-bug-gnu-emacs@m.gmane.org; Mon, 16 May 2011 05:28:06 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:59520) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QLu5s-0004KU-1h for bug-gnu-emacs@gnu.org; Mon, 16 May 2011 05:28:04 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QLu5q-00058h-MD for bug-gnu-emacs@gnu.org; Mon, 16 May 2011 05:28:04 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:41843) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QLu5q-00058Q-Gj for bug-gnu-emacs@gnu.org; Mon, 16 May 2011 05:28:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.69) (envelope-from ) id 1QLu5q-0000Yi-2u; Mon, 16 May 2011 05:28:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: debbugs-submit-bounces@debbugs.gnu.org Resent-To: owner@debbugs.gnu.org Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 16 May 2011 09:28:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 8668 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 8668-submit@debbugs.gnu.org id=B8668.13055380652122 (code B ref 8668); Mon, 16 May 2011 09:28:02 +0000 Original-Received: (at 8668) by debbugs.gnu.org; 16 May 2011 09:27:45 +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 1QLu5Y-0000YA-Gu for submit@debbugs.gnu.org; Mon, 16 May 2011 05:27:44 -0400 Original-Received: from mtaout20.012.net.il ([80.179.55.166]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1QLu5V-0000Xw-05 for 8668@debbugs.gnu.org; Mon, 16 May 2011 05:27:42 -0400 Original-Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0LLA00G007HWUR00@a-mtaout20.012.net.il> for 8668@debbugs.gnu.org; Mon, 16 May 2011 12:27:34 +0300 (IDT) Original-Received: from HOME-C4E4A596F7 ([84.228.84.222]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0LLA00DQU7LVLUN0@a-mtaout20.012.net.il>; Mon, 16 May 2011 12:27:33 +0300 (IDT) In-reply-to: <4DD0CABB.1050606@cs.ucla.edu> X-012-Sender: halo1@inter.net.il X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list Resent-Date: Mon, 16 May 2011 05:28:02 -0400 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) 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:46483 Archived-At: > Date: Sun, 15 May 2011 23:56:59 -0700 > From: Paul Eggert > > Further inspection has found several other problems with Fformat, > having to do with large strings and integer overflow. Here's a > revised patch to address the problems that I've found. This > supersedes the earlier patch. I'll do more testing on this one > but thought I'd give people a heads-up. This patch assumes other > fixes I've sent in recently. This uses snprintf where we previously used sprintf. snprintf is less portable, and not all supported platforms have a C9x compatible implementation. The library used on MSDOS doesn't have snprintf at all, and those used on MS-Windows may return a negative value when the buffer is too short (depending on the version of the library), which your patch doesn't expect. Is it possible to stay with sprintf? We only use it for numerical conversions, whose length can be predicted reliably, no? > + error ("Maximum format conversion size exceeded"); I wonder if we could come up with a less cryptic error message. (What is a "conversion size"?) Maybe something like Format conversion specifies width/precision that are too large. > + if (bufsize <= min_bufsize / bufsize_multiplier) > + bufsize = min_bufsize; > + else if (bufsize <= max_bufsize / bufsize_multiplier) > + bufsize *= bufsize_multiplier; > + else if (bufsize <= max_bufsize) > + bufsize = max_bufsize; > + else > + memory_full (); Why "memory_full"? You didn't yet allocate such a huge buffer, so Emacs could still have plenty of memory. Even after the allocation, Emacs could still have enough memory. I think just a call to `error', or maybe to the new string_overflow or format_overflow functions, is TRT here, because this is simply another case of the "string too large" problem, you just succeeded to detect it in advance. You actually do call string_overflow and format_overflow elsewhere in the patch. > + if (bufsize - used < additional) \ > + { \ > + char *newbuf; \ > + if (INT_ADD_OVERFLOW (used, additional) \ > + || max_bufsize < used + additional) \ > + string_overflow (); \ > + bufsize = used + additional; \ > + bufsize = (bufsize < max_bufsize / 3 * 2 \ > + ? bufsize + bufsize / 2 \ > + : max_bufsize); \ > + SAFE_ALLOCA (newbuf, char *, bufsize); \ > + memcpy (newbuf, buf, used); \ This wastes memory, because the old buffer is not released (when SAFE_ALLOCA uses xmalloc). I'm not sure it is a good idea to use SAFE_ALLOCA in a loop like that. Perhaps xmalloc/xrealloc would be better here, since you are keeping the previous contents of the buffer? > + EMACS_INT i, extralen = 2 * formatlen + pWIDElen + 1; > + if (SIZE_MAX < extralen > + || (SIZE_MAX - extralen) / sizeof (struct info) <= nargs) > + memory_full (); I think this memory_full should also be replaced with a call to `error' or format_overflow (string_overflow doesn't seem to fit here, IMO), for the same reasons pointed out above. > + char conversion; > ... > + conversion = *format; I think this is wrong (and the original code had it also wrong). `format' could be a multibyte string, so it isn't right to take a single byte out of it and assume you've got a full character; it could be the 1st byte of a multibyte sequence. It would also trip you here: > + else if (! (conversion == 'c' || conversion == 'd' > + || conversion == 'e' || conversion == 'f' > + || conversion == 'g' || conversion == 'i' > + || conversion == 'o' || conversion == 'x' > + || conversion == 'X')) > + error ("Invalid format operation %%%c", conversion); If `conversion' is the first byte of a multibyte sequence, the error message will show some random value that has no immediate relation to the actual character after the `%'. I think you need to make `conversion' an int, and assign its value with STRING_CHAR. Or at the very least do that where you call `error' with the above error message, so that the error identifies the problematic character.