[PATCH 0/3] Fix the O_PATH support for FIFOs

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

[PATCH 0/3] Fix the O_PATH support for FIFOs

Ken Brown-6
Commit aa55d22c, "Cygwin: honor the O_PATH flag when opening a FIFO",
fixed a hang but otherwise didn't accomplish the purpose of the O_PATH
flag as stated in the Linux man page for open(2):

    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.

    [The man page goes on to describe operations that *can* be
    performed: close(2), fchdir(2), fstat(2),....]

    Opening a file or directory with the O_PATH flag requires no
    permissions on the object itself (but does require execute
    permission on the directories in the path prefix).

The first problem in the current implementation is that if open(2) is
called on a FIFO, fhandler_base::device_access_denied is called and
tries to open the FIFO with read access, which isn't supposed to be
required.  This is fixed by the first patch in this series.

The second patch makes fhandler_fifo::open call fhandler_base::open_fs
if O_PATH is set, so that we actually obtain a handle that can be used
for the purposes stated above.

The third page tweaks fhandler_fifo::fcntl and fhandler_fifo::dup so
that they work with O_PATH.

In a followup email I'll provide the program I used to test this
implementation.

Ken Brown (3):
  Cygwin: device_access_denied: return false if O_PATH is set
  Cygwin: re-implement fhandler_fifo::open with O_PATH
  Cygwin: FIFO: tweak fcntl and dup when O_PATH is set

 winsup/cygwin/fhandler.cc      |  3 +++
 winsup/cygwin/fhandler_fifo.cc | 15 ++++++---------
 2 files changed, 9 insertions(+), 9 deletions(-)

--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3] Cygwin: device_access_denied: return false if O_PATH is set

Ken Brown-6
If O_PATH is set in the flags argument of
fhandler_base::device_access_denied, return false.  No
read/write/execute access should be required in this case.

Previously, the call to device_access_denied in open(2) would lead to
an attempt to open the file with read access even if the O_PATH flag
was set.
---
 winsup/cygwin/fhandler.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index b0c9c50c3..aeee8fe4d 100644
--- a/winsup/cygwin/fhandler.cc
+++ b/winsup/cygwin/fhandler.cc
@@ -335,6 +335,9 @@ fhandler_base::device_access_denied (int flags)
 {
   int mode = 0;
 
+  if (flags & O_PATH)
+    return false;
+
   if (flags & O_RDWR)
     mode |= R_OK | W_OK;
   if (flags & (O_WRONLY | O_APPEND))
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3] Cygwin: re-implement fhandler_fifo::open with O_PATH

Ken Brown-6
In reply to this post by Ken Brown-6
If the O_PATH flag is set, fhandler_fifo::open now simply calls
fhandler_base::open_fs.

The previous attempt to handle O_PATH in commit aa55d22c, "Cygwin:
honor the O_PATH flag when opening a FIFO", fixed a hang but otherwise
didn't do anything useful.
---
 winsup/cygwin/fhandler_fifo.cc | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index fd8223000..8cbab353c 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -453,17 +453,13 @@ fhandler_fifo::open (int flags, mode_t)
   } res;
 
   if (flags & O_PATH)
-    {
-      query_open (query_read_attributes);
-      nohandle (true);
-    }
+    return open_fs (flags);
 
   /* Determine what we're doing with this fhandler: reading, writing, both */
   switch (flags & O_ACCMODE)
     {
     case O_RDONLY:
-      if (!query_open ())
- reader = true;
+      reader = true;
       break;
     case O_WRONLY:
       writer = true;
@@ -585,8 +581,6 @@ fhandler_fifo::open (int flags, mode_t)
     }
  }
     }
-  if (query_open ())
-    res = success;
 out:
   if (res == error_set_errno)
     __seterrno ();
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] Cygwin: FIFO: tweak fcntl and dup when O_PATH is set

