[PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Ken Brown-6
Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
Following Linux, the first patch in this series allows the call to
succeed if O_PATH is also specified.

According to the Linux man page for 'open', the file descriptor
returned by the call should be usable as the dirfd argument in calls
to fstatat and readlinkat with an empty pathname, to have
the calls operate on the symbolic link.  The second and third patches
achieve this.  For fstatat, we do this by adding support
for the AT_EMPTY_PATH flag.

Note: The man page mentions fchownat and linkat also.  linkat already
supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
don't understand how this could work for fchownat, because fchown
fails with EBADF if its fd argument was opened with O_PATH.  So I
haven't touched fchownat.

Am I missing something?

Ken Brown (3):
  Cygwin: allow opening a symlink with O_PATH | O_NOFOLLOW
  Cygwin: readlinkat: allow pathname to be empty
  Cygwin: fstatat: support the AT_EMPTY_PATH flag

 winsup/cygwin/syscalls.cc | 40 +++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 1/3] Cygwin: allow opening a symlink with O_PATH | O_NOFOLLOW

Ken Brown-6
Up to now, opening a symlink with O_NOFOLLOW fails with ELOOP.
Following Linux, allow this to succeed if O_PATH is also specified.
---
 winsup/cygwin/syscalls.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 20126ce10..038a316db 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1470,7 +1470,7 @@ open (const char *unix_path, int flags, ...)
 
       if (!(fh = build_fh_name (unix_path, opt, stat_suffixes)))
  __leave; /* errno already set */
-      if ((flags & O_NOFOLLOW) && fh->issymlink ())
+      if ((flags & O_NOFOLLOW) && fh->issymlink () && !(flags & O_PATH))
  {
   set_errno (ELOOP);
   __leave;
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 2/3] Cygwin: readlinkat: allow pathname to be empty

Ken Brown-6
In reply to this post by Ken Brown-6
Following Linux, allow the pathname argument to be an empty string,
provided the dirfd argument refers to a symlink opened with O_PATH and
O_NOFOLLOW.  The readlinkat call then operates on that symlink.
---
 winsup/cygwin/syscalls.cc | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 038a316db..2be8693c9 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -4979,8 +4979,23 @@ readlinkat (int dirfd, const char *__restrict pathname, char *__restrict buf,
   __try
     {
       char *path = tp.c_get ();
-      if (gen_full_path_at (path, dirfd, pathname))
- __leave;
+      int res = gen_full_path_at (path, dirfd, pathname);
+      if (res)
+ {
+  if (errno != ENOENT)
+    __leave;
+  /* pathname is an empty string.  This is OK if dirfd refers
+     to a symlink that was opened with O_PATH and O_NOFOLLOW.
+     In this case, readlinkat operates on the symlink. */
+  cygheap_fdget cfd (dirfd);
+  if (cfd < 0)
+    __leave;
+  if (!(cfd->issymlink ()
+ && cfd->get_flags () & O_PATH
+ && cfd->get_flags () & O_NOFOLLOW))
+    __leave;
+  strcpy (path, cfd->get_name ());
+ }
       return readlink (path, buf, bufsize);
     }
   __except (EFAULT) {}
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 3/3] Cygwin: fstatat: support the AT_EMPTY_PATH flag

Ken Brown-6
In reply to this post by Ken Brown-6
Following Linux, allow the pathname argument to be empty if the
AT_EMPTY_PATH is specified.  In this case the dirfd argument can refer
to any type of file, not just a directory, and the call operates on
that file.  In particular, dirfd can refer to a symlink that was
opened with O_PATH and O_NOFOLLOW.
---
 winsup/cygwin/syscalls.cc | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 2be8693c9..9b7d6dbfd 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -4808,14 +4808,27 @@ fstatat (int dirfd, const char *__restrict pathname, struct stat *__restrict st,
   tmp_pathbuf tp;
   __try
     {
-      if (flags & ~AT_SYMLINK_NOFOLLOW)
+      if (flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH))
  {
   set_errno (EINVAL);
   __leave;
  }
       char *path = tp.c_get ();
-      if (gen_full_path_at (path, dirfd, pathname))
- __leave;
+      int res = gen_full_path_at (path, dirfd, pathname);
+      if (res)
+ {
+  if (!(errno == ENOENT && (flags & AT_EMPTY_PATH)))
+    __leave;
+  /* pathname is an empty string.  Operate on dirfd. */
+  if (dirfd == AT_FDCWD)
+    {
+      cwdstuff::cwd_lock.acquire ();
+      strcpy (path, cygheap->cwd.get_posix ());
+      cwdstuff::cwd_lock.release ();
+    }
+  else
+    return fstat (dirfd, st);
+ }
       path_conv pc (path, ((flags & AT_SYMLINK_NOFOLLOW)
    ? PC_SYM_NOFOLLOW : PC_SYM_FOLLOW)
   | PC_POSIX | PC_KEEP_HANDLE, stat_suffixes);
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Brian Inglis
In reply to this post by Ken Brown-6
On 2019-12-29 10:56, Ken Brown wrote:

> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
> Following Linux, the first patch in this series allows the call to
> succeed if O_PATH is also specified.
>
> According to the Linux man page for 'open', the file descriptor
> returned by the call should be usable as the dirfd argument in calls
> to fstatat and readlinkat with an empty pathname, to have
> the calls operate on the symbolic link.  The second and third patches
> achieve this.  For fstatat, we do this by adding support
> for the AT_EMPTY_PATH flag.
>
> Note: The man page mentions fchownat and linkat also.  linkat already
> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
> don't understand how this could work for fchownat, because fchown
> fails with EBADF if its fd argument was opened with O_PATH.  So I
> haven't touched fchownat.
>
> Am I missing something?

WSL $ man 2 chown
...
"AT_EMPTY_PATH (since Linux 2.6.39)
If pathname is an empty string, operate on the file referred to
by dirfd (which may have been obtained using the open(2) O_PATH
flag). In  this case, dirfd can refer to any type of file, not
just a directory. If dirfd is AT_FDCWD, the  call operates on
the current working directory. This flag is Linux-specific; de‐
fine _GNU_SOURCE to obtain its definition."

says chown the dirfd, regardless of what it is,
except if AT_FDCWD, chown the CWD.

WSL $ man 2 open
"O_PATH (since Linux 2.6.39)
Obtain a file descriptor that can be used for two purposes: to
indicate a location in the filesystem tree and to perform
operations that act purely at the file descriptor level.  The
file itself is not opened, and other file operations (e.g.,
read(2), write(2), fchmod(2), fchown(2), fgetxattr(2),
ioctl(2), mmap(2)) fail with the error EBADF."

O_PATH does not open the file, so fchown returns EBADF,
as it requires an fd of an open file.

--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Ken Brown-6
On 12/30/2019 2:18 PM, Brian Inglis wrote:

> On 2019-12-29 10:56, Ken Brown wrote:
>> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
>> Following Linux, the first patch in this series allows the call to
>> succeed if O_PATH is also specified.
>>
>> According to the Linux man page for 'open', the file descriptor
>> returned by the call should be usable as the dirfd argument in calls
>> to fstatat and readlinkat with an empty pathname, to have
>> the calls operate on the symbolic link.  The second and third patches
>> achieve this.  For fstatat, we do this by adding support
>> for the AT_EMPTY_PATH flag.
>>
>> Note: The man page mentions fchownat and linkat also.  linkat already
>> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
>> don't understand how this could work for fchownat, because fchown
>> fails with EBADF if its fd argument was opened with O_PATH.  So I
>> haven't touched fchownat.
>>
>> Am I missing something?
>
> WSL $ man 2 chown
> ...
> "AT_EMPTY_PATH (since Linux 2.6.39)
> If pathname is an empty string, operate on the file referred to
> by dirfd (which may have been obtained using the open(2) O_PATH
> flag). In  this case, dirfd can refer to any type of file, not
> just a directory. If dirfd is AT_FDCWD, the  call operates on
> the current working directory. This flag is Linux-specific; de‐
> fine _GNU_SOURCE to obtain its definition."
>
> says chown the dirfd, regardless of what it is,
> except if AT_FDCWD, chown the CWD.
>
> WSL $ man 2 open
> "O_PATH (since Linux 2.6.39)
> Obtain a file descriptor that can be used for two purposes: to
> indicate a location in the filesystem tree and to perform
> operations that act purely at the file descriptor level.  The
> file itself is not opened, and other file operations (e.g.,
> read(2), write(2), fchmod(2), fchown(2), fgetxattr(2),
> ioctl(2), mmap(2)) fail with the error EBADF."
>
> O_PATH does not open the file, so fchown returns EBADF,
> as it requires an fd of an open file.

I think you've just confirmed what I already said: If fchownat is called with
AT_EMPTY_PATH, with an empty pathname, and with dirfd referring to a file that
was opened with O_PATH, then fchownat will fail with EBADF.

So for the purposes of this patch series, I don't see the point of adding
support for AT_EMPTY_PATH in fchownat.

Am I missing something?

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Brian Inglis
On 2019-12-30 12:53, Ken Brown wrote:

> On 12/30/2019 2:18 PM, Brian Inglis wrote:
>> On 2019-12-29 10:56, Ken Brown wrote:
>>> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
>>> Following Linux, the first patch in this series allows the call to
>>> succeed if O_PATH is also specified.
>>>
>>> According to the Linux man page for 'open', the file descriptor
>>> returned by the call should be usable as the dirfd argument in calls
>>> to fstatat and readlinkat with an empty pathname, to have
>>> the calls operate on the symbolic link.  The second and third patches
>>> achieve this.  For fstatat, we do this by adding support
>>> for the AT_EMPTY_PATH flag.
>>>
>>> Note: The man page mentions fchownat and linkat also.  linkat already
>>> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
>>> don't understand how this could work for fchownat, because fchown
>>> fails with EBADF if its fd argument was opened with O_PATH.  So I
>>> haven't touched fchownat.
>>>
>>> Am I missing something?
>>
>> WSL $ man 2 chown
>> ...
>> "AT_EMPTY_PATH (since Linux 2.6.39)
>> If pathname is an empty string, operate on the file referred to
>> by dirfd (which may have been obtained using the open(2) O_PATH
>> flag). In  this case, dirfd can refer to any type of file, not
>> just a directory. If dirfd is AT_FDCWD, the  call operates on
>> the current working directory. This flag is Linux-specific; de‐
>> fine _GNU_SOURCE to obtain its definition."
>>
>> says chown the dirfd, regardless of what it is,
>> except if AT_FDCWD, chown the CWD.
>>
>> WSL $ man 2 open
>> "O_PATH (since Linux 2.6.39)
>> Obtain a file descriptor that can be used for two purposes: to
>> indicate a location in the filesystem tree and to perform
>> operations that act purely at the file descriptor level.  The
>> file itself is not opened, and other file operations (e.g.,
>> read(2), write(2), fchmod(2), fchown(2), fgetxattr(2),
>> ioctl(2), mmap(2)) fail with the error EBADF."
>>
>> O_PATH does not open the file, so fchown returns EBADF,
>> as it requires an fd of an open file.
>
> I think you've just confirmed what I already said: If fchownat is called with
> AT_EMPTY_PATH, with an empty pathname, and with dirfd referring to a file that
> was opened with O_PATH, then fchownat will fail with EBADF.
>
> So for the purposes of this patch series, I don't see the point of adding
> support for AT_EMPTY_PATH in fchownat.
>
> Am I missing something?

That is the user's problem: it is their responsibility to pass an fd open for
reading or searching, not one opened with O_PATH (on Linux or Cygwin), or
AT_FDCWD; it is Cygwin's responsibility to ensure that valid args succeed and
invalid args return the expected errno.

$ man fchownat
...
"EBADF  The path argument does not specify an absolute path and
        the fd argument is neither AT_FDCWD nor a valid file descriptor
        open for reading or searching."

--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Ken Brown-6
On 12/30/2019 3:55 PM, Brian Inglis wrote:

> On 2019-12-30 12:53, Ken Brown wrote:
>> On 12/30/2019 2:18 PM, Brian Inglis wrote:
>>> On 2019-12-29 10:56, Ken Brown wrote:
>>>> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
>>>> Following Linux, the first patch in this series allows the call to
>>>> succeed if O_PATH is also specified.
>>>>
>>>> According to the Linux man page for 'open', the file descriptor
>>>> returned by the call should be usable as the dirfd argument in calls
>>>> to fstatat and readlinkat with an empty pathname, to have
>>>> the calls operate on the symbolic link.  The second and third patches
>>>> achieve this.  For fstatat, we do this by adding support
>>>> for the AT_EMPTY_PATH flag.
>>>>
>>>> Note: The man page mentions fchownat and linkat also.  linkat already
>>>> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
>>>> don't understand how this could work for fchownat, because fchown
>>>> fails with EBADF if its fd argument was opened with O_PATH.  So I
>>>> haven't touched fchownat.
>>>>
>>>> Am I missing something?
>>>
>>> WSL $ man 2 chown
>>> ...
>>> "AT_EMPTY_PATH (since Linux 2.6.39)
>>> If pathname is an empty string, operate on the file referred to
>>> by dirfd (which may have been obtained using the open(2) O_PATH
>>> flag). In  this case, dirfd can refer to any type of file, not
>>> just a directory. If dirfd is AT_FDCWD, the  call operates on
>>> the current working directory. This flag is Linux-specific; de‐
>>> fine _GNU_SOURCE to obtain its definition."
>>>
>>> says chown the dirfd, regardless of what it is,
>>> except if AT_FDCWD, chown the CWD.
>>>
>>> WSL $ man 2 open
>>> "O_PATH (since Linux 2.6.39)
>>> Obtain a file descriptor that can be used for two purposes: to
>>> indicate a location in the filesystem tree and to perform
>>> operations that act purely at the file descriptor level.  The
>>> file itself is not opened, and other file operations (e.g.,
>>> read(2), write(2), fchmod(2), fchown(2), fgetxattr(2),
>>> ioctl(2), mmap(2)) fail with the error EBADF."
>>>
>>> O_PATH does not open the file, so fchown returns EBADF,
>>> as it requires an fd of an open file.
>>
>> I think you've just confirmed what I already said: If fchownat is called with
>> AT_EMPTY_PATH, with an empty pathname, and with dirfd referring to a file that
>> was opened with O_PATH, then fchownat will fail with EBADF.
>>
>> So for the purposes of this patch series, I don't see the point of adding
>> support for AT_EMPTY_PATH in fchownat.
>>
>> Am I missing something?
>
> That is the user's problem: it is their responsibility to pass an fd open for
> reading or searching, not one opened with O_PATH (on Linux or Cygwin), or
> AT_FDCWD; it is Cygwin's responsibility to ensure that valid args succeed and
> invalid args return the expected errno.

Yes, but Cygwin doesn't claim to support the AT_EMPTY_PATH flag except in
linkat.  So there is no expected errno.  The only way there would be an expected
errno is if we decide to add support for AT_EMPTY_PATH to fchownat.  I'm saying
that I don't see the point in doing that, and I'm asking whether I'm missing
something.  If you think I should add that support, please explain why.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Brian Inglis
On 2019-12-30 14:47, Ken Brown wrote:

> On 12/30/2019 3:55 PM, Brian Inglis wrote:
>> On 2019-12-30 12:53, Ken Brown wrote:
>>> On 12/30/2019 2:18 PM, Brian Inglis wrote:
>>>> On 2019-12-29 10:56, Ken Brown wrote:
>>>>> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
>>>>> Following Linux, the first patch in this series allows the call to
>>>>> succeed if O_PATH is also specified.
>>>>>
>>>>> According to the Linux man page for 'open', the file descriptor
>>>>> returned by the call should be usable as the dirfd argument in calls
>>>>> to fstatat and readlinkat with an empty pathname, to have
>>>>> the calls operate on the symbolic link.  The second and third patches
>>>>> achieve this.  For fstatat, we do this by adding support
>>>>> for the AT_EMPTY_PATH flag.
>>>>>
>>>>> Note: The man page mentions fchownat and linkat also.  linkat already
>>>>> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
>>>>> don't understand how this could work for fchownat, because fchown
>>>>> fails with EBADF if its fd argument was opened with O_PATH.  So I
>>>>> haven't touched fchownat.
>>>>>
>>>>> Am I missing something?
>>>>
>>>> WSL $ man 2 chown
>>>> ...
>>>> "AT_EMPTY_PATH (since Linux 2.6.39)
>>>> If pathname is an empty string, operate on the file referred to
>>>> by dirfd (which may have been obtained using the open(2) O_PATH
>>>> flag). In  this case, dirfd can refer to any type of file, not
>>>> just a directory. If dirfd is AT_FDCWD, the  call operates on
>>>> the current working directory. This flag is Linux-specific; de‐
>>>> fine _GNU_SOURCE to obtain its definition."
>>>>
>>>> says chown the dirfd, regardless of what it is,
>>>> except if AT_FDCWD, chown the CWD.
>>>>
>>>> WSL $ man 2 open
>>>> "O_PATH (since Linux 2.6.39)
>>>> Obtain a file descriptor that can be used for two purposes: to
>>>> indicate a location in the filesystem tree and to perform
>>>> operations that act purely at the file descriptor level.  The
>>>> file itself is not opened, and other file operations (e.g.,
>>>> read(2), write(2), fchmod(2), fchown(2), fgetxattr(2),
>>>> ioctl(2), mmap(2)) fail with the error EBADF."
>>>>
>>>> O_PATH does not open the file, so fchown returns EBADF,
>>>> as it requires an fd of an open file.
>>>
>>> I think you've just confirmed what I already said: If fchownat is called with
>>> AT_EMPTY_PATH, with an empty pathname, and with dirfd referring to a file that
>>> was opened with O_PATH, then fchownat will fail with EBADF.
>>>
>>> So for the purposes of this patch series, I don't see the point of adding
>>> support for AT_EMPTY_PATH in fchownat.
>>>
>>> Am I missing something?
>>
>> That is the user's problem: it is their responsibility to pass an fd open for
>> reading or searching, not one opened with O_PATH (on Linux or Cygwin), or
>> AT_FDCWD; it is Cygwin's responsibility to ensure that valid args succeed and
>> invalid args return the expected errno.
>
> Yes, but Cygwin doesn't claim to support the AT_EMPTY_PATH flag except in
> linkat.  So there is no expected errno.  The only way there would be an expected
> errno is if we decide to add support for AT_EMPTY_PATH to fchownat.  I'm saying
> that I don't see the point in doing that, and I'm asking whether I'm missing
> something.  If you think I should add that support, please explain why.

To allow perms changed on the cwd, directories or files with an open fd, to
avoid race conditions, like the other ...at functions.
I don't get why you don't see those as useful cases.

--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Ken Brown-6
On 12/30/2019 6:09 PM, Brian Inglis wrote:

> On 2019-12-30 14:47, Ken Brown wrote:
>> On 12/30/2019 3:55 PM, Brian Inglis wrote:
>>> On 2019-12-30 12:53, Ken Brown wrote:
>>>> On 12/30/2019 2:18 PM, Brian Inglis wrote:
>>>>> On 2019-12-29 10:56, Ken Brown wrote:
>>>>>> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
>>>>>> Following Linux, the first patch in this series allows the call to
>>>>>> succeed if O_PATH is also specified.
>>>>>>
>>>>>> According to the Linux man page for 'open', the file descriptor
>>>>>> returned by the call should be usable as the dirfd argument in calls
>>>>>> to fstatat and readlinkat with an empty pathname, to have
>>>>>> the calls operate on the symbolic link.  The second and third patches
>>>>>> achieve this.  For fstatat, we do this by adding support
>>>>>> for the AT_EMPTY_PATH flag.
>>>>>>
>>>>>> Note: The man page mentions fchownat and linkat also.  linkat already
>>>>>> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
>>>>>> don't understand how this could work for fchownat, because fchown
>>>>>> fails with EBADF if its fd argument was opened with O_PATH.  So I
>>>>>> haven't touched fchownat.
>>>>>>
>>>>>> Am I missing something?
>>>>>
>>>>> WSL $ man 2 chown
>>>>> ...
>>>>> "AT_EMPTY_PATH (since Linux 2.6.39)
>>>>> If pathname is an empty string, operate on the file referred to
>>>>> by dirfd (which may have been obtained using the open(2) O_PATH
>>>>> flag). In  this case, dirfd can refer to any type of file, not
>>>>> just a directory. If dirfd is AT_FDCWD, the  call operates on
>>>>> the current working directory. This flag is Linux-specific; de‐
>>>>> fine _GNU_SOURCE to obtain its definition."
>>>>>
>>>>> says chown the dirfd, regardless of what it is,
>>>>> except if AT_FDCWD, chown the CWD.
>>>>>
>>>>> WSL $ man 2 open
>>>>> "O_PATH (since Linux 2.6.39)
>>>>> Obtain a file descriptor that can be used for two purposes: to
>>>>> indicate a location in the filesystem tree and to perform
>>>>> operations that act purely at the file descriptor level.  The
>>>>> file itself is not opened, and other file operations (e.g.,
>>>>> read(2), write(2), fchmod(2), fchown(2), fgetxattr(2),
>>>>> ioctl(2), mmap(2)) fail with the error EBADF."
>>>>>
>>>>> O_PATH does not open the file, so fchown returns EBADF,
>>>>> as it requires an fd of an open file.
>>>>
>>>> I think you've just confirmed what I already said: If fchownat is called with
>>>> AT_EMPTY_PATH, with an empty pathname, and with dirfd referring to a file that
>>>> was opened with O_PATH, then fchownat will fail with EBADF.
>>>>
>>>> So for the purposes of this patch series, I don't see the point of adding
>>>> support for AT_EMPTY_PATH in fchownat.
>>>>
>>>> Am I missing something?
>>>
>>> That is the user's problem: it is their responsibility to pass an fd open for
>>> reading or searching, not one opened with O_PATH (on Linux or Cygwin), or
>>> AT_FDCWD; it is Cygwin's responsibility to ensure that valid args succeed and
>>> invalid args return the expected errno.
>>
>> Yes, but Cygwin doesn't claim to support the AT_EMPTY_PATH flag except in
>> linkat.  So there is no expected errno.  The only way there would be an expected
>> errno is if we decide to add support for AT_EMPTY_PATH to fchownat.  I'm saying
>> that I don't see the point in doing that, and I'm asking whether I'm missing
>> something.  If you think I should add that support, please explain why.
>
> To allow perms changed on the cwd, directories or files with an open fd, to
> avoid race conditions, like the other ...at functions.
> I don't get why you don't see those as useful cases.

I think we're mis-communicating.  This is a patch series whose purpose is to add
support for opening a symlink with O_PATH | O_NOFOLLOW.  In that connection I
modified readlinkat and fstatat to allow the resulting fd to be used as the
dirfd argument in those calls, with an empty pathname.  I didn't do the same for
fchownat because it seems to me that it would always fail with EBADF in that
setting.

It's not relevant that AT_EMPTY_PATH might be useful for fchownat in a different
setting.  That could be the subject of a different patch.  If you think it would
be useful *in the context of this patch series*, please explain why.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Corinna Vinschen-2
In reply to this post by Ken Brown-6
Hi Ken,

On Dec 29 17:56, Ken Brown wrote:

> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
> Following Linux, the first patch in this series allows the call to
> succeed if O_PATH is also specified.
>
> According to the Linux man page for 'open', the file descriptor
> returned by the call should be usable as the dirfd argument in calls
> to fstatat and readlinkat with an empty pathname, to have
> the calls operate on the symbolic link.  The second and third patches
> achieve this.  For fstatat, we do this by adding support
> for the AT_EMPTY_PATH flag.
>
> Note: The man page mentions fchownat and linkat also.  linkat already
> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
> don't understand how this could work for fchownat, because fchown
> fails with EBADF if its fd argument was opened with O_PATH.  So I
> haven't touched fchownat.
It was never supposed to work that way.  We can make fchownat work
with AT_EMPTY_PATH, but using it on a file opened with O_PATH
contradicts the Linux open(2) man page, afaics:

 O_PATH (since Linux 2.6.39)
  Obtain a file descriptor that can be used for two  purposes:  to
  indicate a location in the filesystem tree and to perform opera‐
  tions that act purely at the file descriptor  level.   The  file
  itself  is not opened, and other file operations (e.g., read(2),
  write(2), fchmod(2), fchown(2), fgetxattr(2), ioctl(2), mmap(2))
                       ^^^^^^^^^
  fail with the error EBADF.
  ^^^^^^^^^           ^^^^^

That'd from the current F31 man pages.

> Am I missing something?

Good question.  Let me ask in return, did *I* now miss something?


Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Ken Brown-6
On 1/13/2020 10:28 AM, Corinna Vinschen wrote:

> Hi Ken,
>
> On Dec 29 17:56, Ken Brown wrote:
>> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
>> Following Linux, the first patch in this series allows the call to
>> succeed if O_PATH is also specified.
>>
>> According to the Linux man page for 'open', the file descriptor
>> returned by the call should be usable as the dirfd argument in calls
>> to fstatat and readlinkat with an empty pathname, to have
>> the calls operate on the symbolic link.  The second and third patches
>> achieve this.  For fstatat, we do this by adding support
>> for the AT_EMPTY_PATH flag.
>>
>> Note: The man page mentions fchownat and linkat also.  linkat already
>> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
>> don't understand how this could work for fchownat, because fchown
>> fails with EBADF if its fd argument was opened with O_PATH.  So I
>> haven't touched fchownat.
>
> It was never supposed to work that way.  We can make fchownat work
> with AT_EMPTY_PATH, but using it on a file opened with O_PATH
> contradicts the Linux open(2) man page, afaics:
>
>   O_PATH (since Linux 2.6.39)
>    Obtain a file descriptor that can be used for two  purposes:  to
>    indicate a location in the filesystem tree and to perform opera‐
>    tions that act purely at the file descriptor  level.   The  file
>    itself  is not opened, and other file operations (e.g., read(2),
>    write(2), fchmod(2), fchown(2), fgetxattr(2), ioctl(2), mmap(2))
>                         ^^^^^^^^^
>    fail with the error EBADF.
>    ^^^^^^^^^           ^^^^^
>
> That'd from the current F31 man pages.
>
>> Am I missing something?
>
> Good question.  Let me ask in return, did *I* now miss something?

I don't think so.  I think we agree, although maybe I didn't express myself
clearly enough for that to be obvious.  What confused me was the following
paragraph further down in the open(2) man page (still discussing O_PATH):

   If pathname is a symbolic link and the O_NOFOLLOW flag is also
   specified, then the call returns a file descriptor referring
   to the symbolic link.  This file descriptor can be used as the
   dirfd argument in calls to fchownat(2), fstatat(2), linkat(2),
                              ^^^^^^^^^^^
   and readlinkat(2) with an empty pathname to have the calls
   operate on the symbolic link.

I don't know why they include fchownat here, since the resulting call would fail
with EBADF.  So I didn't implement that in my patch series.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Eric Blake (cygwin)-2
On 1/13/20 10:53 AM, Ken Brown wrote:
> On 1/13/2020 10:28 AM, Corinna Vinschen wrote:
>> Hi Ken,
>>
>> On Dec 29 17:56, Ken Brown wrote:
>>> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
>>> Following Linux, the first patch in this series allows the call to
>>> succeed if O_PATH is also specified.
>>>

>>
>>    O_PATH (since Linux 2.6.39)
>>     Obtain a file descriptor that can be used for two  purposes:  to
>>     indicate a location in the filesystem tree and to perform opera‐
>>     tions that act purely at the file descriptor  level.   The  file
>>     itself  is not opened, and other file operations (e.g., read(2),
>>     write(2), fchmod(2), fchown(2), fgetxattr(2), ioctl(2), mmap(2))
>>                          ^^^^^^^^^
>>     fail with the error EBADF.
>>     ^^^^^^^^^           ^^^^^
>>

On BSD systems, you are able to run lchmod to change permissions on a
symlink (with effect on who is able to follow that symlink during
pathname resolution); Linux does not support that, and POSIX does not
mandate support for that, so fchmodat() is allowed to fail on symlinks
even while fchownat() is required to work on symlinks.

>> That'd from the current F31 man pages.
>>
>>> Am I missing something?
>>
>> Good question.  Let me ask in return, did *I* now miss something?
>
> I don't think so.  I think we agree, although maybe I didn't express myself
> clearly enough for that to be obvious.  What confused me was the following
> paragraph further down in the open(2) man page (still discussing O_PATH):
>
>     If pathname is a symbolic link and the O_NOFOLLOW flag is also
>     specified, then the call returns a file descriptor referring
>     to the symbolic link.  This file descriptor can be used as the
>     dirfd argument in calls to fchownat(2), fstatat(2), linkat(2),
>                                ^^^^^^^^^^^
>     and readlinkat(2) with an empty pathname to have the calls
>     operate on the symbolic link.
>
> I don't know why they include fchownat here, since the resulting call would fail
> with EBADF.  So I didn't implement that in my patch series.

I'm not sure if the question here is about fchownat() (where you CAN
change owner of a symlink on Linux, same as with lchown()) or about
fchmodat() (where you would attempt to change permissions of a symlink,
as on BSD, but where Linux lacks lchmod()).

Another wrinkle is that for the longest time, the Linux kernel did not
make it possible to correctly implement fchmodat(AT_SYMLINK_NOFOLLOW);
it is only with the recent introduction of the fchmodat2() syscall that
this has become possible (https://patchwork.kernel.org/patch/9596301/)

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Ken Brown-6
On 1/13/2020 12:24 PM, Eric Blake wrote:

> On 1/13/20 10:53 AM, Ken Brown wrote:
>> On 1/13/2020 10:28 AM, Corinna Vinschen wrote:
>>> Hi Ken,
>>>
>>> On Dec 29 17:56, Ken Brown wrote:
>>>> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
>>>> Following Linux, the first patch in this series allows the call to
>>>> succeed if O_PATH is also specified.
>>>>
>
>>>
>>>    O_PATH (since Linux 2.6.39)
>>>     Obtain a file descriptor that can be used for two  purposes:  to
>>>     indicate a location in the filesystem tree and to perform opera‐
>>>     tions that act purely at the file descriptor  level.   The  file
>>>     itself  is not opened, and other file operations (e.g., read(2),
>>>     write(2), fchmod(2), fchown(2), fgetxattr(2), ioctl(2), mmap(2))
>>>                          ^^^^^^^^^
>>>     fail with the error EBADF.
>>>     ^^^^^^^^^           ^^^^^
>>>
>
> On BSD systems, you are able to run lchmod to change permissions on a symlink
> (with effect on who is able to follow that symlink during pathname resolution);
> Linux does not support that, and POSIX does not mandate support for that, so
> fchmodat() is allowed to fail on symlinks even while fchownat() is required to
> work on symlinks.
>
>>> That'd from the current F31 man pages.
>>>
>>>> Am I missing something?
>>>
>>> Good question.  Let me ask in return, did *I* now miss something?
>>
>> I don't think so.  I think we agree, although maybe I didn't express myself
>> clearly enough for that to be obvious.  What confused me was the following
>> paragraph further down in the open(2) man page (still discussing O_PATH):
>>
>>     If pathname is a symbolic link and the O_NOFOLLOW flag is also
>>     specified, then the call returns a file descriptor referring
>>     to the symbolic link.  This file descriptor can be used as the
>>     dirfd argument in calls to fchownat(2), fstatat(2), linkat(2),
>>                                ^^^^^^^^^^^
>>     and readlinkat(2) with an empty pathname to have the calls
>>     operate on the symbolic link.
>>
>> I don't know why they include fchownat here, since the resulting call would fail
>> with EBADF.  So I didn't implement that in my patch series.
>
> I'm not sure if the question here is about fchownat() (where you CAN change
> owner of a symlink on Linux, same as with lchown())

Yes, the question is about fchownat.  Are you saying you can change the owner
even if the symlink was opened with O_PATH?

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Corinna Vinschen-2
In reply to this post by Ken Brown-6
On Jan 13 16:53, Ken Brown wrote:

> On 1/13/2020 10:28 AM, Corinna Vinschen wrote:
> > On Dec 29 17:56, Ken Brown wrote:
> >> [...]
> >> Note: The man page mentions fchownat and linkat also.  linkat already
> >> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
> >> don't understand how this could work for fchownat, because fchown
> >> fails with EBADF if its fd argument was opened with O_PATH.  So I
> >> haven't touched fchownat.
> >
> > It was never supposed to work that way.  We can make fchownat work
> > with AT_EMPTY_PATH, but using it on a file opened with O_PATH
> > contradicts the Linux open(2) man page, afaics:
> >
> >   O_PATH (since Linux 2.6.39)
> >    Obtain a file descriptor that can be used for two  purposes:  to
> >    indicate a location in the filesystem tree and to perform opera‐
> >    tions that act purely at the file descriptor  level.   The  file
> >    itself  is not opened, and other file operations (e.g., read(2),
> >    write(2), fchmod(2), fchown(2), fgetxattr(2), ioctl(2), mmap(2))
> >                         ^^^^^^^^^
> >    fail with the error EBADF.
> >    ^^^^^^^^^           ^^^^^
> >
> > That'd from the current F31 man pages.
> >
> >> Am I missing something?
> >
> > Good question.  Let me ask in return, did *I* now miss something?
>
> I don't think so.  I think we agree, although maybe I didn't express myself
> clearly enough for that to be obvious.  What confused me was the following
> paragraph further down in the open(2) man page (still discussing O_PATH):
>
>    If pathname is a symbolic link and the O_NOFOLLOW flag is also
>    specified, then the call returns a file descriptor referring
>    to the symbolic link.  This file descriptor can be used as the
>    dirfd argument in calls to fchownat(2), fstatat(2), linkat(2),
>                               ^^^^^^^^^^^
>    and readlinkat(2) with an empty pathname to have the calls
>    operate on the symbolic link.
That's the part I missed, apparently.  Implementing fchownat like this
may be a bit upside down.  The problem is that open(O_PATH) opens the
file with query_read_attributes (aka READ_CONTROL | FILE_READ_ATTRIBUTES),
to make sure the calls mentioned in the snippet I pasted don't succeed.  

If fchownat is supposed to work on a symlink like this, the easiest
approach may be checking for this scenario in fchownat and calling
lchown on the pathname instead.  Or something along these lines.


Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Eric Blake (cygwin)-2
In reply to this post by Ken Brown-6
On 1/13/20 11:44 AM, Ken Brown wrote:

>>> I don't think so.  I think we agree, although maybe I didn't express myself
>>> clearly enough for that to be obvious.  What confused me was the following
>>> paragraph further down in the open(2) man page (still discussing O_PATH):
>>>
>>>      If pathname is a symbolic link and the O_NOFOLLOW flag is also
>>>      specified, then the call returns a file descriptor referring
>>>      to the symbolic link.  This file descriptor can be used as the
>>>      dirfd argument in calls to fchownat(2), fstatat(2), linkat(2),
>>>                                 ^^^^^^^^^^^
>>>      and readlinkat(2) with an empty pathname to have the calls
>>>      operate on the symbolic link.
>>>
>>> I don't know why they include fchownat here, since the resulting call would fail
>>> with EBADF.  So I didn't implement that in my patch series.
>>
>> I'm not sure if the question here is about fchownat() (where you CAN change
>> owner of a symlink on Linux, same as with lchown())
>
> Yes, the question is about fchownat.  Are you saying you can change the owner
> even if the symlink was opened with O_PATH?

Without actually writing a test program on Linux, I'm not sure.
Logically, I'd expect that changing the owner of a symlink is a metadata
operation that affects the containing directory rather than the contents
of the file, but that access to the directory entry is what O_PATH is
supposed to provide, and therefore it seems like fchownat() on an empty
filename should work the same as lchownat().  But if it fails in Linux,
then we don't have to do any better.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Corinna Vinschen-2
In reply to this post by Corinna Vinschen-2
On Jan 13 19:34, Corinna Vinschen wrote:

> On Jan 13 16:53, Ken Brown wrote:
> > On 1/13/2020 10:28 AM, Corinna Vinschen wrote:
> > > On Dec 29 17:56, Ken Brown wrote:
> > >> [...]
> > >> Note: The man page mentions fchownat and linkat also.  linkat already
> > >> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
> > >> don't understand how this could work for fchownat, because fchown
> > >> fails with EBADF if its fd argument was opened with O_PATH.  So I
> > >> haven't touched fchownat.
> > >
> > > It was never supposed to work that way.  We can make fchownat work
> > > with AT_EMPTY_PATH, but using it on a file opened with O_PATH
> > > contradicts the Linux open(2) man page, afaics:
> > >
> > >   O_PATH (since Linux 2.6.39)
> > >    Obtain a file descriptor that can be used for two  purposes:  to
> > >    indicate a location in the filesystem tree and to perform opera‐
> > >    tions that act purely at the file descriptor  level.   The  file
> > >    itself  is not opened, and other file operations (e.g., read(2),
> > >    write(2), fchmod(2), fchown(2), fgetxattr(2), ioctl(2), mmap(2))
> > >                         ^^^^^^^^^
> > >    fail with the error EBADF.
> > >    ^^^^^^^^^           ^^^^^
> > >
> > > That'd from the current F31 man pages.
> > >
> > >> Am I missing something?
> > >
> > > Good question.  Let me ask in return, did *I* now miss something?
> >
> > I don't think so.  I think we agree, although maybe I didn't express myself
> > clearly enough for that to be obvious.  What confused me was the following
> > paragraph further down in the open(2) man page (still discussing O_PATH):
> >
> >    If pathname is a symbolic link and the O_NOFOLLOW flag is also
> >    specified, then the call returns a file descriptor referring
> >    to the symbolic link.  This file descriptor can be used as the
> >    dirfd argument in calls to fchownat(2), fstatat(2), linkat(2),
> >                               ^^^^^^^^^^^
> >    and readlinkat(2) with an empty pathname to have the calls
> >    operate on the symbolic link.
>
> That's the part I missed, apparently.  Implementing fchownat like this
> may be a bit upside down.  The problem is that open(O_PATH) opens the
> file with query_read_attributes (aka READ_CONTROL | FILE_READ_ATTRIBUTES),
> to make sure the calls mentioned in the snippet I pasted don't succeed.  
>
> If fchownat is supposed to work on a symlink like this, the easiest
> approach may be checking for this scenario in fchownat and calling
> lchown on the pathname instead.  Or something along these lines.
The alternative would be to open the sylink with more permissions, i.e.,
query_write_control, aka READ_CONTROL | WRITE_OWNER | WRITE_DAC |
FILE_WRITE_ATTRIBUTES.  I'm just hesitant to open up the descriptor
like this.  It probably allows too many actions on the descriptor
from user space...


Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

Ken Brown-6
On 1/13/2020 1:39 PM, Corinna Vinschen wrote:

> On Jan 13 19:34, Corinna Vinschen wrote:
>> On Jan 13 16:53, Ken Brown wrote:
>>> On 1/13/2020 10:28 AM, Corinna Vinschen wrote:
>>>> On Dec 29 17:56, Ken Brown wrote:
>>>>> [...]
>>>>> Note: The man page mentions fchownat and linkat also.  linkat already
>>>>> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
>>>>> don't understand how this could work for fchownat, because fchown
>>>>> fails with EBADF if its fd argument was opened with O_PATH.  So I
>>>>> haven't touched fchownat.
>>>>
>>>> It was never supposed to work that way.  We can make fchownat work
>>>> with AT_EMPTY_PATH, but using it on a file opened with O_PATH
>>>> contradicts the Linux open(2) man page, afaics:
>>>>
>>>>    O_PATH (since Linux 2.6.39)
>>>>     Obtain a file descriptor that can be used for two  purposes:  to
>>>>     indicate a location in the filesystem tree and to perform opera‐
>>>>     tions that act purely at the file descriptor  level.   The  file
>>>>     itself  is not opened, and other file operations (e.g., read(2),
>>>>     write(2), fchmod(2), fchown(2), fgetxattr(2), ioctl(2), mmap(2))
>>>>                          ^^^^^^^^^
>>>>     fail with the error EBADF.
>>>>     ^^^^^^^^^           ^^^^^
>>>>
>>>> That'd from the current F31 man pages.
>>>>
>>>>> Am I missing something?
>>>>
>>>> Good question.  Let me ask in return, did *I* now miss something?
>>>
>>> I don't think so.  I think we agree, although maybe I didn't express myself
>>> clearly enough for that to be obvious.  What confused me was the following
>>> paragraph further down in the open(2) man page (still discussing O_PATH):
>>>
>>>     If pathname is a symbolic link and the O_NOFOLLOW flag is also
>>>     specified, then the call returns a file descriptor referring
>>>     to the symbolic link.  This file descriptor can be used as the
>>>     dirfd argument in calls to fchownat(2), fstatat(2), linkat(2),
>>>                                ^^^^^^^^^^^
>>>     and readlinkat(2) with an empty pathname to have the calls
>>>     operate on the symbolic link.
>>
>> That's the part I missed, apparently.  Implementing fchownat like this
>> may be a bit upside down.  The problem is that open(O_PATH) opens the
>> file with query_read_attributes (aka READ_CONTROL | FILE_READ_ATTRIBUTES),
>> to make sure the calls mentioned in the snippet I pasted don't succeed.
>>
>> If fchownat is supposed to work on a symlink like this, the easiest
>> approach may be checking for this scenario in fchownat and calling
>> lchown on the pathname instead.  Or something along these lines.
>
> The alternative would be to open the sylink with more permissions, i.e.,
> query_write_control, aka READ_CONTROL | WRITE_OWNER | WRITE_DAC |
> FILE_WRITE_ATTRIBUTES.  I'm just hesitant to open up the descriptor
> like this.  It probably allows too many actions on the descriptor
> from user space...

OK, I'll follow your first suggestion, then, after verifying that fchownat
really does work on a symlink opened this way on Linux.  I guess I was
misinterpreting the man page.

Ken