public inbox for cygwin-patches@cygwin.com
 help / color / mirror / Atom feed
* [PATCH] Cygwin: tweak handling of native symlinks from chdir
@ 2021-05-29 20:34 Jeremy Drake
  2021-05-29 23:15 ` Jeremy Drake
  0 siblings, 1 reply; 17+ messages in thread
From: Jeremy Drake @ 2021-05-29 20:34 UTC (permalink / raw)
  To: cygwin-patches

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

First, revert the handling of virtual drives as non-symlinks.  This is no
longer necessary.

Then, change the handling that really matters for my situation:

The new GetFinalPathNameW handling for native symlinks in inner path
components is disabled in chdir, allowing native processes to see their
cwd potentially including native symlinks, rather than dereferencing
them.

This seems to work for virtual drives being the CWD for native proceses,
both for the root and subdirectories.

Thoughts?

[-- Attachment #2: Type: text/plain, Size: 2140 bytes --]

From 352de39c3474fdb73570121820b493bd81a73bb5 Mon Sep 17 00:00:00 2001
From: Jeremy Drake <cygwin@jdrake.com>
Date: Sat, 29 May 2021 11:48:11 -0700
Subject: [PATCH 2/2] Cygwin: tweak handling of native symlinks from chdir

The new GetFinalPathNameW handling for native symlinks in inner path
components is disabled in chdir, allowing native processes to see their
cwd potentially including native symlinks, rather than dereferencing
them.
---
 winsup/cygwin/path.cc | 5 +++--
 winsup/cygwin/path.h  | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index e62f8fe2b..130926cbc 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -751,7 +751,7 @@ path_conv::check (const char *src, unsigned opt,
 	      if (error)
 		return;
 
-	      sym.pc_flags = pc_flags;
+	      sym.pc_flags = pc_flags | (opt & PC_NO_NATIVE_SYM_INNER);
 
 	      if (!dev.exists ())
 		{
@@ -3480,6 +3480,7 @@ restart:
 	    goto file_not_symlink;
 	}
 #endif /* __i386__ */
+      if (!(pc_flags & PC_NO_NATIVE_SYM_INNER))
       {
 	PWCHAR fpbuf = tp.w_get ();
 	DWORD ret;
@@ -3704,7 +3705,7 @@ chdir (const char *in_dir)
 
       /* Convert path.  PC_NONULLEMPTY ensures that we don't check for
 	 NULL/empty/invalid again. */
-      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY);
+      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY | PC_NO_NATIVE_SYM_INNER);
       if (path.error)
 	{
 	  set_errno (path.error);
diff --git a/winsup/cygwin/path.h b/winsup/cygwin/path.h
index adb0ca11f..d3b14a40c 100644
--- a/winsup/cygwin/path.h
+++ b/winsup/cygwin/path.h
@@ -59,6 +59,7 @@ enum pathconv_arg
   PC_KEEP_HANDLE	 = _BIT (12),	/* keep handle for later stat calls */
   PC_NO_ACCESS_CHECK	 = _BIT (13),	/* helper flag for error check */
   PC_SYM_NOFOLLOW_DIR	 = _BIT (14),	/* don't follow a trailing slash */
+  PC_NO_NATIVE_SYM_INNER = _BIT (15),	/* skip native symlink inner handling */
   PC_DONT_USE		 = _BIT (31)	/* conversion to signed happens. */
 };
 
-- 
2.31.1.windows.1


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

From 4bb959b57606465d5a7abe7d3ae168db66f5f6fa Mon Sep 17 00:00:00 2001
From: Jeremy Drake <cygwin@jdrake.com>
Date: Sat, 29 May 2021 13:17:08 -0700
Subject: [PATCH 1/2] Revert "Cygwin: Handle virtual drives as non-symlinks"

This reverts commit c8949d04001e3dbc03651475b6cd1c5623400835.
---
 winsup/cygwin/path.cc | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 6a07f0d06..e62f8fe2b 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -3505,9 +3505,14 @@ restart:
 
 		     subst X: C:\foo\bar
 
-		   Treat it as a normal file. */
+		   Treat it like a symlink.  This is required to tell an
+		   lstat caller that the "drive" is actually pointing
+		   somewhere else, thus, it's a symlink in POSIX speak. */
 		if (upath.Length == 14)	/* \??\X:\ */
-		  goto file_not_symlink;
+		  {
+		    fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
+		    path_flags |= PATH_SYMLINK;
+		  }
 		/* For final paths differing in inner path components return
 		   length as negative value.  This informs path_conv::check
 		   to skip realpath handling on the last path component. */
-- 
2.31.1.windows.1


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

* Re: [PATCH] Cygwin: tweak handling of native symlinks from chdir
  2021-05-29 20:34 [PATCH] Cygwin: tweak handling of native symlinks from chdir Jeremy Drake
@ 2021-05-29 23:15 ` Jeremy Drake
  2021-05-30  6:05   ` Jeremy Drake
  0 siblings, 1 reply; 17+ messages in thread
From: Jeremy Drake @ 2021-05-29 23:15 UTC (permalink / raw)
  To: cygwin-patches

On Sat, 29 May 2021, Jeremy Drake wrote:

> First, revert the handling of virtual drives as non-symlinks.  This is no
> longer necessary.
>

Spoke too soon, it seems that `makepkg` uses `pwd -P` and that still
dereferences symlinks, as does `pwd -W` and `cygpath -w .`  :(

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

* Re: [PATCH] Cygwin: tweak handling of native symlinks from chdir
  2021-05-29 23:15 ` Jeremy Drake
