X-Recipient:	archive-cygwin AT delorie DOT com
DomainKey-Signature:	a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id
	:list-unsubscribe:list-subscribe:list-archive:list-post
	:list-help:sender:date:from:to:subject:message-id:reply-to
	:references:mime-version:content-type:in-reply-to; q=dns; s=
	default; b=P8h6iLY7XHuoUuLf1H/YZxKlafPx1HAUoZaRCaLW93unf9WXbohER
	tpaE8sQxZ3NgfxsIajgUwostPTwrOs+doYrgFGxfWlWL9Sazi8KrHRhlxfVNY7hj
	u1qLRG/7ORLJyUR2IuPQwjbwuIhg/5x1HX/84DJ3BgNBxRNqODbFcQ=
DKIM-Signature:	v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id
	:list-unsubscribe:list-subscribe:list-archive:list-post
	:list-help:sender:date:from:to:subject:message-id:reply-to
	:references:mime-version:content-type:in-reply-to; s=default;
	bh=7vvdqAhfu6P5MTf4sPsVMQBY2JQ=; b=YAD40VFWAwjKDkbxeBpG2/yz3Eys
	3a/skTAknpNewI9BdII9AKhDzXjjvslS8DrZli+C14GwvtVv0bBd45uxjcMA68q0
	SYaCTraRYsuzFI/R+yJvznovgIRcwMrSCQXvKzLbu0BdU85kWX3TKn3QT5aRlWC1
	Rn54xZwPKTJgpTE=
Mailing-List:	contact cygwin-help AT cygwin DOT com; run by ezmlm
List-Id:	<cygwin.cygwin.com>
List-Subscribe:	<mailto:cygwin-subscribe AT cygwin DOT com>
List-Archive:	<http://sourceware.org/ml/cygwin/>
List-Post:	<mailto:cygwin AT cygwin DOT com>
List-Help:	<mailto:cygwin-help AT cygwin DOT com>, <http://sourceware.org/ml/#faqs>
Sender:	cygwin-owner AT cygwin DOT com
Mail-Followup-To:	cygwin AT cygwin DOT com
Delivered-To:	mailing list cygwin AT cygwin DOT com
X-Spam-SWARE-Status:	No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_HOSTKARMA_YE autolearn=ham version=3.3.1
X-Mail-Handler:	Dyn Standard SMTP by Dyn
X-Report-Abuse-To:	abuse AT dyndns DOT com (see http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse reporting information)
X-MHO-User:	U2FsdGVkX1/AJ9Tjb+bIP9Y7TikqiT6g
Date:	Tue, 11 Jun 2013 11:17:08 -0400
From:	Christopher Faylor <cgf-use-the-mailinglist-please AT cygwin DOT com>
To:	cygwin AT cygwin DOT com
Subject:	Re: [PATCH] Check for existence of the path before processing '..'
Message-ID:	<20130611151708.GB2235@ednor.casa.cgf.cx>
Reply-To:	cygwin AT cygwin DOT com
Mail-Followup-To:	cygwin AT cygwin DOT com
References:	<001401ce66a4$bb9029a0$32b07ce0$%fedin AT samsung DOT com> <20130611142002 DOT GA1927 AT ednor DOT casa DOT cgf DOT cx> <20130611150446 DOT GB9484 AT calimero DOT vinschen DOT de>
MIME-Version:	1.0
In-Reply-To:	<20130611150446.GB9484@calimero.vinschen.de>
User-Agent:	Mutt/1.5.20 (2009-06-14)

On Tue, Jun 11, 2013 at 05:04:46PM +0200, Corinna Vinschen wrote:
>On Jun 11 10:20, Christopher Faylor wrote:
>> On Tue, Jun 11, 2013 at 05:08:13PM +0400, Fedin Pavel wrote:
>> > Hello!
>> >
>> > Some time ago i reported ability to access things like
>> >"/usr/nonexistent/..bin". I still had this problem and i tried my hands on
>> >fixing it.
>> > The patch works by checking the actual existence of the path before
>> >removing the last component from it. For performance reasons, only one check
>> >is done for things like "../..". Because, obviously, if "/foo/bar/baz"
>> >exists, then "/foo/bar" exists too. Also, the check is done only after some
>> >components have been added to the path. So, for example, current directory
>> >(obtained when processing relative paths), will not be checked.
>> > I tried to add a similar test also to normalize_win32_path() function,
>> >however this broke things like "cd /usr/src/..". For some reason, a POSIX
>> >version of the path (but with reversed slashes) is passed to this routine
>> >when expanding mount points, so, consequently, test for "\usr\src" using
>> >GetFileType() fails.
>> > I think it's ok, at least POSIX paths now behave in POSIX way. I have
>> >tested against performance, there is some loss (~0.2 seconds), but only for
>> >referencing '..'.
>> > With this patch i am able to compile the latest version of glibc with no
>> >problems.
>> 
>> You introduce a check_parent flag which is set every time a non-slash
>> character is found.  That doesn't seem right.  It seems like it should
>> be set whenever you see a slash.
>
>Indeed.  I moved setting check_parent before the while expression in
>the else branch instead and it still works.

I'll bet you wouldn't see much of a hit if you just got rid of the
check_parent flag entirely.

>> Also you are calling path_conv recursively.  I assume that is where you
>> are seeing a performance hit.
>
>I don't see how do this without calling path_conv, though.  You have to
>perform the full conversion on the parent path, with symlinks and
>everything to get the right result.

Yes, but it is a HUGE stack hit to call path_conv recursively here.

>However, I'm rather impressed by the low impact of this change.  I moved
>the check_parent setting so it's only set when a slash occurs, and then
>I made a couple of runs building coreutils.  As you know, GCC uses ..
>paths a lot.  The performance hit is almost unnoticable:  72.3 seconds
>without, 73.4 seconds with the patch.

If we are considering doing this, then couldn't we somehow just avoid
eliminating "/.." until after the path is fully parsed and then collapse
all of them in one final loop?  Also, don't we have the same problem for
foo/./bar?  We change that to foo/bar but foo may not exist.

cgf

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

- Raw text -