public inbox for cygwin@cygwin.com
 help / color / mirror / Atom feed
* [PATCH] Check for existence of the path before processing '..'
@ 2013-06-11 13:08 Fedin Pavel
  2013-06-11 13:48 ` Corinna Vinschen
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Fedin Pavel @ 2013-06-11 13:08 UTC (permalink / raw)
  To: cygwin

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

 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.
 

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia



[-- Attachment #2: cygwin-1.7.19-8-check-parent-path-before-double-dot.diff --]
[-- Type: application/octet-stream, Size: 1447 bytes --]

diff -ru src.orig/winsup/cygwin/path.cc src/winsup/cygwin/path.cc
--- src.orig/winsup/cygwin/path.cc	2013-05-23 19:23:01.000000000 +0500
+++ src/winsup/cygwin/path.cc	2013-06-05 10:47:46.337297200 +0500
@@ -240,6 +240,7 @@
 {
   const char *in_src = src;
   char *dst_start = dst;
+  bool check_parent = false;
   syscall_printf ("src %s", src);
 
   if ((isdrive (src) && isdirsep (src[2])) || *src == '\\')
@@ -275,7 +276,10 @@
 	goto win32_path;
       /* Strip runs of /'s.  */
       if (!isslash (*src))
-	*tail++ = *src++;
+        {
+	  *tail++ = *src++;
+	  check_parent = true;
+	}
       else
 	{
 	  while (*++src)
@@ -301,6 +305,22 @@
 		break;
 	      else
 		{
+		  /* According to POSIX semantics all elements of path must exist.
+		     In order to follow it, we must validate our path before removing
+		     the trailing component.
+		     The trick with check_parent is needed for performance optimization,
+		     in order not to verify paths which are already verified. For example
+		     this prevents double check in case of foo/bar/../..
+		   */
+		  if (check_parent)
+		    {
+		      *tail = 0;
+		      debug_printf ("checking %s before '..'", dst_start);
+		      path_conv head (dst_start);
+		      if (!head.isdir())
+		        return ENOENT;
+		      check_parent = false;
+		    }
 		  while (tail > dst_start && !isslash (*--tail))
 		    continue;
 		  src++;


[-- Attachment #3: Type: text/plain, Size: 218 bytes --]

--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-11 13:08 [PATCH] Check for existence of the path before processing '..' Fedin Pavel
@ 2013-06-11 13:48 ` Corinna Vinschen
  2013-06-13  6:10   ` Fedin Pavel
  2013-06-11 14:20 ` Christopher Faylor
  2013-06-12 17:46 ` [GOLDSTAR] " Corinna Vinschen
  2 siblings, 1 reply; 16+ messages in thread
From: Corinna Vinschen @ 2013-06-11 13:48 UTC (permalink / raw)
  To: cygwin

Hi Ferdin,

On Jun 11 17:08, 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 '..'.

Thanks for the patch.  The idea sounds good, and I think it's the right
thing to do *not* to add this to normalize_win32_path, because the ..
semantics on WINdows are so that a .. is allowed even if the parent
doesn't exist.

I didn't test your patch so far, but I'm a bit puzzled about your
performance claim: ~0.2 secs compared to what?  What's your test case?
I mean, this tiny code snippet can't take 0.2 secs per single call,
right?


Corinna


> --
> 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


-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-11 13:08 [PATCH] Check for existence of the path before processing '..' Fedin Pavel
  2013-06-11 13:48 ` Corinna Vinschen
@ 2013-06-11 14:20 ` Christopher Faylor
  2013-06-11 15:04   ` Corinna Vinschen
  2013-06-12 17:46 ` [GOLDSTAR] " Corinna Vinschen
  2 siblings, 1 reply; 16+ messages in thread
From: Christopher Faylor @ 2013-06-11 14:20 UTC (permalink / raw)
  To: cygwin

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.

Also you are calling path_conv recursively.  I assume that is where you
are seeing a performance hit.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-11 14:20 ` Christopher Faylor
@ 2013-06-11 15:04   ` Corinna Vinschen
  2013-06-11 15:09     ` Corinna Vinschen
  2013-06-11 15:17     ` Christopher Faylor
  0 siblings, 2 replies; 16+ messages in thread
From: Corinna Vinschen @ 2013-06-11 15:04 UTC (permalink / raw)
  To: cygwin

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.

> 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.

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.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-11 15:04   ` Corinna Vinschen
@ 2013-06-11 15:09     ` Corinna Vinschen
  2013-06-11 15:17     ` Christopher Faylor
  1 sibling, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2013-06-11 15:09 UTC (permalink / raw)
  To: cygwin

