Re: [PATCH v3] Cygwin: respect PC_SYM_FOLLOW and PC_SYM_NOFOLLOW_REP with inner links

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

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