@ 2021-05-30  6:05   ` Jeremy Drake
  2021-05-30 19:58     ` [PATCH v2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links Jeremy Drake
  0 siblings, 1 reply; 17+ messages in thread
From: Jeremy Drake @ 2021-05-30  6:05 UTC (permalink / raw)
  To: cygwin-patches

On Sat, 29 May 2021, Jeremy Drake via Cygwin-patches wrote:

> On Sat, 29 May 2021, Jeremy Drake wrote:
>
> > First, revert the handling of virtual drives as non-symlinks.  This is no
> > longer necessary.
> >
>
> Spoke too soon, it seems that `makepkg` uses `pwd -P` and that still
> dereferences symlinks, as does `pwd -W` and `cygpath -w .`  :(

With the two patches from the OP, plus patching `makepkg` to change all
`pwd -P` to straight `pwd`, AND patching msys2's msys2_path_conv.cc
`posix_to_win32_path` to use the new flag to path_conv, I was finally able
to build the problematic package with the long name...  I imagine
`cygwin_conv_path` should have that flag too, for cygpath and `pwd -W`.

Hopefully there's a better (less invasive) approach that I'm not seeing.

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

* [PATCH v2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-05-30  6:05   ` Jeremy Drake
@ 2021-05-30 19:58     ` Jeremy Drake
  2021-05-31  8:02       ` Corinna Vinschen
  0 siblings, 1 reply; 17+ messages in thread
From: Jeremy Drake @ 2021-05-30 19:58 UTC (permalink / raw)
  To: cygwin-patches

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

First, revert the handling of virtual drives as non-symlinks.  This is no
longer necessary.

The new GetFinalPathNameW handling for native symlinks in inner path
components is disabled if caller doesn't want to follow symlinks, or
doesn't want to follow reparse points.  Set flag to not follow reparse
points in chdir, allowing native processes to see their cwd potentially
including native symlinks, rather than dereferencing them.
---

For v2, I realized the PC_SYM_NOFOLLOW_REP flag was supposed to do this,
and that lack of PC_SYM_FOLLOW was not being respected either.  With this,
and patching `pwd -P` to `pwd` in makepkg, the long-named package builds
successfully.  I did not re-indent the code for the addition of the if due
to having learned from my patch to rebase, but it looks kind of ugly.

 winsup/cygwin/path.cc | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index e62f8fe2b..2ce5aef81 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -722,9 +722,9 @@ path_conv::check (const char *src, unsigned opt,
 	  int symlen = 0;

 	  /* Make sure to check certain flags on last component only. */
-	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE);
+	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE | PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP);
 	       ;
-	       pc_flags = 0)
+	       pc_flags = opt & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP))
 	    {
 	      const suffix_info *suff;
 	      char *full_path;
@@ -3452,6 +3452,8 @@ restart:
 	    break;
 	}

+      if ((pc_flags & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP)) == PC_SYM_FOLLOW)
+      {
       /* Check if the inner path components contain native symlinks or
 	 junctions, or if the drive is a virtual drive.  Compare incoming
 	 path with path returned by GetFinalPathNameByHandleA.  If they
@@ -3522,6 +3524,7 @@ restart:
 	      }
 	  }
       }
+      }

     /* Normal file. */
     file_not_symlink:
@@ -3704,7 +3707,7 @@ chdir (const char *in_dir)

       /* Convert path.  PC_NONULLEMPTY ensures that we don't check for
 	 NULL/empty/invalid again. */
-      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY);
+      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY | PC_SYM_NOFOLLOW_REP);
       if (path.error)
 	{
 	  set_errno (path.error);
-- 
2.31.1.windows.1

[-- Attachment #2: Type: text/plain, Size: 1216 bytes --]

From 4bb959b57606465d5a7abe7d3ae168db66f5f6fa Mon Sep 17 00:00:00 2001
From: Jeremy Drake <cygwin@jdrake.com>
Date: Sat, 29 May 2021 13:17:08 -0700
Subject: [PATCH 1/2] Revert "Cygwin: Handle virtual drives as non-symlinks"

This reverts commit c8949d04001e3dbc03651475b6cd1c5623400835.
---
 winsup/cygwin/path.cc | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 6a07f0d06..e62f8fe2b 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -3505,9 +3505,14 @@ restart:
 
 		     subst X: C:\foo\bar
 
-		   Treat it as a normal file. */
+		   Treat it like a symlink.  This is required to tell an
+		   lstat caller that the "drive" is actually pointing
+		   somewhere else, thus, it's a symlink in POSIX speak. */
 		if (upath.Length == 14)	/* \??\X:\ */
-		  goto file_not_symlink;
+		  {
+		    fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
+		    path_flags |= PATH_SYMLINK;
+		  }
 		/* For final paths differing in inner path components return
 		   length as negative value.  This informs path_conv::check
 		   to skip realpath handling on the last path component. */
-- 
2.31.1.windows.1


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

From 9a1d868c3e027416876d9bd110161562f8b77b0a Mon Sep 17 00:00:00 2001
From: Jeremy Drake <cygwin@jdrake.com>
Date: Sat, 29 May 2021 11:48:11 -0700
Subject: [PATCH 2/2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP
 with inner links.

The new GetFinalPathNameW handling for native symlinks in inner path
components is disabled if caller doesn't want to follow symlinks, or
doesn't want to follow reparse points.  Set flag to not follow reparse
points in chdir, allowing native processes to see their cwd potentially
including native symlinks, rather than dereferencing them.
---
 winsup/cygwin/path.cc | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index e62f8fe2b..2ce5aef81 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -722,9 +722,9 @@ path_conv::check (const char *src, unsigned opt,
 	  int symlen = 0;
 
 	  /* Make sure to check certain flags on last component only. */
-	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE);
+	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE | PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP);
 	       ;
-	       pc_flags = 0)
+	       pc_flags = opt & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP))
 	    {
 	      const suffix_info *suff;
 	      char *full_path;
@@ -3452,6 +3452,8 @@ restart:
 	    break;
 	}
 
+      if ((pc_flags & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP)) == PC_SYM_FOLLOW)
+      {
       /* Check if the inner path components contain native symlinks or
 	 junctions, or if the drive is a virtual drive.  Compare incoming
 	 path with path returned by GetFinalPathNameByHandleA.  If they
@@ -3522,6 +3524,7 @@ restart:
 	      }
 	  }
       }
+      }
 
     /* Normal file. */
     file_not_symlink:
@@ -3704,7 +3707,7 @@ chdir (const char *in_dir)
 
       /* Convert path.  PC_NONULLEMPTY ensures that we don't check for
 	 NULL/empty/invalid again. */
-      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY);
+      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY | PC_SYM_NOFOLLOW_REP);
       if (path.error)
 	{
 	  set_errno (path.error);
-- 
2.31.1.windows.1


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

* Re: [PATCH v2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-05-30 19:58     ` [PATCH v2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links Jeremy Drake
@ 2021-05-31  8:02       ` Corinna Vinschen
  2021-05-31  8:17         ` Corinna Vinschen
  2021-06-03 20:29         ` [PATCH v3] " Jeremy Drake
  0 siblings, 2 replies; 17+ messages in thread
From: Corinna Vinschen @ 2021-05-31  8:02 UTC (permalink / raw)
  To: cygwin-patches

On May 30 12:58, Jeremy Drake via Cygwin-patches wrote:
> First, revert the handling of virtual drives as non-symlinks.  This is no
> longer necessary.

I'm all for it, because I like the idea that Cygwin can see virtual
drives as symlinks, but...

> The new GetFinalPathNameW handling for native symlinks in inner path
> components is disabled if caller doesn't want to follow symlinks, or
> doesn't want to follow reparse points.  Set flag to not follow reparse
> points in chdir, allowing native processes to see their cwd potentially
> including native symlinks, rather than dereferencing them.

So you're trying to keep the path length of the native CWD below
MAX_PATH?  I understand what you're trying to accomplish, but are
you sure this doesn't break Cygwin processes?  The idea of what
the native path of a directory is differs depending on calling
chdir and stuff like mkdir.

> For v2, I realized the PC_SYM_NOFOLLOW_REP flag was supposed to do this,
> and that lack of PC_SYM_FOLLOW was not being respected either.  With this,
> and patching `pwd -P` to `pwd` in makepkg, the long-named package builds
> successfully.  I did not re-indent the code for the addition of the if due
> to having learned from my patch to rebase, but it looks kind of ugly.

Formatting should try to stick to 80 chars max line length, if possible.
Kind of like this, just with TABs:

-	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE);
+	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE
					  | PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP);


Thanks,
Corinna

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

* Re: [PATCH v2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-05-31  8:02       ` Corinna Vinschen
@ 2021-05-31  8:17         ` Corinna Vinschen
  2021-05-31 17:55           ` Jeremy Drake
                             ` (2 more replies)
  2021-06-03 20:29         ` [PATCH v3] " Jeremy Drake
  1 sibling, 3 replies; 17+ messages in thread
From: Corinna Vinschen @ 2021-05-31  8:17 UTC (permalink / raw)
  To: cygwin-patches

On May 31 10:02, Corinna Vinschen wrote:
> On May 30 12:58, Jeremy Drake via Cygwin-patches wrote:
> > First, revert the handling of virtual drives as non-symlinks.  This is no
> > longer necessary.
> 
> I'm all for it, because I like the idea that Cygwin can see virtual
> drives as symlinks, but...
> 
> > The new GetFinalPathNameW handling for native symlinks in inner path
> > components is disabled if caller doesn't want to follow symlinks, or
> > doesn't want to follow reparse points.  Set flag to not follow reparse
> > points in chdir, allowing native processes to see their cwd potentially
> > including native symlinks, rather than dereferencing them.
> 
> So you're trying to keep the path length of the native CWD below
> MAX_PATH?  I understand what you're trying to accomplish, but are
> you sure this doesn't break Cygwin processes?  The idea of what
> the native path of a directory is differs depending on calling
> chdir and stuff like mkdir.

What bugs me here is that there's no guarantee that you can keep your
path below MAX_PATH, independently of what you do here.  This is all
a bit like patching up left and right just to keep dumb native tools
running even in scenarios where they just fail otherwise.

So we have two contradict problems, one which is solved by following
inner symlinks, one which is solved by not doing that... I'm not overly
keen to support this scenario.

Wouldn't that be something more suited for an MSYS2-local patch?


Corinna

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

* Re: [PATCH v2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-05-31  8:17         ` Corinna Vinschen
@ 2021-05-31 17:55           ` Jeremy Drake
  2021-07-03 21:01           ` Jeremy Drake
  2021-07-07 18:52           ` Jeremy Drake
  2 siblings, 0 replies; 17+ messages in thread
From: Jeremy Drake @ 2021-05-31 17:55 UTC (permalink / raw)
  To: cygwin-patches

On Mon, 31 May 2021, Corinna Vinschen wrote:

> > So you're trying to keep the path length of the native CWD below
> > MAX_PATH?  I understand what you're trying to accomplish, but are
> > you sure this doesn't break Cygwin processes?  The idea of what
> > the native path of a directory is differs depending on calling
> > chdir and stuff like mkdir.

I'm not sure.  I've been running builds with this patch for a bit, and
haven't seen any issue, but MSYS2 doesn't use native symlinks so that
aspect of it hasn't been exercised.

>
> What bugs me here is that there's no guarantee that you can keep your
> path below MAX_PATH, independently of what you do here.  This is all
> a bit like patching up left and right just to keep dumb native tools
> running even in scenarios where they just fail otherwise.

Basically.  I wish there was a viable alternative (requiring everyone
trying to use them to set a registry value/policy, manifesting them for
long paths, and potentially patching them to be safe with long paths isn't
very viable).

> So we have two contradict problems, one which is solved by following
> inner symlinks, one which is solved by not doing that... I'm not overly
> keen to support this scenario.
>
> Wouldn't that be something more suited for an MSYS2-local patch?

Just the changing of the flag in chdir?  Because it seems like not
respecting the symlink-related PC flags for native inner links is a
bona-fide issue.


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

* [PATCH v3] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-05-31  8:02       ` Corinna Vinschen
  2021-05-31  8:17         ` Corinna Vinschen
@ 2021-06-03 20:29         ` Jeremy Drake
  2021-06-03 20:57           ` Jeremy Drake
  1 sibling, 1 reply; 17+ messages in thread
From: Jeremy Drake @ 2021-06-03 20:29 UTC (permalink / raw)
  To: cygwin-patches

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

Just updated for formatting.

> Formatting should try to stick to 80 chars max line length, if possible.
> Kind of like this, just with TABs:
>
> -	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE);
> +	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE
> 					  | PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP);
>

[-- Attachment #2: Type: text/plain, Size: 1216 bytes --]

From 4bb959b57606465d5a7abe7d3ae168db66f5f6fa Mon Sep 17 00:00:00 2001
From: Jeremy Drake <cygwin@jdrake.com>
Date: Sat, 29 May 2021 13:17:08 -0700
Subject: [PATCH 1/2] Revert "Cygwin: Handle virtual drives as non-symlinks"

This reverts commit c8949d04001e3dbc03651475b6cd1c5623400835.
---
 winsup/cygwin/path.cc | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 6a07f0d06..e62f8fe2b 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -3505,9 +3505,14 @@ restart:
 
 		     subst X: C:\foo\bar
 
-		   Treat it as a normal file. */
+		   Treat it like a symlink.  This is required to tell an
+		   lstat caller that the "drive" is actually pointing
+		   somewhere else, thus, it's a symlink in POSIX speak. */
 		if (upath.Length == 14)	/* \??\X:\ */
-		  goto file_not_symlink;
+		  {
+		    fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
+		    path_flags |= PATH_SYMLINK;
+		  }
 		/* For final paths differing in inner path components return
 		   length as negative value.  This informs path_conv::check
 		   to skip realpath handling on the last path component. */
-- 
2.31.1.windows.1


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

From ea36ccb13b2080663535d867e6fe8edf246efe83 Mon Sep 17 00:00:00 2001
From: Jeremy Drake <github@jdrake.com>
Date: Sat, 29 May 2021 11:48:11 -0700
Subject: [PATCH 2/2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP
 with inner links.

The new GetFinalPathNameW handling for native symlinks in inner path
components is disabled if caller doesn't want to follow symlinks, or
doesn't want to follow reparse points.  Set flag to not follow reparse
points in chdir, allowing native processes to see their cwd potentially
including native symlinks, rather than dereferencing them.
---
 winsup/cygwin/path.cc | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index e62f8fe2b..a6bb3aeff 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -722,9 +722,10 @@ path_conv::check (const char *src, unsigned opt,
 	  int symlen = 0;
 
 	  /* Make sure to check certain flags on last component only. */
-	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE);
+	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE
+					 | PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP);
 	       ;
-	       pc_flags = 0)
+	       pc_flags = opt & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP))
 	    {
 	      const suffix_info *suff;
 	      char *full_path;
@@ -3452,6 +3453,8 @@ restart:
 	    break;
 	}
 
+      if ((pc_flags & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP)) == PC_SYM_FOLLOW)
+      {
       /* Check if the inner path components contain native symlinks or
 	 junctions, or if the drive is a virtual drive.  Compare incoming
 	 path with path returned by GetFinalPathNameByHandleA.  If they
@@ -3522,6 +3525,7 @@ restart:
 	      }
 	  }
       }
+      }
 
     /* Normal file. */
     file_not_symlink:
@@ -3704,7 +3708,8 @@ chdir (const char *in_dir)
 
       /* Convert path.  PC_NONULLEMPTY ensures that we don't check for
 	 NULL/empty/invalid again. */
-      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY);
+      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY
+			      | PC_SYM_NOFOLLOW_REP);
       if (path.error)
 	{
 	  set_errno (path.error);
-- 
2.31.1.windows.1


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

* Re: [PATCH v3] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-06-03 20:29         ` [PATCH v3] " Jeremy Drake
@ 2021-06-03 20:57           ` Jeremy Drake
  2021-07-06 14:57             ` Corinna Vinschen
  0 siblings, 1 reply; 17+ messages in thread
From: Jeremy Drake @ 2021-06-03 20:57 UTC (permalink / raw)
  To: cygwin-patches

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

On Thu, 3 Jun 2021, Jeremy Drake via Cygwin-patches wrote:

> Just updated for formatting.

Oops, forgot to edit the email address on patch 2.  Resending with that
fixed.

[-- Attachment #2: Type: text/plain, Size: 1216 bytes --]

From 4bb959b57606465d5a7abe7d3ae168db66f5f6fa Mon Sep 17 00:00:00 2001
From: Jeremy Drake <cygwin@jdrake.com>
Date: Sat, 29 May 2021 13:17:08 -0700
Subject: [PATCH 1/2] Revert "Cygwin: Handle virtual drives as non-symlinks"

This reverts commit c8949d04001e3dbc03651475b6cd1c5623400835.
---
 winsup/cygwin/path.cc | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 6a07f0d06..e62f8fe2b 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -3505,9 +3505,14 @@ restart:
 
 		     subst X: C:\foo\bar
 
-		   Treat it as a normal file. */
+		   Treat it like a symlink.  This is required to tell an
+		   lstat caller that the "drive" is actually pointing
+		   somewhere else, thus, it's a symlink in POSIX speak. */
 		if (upath.Length == 14)	/* \??\X:\ */
-		  goto file_not_symlink;
+		  {
+		    fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
+		    path_flags |= PATH_SYMLINK;
+		  }
 		/* For final paths differing in inner path components return
 		   length as negative value.  This informs path_conv::check
 		   to skip realpath handling on the last path component. */
-- 
2.31.1.windows.1


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

From ea36ccb13b2080663535d867e6fe8edf246efe83 Mon Sep 17 00:00:00 2001
From: Jeremy Drake <cygwin@jdrake.com>
Date: Sat, 29 May 2021 11:48:11 -0700
Subject: [PATCH 2/2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP
 with inner links.

The new GetFinalPathNameW handling for native symlinks in inner path
components is disabled if caller doesn't want to follow symlinks, or
doesn't want to follow reparse points.  Set flag to not follow reparse
points in chdir, allowing native processes to see their cwd potentially
including native symlinks, rather than dereferencing them.
---
 winsup/cygwin/path.cc | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index e62f8fe2b..a6bb3aeff 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -722,9 +722,10 @@ path_conv::check (const char *src, unsigned opt,
 	  int symlen = 0;
 
 	  /* Make sure to check certain flags on last component only. */
-	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE);
+	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE
+					 | PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP);
 	       ;
-	       pc_flags = 0)
+	       pc_flags = opt & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP))
 	    {
 	      const suffix_info *suff;
 	      char *full_path;
@@ -3452,6 +3453,8 @@ restart:
 	    break;
 	}
 
+      if ((pc_flags & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP)) == PC_SYM_FOLLOW)
+      {
       /* Check if the inner path components contain native symlinks or
 	 junctions, or if the drive is a virtual drive.  Compare incoming
 	 path with path returned by GetFinalPathNameByHandleA.  If they
@@ -3522,6 +3525,7 @@ restart:
 	      }
 	  }
       }
+      }
 
     /* Normal file. */
     file_not_symlink:
@@ -3704,7 +3708,8 @@ chdir (const char *in_dir)
 
       /* Convert path.  PC_NONULLEMPTY ensures that we don't check for
 	 NULL/empty/invalid again. */
-      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY);
+      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY
+			      | PC_SYM_NOFOLLOW_REP);
       if (path.error)
 	{
 	  set_errno (path.error);
-- 
2.31.1.windows.1


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

* Re: [PATCH v2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-05-31  8:17         ` Corinna Vinschen
  2021-05-31 17:55           ` Jeremy Drake
@ 2021-07-03 21:01           ` Jeremy Drake
  2021-07-07 18:52           ` Jeremy Drake
  2 siblings, 0 replies; 17+ messages in thread
From: Jeremy Drake @ 2021-07-03 21:01 UTC (permalink / raw)
  To: cygwin-patches

On Mon, 31 May 2021, Corinna Vinschen wrote:

> What bugs me here is that there's no guarantee that you can keep your
> path below MAX_PATH, independently of what you do here.  This is all
> a bit like patching up left and right just to keep dumb native tools
> running even in scenarios where they just fail otherwise.
>
> So we have two contradict problems, one which is solved by following
> inner symlinks, one which is solved by not doing that... I'm not overly
> keen to support this scenario.
>
> Wouldn't that be something more suited for an MSYS2-local patch?

I discussed this with the MSYS2 maintainers, and while they are open to
disabling this code in the short term, they would like to minimize the
patches against upstream Cygwin they carry.

For now I've proposed https://github.com/msys2/msys2-runtime/pull/54
there, and will test that is indeed the 'fix' for this (and a couple of
apparently related issues with mapped-network-drives turning into UNC for
Windows processes)

> Corinna


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

* Re: [PATCH v3] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-06-03 20:57           ` Jeremy Drake
@ 2021-07-06 14:57             ` Corinna Vinschen
  2021-07-06 17:38               ` Jeremy Drake
                                 ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Corinna Vinschen @ 2021-07-06 14:57 UTC (permalink / raw)
  To: Jeremy Drake; +Cc: cygwin-patches

On Jun  3 13:57, Jeremy Drake via Cygwin-patches wrote:
> [...]
> The new GetFinalPathNameW handling for native symlinks in inner path
> components is disabled if caller doesn't want to follow symlinks, or
> doesn't want to follow reparse points.  Set flag to not follow reparse
> points in chdir, allowing native processes to see their cwd potentially
> including native symlinks, rather than dereferencing them.
> ---
>  winsup/cygwin/path.cc | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
> index e62f8fe2b..a6bb3aeff 100644
> --- a/winsup/cygwin/path.cc
> +++ b/winsup/cygwin/path.cc
> @@ -722,9 +722,10 @@ path_conv::check (const char *src, unsigned opt,
>  	  int symlen = 0;
>  
>  	  /* Make sure to check certain flags on last component only. */
> -	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE);
> +	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE
> +					 | PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP);
>  	       ;
> -	       pc_flags = 0)
> +	       pc_flags = opt & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP))
>  	    {
>  	      const suffix_info *suff;
>  	      char *full_path;
> @@ -3452,6 +3453,8 @@ restart:
>  	    break;
>  	}
>  
> +      if ((pc_flags & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP)) == PC_SYM_FOLLOW)
> +      {
>        /* Check if the inner path components contain native symlinks or
>  	 junctions, or if the drive is a virtual drive.  Compare incoming
>  	 path with path returned by GetFinalPathNameByHandleA.  If they
> @@ -3522,6 +3525,7 @@ restart:
>  	      }
>  	  }
>        }
> +      }

This formatting is just ugly.  I suggest to move the PC_SYM_* test
to the block after the 32 bit code and reuse the existing braces,
just with adapted indentation, i. e.:

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 6a07f0d06850..cb0386e24005 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -3480,43 +3480,44 @@ restart:
 	    goto file_not_symlink;
 	}
 #endif /* __i386__ */
-      {
-	PWCHAR fpbuf = tp.w_get ();
-	DWORD ret;
+      if ((pc_flags & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP)) == PC_SYM_FOLLOW)
+	{
+	  PWCHAR fpbuf = tp.w_get ();
+	  DWORD ret;
[...indent all lines inside the block accordingly...] 
-      }
+	}
 
     /* Normal file. */
     file_not_symlink:

