[PATCH 0/5] Fix AT_EMPTY_PATH handling

[PATCH 0/5] Fix AT_EMPTY_PATH handling

[Corinna Vinschen]

From: Corinna Vinschen <corinna@vinschen.de>

The GLIBC extension AT_EMPTY_PATH allows the functions fchownat
and fstatat to operate on dirfd alone, if the given pathname is an
empty string.  This also allows to operate on any file type, not
only directories.

Commit fa84aa4dd2fb4 broke this.  It only allows dirfd to be a
directory in calls to these two functions.

Fix that by handling AT_EMPTY_PATH right in gen_full_path_at.
A valid dirfd and an empty pathname is now a valid combination
and, noticably, this returns a valid path in path_ret.  That
in turn allows to remove the additional path generation code
from the callers.

Fixes: fa84aa4dd2fb ("Cygwin: fix errno values set by readlinkat")
Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>

Corinna Vinschen (5):
  Cygwin: gen_full_path_at: drop never reached code
  Define _AT_NULL_PATHNAME_ALLOWED
  Cygwin: use new _AT_NULL_PATHNAME_ALLOWED flag
  Cygwin: Fix and streamline AT_EMPTY_PATH handling
  Cygwin: add AT_EMPTY_PATH fix to release message

 newlib/libc/include/sys/_default_fcntl.h | 11 +++--
 winsup/cygwin/release/3.4.8              |  4 ++
 winsup/cygwin/syscalls.cc                | 61 ++++++------------------
 3 files changed, 25 insertions(+), 51 deletions(-)

-- 
2.40.1

[PATCH 1/5] Cygwin: gen_full_path_at: drop never reached code

[Corinna Vinschen]

From: Corinna Vinschen <corinna@vinschen.de>

The check if the local variable p is NULL is useless.  The preceeding
code always sets p to a valid pointer, or it crashes if path_ret is
invalid (which would be a bug in Cygwin).

Fixes:c57b57e5c43a ("* cygwin.din: Sort.")
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 winsup/cygwin/syscalls.cc | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 73343ecc1f07..f0ef8955cee8 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -4447,11 +4447,6 @@ gen_full_path_at (char *path_ret, int dirfd, const char *pathname,
 	    }
 	  p = stpcpy (path_ret, cfd->get_name ());
 	}