On Jun 11 17:04, 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
> > [...]
> > 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.
> 
> 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.

Ouch, I computed 2 minutes as 60 seconds here :-P

Correctly computed it's even more impressive: 132.3 secs without, 133.4
secs with the patch.  These are average numbers over 5 runs each.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-11 15:04   ` Corinna Vinschen
  2013-06-11 15:09     ` Corinna Vinschen
@ 2013-06-11 15:17     ` Christopher Faylor
  2013-06-11 15:26       ` Corinna Vinschen
  2013-06-11 17:30       ` Corinna Vinschen
  1 sibling, 2 replies; 16+ messages in thread
From: Christopher Faylor @ 2013-06-11 15:17 UTC (permalink / raw)
  To: cygwin

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-11 15:17     ` Christopher Faylor
@ 2013-06-11 15:26       ` Corinna Vinschen
  2013-06-11 17:30       ` Corinna Vinschen
  1 sibling, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2013-06-11 15:26 UTC (permalink / raw)
  To: cygwin

On Jun 11 11:17, Christopher Faylor wrote:
> On Tue, Jun 11, 2013 at 05:04:46PM +0200, Corinna Vinschen wrote:
> >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.

Shouldn't that be much reduced by the fact that the temporary buffers
used by path_conv are tmp_pathbuf buffers?  Originally, when we started
with 1.7, we had all those 64K buffers on the stack and thus a lot of
spurious crashes due to stack overflow.  But since using tmp_pathbuf
buffers we got rid of those.

Maybe we should contemplate the idea to raise the maximum number of
tmp_pathbuf buffers to accommodate situations we're not aware of at
this point in time.

> >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.

The problem with .. is that a path component disappears while normalizing
the path, without checking it's existence.  Therefore foo/./bar is no
problem here, because the final patch still contains foo and thus it's
existence will be checked anyway.

