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 22:38:59 +0300 Message-ID: <83hbahyd64.fsf@gnu.org> References: <4D9574F2.20108@cs.ucla.edu> <83vcyypdzy.fsf@gnu.org> <4D9620CC.4000806@cs.ucla.edu> Reply-To: Eli Zaretskii NNTP-Posting-Host: lo.gmane.org X-Trace: dough.gmane.org 1301688444 9012 80.91.229.12 (1 Apr 2011 20:07:24 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Fri, 1 Apr 2011 20:07:24 +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 22:07:20 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 1Q5kcq-00057J-GC for geb-bug-gnu-emacs@m.gmane.org; Fri, 01 Apr 2011 22:07:20 +0200 Original-Received: from localhost ([127.0.0.1]:51072 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q5kcp-0005vu-Rt for geb-bug-gnu-emacs@m.gmane.org; Fri, 01 Apr 2011 16:07:19 -0400 Original-Received: from [140.186.70.92] (port=54693 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q5kcj-0005sX-Qu for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 16:07:14 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q5kci-0002sY-Na for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 16:07:13 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:56271) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q5kci-0002sU-Kg for bug-gnu-emacs@gnu.org; Fri, 01 Apr 2011 16:07:12 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.69) (envelope-from ) id 1Q5kCQ-0000TF-Ts; Fri, 01 Apr 2011 15:40:03 -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 19:40: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.13016867981796 (code B ref 8401); Fri, 01 Apr 2011 19:40:02 +0000 Original-Received: (at 8401) by debbugs.gnu.org; 1 Apr 2011 19:39:58 +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 1Q5kCM-0000Sv-ER for submit@debbugs.gnu.org; Fri, 01 Apr 2011 15:39:58 -0400 Original-Received: from mtaout20.012.net.il ([80.179.55.166]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Q5kCJ-0000Sh-UU for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 15:39:57 -0400 Original-Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0LIZ00G00NT61100@a-mtaout20.012.net.il> for 8401@debbugs.gnu.org; Fri, 01 Apr 2011 22:38:57 +0300 (IDT) Original-Received: from HOME-C4E4A596F7 ([77.126.47.180]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0LIZ00GQ1NWW1300@a-mtaout20.012.net.il>; Fri, 01 Apr 2011 22:38:57 +0300 (IDT) In-reply-to: <4D9620CC.4000806@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 15:40: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:45566 Archived-At: > Date: Fri, 01 Apr 2011 12:00:28 -0700 > From: Paul Eggert > CC: 8401@debbugs.gnu.org > > On 04/01/2011 01:33 AM, Eli Zaretskii wrote: > > Isn't much easier and much more elegant to use ssize_t instead of an > > int for the buffer sizes in both cases? > > 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. > and it should check that neither type overflows. Are we really going to consider seriously the case of a link name overflowing a 64-bit ssize_t data type? On what platform can this happen? I could perhaps be sympathetic to defensive programming in this area if it were a simple enough test. 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? > We could modify both copies of Emacs's readlink-using > code to fix these problems, but when there's duplication like > this, it's typically better to have just one copy of the code, > and make any necessary fixes in that copy. We could refactor the duplicated code into a short function, and use that. > On 04/01/2011 01:33 AM, Eli Zaretskii wrote: > > If this patch is accepted, the new emacs_readlink function will be a > > trivial "fail" stub on Windows. > > That would introduce an unnecessary "#ifdef DOS_NT" into the mainline > code. We should strive to keep the mainline code free of > porting #ifdefs when it is easy, as it is in this case. > The proposed code should run just fine on Windows, using > the already-existing stubs. We shouldn't need to clutter up > up the mainline code with unnecessary Windows-specific > microoptimizations. I'm all for that, but if you want to help, please restructure the code so that neither allocator.h nor careadlinkat are not used on platforms whose readlink is an always-fail stub. Sorry, but there are limits to what I can stand in code inelegance and gratuitous complexity without having my stomach spilled out. I have no authority to reject your patch, but I _can_ leave the parts of code I'm responsible for as unaffected by it as possible. On top of all that, 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.). The original code used xmalloc/xrealloc instead, which have Emacs-specific implementations to avoid that limitation.