> @@ -3704,7 +3708,8 @@ chdir (const char *in_dir)
> 
>        /* Convert path.  PC_NONULLEMPTY ensures that we don't check for
>          NULL/empty/invalid again. */
> -      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY);
> +      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY
> +                             | PC_SYM_NOFOLLOW_REP);
>        if (path.error)
>         {
>           set_errno (path.error);

I'm still not convinced that we should do this.  I'm pretty certain this
will result in problems in Cygwin processes when you least expect them.

Consider that the output of getcwd and realpath/readlink on the same
path may differ after this patch.  Using PC_SYM_NOFOLLOW_REP like this
also changes the normal sym follow handling for the last path component
in path_copnv::check, potentially.

This looks like here be dragons.  A good solution would change the
used native tools to allow paths > MAX_PATH finally, or to use other,
equivalent tools already allowing that.

Patch 1 is ok, of course.  I pushed it.


Thanks,
Corinna

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

* Re: [PATCH v3] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-07-06 14:57             ` Corinna Vinschen
@ 2021-07-06 17:38               ` Jeremy Drake
  2021-07-06 17:40               ` [PATCH v4] " Jeremy Drake
  2021-07-07  6:50               ` [PATCH v3] " Jeremy Drake
  2 siblings, 0 replies; 17+ messages in thread
From: Jeremy Drake @ 2021-07-06 17:38 UTC (permalink / raw)
  To: cygwin-patches

On Tue, 6 Jul 2021, Corinna Vinschen wrote:

> This formatting is just ugly.  I suggest to move the PC_SYM_* test
> to the block after the 32 bit code and reuse the existing braces,
> just with adapted indentation, i. e.:

+1.  I was trying to avoid reformatting otherwise unchanged lines to
reduce patch size.

> > @@ -3704,7 +3708,8 @@ chdir (const char *in_dir)
> >
> >        /* Convert path.  PC_NONULLEMPTY ensures that we don't check for
> >          NULL/empty/invalid again. */
> > -      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY);
> > +      path_conv path (in_dir, PC_SYM_FOLLOW | PC_POSIX | PC_NONULLEMPTY
> > +                             | PC_SYM_NOFOLLOW_REP);
> >        if (path.error)
> >         {
> >           set_errno (path.error);
>
> I'm still not convinced that we should do this.  I'm pretty certain this
> will result in problems in Cygwin processes when you least expect them.
>
> Consider that the output of getcwd and realpath/readlink on the same
> path may differ after this patch.  Using PC_SYM_NOFOLLOW_REP like this
> also changes the normal sym follow handling for the last path component
> in path_copnv::check, potentially.
>
> This looks like here be dragons.  A good solution would change the
> used native tools to allow paths > MAX_PATH finally, or to use other,
> equivalent tools already allowing that.

I am not convinced that this even completely solved the issues I was
seeing, or some of the reports of issues with unc paths suddenly showing
up instead of mapped drives in native tools that weren't expecting them.

But, I do think respecting the PC_SYM_NOFOLLOW_REP flag for inner links is
correct, and I am sending a new version.

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

* [PATCH v4] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-07-06 14:57             ` Corinna Vinschen
  2021-07-06 17:38               ` Jeremy Drake
@ 2021-07-06 17:40               ` Jeremy Drake
  2021-07-07  8:47                 ` Corinna Vinschen
  2021-07-07  6:50               ` [PATCH v3] " Jeremy Drake
  2 siblings, 1 reply; 17+ messages in thread
From: Jeremy Drake @ 2021-07-06 17:40 UTC (permalink / raw)
  To: cygwin-patches

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

The new GetFinalPathNameW handling for native symlinks in inner path
components is disabled if caller doesn't want to follow symlinks, or
doesn't want to follow reparse points.
---
 winsup/cygwin/path.cc | 88 ++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 43 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index e62f8fe2b..1869fb8c8 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -722,9 +722,10 @@ path_conv::check (const char *src, unsigned opt,
 	  int symlen = 0;

 	  /* Make sure to check certain flags on last component only. */
-	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE);
+	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE
+					 | PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP);
 	       ;
-	       pc_flags = 0)
+	       pc_flags = opt & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP))
 	    {
 	      const suffix_info *suff;
 	      char *full_path;
@@ -3480,48 +3481,49 @@ restart:
 	    goto file_not_symlink;
 	}
 #endif /* __i386__ */
-      {
-	PWCHAR fpbuf = tp.w_get ();
-	DWORD ret;
-
-	ret = GetFinalPathNameByHandleW (h, fpbuf, NT_MAX_PATH, 0);
-	if (ret)
-	  {
-	    UNICODE_STRING fpath;
+      if ((pc_flags & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP)) == PC_SYM_FOLLOW)
+	{
+	  PWCHAR fpbuf = tp.w_get ();
+	  DWORD ret;

-	    RtlInitCountedUnicodeString (&fpath, fpbuf, ret * sizeof (WCHAR));
-	    fpbuf[1] = L'?';	/* \\?\ --> \??\ */
-	    if (!RtlEqualUnicodeString (&upath, &fpath, !!ci_flag))
-	      {
-		issymlink = true;
-		/* upath.Buffer is big enough and unused from this point on.
-		   Reuse it here, avoiding yet another buffer allocation. */
-		char *nfpath = (char *) upath.Buffer;
-		sys_wcstombs (nfpath, NT_MAX_PATH, fpbuf);
-		res = posixify (nfpath);
-
-		/* If the incoming path consisted of a drive prefix only,
-		   we just handle a virtual drive, created with, e.g.
-
-		     subst X: C:\foo\bar
-
-		   Treat it like a symlink.  This is required to tell an
-		   lstat caller that the "drive" is actually pointing
-		   somewhere else, thus, it's a symlink in POSIX speak. */
-		if (upath.Length == 14)	/* \??\X:\ */
-		  {
-		    fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
-		    path_flags |= PATH_SYMLINK;
-		  }
-		/* For final paths differing in inner path components return
-		   length as negative value.  This informs path_conv::check
-		   to skip realpath handling on the last path component. */
-		else
-		  res = -res;
-		break;
-	      }
-	  }
-      }
+	  ret = GetFinalPathNameByHandleW (h, fpbuf, NT_MAX_PATH, 0);
+	  if (ret)
+	    {
+	      UNICODE_STRING fpath;
+
+	      RtlInitCountedUnicodeString (&fpath, fpbuf, ret * sizeof (WCHAR));
+	      fpbuf[1] = L'?';	/* \\?\ --> \??\ */
+	      if (!RtlEqualUnicodeString (&upath, &fpath, !!ci_flag))
+	        {
+		  issymlink = true;
+		  /* upath.Buffer is big enough and unused from this point on.
+		     Reuse it here, avoiding yet another buffer allocation. */
+		  char *nfpath = (char *) upath.Buffer;
+		  sys_wcstombs (nfpath, NT_MAX_PATH, fpbuf);
+		  res = posixify (nfpath);
+
+		  /* If the incoming path consisted of a drive prefix only,
+		     we just handle a virtual drive, created with, e.g.
+
+		       subst X: C:\foo\bar
+
+		     Treat it like a symlink.  This is required to tell an
+		     lstat caller that the "drive" is actually pointing
+		     somewhere else, thus, it's a symlink in POSIX speak. */
+		  if (upath.Length == 14)	/* \??\X:\ */
+		    {
+		      fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
+		      path_flags |= PATH_SYMLINK;
+		    }
+		  /* For final paths differing in inner path components return
+		     length as negative value.  This informs path_conv::check
+		     to skip realpath handling on the last path component. */
+		  else
+		    res = -res;
+		  break;
+	        }
+	    }
+	}

     /* Normal file. */
     file_not_symlink:
-- 
2.32.0.windows.1

[-- Attachment #2: Type: text/plain, Size: 4296 bytes --]

From 67a276c35a3b48697c6b61caaf4ffea5a174c75b Mon Sep 17 00:00:00 2001
From: Jeremy Drake <cygwin@jdrake.com>
Date: Sat, 29 May 2021 11:48:11 -0700
Subject: [PATCH] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with
 inner links.

The new GetFinalPathNameW handling for native symlinks in inner path
components is disabled if caller doesn't want to follow symlinks, or
doesn't want to follow reparse points.
---
 winsup/cygwin/path.cc | 88 ++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 43 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index e62f8fe2b..1869fb8c8 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -722,9 +722,10 @@ path_conv::check (const char *src, unsigned opt,
 	  int symlen = 0;
 
 	  /* Make sure to check certain flags on last component only. */
-	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE);
+	  for (unsigned pc_flags = opt & (PC_NO_ACCESS_CHECK | PC_KEEP_HANDLE
+					 | PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP);
 	       ;
-	       pc_flags = 0)
+	       pc_flags = opt & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP))
 	    {
 	      const suffix_info *suff;
 	      char *full_path;
@@ -3480,48 +3481,49 @@ restart:
 	    goto file_not_symlink;
 	}
 #endif /* __i386__ */
-      {
-	PWCHAR fpbuf = tp.w_get ();
-	DWORD ret;
-
-	ret = GetFinalPathNameByHandleW (h, fpbuf, NT_MAX_PATH, 0);
-	if (ret)
-	  {
-	    UNICODE_STRING fpath;
+      if ((pc_flags & (PC_SYM_FOLLOW | PC_SYM_NOFOLLOW_REP)) == PC_SYM_FOLLOW)
+	{
+	  PWCHAR fpbuf = tp.w_get ();
+	  DWORD ret;
 
-	    RtlInitCountedUnicodeString (&fpath, fpbuf, ret * sizeof (WCHAR));
-	    fpbuf[1] = L'?';	/* \\?\ --> \??\ */
-	    if (!RtlEqualUnicodeString (&upath, &fpath, !!ci_flag))
-	      {
-		issymlink = true;
-		/* upath.Buffer is big enough and unused from this point on.
-		   Reuse it here, avoiding yet another buffer allocation. */
-		char *nfpath = (char *) upath.Buffer;
-		sys_wcstombs (nfpath, NT_MAX_PATH, fpbuf);
-		res = posixify (nfpath);
-
-		/* If the incoming path consisted of a drive prefix only,
-		   we just handle a virtual drive, created with, e.g.
-
-		     subst X: C:\foo\bar
-
-		   Treat it like a symlink.  This is required to tell an
-		   lstat caller that the "drive" is actually pointing
-		   somewhere else, thus, it's a symlink in POSIX speak. */
-		if (upath.Length == 14)	/* \??\X:\ */
-		  {
-		    fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
-		    path_flags |= PATH_SYMLINK;
-		  }
-		/* For final paths differing in inner path components return
-		   length as negative value.  This informs path_conv::check
-		   to skip realpath handling on the last path component. */
-		else
-		  res = -res;
-		break;
-	      }
-	  }
-      }
+	  ret = GetFinalPathNameByHandleW (h, fpbuf, NT_MAX_PATH, 0);
+	  if (ret)
+	    {
+	      UNICODE_STRING fpath;
+
+	      RtlInitCountedUnicodeString (&fpath, fpbuf, ret * sizeof (WCHAR));
+	      fpbuf[1] = L'?';	/* \\?\ --> \??\ */
+	      if (!RtlEqualUnicodeString (&upath, &fpath, !!ci_flag))
+	        {
+		  issymlink = true;
+		  /* upath.Buffer is big enough and unused from this point on.
+		     Reuse it here, avoiding yet another buffer allocation. */
+		  char *nfpath = (char *) upath.Buffer;
+		  sys_wcstombs (nfpath, NT_MAX_PATH, fpbuf);
+		  res = posixify (nfpath);
+
+		  /* If the incoming path consisted of a drive prefix only,
+		     we just handle a virtual drive, created with, e.g.
+
+		       subst X: C:\foo\bar
+
+		     Treat it like a symlink.  This is required to tell an
+		     lstat caller that the "drive" is actually pointing
+		     somewhere else, thus, it's a symlink in POSIX speak. */
+		  if (upath.Length == 14)	/* \??\X:\ */
+		    {
+		      fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
+		      path_flags |= PATH_SYMLINK;
+		    }
+		  /* For final paths differing in inner path components return
+		     length as negative value.  This informs path_conv::check
+		     to skip realpath handling on the last path component. */
+		  else
+		    res = -res;
+		  break;
+	        }
+	    }
+	}
 
     /* Normal file. */
     file_not_symlink:
-- 
2.32.0.windows.1


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

* Re: [PATCH v3] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-07-06 14:57             ` Corinna Vinschen
  2021-07-06 17:38               ` Jeremy Drake
  2021-07-06 17:40               ` [PATCH v4] " Jeremy Drake
@ 2021-07-07  6:50               ` Jeremy Drake
  2 siblings, 0 replies; 17+ messages in thread
From: Jeremy Drake @ 2021-07-07  6:50 UTC (permalink / raw)
  To: cygwin-patches

On Tue, 6 Jul 2021, Corinna Vinschen wrote:

> This looks like here be dragons.  A good solution would change the
> used native tools to allow paths > MAX_PATH finally, or to use other,
> equivalent tools already allowing that.

BTW, this seems pretty unlikely.  Portable tools are most likely using C
or C++ standard library file IO, not native Windows APIs, and while MS did
add an option to remove MAX_PATH limits from normal Win32 file/directory
APIs without the '\\?\' passthrough to NT trick [1], there are a lot of
hoops to jump through, and it would be hard to guarantee a whole stack of
libraries that might be linked in would be able to handle it.

[1]:
https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation#enable-long-paths-in-windows-10-version-1607-and-later

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

* Re: [PATCH v4] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-07-06 17:40               ` [PATCH v4] " Jeremy Drake
@ 2021-07-07  8:47                 ` Corinna Vinschen
  0 siblings, 0 replies; 17+ messages in thread
From: Corinna Vinschen @ 2021-07-07  8:47 UTC (permalink / raw)
  To: cygwin-patches

On Jul  6 10:40, Jeremy Drake via Cygwin-patches wrote:
> The new GetFinalPathNameW handling for native symlinks in inner path
> components is disabled if caller doesn't want to follow symlinks, or
> doesn't want to follow reparse points.
> ---
>  winsup/cygwin/path.cc | 88 ++++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 43 deletions(-)

Pushed.


Thanks,
Corinna

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

* Re: [PATCH v2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-05-31  8:17         ` Corinna Vinschen
  2021-05-31 17:55           ` Jeremy Drake
  2021-07-03 21:01           ` Jeremy Drake
@ 2021-07-07 18:52           ` Jeremy Drake
  2021-07-08 14:48             ` Corinna Vinschen
  2 siblings, 1 reply; 17+ messages in thread
From: Jeremy Drake @ 2021-07-07 18:52 UTC (permalink / raw)
  To: cygwin-patches

On Mon, 31 May 2021, Corinna Vinschen wrote:

> So we have two contradict problems, one which is solved by following
> inner symlinks, one which is solved by not doing that...

I hesitate to suggest it, but maybe an option/setting in the CYGWIN
variable as to whether to use this new behavior?  I am pretty much out of
ideas on how to make it work with native programs where they expect to see
the subst or mapped drive, not the target or UNC path.  Then MSYS2 could
either patch to change the default, or else just tell the (probably few)
people who hit it how to change the setting.

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

* Re: [PATCH v2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links
  2021-07-07 18:52           ` Jeremy Drake
@ 2021-07-08 14:48             ` Corinna Vinschen
  0 siblings, 0 replies; 17+ messages in thread
From: Corinna Vinschen @ 2021-07-08 14:48 UTC (permalink / raw)
  To: cygwin-patches

On Jul  7 11:52, Jeremy Drake via Cygwin-patches wrote:
> On Mon, 31 May 2021, Corinna Vinschen wrote:
> 
> > So we have two contradict problems, one which is solved by following
> > inner symlinks, one which is solved by not doing that...
> 
> I hesitate to suggest it, but maybe an option/setting in the CYGWIN
> variable as to whether to use this new behavior?  I am pretty much out of
> ideas on how to make it work with native programs where they expect to see
> the subst or mapped drive, not the target or UNC path.  Then MSYS2 could
> either patch to change the default, or else just tell the (probably few)
> people who hit it how to change the setting.

If MSYS2 has to patch the code anyway, why not go the entire way and
move the full patch to MSYS2?  Lets not add more CYGWIN options for rare
border cases, please.


Corinna

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

end of thread, other threads:[~2021-07-08 14:48 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-29 20:34 [PATCH] Cygwin: tweak handling of native symlinks from chdir Jeremy Drake
2021-05-29 23:15 ` Jeremy Drake
2021-05-30  6:05   ` Jeremy Drake
2021-05-30 19:58     ` [PATCH v2] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links Jeremy Drake
2021-05-31  8:02       ` Corinna Vinschen
2021-05-31  8:17         ` Corinna Vinschen
2021-05-31 17:55           ` Jeremy Drake
2021-07-03 21:01           ` Jeremy Drake
2021-07-07 18:52           ` Jeremy Drake
2021-07-08 14:48             ` Corinna Vinschen
2021-06-03 20:29         ` [PATCH v3] " Jeremy Drake
2021-06-03 20:57           ` Jeremy Drake
2021-07-06 14:57             ` Corinna Vinschen
2021-07-06 17:38               ` Jeremy Drake
2021-07-06 17:40               ` [PATCH v4] " Jeremy Drake
2021-07-07  8:47                 ` Corinna Vinschen
2021-07-07  6:50               ` [PATCH v3] " Jeremy Drake

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