-      if (!p)
-	{
-	  set_errno (ENOTDIR);
-	  return -1;
-	}
       if (pathname)
 	{
 	  if (!*pathname)
-- 
2.40.1

[PATCH 2/5] Define _AT_NULL_PATHNAME_ALLOWED

[Corinna Vinschen]

From: Corinna Vinschen <corinna@vinschen.de>

Cygwin needs an internal flag to allow specifying an empty pathname
in utimesat (GLIBC extension). We define it in _default_fcntl.h to
make sure we never introduce a value collision accidentally.
While at it, define the values as 16 bit hex values.

Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 newlib/libc/include/sys/_default_fcntl.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/newlib/libc/include/sys/_default_fcntl.h b/newlib/libc/include/sys/_default_fcntl.h
index 48914c92eab4..ce721fa23c02 100644
--- a/newlib/libc/include/sys/_default_fcntl.h
+++ b/newlib/libc/include/sys/_default_fcntl.h
@@ -162,12 +162,13 @@ extern "C" {
 #define AT_FDCWD -2
 
 /* Flag values for faccessat2) et al. */
-#define AT_EACCESS              1
-#define AT_SYMLINK_NOFOLLOW     2
-#define AT_SYMLINK_FOLLOW       4
-#define AT_REMOVEDIR            8
+#define AT_EACCESS                 0x0001
+#define AT_SYMLINK_NOFOLLOW        0x0002
+#define AT_SYMLINK_FOLLOW          0x0004
+#define AT_REMOVEDIR               0x0008
 #if __GNU_VISIBLE
-#define AT_EMPTY_PATH          16
+#define AT_EMPTY_PATH              0x0010
+#define _AT_NULL_PATHNAME_ALLOWED  0x4000 /* Internal flag used by futimesat */
 #endif
 #endif
 
-- 
2.40.1

[PATCH 3/5] Cygwin: use new _AT_NULL_PATHNAME_ALLOWED flag

[Corinna Vinschen]

From: Corinna Vinschen <corinna@vinschen.de>

Convert gen_full_path_at to take flag values from the caller, rather
than just a bool indicating that empty paths are allowed.  This is in
preparation of a better AT_EMPTY_PATH handling in a followup patch.

Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 winsup/cygwin/syscalls.cc | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index f0ef8955cee8..cf8c4e0cfb9f 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -4414,11 +4414,10 @@ pclose (FILE *fp)
 
 static int
 gen_full_path_at (char *path_ret, int dirfd, const char *pathname,
-		  bool null_pathname_allowed = false)
+		  int flags = 0)
 {
-  /* Set null_pathname_allowed to true to allow GLIBC compatible behaviour
-     for NULL pathname.  Only used by futimesat. */
-  if (!pathname && !null_pathname_allowed)
+  /* futimesat allows a NULL pathname. */
+  if (!pathname && !(flags & _AT_NULL_PATHNAME_ALLOWED))
     {
       set_errno (EFAULT);
       return -1;
@@ -4676,7 +4675,7 @@ futimesat (int dirfd, const char *pathname, const struct timeval times[2])
   __try
     {
       char *path = tp.c_get ();
-      if (gen_full_path_at (path, dirfd, pathname, true))
+      if (gen_full_path_at (path, dirfd, pathname, _AT_NULL_PATHNAME_ALLOWED))
 	__leave;
       return utimes (path, times);
     }
-- 
2.40.1

[PATCH 4/5] Cygwin: Fix and streamline AT_EMPTY_PATH handling

[Corinna Vinschen]

From: Corinna Vinschen <corinna@vinschen.de>

The GLIBC extension AT_EMPTY_PATH allows the functions fchownat
and fstatat to operate on dirfd alone, if the given pathname is an
empty string.  This also allows to operate on any file type, not
only directories.

Commit fa84aa4dd2fb4 broke this.  It only allows dirfd to be a
directory in calls to these two functions.

Fix that by handling AT_EMPTY_PATH right in gen_full_path_at.
A valid dirfd and an empty pathname is now a valid combination
and, noticably, this returns a valid path in path_ret.  That
in turn allows to remove the additional path generation code
from the callers.

Fixes: fa84aa4dd2fb ("Cygwin: fix errno values set by readlinkat")
Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 winsup/cygwin/syscalls.cc | 47 +++++++++------------------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index cf8c4e0cfb9f..c369c7713b19 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -4439,7 +4439,7 @@ gen_full_path_at (char *path_ret, int dirfd, const char *pathname,
 	  cygheap_fdget cfd (dirfd);
 	  if (cfd < 0)
 	    return -1;
-	  if (!cfd->pc.isdir ())
+	  if (!cfd->pc.isdir () && !(flags & AT_EMPTY_PATH))
 	    {
 	      set_errno (ENOTDIR);
 	      return -1;
@@ -4450,6 +4450,8 @@ gen_full_path_at (char *path_ret, int dirfd, const char *pathname,
 	{
 	  if (!*pathname)
 	    {
+	      if (flags & AT_EMPTY_PATH)
+		return 0;
 	      set_errno (ENOENT);
 	      return -1;
 	    }
@@ -4571,29 +4573,14 @@ fchownat (int dirfd, const char *pathname, uid_t uid, gid_t gid, int flags)
 	  __leave;
 	}
       char *path = tp.c_get ();
-      int res = gen_full_path_at (path, dirfd, pathname);
+      int res = gen_full_path_at (path, dirfd, pathname, flags);
       if (res)
+	__leave;
+      if (!*pathname) /* Implies AT_EMPTY_PATH */
 	{
-	  if (!(errno == ENOENT && (flags & AT_EMPTY_PATH)))
-	    __leave;
-	  /* pathname is an empty string.  Operate on dirfd. */
-	  if (dirfd == AT_FDCWD)
-	    {
-	      cwdstuff::acquire_read ();
-	      strcpy (path, cygheap->cwd.get_posix ());
-	      cwdstuff::release_read ();
-	    }
-	  else
-	    {
-	      cygheap_fdget cfd (dirfd);
-	      if (cfd < 0)
-		__leave;
-	      strcpy (path, cfd->get_name ());
-	      /* If dirfd refers to a symlink (which was necessarily
-		 opened with O_PATH | O_NOFOLLOW), we must operate
-		 directly on that symlink.. */
-	      flags = AT_SYMLINK_NOFOLLOW;
-	    }
+	  /* If dirfd refers to a symlink (which was necessarily opened with
+	     O_PATH | O_NOFOLLOW), we must operate directly on that symlink. */
+	  flags = AT_SYMLINK_NOFOLLOW;
 	}
       return chown_worker (path, (flags & AT_SYMLINK_NOFOLLOW)
 				 ? PC_SYM_NOFOLLOW : PC_SYM_FOLLOW, uid, gid);
@@ -4616,21 +4603,9 @@ fstatat (int dirfd, const char *__restrict pathname, struct stat *__restrict st,
 	  __leave;
 	}
       char *path = tp.c_get ();
-      int res = gen_full_path_at (path, dirfd, pathname);
+      int res = gen_full_path_at (path, dirfd, pathname, flags);
       if (res)
-	{
-	  if (!(errno == ENOENT && (flags & AT_EMPTY_PATH)))
-	    __leave;
-	  /* pathname is an empty string.  Operate on dirfd. */
-	  if (dirfd == AT_FDCWD)
-	    {
-	      cwdstuff::acquire_read ();
-	      strcpy (path, cygheap->cwd.get_posix ());
-	      cwdstuff::release_read ();
-	    }
-	  else
-	    return fstat (dirfd, st);
-	}
+	  __leave;
       path_conv pc (path, ((flags & AT_SYMLINK_NOFOLLOW)
 			   ? PC_SYM_NOFOLLOW : PC_SYM_FOLLOW)
 			  | PC_POSIX | PC_KEEP_HANDLE, stat_suffixes);
-- 
2.40.1

[PATCH 5/5] Cygwin: add AT_EMPTY_PATH fix to release message

[Corinna Vinschen]

From: Corinna Vinschen <corinna@vinschen.de>

Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 winsup/cygwin/release/3.4.8 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/winsup/cygwin/release/3.4.8 b/winsup/cygwin/release/3.4.8
index d1e34ce3c633..581ed5a140ff 100644
--- a/winsup/cygwin/release/3.4.8
+++ b/winsup/cygwin/release/3.4.8
@@ -3,3 +3,7 @@ Bug Fixes
 
 - Make <sys/cpuset.h> safe for c89 compilations.
   Addresses: https://cygwin.com/pipermail/cygwin-patches/2023q3/012308.html
+
+- Fix AT_EMPTY_PATH handling in fchmodat and fstatat if dirfd referres to
+  a file other than a directory
+  Addresses: https://cygwin.com/pipermail/cygwin-patches/2023q2/012306.html
-- 
2.40.1

Re: [PATCH 0/5] Fix AT_EMPTY_PATH handling

[Corinna Vinschen]

Johannes? Ping?

On Jul 12 14:07, Corinna Vinschen wrote:
> From: Corinna Vinschen <corinna@vinschen.de>
> 
> The GLIBC extension AT_EMPTY_PATH allows the functions fchownat
> and fstatat to operate on dirfd alone, if the given pathname is an
> empty string.  This also allows to operate on any file type, not
> only directories.
> 
> Commit fa84aa4dd2fb4 broke this.  It only allows dirfd to be a
> directory in calls to these two functions.
> 
> Fix that by handling AT_EMPTY_PATH right in gen_full_path_at.
> A valid dirfd and an empty pathname is now a valid combination
> and, noticably, this returns a valid path in path_ret.  That
> in turn allows to remove the additional path generation code
> from the callers.
> 
> Fixes: fa84aa4dd2fb ("Cygwin: fix errno values set by readlinkat")
> Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
> 
> Corinna Vinschen (5):
>   Cygwin: gen_full_path_at: drop never reached code
>   Define _AT_NULL_PATHNAME_ALLOWED
>   Cygwin: use new _AT_NULL_PATHNAME_ALLOWED flag
>   Cygwin: Fix and streamline AT_EMPTY_PATH handling
>   Cygwin: add AT_EMPTY_PATH fix to release message
> 
>  newlib/libc/include/sys/_default_fcntl.h | 11 +++--
>  winsup/cygwin/release/3.4.8              |  4 ++
>  winsup/cygwin/syscalls.cc                | 61 ++++++------------------
>  3 files changed, 25 insertions(+), 51 deletions(-)
> 
> -- 
> 2.40.1

Re: [PATCH 0/5] Fix AT_EMPTY_PATH handling

[Johannes Schindelin]

Hi Corinna,

I had a look over the patches and they all make sense. I also tested the
code to make sure that the `tar xf` regression I needed to be fixed is
also addressed by this patch series.

As I don't really know the customs of the Cygwin project, please feel free
to add any Reviewed-by:, Acked-by: or whatever footers (or, if none of
those are appropriate, I am of course totally okay with no footer at all).

Thank you so much for fixing this!
Johannes


On Mon, 24 Jul 2023, Corinna Vinschen wrote:

> Johannes? Ping?
>
> On Jul 12 14:07, Corinna Vinschen wrote:
> > From: Corinna Vinschen <corinna@vinschen.de>
> >
> > The GLIBC extension AT_EMPTY_PATH allows the functions fchownat
> > and fstatat to operate on dirfd alone, if the given pathname is an
> > empty string.  This also allows to operate on any file type, not
> > only directories.
> >
> > Commit fa84aa4dd2fb4 broke this.  It only allows dirfd to be a
> > directory in calls to these two functions.
> >
> > Fix that by handling AT_EMPTY_PATH right in gen_full_path_at.
> > A valid dirfd and an empty pathname is now a valid combination
> > and, noticably, this returns a valid path in path_ret.  That
> > in turn allows to remove the additional path generation code
> > from the callers.
> >
> > Fixes: fa84aa4dd2fb ("Cygwin: fix errno values set by readlinkat")
> > Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
> >
> > Corinna Vinschen (5):
> >   Cygwin: gen_full_path_at: drop never reached code
> >   Define _AT_NULL_PATHNAME_ALLOWED
> >   Cygwin: use new _AT_NULL_PATHNAME_ALLOWED flag
> >   Cygwin: Fix and streamline AT_EMPTY_PATH handling
> >   Cygwin: add AT_EMPTY_PATH fix to release message
> >
> >  newlib/libc/include/sys/_default_fcntl.h | 11 +++--
> >  winsup/cygwin/release/3.4.8              |  4 ++
> >  winsup/cygwin/syscalls.cc                | 61 ++++++------------------
> >  3 files changed, 25 insertions(+), 51 deletions(-)
> >
> > --
> > 2.40.1
>

Re: [PATCH 0/5] Fix AT_EMPTY_PATH handling

[Corinna Vinschen]

On Jul 26 11:10, Johannes Schindelin wrote:
> Hi Corinna,
> 
> I had a look over the patches and they all make sense. I also tested the
> code to make sure that the `tar xf` regression I needed to be fixed is
> also addressed by this patch series.
> 
> As I don't really know the customs of the Cygwin project, please feel free
> to add any Reviewed-by:, Acked-by: or whatever footers (or, if none of
> those are appropriate, I am of course totally okay with no footer at all).
> 
> Thank you so much for fixing this!
> Johannes

Great, thanks for reviewing and testing!

I'll push it later today.


Thanks,
Corinna

Re: [PATCH 5/5] Cygwin: add AT_EMPTY_PATH fix to release message

[Pedro Alves]

On 2023-07-12 13:08, Corinna Vinschen wrote:
> +- Fix AT_EMPTY_PATH handling in fchmodat and fstatat if dirfd referres to

referres -> refers

Re: [PATCH 5/5] Cygwin: add AT_EMPTY_PATH fix to release message

[Corinna Vinschen]

On Jul 28 17:11, Pedro Alves wrote:
> On 2023-07-12 13:08, Corinna Vinschen wrote:
> > +- Fix AT_EMPTY_PATH handling in fchmodat and fstatat if dirfd referres to
> 
> referres -> refers

Oops, sorry, too late :}