I may be missing something, but if we don't remove .. and . from the
path right at the start of path_conv, then we have exactly the problem
which so far kept us from doing the check:  A . or .. component spoils
the mount_table->conv_to_win32_path path, and it might (I'm *not* sure)
break the sym.check call as well.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-11 15:17     ` Christopher Faylor
  2013-06-11 15:26       ` Corinna Vinschen
@ 2013-06-11 17:30       ` Corinna Vinschen
  1 sibling, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2013-06-11 17:30 UTC (permalink / raw)
  To: cygwin

On Jun 11 11:17, Christopher Faylor wrote:
> On Tue, Jun 11, 2013 at 05:04:46PM +0200, Corinna Vinschen wrote:
> >On Jun 11 10:20, Christopher Faylor wrote:
> >> 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.

Actually, to my own surprise, it does.  I removed the flag and the code
takes 3 seconds longer on average to build coreutils:  136.2 vs. 133.4
vs. 132.3.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [GOLDSTAR] Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-11 13:08 [PATCH] Check for existence of the path before processing '..' Fedin Pavel
  2013-06-11 13:48 ` Corinna Vinschen
  2013-06-11 14:20 ` Christopher Faylor
@ 2013-06-12 17:46 ` Corinna Vinschen
  2013-06-12 19:44   ` Corinna Vinschen
                     ` (2 more replies)
  2 siblings, 3 replies; 16+ messages in thread
From: Corinna Vinschen @ 2013-06-12 17:46 UTC (permalink / raw)
  To: cygwin

Hi Fedin,

On Jun 11 17:08, 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.

I applied your patch with a little stretch of imagination as falling
under the trivial patch rule, which doesn't require a copyright
assignment.  I only tweaked it slightly since I found that moving the
setting of check_parent has a tiny performance advantage.

Cgf and I talked privately about this patch and we're both happy you
found such a simple solution to fix a long-standing problem.  Sometimes,
when you're working long enough on some code, you just miss to see the
wood for the trees.

Andrew, can you please polish one of the goldstar's in your vault and
give it to Fedin?

Btw., Fedin, even if I let this go in under the trivial patch rule, it
would be very nice if you could fill out the Cygwin copyright assignment
form http://cygwin.com/assign.txt and send it to the given address.
This allows you to provide any length of patch in future.


Thanks again,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GOLDSTAR] Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-12 17:46 ` [GOLDSTAR] " Corinna Vinschen
@ 2013-06-12 19:44   ` Corinna Vinschen
  2013-06-13  9:09     ` Fedin Pavel
  2013-06-13  1:58   ` Andrew Schulman
  2013-06-17 12:38   ` Fedin Pavel
  2 siblings, 1 reply; 16+ messages in thread
From: Corinna Vinschen @ 2013-06-12 19:44 UTC (permalink / raw)
  To: cygwin

On Jun 12 19:46, Corinna Vinschen wrote:
> Hi Fedin,
> 
> On Jun 11 17:08, 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.
> 
> I applied your patch with a little stretch of imagination as falling
> under the trivial patch rule, which doesn't require a copyright
> assignment.  I only tweaked it slightly since I found that moving the
> setting of check_parent has a tiny performance advantage.

FYI, I just uploaded a new 32 bit snapshot, as well as the 64 bit
test package 1.7.21-2 containing this patch.

Please give it a try.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GOLDSTAR] Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-12 17:46 ` [GOLDSTAR] " Corinna Vinschen
  2013-06-12 19:44   ` Corinna Vinschen
@ 2013-06-13  1:58   ` Andrew Schulman
  2013-06-17 12:38   ` Fedin Pavel
  2 siblings, 0 replies; 16+ messages in thread
From: Andrew Schulman @ 2013-06-13  1:58 UTC (permalink / raw)
  To: cygwin

> Cgf and I talked privately about this patch and we're both happy you
> found such a simple solution to fix a long-standing problem.  Sometimes,
> when you're working long enough on some code, you just miss to see the
> wood for the trees.
> 
> Andrew, can you please polish one of the goldstar's in your vault and
> give it to Fedin?

Awarded!  http://cygwin.com/goldstars/#FP


--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH] Check for existence of the path before processing '..'
  2013-06-11 13:48 ` Corinna Vinschen
@ 2013-06-13  6:10   ` Fedin Pavel
  0 siblings, 0 replies; 16+ messages in thread
From: Fedin Pavel @ 2013-06-13  6:10 UTC (permalink / raw)
  To: cygwin

 Hello! Sorry for delayed replies, at home i'm not subscribed to Cygwin ML, and in Russia we had a holiday yesterday.

> Thanks for the patch.  The idea sounds good, and I think it's the right
> thing to do *not* to add this to normalize_win32_path, because the ..
> semantics on WINdows are so that a .. is allowed even if the parent
> doesn't exist.

 Yes, i agree. After all, POSIX rules apply to POSIX paths. And having Win32 paths in POSIX environment is kind of supernatural. :)

> I didn't test your patch so far, but I'm a bit puzzled about your
> performance claim: ~0.2 secs compared to what?  What's your test case?

 I don't remember exactly, but i've done something like 'time ls -l ../bin' after cd'ing to /usr/src.

> I mean, this tiny code snippet can't take 0.2 secs per single call,

 Yes, but:
 1. From strace it seems that 'ls -l' fstat()s every file with file name appended to the given path. And each file gets '..' in its path.
 2. Path conversion implies reading mount tables, symlinks, etc. I tried to imagine how this could be optimized, but found no simple solution. Because in general case this should also work for things like '/mnt/drive1/../symlink2/../drive3', across all mounts and symlinks. So, this solution is kind of balance between performance and simplicity.

 Actually, this even can be optimized by implementing a part-by-part path conversion, for example (imagine we get /usr/src/../bin as argument):
 1. Start conversion until we meet '..'. Remember this place.
 2. Convert '/usr/src' to Windows format, get 'C:\cygwin64\usr\src'.
 3. Check for existence.
 4. Step one component back in Win32 path, get 'C:\cygwin64\usr'.
 5. Proceed with conversion from the point remembered in (1), the part of path is already converted and checked, we don't need to re-convert it.
 But this approach would really require total overhaul of all the path handling which is difficult.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia



--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [GOLDSTAR] Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-12 19:44   ` Corinna Vinschen
@ 2013-06-13  9:09     ` Fedin Pavel
  0 siblings, 0 replies; 16+ messages in thread
From: Fedin Pavel @ 2013-06-13  9:09 UTC (permalink / raw)
  To: cygwin

 Hello!

> FYI, I just uploaded a new 32 bit snapshot, as well as the 64 bit test
> package 1.7.21-2 containing this patch.
> 
> Please give it a try.

 I have tested new Cygwin64 using original case, by attempting to rebuild eglibc (ARM-Linux targeted cross-build) from scratch (make clean; make). Works great. Thank you!

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia



--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [GOLDSTAR] Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-12 17:46 ` [GOLDSTAR] " Corinna Vinschen
  2013-06-12 19:44   ` Corinna Vinschen
  2013-06-13  1:58   ` Andrew Schulman
