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#8401: removing duplication and improving the readlink code Date: Fri, 01 Apr 2011 23:57:54 +0300 Message-ID: <838vvty9il.fsf@gnu.org> References: <4D9574F2.20108@cs.ucla.edu> <83vcyypdzy.fsf@gnu.org> <4D9620CC.4000806@cs.ucla.edu> <83hbahyd64.fsf@gnu.org> <4D9630F2.1010806@cs.ucla.edu> Reply-To: Eli Zaretskii NNTP-Posting-Host: lo.gmane.org X-Trace: dough.gmane.org 1301692056 28917 80.91.229.12 (1 Apr 2011 21:07:36 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Fri, 1 Apr 2011 21:07:36 +0000 (UTC) Cc: 8401@debbugs.gnu.org To: Paul Eggert Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Fri Apr 01 23:07:28 2011 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5lYw-0008Fa-Rz for geb-bug-gnu-emacs@m.gmane.org; Fri, 01 Apr 2011 23:07:23 +0200 Original-Received: from localhost ([127.0.0.1]:58252 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q5lYw-0001qM-2N for geb-bug-gnu-emacs@m.gmane.org; Fri, 01 Apr 2011 17:07:22 -0400 Original-Received: from [140.186.70.92] (port=46772 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q5lYo-0001hC-5P for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 17:07:16 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q5lYm-0006TP-QN for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 17:07:14 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:36685) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q5lYm-0006TL-NO for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 17:07:12 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.69) (envelope-from ) id 1Q5lQs-0002EB-Iu; Fri, 01 Apr 2011 16:59: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: Fri, 01 Apr 2011 20:59:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 8401 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 8401-submit@debbugs.gnu.org id=B8401.13016914948502 (code B ref 8401); Fri, 01 Apr 2011 20:59:02 +0000 Original-Received: (at 8401) by debbugs.gnu.org; 1 Apr 2011 20:58:14 +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 1Q5lQ5-0002D3-S7 for submit@debbugs.gnu.org; Fri, 01 Apr 2011 16:58:14 -0400 Original-Received: from mtaout20.012.net.il ([80.179.55.166]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5lQ3-0002Cn-8x for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 16:58:12 -0400 Original-Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0LIZ00G00QAVDY00@a-mtaout20.012.net.il> for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 23:57:55 +0300 (IDT) Original-Received: from HOME-C4E4A596F7 ([77.126.47.180]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0LIZ00GTTRKFNN30@a-mtaout20.012.net.il>; Fri, 01 Apr 2011 23:57:53 +0300 (IDT) In-reply-to: <4D9630F2.1010806@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: Fri, 01 Apr 2011 16:59: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: , Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:45568 Archived-At: > Date: Fri, 01 Apr 2011 13:09:22 -0700 > From: Paul Eggert > CC: 8401@debbugs.gnu.org > > On 04/01/2011 12:38 PM, Eli Zaretskii wrote: > > > the functions you introduce use malloc/realloc, > > which I think will prevent their callers from being safe for > > asynchronous calls triggered by external events (mouse etc.). > > No, because the functions always use Emacs xmalloc / xrealloc when > Emacs invokes them. This is because emacs_readlink tells the > lower-level primitives to use xmalloc and xrealloc. You are right, I stand corrected. However, this just underlines the difficulty of reading the convoluted arrangement that this patch introduces. Any such difficulty is an impediment to maintenance. > >> That doesn't suffice; the code should not only use ssize_t for > >> readlink's returned value, but it should also use size_t for the > >> buffer size > > > > Well, let's use that as well, then. > > What I sense that you're suggesting, is that we take all the fixes in > the gnulib code, and port them all into the two duplicates of similar > code that's in Emacs. No. Gnulib code that you suggest to add does much more than just avoid overflow. See below. > > Are we really going to consider seriously the case of a link name > > overflowing a 64-bit ssize_t data type? > > As a general rule, GNU code shouldn't have arbitrary limits, and > should defend against limits in underyling systems. Sorry, that doesn't answer the question. The issue at hand is the price (in complexity and gratuitous added code) we should pay for a simple overflow test. I submit that the price you are asking for is way too high in this case. Where a single comparison will suffice to resolve a very remote possibility of overflow, you suggest to add 2 non-trivial gnulib modules. > Emacs is not as good as it should be about this, and I'm trying to > make it better. Your efforts are appreciated. But the issue at hand is not whether Emacs code can and should be made better. The issue is _how_ to do that. I respectfully submit that in this particular case, the technique selected for improving the code in this particular area is not the best alternative. > This is just one example of many, found by static checking, and we > might as well fix it while we're fixing all the others. I support fixing possible overflows, just not with sledgehammer style techniques such as this one. > > We could refactor the duplicated code into a short function, and use > > that. > > That's what's being done here. The function does even more than that, > though Exactly! > it avoids the need to call malloc and free entirely, in the usual > case. This is a performance win in the typical case. There should be a good reason for introducing this additional code; a remote possibility of an overflow is not such a good reason. As for performance win, the callers of this code are not performance critical, AFAICT. So this is an example of premature optimization, IMO. What I'm suggesting is to solve the overflow, and nothing else. Again, I can hardly believe that doing so would need more than a simple comparison, and I agree that a function which allocates a buffer and calls readlink with that buffer, while avoiding overflow, could be used in both places, to avoid code replication.