Re: [PATCH] fchmodat/fstatat: fix regression with empty `pathname`
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
Re: [PATCH] fchmodat/fstatat: fix regression with empty `pathname`
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 > > > > 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`
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=r...@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`
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 > > 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`
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 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
Re: [PATCH] fchmodat/fstatat: fix regression with empty `pathname`
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.
[PATCH] fchmodat/fstatat: fix regression with empty `pathname`
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
---
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