@ 2013-06-17 12:38   ` Fedin Pavel
  2013-06-17 12:51     ` Fedin Pavel
  2013-06-17 12:55     ` Corinna Vinschen
  2 siblings, 2 replies; 16+ messages in thread
From: Fedin Pavel @ 2013-06-17 12:38 UTC (permalink / raw)
  To: cygwin; +Cc: 'Corinna Vinschen'

 Hello!
 How can i contact you in private ? The address specified in messages can be used only within the list, and the server forces me to cc: to the public.

> Btw., Fedin, even if I let this go in under the trivial patch rule, it
> would be very nice if you could fill out the Cygwin copyright
> assignment form http://cygwin.com/assign.txt and send it to the given
> address.
> This allows you to provide any length of patch in future.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia



--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [GOLDSTAR] Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-17 12:38   ` Fedin Pavel
@ 2013-06-17 12:51     ` Fedin Pavel
  2013-06-17 12:55     ` Corinna Vinschen
  1 sibling, 0 replies; 16+ messages in thread
From: Fedin Pavel @ 2013-06-17 12:51 UTC (permalink / raw)
  To: cygwin; +Cc: 'Corinna Vinschen'

 Hello!
 How can i contact you in private ? The address specified in messages can be used only within the list, and the server forces me to cc: to the public.

> Btw., Fedin, even if I let this go in under the trivial patch rule, it
> would be very nice if you could fill out the Cygwin copyright
> assignment form http://cygwin.com/assign.txt and send it to the given
> address.
> This allows you to provide any length of patch in future.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia



--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GOLDSTAR] Re: [PATCH] Check for existence of the path before processing '..'
  2013-06-17 12:38   ` Fedin Pavel
  2013-06-17 12:51     ` Fedin Pavel
@ 2013-06-17 12:55     ` Corinna Vinschen
  1 sibling, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2013-06-17 12:55 UTC (permalink / raw)
  To: cygwin

On Jun 17 16:37, Fedin Pavel wrote:
>  Hello!
>  How can i contact you in private ? The address specified in messages
>  can be used only within the list, and the server forces me to cc: to
>  the public.

That's deliberate, of course ;)

If it's something which requires privacy you can try the address in my
Cygwin ChangeLog entries.  Just if you want to send me your assignment,
it's of no use.  You have to send it via snail mail to the address given
in the assign.txt file itself.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

--
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2013-06-17 12:54 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-11 13:08 [PATCH] Check for existence of the path before processing '..' Fedin Pavel
2013-06-11 13:48 ` Corinna Vinschen
2013-06-13  6:10   ` Fedin Pavel
2013-06-11 14:20 ` Christopher Faylor
2013-06-11 15:04   ` Corinna Vinschen
2013-06-11 15:09     ` Corinna Vinschen
2013-06-11 15:17     ` Christopher Faylor
2013-06-11 15:26       ` Corinna Vinschen
2013-06-11 17:30       ` Corinna Vinschen
2013-06-12 17:46 ` [GOLDSTAR] " Corinna Vinschen
2013-06-12 19:44   ` Corinna Vinschen
2013-06-13  9:09     ` Fedin Pavel
2013-06-13  1:58   ` Andrew Schulman
2013-06-17 12:38   ` Fedin Pavel
2013-06-17 12:51     ` Fedin Pavel
2013-06-17 12:55     ` Corinna Vinschen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).