[PATCH v2] Cygwin: fchmodat: add limited support for AT_SYMLINK_NOFOLLOW

[PATCH v2] Cygwin: fchmodat: add limited support for AT_SYMLINK_NOFOLLOW

[Ken Brown]

Allow fchmodat with the AT_SYMLINK_NOFOLLOW flag to succeed on
non-symlinks.  Previously it always failed, as it does on Linux.  But
POSIX permits it to succeed on non-symlinks even if it fails on
symlinks.

The reason for following POSIX rather than Linux is to make gnulib
report that fchmodat works on Cygwin.  This improves the efficiency of
packages like GNU tar that use gnulib's fchmodat module.  Previously
such packages would use a gnulib replacement for fchmodat on Cygwin.
---
 winsup/cygwin/syscalls.cc | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 4cc8d07f5..5da05b18a 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -4787,17 +4787,27 @@ fchmodat (int dirfd, const char *pathname, mode_t mode, int flags)
   tmp_pathbuf tp;
   __try
     {
-      if (flags)
+      if (flags & ~AT_SYMLINK_NOFOLLOW)
 	{
-	  /* BSD has lchmod, but Linux does not.  POSIX says
-	     AT_SYMLINK_NOFOLLOW is allowed to fail on symlinks; but Linux
-	     blindly fails even for non-symlinks.  */
-	  set_errno ((flags & ~AT_SYMLINK_NOFOLLOW) ? EINVAL : EOPNOTSUPP);
+	  set_errno (EINVAL);
 	  __leave;
 	}
       char *path = tp.c_get ();
       if (gen_full_path_at (path, dirfd, pathname))
 	__leave;
+      if (flags)
+	{
+          /* BSD has lchmod, but Linux does not.  POSIX says
+	     AT_SYMLINK_NOFOLLOW is allowed to fail on symlinks.
+	     Linux blindly fails even for non-symlinks, but we allow
+	     it to succeed. */
+	  path_conv pc (path, PC_SYM_NOFOLLOW, stat_suffixes);
+	  if (pc.issymlink ())
+	    {
+	      set_errno (EOPNOTSUPP);
+	      __leave;
+	    }
+	}
       return chmod (path, mode);
     }
   __except (EFAULT) {}
-- 
2.30.0

Re: [PATCH v2] Cygwin: fchmodat: add limited support for AT_SYMLINK_NOFOLLOW

[Corinna Vinschen]

On Jan 27 13:53, Ken Brown via Cygwin-patches wrote:
> Allow fchmodat with the AT_SYMLINK_NOFOLLOW flag to succeed on
> non-symlinks.  Previously it always failed, as it does on Linux.  But
> POSIX permits it to succeed on non-symlinks even if it fails on
> symlinks.
> 
> The reason for following POSIX rather than Linux is to make gnulib
> report that fchmodat works on Cygwin.  This improves the efficiency of
> packages like GNU tar that use gnulib's fchmodat module.  Previously
> such packages would use a gnulib replacement for fchmodat on Cygwin.
> ---
>  winsup/cygwin/syscalls.cc | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> index 4cc8d07f5..5da05b18a 100644
> --- a/winsup/cygwin/syscalls.cc
> +++ b/winsup/cygwin/syscalls.cc
> @@ -4787,17 +4787,27 @@ fchmodat (int dirfd, const char *pathname, mode_t mode, int flags)
>    tmp_pathbuf tp;
>    __try
>      {
> -      if (flags)
> +      if (flags & ~AT_SYMLINK_NOFOLLOW)
>  	{
> -	  /* BSD has lchmod, but Linux does not.  POSIX says
> -	     AT_SYMLINK_NOFOLLOW is allowed to fail on symlinks; but Linux
> -	     blindly fails even for non-symlinks.  */
> -	  set_errno ((flags & ~AT_SYMLINK_NOFOLLOW) ? EINVAL : EOPNOTSUPP);
> +	  set_errno (EINVAL);
>  	  __leave;
>  	}
>        char *path = tp.c_get ();
>        if (gen_full_path_at (path, dirfd, pathname))
>  	__leave;
> +      if (flags)

For clarity, and on the off-chance that new flags are added to fchmodat,
it might be better to check for (flags & AT_SYMLINK_NOFOLLOW) here
explicitely.  Your choice.

> +	{
> +          /* BSD has lchmod, but Linux does not.  POSIX says
> +	     AT_SYMLINK_NOFOLLOW is allowed to fail on symlinks.
> +	     Linux blindly fails even for non-symlinks, but we allow
> +	     it to succeed. */
> +	  path_conv pc (path, PC_SYM_NOFOLLOW, stat_suffixes);
> +	  if (pc.issymlink ())
> +	    {
> +	      set_errno (EOPNOTSUPP);
> +	      __leave;
> +	    }
> +	}
>        return chmod (path, mode);
>      }
>    __except (EFAULT) {}
> -- 
> 2.30.0

Looks good.


Thanks,
Corinna