Re: vboxsharedfs - Too many levels of symbolic links

[Takashi Yano <takashi.yano@nifty.ne.jp>]

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

On Tue, 7 Dec 2021 18:15:42 +0100
Corinna Vinschen wrote:
> Hi Takashi,
> 
> ----- Forwarded message from Corinna Vinschen <corinna-cygwin@cygwin.com> -----
> > The idea of the GFPNBH call is to short-circuit the path_conv handling
> > in case we have native Windows symlinks in the path.  My example above
> > was only considering what comes out of the `if ((pc_flags & ...) { ... }
> > ' expression starting in line 3485 (assuming "b" is a native symlink).
> > 
> > What I mean is this: Your patch disregards the entire string returned by
> > GFPNBH, if the returned path is an UNC path, no matter what.
> > 
> > But what if the incoming path already *was* an UNC path, and potentially
> > contains native symlinks?  In that case you have no reason to disregard
> > the resulting path from GFPNBH.
> > 
> > And if it was a drive letter path, wouldn't it be nicer to just convert
> > the UNC path prefix back to the drive letter and keep the rest of the
> > final path intact?
> > 
> > However, both of these scenarios require extra code, which isn't that
> > important for now, so, never mind.
> ----- End forwarded message -----
> 
> What I meant is something like the below (entirely untested).  Yeah, I'm
> not sure myself, if it's worth the effort...
> 
> diff --git a/winsup/cygwin/autoload.cc b/winsup/cygwin/autoload.cc
> index e254397257fd..06afd2d62996 100644
> --- a/winsup/cygwin/autoload.cc
> +++ b/winsup/cygwin/autoload.cc
> @@ -630,6 +630,7 @@ LoadDLLfunc (LdapMapErrorToWin32, 0, wldap32)
>  
>  LoadDLLfunc (WNetCloseEnum, 4, mpr)
>  LoadDLLfunc (WNetEnumResourceW, 16, mpr)
> +LoadDLLfunc (WNetGetConnectionW, 12, mpr)
>  LoadDLLfunc (WNetGetProviderNameW, 12, mpr)
>  LoadDLLfunc (WNetGetResourceInformationW, 16, mpr)
>  LoadDLLfunc (WNetOpenEnumW, 20, mpr)
> diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
> index baf04ce89a08..c7b29acf29b3 100644
> --- a/winsup/cygwin/path.cc
> +++ b/winsup/cygwin/path.cc
> @@ -3492,10 +3492,41 @@ restart:
>  	    {
>  	      UNICODE_STRING fpath;
>  
> -	      RtlInitCountedUnicodeString (&fpath, fpbuf, ret * sizeof (WCHAR));
> +	      /* If incoming path has no trailing backslash, but final path
> +		 has one, drop trailing backslash from final path so the
> +		 below string comparison has a chance to succeed. */
> +	      if (upath.Buffer[(upath.Length - 1) / sizeof (WCHAR)] != L'\\'
> +                  && fpbuf[ret - 1] == L'\\')
> +                fpbuf[--ret] = L'\0';
>  	      fpbuf[1] = L'?';	/* \\?\ --> \??\ */
> +	      RtlInitCountedUnicodeString (&fpath, fpbuf, ret * sizeof (WCHAR));
>  	      if (!RtlEqualUnicodeString (&upath, &fpath, !!ci_flag))
>  	        {
> +		  /* Check if the final path is an UNC path and the incoming
> +		     path isn't.  If so... */
> +		  if (RtlEqualUnicodePathPrefix (&fpath, &ro_u_uncp, TRUE)
> +		      && !RtlEqualUnicodePathPrefix (&upath, &ro_u_uncp, TRUE))
> +		    {
> +		      /* ...extract the drive letter, get the remote path,
> +			 replace remote path with drive letter, check again. */
> +		      WCHAR drive[3] = { upath.Buffer[4], L':', L'\0' };
> +		      WCHAR remote[MAX_PATH];
> +
> +		      if (WNetGetConnectionW (drive, remote, MAX_PATH)
> +			  == NO_ERROR)
> +			{
> +			  /* Hackfest */
> +			  fpath.Buffer[4] = drive[0];
> +			  fpath.Buffer[5] = L':';
> +			  WCHAR to = fpath.Buffer + 6;
> +			  WCHAR from = to + wcslen (remote);
> +			  memmove (to, from,
> +				   (wcslen (from) + 1) * sizeof (WCHAR));
> +			  fpath.Length -= (from - to) * sizeof (WCHAR);
> +			  if (RtlEqualUnicodeString (&upath, &fpath, !!ci_flag))
> +			    goto file_not_symlink;
> +			}
> +		    }
>  		  issymlink = true;
>  		  /* upath.Buffer is big enough and unused from this point on.
>  		     Reuse it here, avoiding yet another buffer allocation. */

I confirmed that your patch works nicely.

...except when the drive is created by subst using UNC path,
e.g. subst w: \\server\share.

In this case, WNetGetConnectionW() fails with ERROR_NO_NET_OR_BAD_PATH.

So, I modified your patch a bit.

What about attached patch?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

[-- Attachment #2: 0001-Cygwin-path-Convert-UNC-path-prefix-back-to-drive-le.patch --]
[-- Type: application/octet-stream, Size: 2766 bytes --]

From dc222ec73f537e9e47558bbf156dd1735b9095e5 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Wed, 8 Dec 2021 17:09:14 +0900
Subject: [PATCH] Cygwin: path: Convert UNC path prefix back to drive letter.

- When symlink_info::check() is called with the path having drive
  letter and UNC path is mounted to the drive, the path is replaced
  with UNC path. With this patch, UNC path prefix is converted back
  to drive letter.  This fixes the issue:
  https://cygwin.com/pipermail/cygwin/2021-November/250087.html
  https://cygwin.com/pipermail/cygwin/2021-December/250103.html
---
 winsup/cygwin/path.cc | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index baf04ce89..eb1255849 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -3492,10 +3492,45 @@ restart:
 	    {
 	      UNICODE_STRING fpath;
 
-	      RtlInitCountedUnicodeString (&fpath, fpbuf, ret * sizeof (WCHAR));
+	      /* If incoming path has no trailing backslash, but final path
+		 has one, drop trailing backslash from final path so the
+		 below string comparison has a chance to succeed. */
+	      if (upath.Buffer[(upath.Length - 1) / sizeof (WCHAR)] != L'\\'
+                  && fpbuf[ret - 1] == L'\\')
+                fpbuf[--ret] = L'\0';
 	      fpbuf[1] = L'?';	/* \\?\ --> \??\ */
+	      RtlInitCountedUnicodeString (&fpath, fpbuf, ret * sizeof (WCHAR));
 	      if (!RtlEqualUnicodeString (&upath, &fpath, !!ci_flag))
 	        {
+		  /* Check if the final path is an UNC path and the incoming
+		     path isn't.  If so... */
+		  if (RtlEqualUnicodePathPrefix (&fpath, &ro_u_uncp, TRUE)
+		      && !RtlEqualUnicodePathPrefix (&upath, &ro_u_uncp, TRUE))
+		    {
+		      /* ...get the remote path from the volume path name,
+			 replace remote path with drive letter, check again. */
+		      WCHAR remote[MAX_PATH];
+
+		      fpbuf[1] = L'\\';
+		      BOOL r = GetVolumePathNameW (fpbuf, remote, MAX_PATH);
+		      fpbuf[1] = L'?';
+		      if (r)
+			{
+			  int remlen = wcslen (remote);
+			  if (remote[remlen - 1] == L'\\')
+			    remlen--;
+			  /* Hackfest */
+			  fpath.Buffer[4] = upath.Buffer[4]; /* Drive letter */
+			  fpath.Buffer[5] = L':';
+			  WCHAR *to = fpath.Buffer + 6;
+			  WCHAR *from = to + remlen - 6;
+			  memmove (to, from,
+				   (wcslen (from) + 1) * sizeof (WCHAR));
+			  fpath.Length -= (from - to) * sizeof (WCHAR);
+			  if (RtlEqualUnicodeString (&upath, &fpath, !!ci_flag))
+			    goto file_not_symlink;
+			}
+		    }
 		  issymlink = true;
 		  /* upath.Buffer is big enough and unused from this point on.
 		     Reuse it here, avoiding yet another buffer allocation. */
-- 
2.34.1