Ken Brown-6
In reply to this post by Ken Brown-6
fhandler_fifo::fcntl and fhandler_fifo::dup now call the corresponding
fhandler_base methods if the FIFO was opened with the O_PATH flag.
---
 winsup/cygwin/fhandler_fifo.cc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 8cbab353c..a338f12cc 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -997,7 +997,7 @@ fhandler_fifo::close ()
 int
 fhandler_fifo::fcntl (int cmd, intptr_t arg)
 {
-  if (cmd != F_SETFL || nohandle ())
+  if (cmd != F_SETFL || nohandle () || (get_flags () & O_PATH))
     return fhandler_base::fcntl (cmd, arg);
 
   const bool was_nonblocking = is_nonblocking ();
@@ -1014,6 +1014,9 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
   int ret = -1;
   fhandler_fifo *fhf = NULL;
 
+  if (get_flags () & O_PATH)
+    return fhandler_base::dup (child, flags);
+
   if (fhandler_base::dup (child, flags))
     goto out;
 
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] Fix the O_PATH support for FIFOs

Ken Brown-6
In reply to this post by Ken Brown-6
On 1/23/2020 11:31 AM, Ken Brown wrote:

> Commit aa55d22c, "Cygwin: honor the O_PATH flag when opening a FIFO",
> fixed a hang but otherwise didn't accomplish the purpose of the O_PATH
> flag as stated in the Linux man page for open(2):
>
>      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.
>
>      [The man page goes on to describe operations that *can* be
>      performed: close(2), fchdir(2), fstat(2),....]
>
>      Opening a file or directory with the O_PATH flag requires no
>      permissions on the object itself (but does require execute
>      permission on the directories in the path prefix).
>
> The first problem in the current implementation is that if open(2) is
> called on a FIFO, fhandler_base::device_access_denied is called and
> tries to open the FIFO with read access, which isn't supposed to be
> required.  This is fixed by the first patch in this series.
>
> The second patch makes fhandler_fifo::open call fhandler_base::open_fs
> if O_PATH is set, so that we actually obtain a handle that can be used
> for the purposes stated above.
>
> The third page tweaks fhandler_fifo::fcntl and fhandler_fifo::dup so
> that they work with O_PATH.
>
> In a followup email I'll provide the program I used to test this
> implementation.
Test program attached.

$ gcc -o o_path_fifo_test o_path_fifo_test.c

$ ./o_path_fifo_test.exe
The following calls should fail with EBADF:
read: OK
write: OK
fchmod: OK
fchown: OK
ioctl: OK
fgetxattr: OK
mmap: OK

The following calls should succeed:
fstat: OK
fstatfs: OK
fcntl_dup: OK
fcntl_getfl: OK
fcntl_setfl: OK
fcntl_getfd: OK
fcntl_setfd: OK
close: OK

o_path_fifo_test.c (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] Fix the O_PATH support for FIFOs

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

> Commit aa55d22c, "Cygwin: honor the O_PATH flag when opening a FIFO",
> fixed a hang but otherwise didn't accomplish the purpose of the O_PATH
> flag as stated in the Linux man page for open(2):
>
>     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.
>
>     [The man page goes on to describe operations that *can* be
>     performed: close(2), fchdir(2), fstat(2),....]
>
>     Opening a file or directory with the O_PATH flag requires no
>     permissions on the object itself (but does require execute
>     permission on the directories in the path prefix).
>
> The first problem in the current implementation is that if open(2) is
> called on a FIFO, fhandler_base::device_access_denied is called and
> tries to open the FIFO with read access, which isn't supposed to be
> required.  This is fixed by the first patch in this series.
>
> The second patch makes fhandler_fifo::open call fhandler_base::open_fs
> if O_PATH is set, so that we actually obtain a handle that can be used
> for the purposes stated above.
>
> The third page tweaks fhandler_fifo::fcntl and fhandler_fifo::dup so
> that they work with O_PATH.
>
> In a followup email I'll provide the program I used to test this
> implementation.
>
> Ken Brown (3):
>   Cygwin: device_access_denied: return false if O_PATH is set
>   Cygwin: re-implement fhandler_fifo::open with O_PATH
>   Cygwin: FIFO: tweak fcntl and dup when O_PATH is set
>
>  winsup/cygwin/fhandler.cc      |  3 +++
>  winsup/cygwin/fhandler_fifo.cc | 15 ++++++---------
>  2 files changed, 9 insertions(+), 9 deletions(-)
>
> --
> 2.21.0
Pushed.  I'll create new developer snapshots.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment