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#8401: removing duplication and improving the readlink code Date: Fri, 01 Apr 2011 13:09:22 -0700 Organization: UCLA Computer Science Department Message-ID: <4D9630F2.1010806@cs.ucla.edu> References: <4D9574F2.20108@cs.ucla.edu> <83vcyypdzy.fsf@gnu.org> <4D9620CC.4000806@cs.ucla.edu> <83hbahyd64.fsf@gnu.org> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Trace: dough.gmane.org 1301690426 20011 80.91.229.12 (1 Apr 2011 20:40:26 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Fri, 1 Apr 2011 20:40:26 +0000 (UTC) Cc: 8401@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Fri Apr 01 22:40:21 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 1Q5l7Q-0004RF-1Z for geb-bug-gnu-emacs@m.gmane.org; Fri, 01 Apr 2011 22:40:19 +0200 Original-Received: from localhost ([127.0.0.1]:42538 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q5l7G-0006jY-T3 for geb-bug-gnu-emacs@m.gmane.org; Fri, 01 Apr 2011 16:38:46 -0400 Original-Received: from [140.186.70.92] (port=51396 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q5l60-00061T-Ho for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 16:37:56 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q5l5k-00007c-PZ for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 16:37:15 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:34920) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q5l5k-00007Y-O3 for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 16:37:12 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.69) (envelope-from ) id 1Q5kfT-00018k-Sw; Fri, 01 Apr 2011 16:10:03 -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: Fri, 01 Apr 2011 20:10:03 +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.13016885724340 (code B ref 8401); Fri, 01 Apr 2011 20:10:03 +0000 Original-Received: (at 8401) by debbugs.gnu.org; 1 Apr 2011 20:09:32 +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 1Q5kex-00017x-T4 for submit@debbugs.gnu.org; Fri, 01 Apr 2011 16:09:32 -0400 Original-Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5kev-00017i-4c for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 16:09:30 -0400 Original-Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id 59A0639E80F8; Fri, 1 Apr 2011 13:09:23 -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 6KFUfj6K7hqR; Fri, 1 Apr 2011 13:09:22 -0700 (PDT) Original-Received: from [131.179.64.200] (Penguin.CS.UCLA.EDU [131.179.64.200]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id A2F3739E80B1; Fri, 1 Apr 2011 13:09:22 -0700 (PDT) User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.15) Gecko/20110307 Fedora/3.1.9-0.39.b3pre.fc14 Thunderbird/3.1.9 In-Reply-To: <83hbahyd64.fsf@gnu.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list Resent-Date: Fri, 01 Apr 2011 16:10:03 -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:45567 Archived-At: 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. >> 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. That would be an error-prone process and would leave us with three copies of the code to maintain. It's better to have just one copy of the code. > 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. Emacs is not as good as it should be about this, and I'm trying to make it better. 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. > But why do we need to introduce the allocator and careadlinkat > modules, and all the nested function calls needed for them, just to > protect a simple code fragment from overflowing? It's more reliable to put the overflow checks in one place, where they can be carefully checked, than to duplicate the code in multiple places where it's easy for programmers to get it wrong. > 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, as it avoids the need to call malloc and free entirely, in the usual case. This is a performance win in the typical case. > please restructure the code so that neither allocator.h nor > careadlinkat are not used on platforms whose readlink is an > always-fail stub. That's not possible to do, unless we add more "#ifdef DOS_NT"s to the mainstream code. But that would be undesirable. The mainstream code should be written for the usual case, and platforms that lack the necessary primitives should supply their own stubs (which may always fail, but that's OK). That is standard porting technology, and it's an improvement over sprinking #ifdefs throughout the mainstream code.