[PATCH] fchmodat/fstatat: fix regression with empty `pathname`

[PATCH] fchmodat/fstatat: fix regression with empty `pathname`

[Johannes Schindelin]

In 4b8222983f (Cygwin: fix errno values set by readlinkat, 2023-04-18)
the code of `readlinkat()` was adjusted to align the `errno` with Linux'
behavior.

To accommodate for that, the `gen_full_path_at()` function was modified,
and the caller was adjusted to expect either `ENOENT` or `ENOTDIR` in
the case of an empty `pathname`, not just `ENOENT`.

However, `readlinkat()` is not the only caller of that helper function.

And while most other callers simply propagate the `errno` produced by
`gen_full_path_at()`, two other callers also want to special-case empty
`pathnames` much like `readlinkat()`: `fchmodat()` and `fstatat()`.

Therefore, these two callers need to be changed to expect `ENOTDIR` in
case of an empty `pathname`, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/msys2-runtime/releases/tag/fix-tar-xf-with-symlinks-cygwin-v1
Fetch-It-Via: git fetch https://github.com/dscho/msys2-runtime fix-tar-xf-with-symlinks-cygwin-v1

	I noticed this issue when one of my workflows failed consistently
	while trying to untar an archive containing a symbolic link and
	claiming this:

		Cannot change mode to rwxr-xr-x: Not a directory

	With this here fix, things start working as expected again.

 winsup/cygwin/syscalls.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 73343ecc1f..c1889aec91 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -4580,7 +4580,7 @@ fchownat (int dirfd, const char *pathname, uid_t uid, gid_t gid, int flags)
       int res = gen_full_path_at (path, dirfd, pathname);
       if (res)
 	{
-	  if (!(errno == ENOENT && (flags & AT_EMPTY_PATH)))
+	  if (!((errno == ENOENT || errno == ENOTDIR) && (flags & AT_EMPTY_PATH)))
 	    __leave;
 	  /* pathname is an empty string.  Operate on dirfd. */
 	  if (dirfd == AT_FDCWD)
@@ -4625,7 +4625,7 @@ fstatat (int dirfd, const char *__restrict pathname, struct stat *__restrict st,
       int res = gen_full_path_at (path, dirfd, pathname);
       if (res)
 	{
-	  if (!(errno == ENOENT && (flags & AT_EMPTY_PATH)))
+	  if (!((errno == ENOENT || errno == ENOTDIR) && (flags & AT_EMPTY_PATH)))
 	    __leave;
 	  /* pathname is an empty string.  Operate on dirfd. */
 	  if (dirfd == AT_FDCWD)

base-commit: 4c7d0dfec5793cbf5cf3930b91f930479126d8ce
--
2.41.0.windows.1

Re: [PATCH] fchmodat/fstatat: fix regression with empty `pathname`

[Jeremy Drake]

On Tue, 27 Jun 2023, Johannes Schindelin wrote:

> In 4b8222983f (Cygwin: fix errno values set by readlinkat, 2023-04-18)
> the code of `readlinkat()` was adjusted to align the `errno` with Linux'
> behavior.
>
> 	I noticed this issue when one of my workflows failed consistently
> 	while trying to untar an archive containing a symbolic link and
> 	claiming this:
>
> 		Cannot change mode to rwxr-xr-x: Not a directory
>

I wonder if this is related to the issue from the thread
https://cygwin.com/pipermail/cygwin/2023-May/253738.html (sounds like it).
If so, tar was
rebuilt to pick up the new behavior in 3.4.7 (presumably via configure
checks), it may need another rebuild to pick up the fixed behavior after
this fix.

Re: [PATCH] fchmodat/fstatat: fix regression with empty `pathname`

[Corinna Vinschen]

Hi Johannes,

On Jun 27 22:51, Johannes Schindelin wrote:
> In 4b8222983f (Cygwin: fix errno values set by readlinkat, 2023-04-18)
> the code of `readlinkat()` was adjusted to align the `errno` with Linux'
> behavior.
> 
> To accommodate for that, the `gen_full_path_at()` function was modified,
> and the caller was adjusted to expect either `ENOENT` or `ENOTDIR` in
> the case of an empty `pathname`, not just `ENOENT`.
> 
> However, `readlinkat()` is not the only caller of that helper function.
> 
> And while most other callers simply propagate the `errno` produced by
> `gen_full_path_at()`, two other callers also want to special-case empty
> `pathnames` much like `readlinkat()`: `fchmodat()` and `fstatat()`.
> 
> Therefore, these two callers need to be changed to expect `ENOTDIR` in
> case of an empty `pathname`, too.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Looks like a good catch. Can you please also add a "Fixes:" tag line
and move the tar error description up into the commit message?


Thanks,
Corinna

[PATCH v2] fchmodat/fstatat: fix regression with empty `pathname`

[Johannes Schindelin]

In 4b8222983f (Cygwin: fix errno values set by readlinkat, 2023-04-18)
the code of `readlinkat()` was adjusted to align the `errno` with Linux'
behavior.

To accommodate for that, the `gen_full_path_at()` function was modified,
and the caller was adjusted to expect either `ENOENT` or `ENOTDIR` in
the case of an empty `pathname`, not just `ENOENT`.

However, `readlinkat()` is not the only caller of that helper function.

And while most other callers simply propagate the `errno` produced by
`gen_full_path_at()`, two other callers also want to special-case empty
`pathnames` much like `readlinkat()`: `fchmodat()` and `fstatat()`.

Therefore, these two callers need to be changed to expect `ENOTDIR` in
case of an empty `pathname`, too.

I noticed this issue when one of my workflows failed consistently
while trying to untar an archive containing a symbolic link and
claiming this:

    Cannot change mode to rwxr-xr-x: Not a directory

With this here fix, things start working as expected again.

Fixes: 4b8222983f (Cygwin: fix errno values set by readlinkat, 2023-04-18)
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/msys2-runtime/releases/tag/fix-tar-xf-with-symlinks-cygwin-v2
Fetch-It-Via: git fetch https://github.com/dscho/msys2-runtime fix-tar-xf-with-symlinks-cygwin-v2

Range-diff:
1:  c985ab15b2 ! 1:  072b5c57a7 fchmodat/fstatat: fix regression with empty `pathname`
    @@ Commit message
         Therefore, these two callers need to be changed to expect `ENOTDIR` in
         case of an empty `pathname`, too.

    +    I noticed this issue when one of my workflows failed consistently
    +    while trying to untar an archive containing a symbolic link and
    +    claiming this:
    +
    +        Cannot change mode to rwxr-xr-x: Not a directory
    +
    +    With this here fix, things start working as expected again.
    +
    +    Fixes: 4b8222983f (Cygwin: fix errno values set by readlinkat, 2023-04-18)
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

      ## winsup/cygwin/syscalls.cc ##

 winsup/cygwin/syscalls.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 73343ecc1f..c1889aec91 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -4580,7 +4580,7 @@ fchownat (int dirfd, const char *pathname, uid_t uid, gid_t gid, int flags)
       int res = gen_full_path_at (path, dirfd, pathname);
       if (res)
 	{
-	  if (!(errno == ENOENT && (flags & AT_EMPTY_PATH)))
+	  if (!((errno == ENOENT || errno == ENOTDIR) && (flags & AT_EMPTY_PATH)))
 	    __leave;
 	  /* pathname is an empty string.  Operate on dirfd. */
 	  if (dirfd == AT_FDCWD)
@@ -4625,7 +4625,7 @@ fstatat (int dirfd, const char *__restrict pathname, struct stat *__restrict st,
       int res = gen_full_path_at (path, dirfd, pathname);
       if (res)
 	{
-	  if (!(errno == ENOENT && (flags & AT_EMPTY_PATH)))
+	  if (!((errno == ENOENT || errno == ENOTDIR) && (flags & AT_EMPTY_PATH)))
 	    __leave;
 	  /* pathname is an empty string.  Operate on dirfd. */
 	  if (dirfd == AT_FDCWD)

base-commit: 4c7d0dfec5793cbf5cf3930b91f930479126d8ce
--
2.41.0.windows.1

Re: [PATCH] fchmodat/fstatat: fix regression with empty `pathname`

[Johannes Schindelin]

Hi Corinna,

On Mon, 3 Jul 2023, Corinna Vinschen wrote:

> Hi Johannes,
>
> On Jun 27 22:51, Johannes Schindelin wrote:
> > In 4b8222983f (Cygwin: fix errno values set by readlinkat, 2023-04-18)
> > the code of `readlinkat()` was adjusted to align the `errno` with Linux'
> > behavior.
> >
> > To accommodate for that, the `gen_full_path_at()` function was modified,
> > and the caller was adjusted to expect either `ENOENT` or `ENOTDIR` in
> > the case of an empty `pathname`, not just `ENOENT`.
> >
> > However, `readlinkat()` is not the only caller of that helper function.
> >
> > And while most other callers simply propagate the `errno` produced by
> > `gen_full_path_at()`, two other callers also want to special-case empty
> > `pathnames` much like `readlinkat()`: `fchmodat()` and `fstatat()`.
> >
> > Therefore, these two callers need to be changed to expect `ENOTDIR` in
> > case of an empty `pathname`, too.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Looks like a good catch. Can you please also add a "Fixes:" tag line
> and move the tar error description up into the commit message?

Done.

BTW a colleague and I were wondering whether we really want to set
`errno=ENOTDIR` in `gen_full_path_at()` for empty paths when
`AT_EMPTY_PATH` is _not_ specified. As far as we can tell, Linux sets
`errno=ENOENT` in that instance.

Ciao,
Johannes

Re: [PATCH] fchmodat/fstatat: fix regression with empty `pathname`

[Johannes Schindelin]

Hi Jeremy,

On Wed, 28 Jun 2023, Jeremy Drake wrote:

> On Tue, 27 Jun 2023, Johannes Schindelin wrote:
>
> > In 4b8222983f (Cygwin: fix errno values set by readlinkat, 2023-04-18)
> > the code of `readlinkat()` was adjusted to align the `errno` with Linux'
> > behavior.
> >
> > 	I noticed this issue when one of my workflows failed consistently
> > 	while trying to untar an archive containing a symbolic link and
> > 	claiming this:
> >
> > 		Cannot change mode to rwxr-xr-x: Not a directory
> >
>
> I wonder if this is related to the issue from the thread
> https://cygwin.com/pipermail/cygwin/2023-May/253738.html (sounds like it).

For reference, this is the link I am looking at (because it has superior
thread navigation):
https://inbox.sourceware.org/cygwin/CAJQQdJjUarc1hkZCVX-GWD=Cq7XF4bnWE+ArzLxrUqWWpC7=rg@mail.gmail.com/T/#t

And yes, it looks like it's that very same issue.

> If so, tar was
> rebuilt to pick up the new behavior in 3.4.7 (presumably via configure
> checks), it may need another rebuild to pick up the fixed behavior after
> this fix.

Likely? But then, the patch in question should not _break_ the re-built
`tar`. Or does it?

Ciao,
Johannes

Re: [PATCH] fchmodat/fstatat: fix regression with empty `pathname`

[Corinna Vinschen]

On Jul  4 17:45, Johannes Schindelin wrote:
> Hi Corinna,
> 
> On Mon, 3 Jul 2023, Corinna Vinschen wrote:
> 
> > Hi Johannes,
> >
> > On Jun 27 22:51, Johannes Schindelin wrote:
> > > In 4b8222983f (Cygwin: fix errno values set by readlinkat, 2023-04-18)
> > > the code of `readlinkat()` was adjusted to align the `errno` with Linux'
> > > behavior.
> > >
> > > To accommodate for that, the `gen_full_path_at()` function was modified,
> > > and the caller was adjusted to expect either `ENOENT` or `ENOTDIR` in
> > > the case of an empty `pathname`, not just `ENOENT`.
> > >
> > > However, `readlinkat()` is not the only caller of that helper function.
> > >
> > > And while most other callers simply propagate the `errno` produced by
> > > `gen_full_path_at()`, two other callers also want to special-case empty
> > > `pathnames` much like `readlinkat()`: `fchmodat()` and `fstatat()`.
> > >
> > > Therefore, these two callers need to be changed to expect `ENOTDIR` in
> > > case of an empty `pathname`, too.
> > >
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Looks like a good catch. Can you please also add a "Fixes:" tag line
> > and move the tar error description up into the commit message?
> 
> Done.
> 
> BTW a colleague and I were wondering whether we really want to set
> `errno=ENOTDIR` in `gen_full_path_at()` for empty paths when
> `AT_EMPTY_PATH` is _not_ specified. As far as we can tell, Linux sets
> `errno=ENOENT` in that instance.

I wonder if that's really what you mean.  gen_full_path_at() generates
ENOTDIR in two scenarios:

- At line 4443, if Cygwin can't resolve dirfd into a valid directory.

- At line 4450 if ... actually... never.  Given that p is always
  set to the end of the directory string copied into path_ret, it
  can never be NULL. Looks like this check for !p is a remnant from
  the past.  We should remove it.

The actual check for an empty path is done in line 4457, and this
results in ENOENT, as desired.

So, by any chance, do you mean the situation handled in line 4443,
that is, returning ENOTDIR if dirfd doesn't resolve to a directory?

Yeah, it slightly complicates the caller, but it's not exactly
wrong, given your patch.

OTOH, this entire thing doesn't look overly well thought out.  We try
to generate a full path in gen_full_path_at() and if it fails in
a certain way and AT_EMPTY_PATH is given, we basically repeat
trying to create a full path in the caller.  Maybe some
streamlining would be in order...


Corinna

Re: [PATCH] fchmodat/fstatat: fix regression with empty `pathname`

[Corinna Vinschen]

Hi Johannes,

On Jul  4 20:38, Corinna Vinschen wrote:
> On Jul  4 17:45, Johannes Schindelin wrote:
> > [...]
> > BTW a colleague and I were wondering whether we really want to set
> > `errno=ENOTDIR` in `gen_full_path_at()` for empty paths when
> > `AT_EMPTY_PATH` is _not_ specified. As far as we can tell, Linux sets
> > `errno=ENOENT` in that instance.
> 
> I wonder if that's really what you mean.  gen_full_path_at() generates
> ENOTDIR in two scenarios:
> 
> - At line 4443, if Cygwin can't resolve dirfd into a valid directory.
> 
> - At line 4450 if ... actually... never.  Given that p is always
>   set to the end of the directory string copied into path_ret, it
>   can never be NULL. Looks like this check for !p is a remnant from
>   the past.  We should remove it.
> 
> The actual check for an empty path is done in line 4457, and this
> results in ENOENT, as desired.
> 
> So, by any chance, do you mean the situation handled in line 4443,
> that is, returning ENOTDIR if dirfd doesn't resolve to a directory?
> 
> Yeah, it slightly complicates the caller, but it's not exactly
> wrong, given your patch.
> 
> OTOH, this entire thing doesn't look overly well thought out.  We try
> to generate a full path in gen_full_path_at() and if it fails in
> a certain way and AT_EMPTY_PATH is given, we basically repeat
> trying to create a full path in the caller.  Maybe some
> streamlining would be in order...

I actually found some time, to do that.  So I now have a counter
proposal to your patch.  I'll send the patch series in a minute.  Would
you mind to take a discerning look and, perhaps, give it a try, too?


Thanks,
Corinna