[PATCH 0/3] Some O_PATH fixes

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

[PATCH 0/3] Some O_PATH fixes

Ken Brown-6
Ken Brown (3):
  Cygwin: fhandler_base::fstat_fs: accomodate the O_PATH flag
  Cygwin: fhandler_disk_file::fstatvfs: refactor
  Cygwin: FIFO::fstatvfs: use our handle if O_PATH is set

 winsup/cygwin/fhandler.h            |  1 +
 winsup/cygwin/fhandler_disk_file.cc | 24 +++++++++++++++++-------
 winsup/cygwin/fhandler_fifo.cc      |  8 ++++++++
 3 files changed, 26 insertions(+), 7 deletions(-)

--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3] Cygwin: fhandler_base::fstat_fs: accomodate the O_PATH flag

Ken Brown-6
Treat a special file opened with O_PATH the same as a regular file,
i.e., use its handle to get the stat information.

Before this change, fstat_fs opened the file a second time, with the
wrong flags and without closing the existing handle.  A side effect
was to change the openflags of the file, possibly causing further
system calls to fail.

Currently this change only affects FIFOs, but it will affect
AF_LOCAL/AF_UNIX sockets too once they support O_PATH.
---
 winsup/cygwin/fhandler_disk_file.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
index 32381a0b0..a1ab2bbdd 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -359,7 +359,7 @@ fhandler_base::fstat_fs (struct stat *buf)
 
   if (get_stat_handle ())
     {
-      if (!nohandle () && !is_fs_special ())
+      if (!nohandle () && (!is_fs_special () || get_flags () & O_PATH))
  res = pc.fs_is_nfs () ? fstat_by_nfs_ea (buf) : fstat_by_handle (buf);
       if (res)
  res = fstat_by_name (buf);
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3] Cygwin: fhandler_disk_file::fstatvfs: refactor

Ken Brown-6
In reply to this post by Ken Brown-6
Define a new method fhandler_base::fstatvfs_by_handle, extracted from
fhandler_disk_file::fstatvfs, which gets the statvfs information when
a handle is available.

This will be used in future commits for special files that have been
opened with O_PATH.
---
 winsup/cygwin/fhandler.h            |  1 +
 winsup/cygwin/fhandler_disk_file.cc | 22 ++++++++++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index c0d56b4da..5fa720a83 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -381,6 +381,7 @@ private:
   int __reg2 fstat_by_name (struct stat *buf);
 public:
   virtual int __reg2 fstatvfs (struct statvfs *buf);
+  int __reg2 fstatvfs_by_handle (HANDLE h, struct statvfs *buf);
   int __reg2 utimens_fs (const struct timespec *);
   virtual int __reg1 fchmod (mode_t mode);
   virtual int __reg2 fchown (uid_t uid, gid_t gid);
diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
index a1ab2bbdd..89e651029 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -600,9 +600,7 @@ int __reg2
 fhandler_disk_file::fstatvfs (struct statvfs *sfs)
 {
   int ret = -1, opened = 0;
-  NTSTATUS status;
   IO_STATUS_BLOCK io;
-  FILE_FS_FULL_SIZE_INFORMATION full_fsi;
   /* We must not use the stat handle here, even if it exists.  The handle
      has been opened with FILE_OPEN_REPARSE_POINT, thus, in case of a volume
      mount point, it points to the FS of the mount point, rather than to the
@@ -630,6 +628,22 @@ fhandler_disk_file::fstatvfs (struct statvfs *sfs)
  }
     }
 
+  ret = fstatvfs_by_handle (fh, sfs);
+out:
+  if (opened)
+    NtClose (fh);
+  syscall_printf ("%d = fstatvfs(%s, %p)", ret, get_name (), sfs);
+  return ret;
+}
+
+int __reg2
+fhandler_base::fstatvfs_by_handle (HANDLE fh, struct statvfs *sfs)
+{
+  int ret = -1;
+  NTSTATUS status;
+  IO_STATUS_BLOCK io;
+  FILE_FS_FULL_SIZE_INFORMATION full_fsi;
+
   sfs->f_files = ULONG_MAX;
   sfs->f_ffree = ULONG_MAX;
   sfs->f_favail = ULONG_MAX;
@@ -688,10 +702,6 @@ fhandler_disk_file::fstatvfs (struct statvfs *sfs)
     debug_printf ("%y = NtQueryVolumeInformationFile"
   "(%S, FileFsFullSizeInformation)",
   status, pc.get_nt_native_path ());
-out:
-  if (opened)
-    NtClose (fh);
-  syscall_printf ("%d = fstatvfs(%s, %p)", ret, get_name (), sfs);
   return ret;
 }
 
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] Cygwin: FIFO::fstatvfs: use our handle if O_PATH is set

Ken Brown-6
In reply to this post by Ken Brown-6
If O_PATH is set, then the fhandler_fifo object has a handle that can
be used for getting the statvfs information.  Use it by calling
fhandler_base::fstatvfs_by_handle.  Before this change,
fhandler_disk_file::fstatfvs was called on a new fhandler_disk object,
which would then have to be opened.
---
 winsup/cygwin/fhandler_fifo.cc | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index a338f12cc..ef568f6fe 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -906,6 +906,14 @@ errout:
 int __reg2
 fhandler_fifo::fstatvfs (struct statvfs *sfs)
 {
+  if (get_flags () & O_PATH)
+    /* We already have a handle. */
+    {
+      HANDLE h = get_handle ();
+      if (h)
+ return fstatvfs_by_handle (h, sfs);
+    }
+
   fhandler_disk_file fh (pc);
   fh.get_device () = FH_FS;
   return fh.fstatvfs (sfs);
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] Some O_PATH fixes

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

> Ken Brown (3):
>   Cygwin: fhandler_base::fstat_fs: accomodate the O_PATH flag
>   Cygwin: fhandler_disk_file::fstatvfs: refactor
>   Cygwin: FIFO::fstatvfs: use our handle if O_PATH is set
>
>  winsup/cygwin/fhandler.h            |  1 +
>  winsup/cygwin/fhandler_disk_file.cc | 24 +++++++++++++++++-------
>  winsup/cygwin/fhandler_fifo.cc      |  8 ++++++++
>  3 files changed, 26 insertions(+), 7 deletions(-)
>
> --
> 2.21.0
Patches are looking good to me.

As outlined on IRC, I found a problem with the ACLs created on new
FIFOs and frixed that (I think).  However, Cygwin doesn't actually
return the real permissions in stat(), only the constant perms 0666,
kind of like for symlinks.  I didn't have time to look into that yet,
but it would be great if we could fix that, too.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

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

Re: [PATCH 0/3] Some O_PATH fixes

Ken Brown-6
On 1/28/2020 12:06 PM, Corinna Vinschen wrote:

> On Jan 27 13:21, Ken Brown wrote:
>> Ken Brown (3):
>>    Cygwin: fhandler_base::fstat_fs: accomodate the O_PATH flag
>>    Cygwin: fhandler_disk_file::fstatvfs: refactor
>>    Cygwin: FIFO::fstatvfs: use our handle if O_PATH is set
>>
>>   winsup/cygwin/fhandler.h            |  1 +
>>   winsup/cygwin/fhandler_disk_file.cc | 24 +++++++++++++++++-------
>>   winsup/cygwin/fhandler_fifo.cc      |  8 ++++++++
>>   3 files changed, 26 insertions(+), 7 deletions(-)
>>
>> --
>> 2.21.0
>
> Patches are looking good to me.

OK, I'll push them.

> As outlined on IRC, I found a problem with the ACLs created on new
> FIFOs and frixed that (I think).  However, Cygwin doesn't actually
> return the real permissions in stat(), only the constant perms 0666,
> kind of like for symlinks.  I didn't have time to look into that yet,
> but it would be great if we could fix that, too.

I'll take a look if you don't get to it first.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] Some O_PATH fixes

Ken Brown-6
On 1/28/2020 2:01 PM, Ken Brown wrote:

> On 1/28/2020 12:06 PM, Corinna Vinschen wrote:
>> On Jan 27 13:21, Ken Brown wrote:
>>> Ken Brown (3):
>>>     Cygwin: fhandler_base::fstat_fs: accomodate the O_PATH flag
>>>     Cygwin: fhandler_disk_file::fstatvfs: refactor
>>>     Cygwin: FIFO::fstatvfs: use our handle if O_PATH is set
>>>
>>>    winsup/cygwin/fhandler.h            |  1 +
>>>    winsup/cygwin/fhandler_disk_file.cc | 24 +++++++++++++++++-------
>>>    winsup/cygwin/fhandler_fifo.cc      |  8 ++++++++
>>>    3 files changed, 26 insertions(+), 7 deletions(-)
>>>
>>> --
>>> 2.21.0
>>
>> Patches are looking good to me.
>
> OK, I'll push them.
>
>> As outlined on IRC, I found a problem with the ACLs created on new
>> FIFOs and frixed that (I think).  However, Cygwin doesn't actually
>> return the real permissions in stat(), only the constant perms 0666,
>> kind of like for symlinks.  I didn't have time to look into that yet,
>> but it would be great if we could fix that, too.
>
> I'll take a look if you don't get to it first.

Two quick thoughts, and then I won't have time to think about this any more
until tomorrow:

First, I wonder why in fstat_fs we're not using the stat handle (i.e., pc.handle()).

Second, in the call to get_file_attribute in fstat_helper
(fhandler_disk_file.cc:478), why do we set the first argument to NULL instead of
using our handle?

In both cases I don't immediately see a connection with the permissions problem,
but it seems inefficient and makes the code confusing.  I might well be missing
something, however.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] Some O_PATH fixes

Ken Brown-6
On 1/28/2020 3:48 PM, Ken Brown wrote:

> On 1/28/2020 2:01 PM, Ken Brown wrote:
>> On 1/28/2020 12:06 PM, Corinna Vinschen wrote:
>>> On Jan 27 13:21, Ken Brown wrote:
>>>> Ken Brown (3):
>>>>      Cygwin: fhandler_base::fstat_fs: accomodate the O_PATH flag
>>>>      Cygwin: fhandler_disk_file::fstatvfs: refactor
>>>>      Cygwin: FIFO::fstatvfs: use our handle if O_PATH is set
>>>>
>>>>     winsup/cygwin/fhandler.h            |  1 +
>>>>     winsup/cygwin/fhandler_disk_file.cc | 24 +++++++++++++++++-------
>>>>     winsup/cygwin/fhandler_fifo.cc      |  8 ++++++++
>>>>     3 files changed, 26 insertions(+), 7 deletions(-)
>>>>
>>>> --
>>>> 2.21.0
>>>
>>> Patches are looking good to me.
>>
>> OK, I'll push them.
>>
>>> As outlined on IRC, I found a problem with the ACLs created on new
>>> FIFOs and frixed that (I think).  However, Cygwin doesn't actually
>>> return the real permissions in stat(), only the constant perms 0666,
>>> kind of like for symlinks.  I didn't have time to look into that yet,
>>> but it would be great if we could fix that, too.
>>
>> I'll take a look if you don't get to it first.
>
> Two quick thoughts, and then I won't have time to think about this any more
> until tomorrow:
>
> First, I wonder why in fstat_fs we're not using the stat handle (i.e., pc.handle()).

Ignore this.  I was confused.

> Second, in the call to get_file_attribute in fstat_helper
> (fhandler_disk_file.cc:478), why do we set the first argument to NULL instead of
> using our handle?
>
> In both cases I don't immediately see a connection with the permissions problem,
> but it seems inefficient and makes the code confusing.  I might well be missing
> something, however.
>
> Ken
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] Some O_PATH fixes

Corinna Vinschen-2
On Jan 29 03:08, Ken Brown wrote:

> On 1/28/2020 3:48 PM, Ken Brown wrote:
> > On 1/28/2020 2:01 PM, Ken Brown wrote:
> >> On 1/28/2020 12:06 PM, Corinna Vinschen wrote:
> >>> On Jan 27 13:21, Ken Brown wrote:
> >>>> Ken Brown (3):
> >>>>      Cygwin: fhandler_base::fstat_fs: accomodate the O_PATH flag
> >>>>      Cygwin: fhandler_disk_file::fstatvfs: refactor
> >>>>      Cygwin: FIFO::fstatvfs: use our handle if O_PATH is set
> >>>>
> >>>>     winsup/cygwin/fhandler.h            |  1 +
> >>>>     winsup/cygwin/fhandler_disk_file.cc | 24 +++++++++++++++++-------
> >>>>     winsup/cygwin/fhandler_fifo.cc      |  8 ++++++++
> >>>>     3 files changed, 26 insertions(+), 7 deletions(-)
> >>>>
> >>>> --
> >>>> 2.21.0
> >>>
> >>> Patches are looking good to me.
> >>
> >> OK, I'll push them.
> >>
> >>> As outlined on IRC, I found a problem with the ACLs created on new
> >>> FIFOs and frixed that (I think).  However, Cygwin doesn't actually
> >>> return the real permissions in stat(), only the constant perms 0666,
> >>> kind of like for symlinks.  I didn't have time to look into that yet,
> >>> but it would be great if we could fix that, too.
> >>
> >> I'll take a look if you don't get to it first.
> >
> > Two quick thoughts, and then I won't have time to think about this any more
> > until tomorrow:
> >
> > First, I wonder why in fstat_fs we're not using the stat handle (i.e., pc.handle()).
>
> Ignore this.  I was confused.
>
> > Second, in the call to get_file_attribute in fstat_helper
> > (fhandler_disk_file.cc:478), why do we set the first argument to NULL instead of
> > using our handle?
The handle is a pipe handle, not the file handle, and the permissions
on the pipe handle were not reflecting the permissions on the file.
The NULL pointer was trying to make sure that the file gets opened
for fetching the security descriptor in get_file_sd().


Corinna

--
Corinna Vinschen
Cygwin Maintainer

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

Re: [PATCH 0/3] Some O_PATH fixes

Corinna Vinschen-2
On Jan 29 10:52, Corinna Vinschen wrote:

> On Jan 29 03:08, Ken Brown wrote:
> > On 1/28/2020 3:48 PM, Ken Brown wrote:
> > > On 1/28/2020 2:01 PM, Ken Brown wrote:
> > >> On 1/28/2020 12:06 PM, Corinna Vinschen wrote:
> > >>> As outlined on IRC, I found a problem with the ACLs created on new
> > >>> FIFOs and frixed that (I think).  However, Cygwin doesn't actually
> > >>> return the real permissions in stat(), only the constant perms 0666,
> > >>> kind of like for symlinks.  I didn't have time to look into that yet,
> > >>> but it would be great if we could fix that, too.
> > >>
> > >> I'll take a look if you don't get to it first.
> > >
> > > Two quick thoughts, and then I won't have time to think about this any more
> > > until tomorrow:
> > >
> > > First, I wonder why in fstat_fs we're not using the stat handle (i.e., pc.handle()).
> >
> > Ignore this.  I was confused.
> >
> > > Second, in the call to get_file_attribute in fstat_helper
> > > (fhandler_disk_file.cc:478), why do we set the first argument to NULL instead of
> > > using our handle?
>
> The handle is a pipe handle, not the file handle, and the permissions
> on the pipe handle were not reflecting the permissions on the file.
> The NULL pointer was trying to make sure that the file gets opened
> for fetching the security descriptor in get_file_sd().
I pushed a fix for the permission problem, but I didn't touch the
get_file_attribute() call in fstat_helper.  If you think this can
be further streamlined, go ahead.


Corinna

--
Corinna Vinschen
Cygwin Maintainer

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

Re: [PATCH 0/3] Some O_PATH fixes

Ken Brown-6
On 1/29/2020 9:22 AM, Corinna Vinschen wrote:

> On Jan 29 10:52, Corinna Vinschen wrote:
>> On Jan 29 03:08, Ken Brown wrote:
>>> On 1/28/2020 3:48 PM, Ken Brown wrote:
>>>> On 1/28/2020 2:01 PM, Ken Brown wrote:
>>>>> On 1/28/2020 12:06 PM, Corinna Vinschen wrote:
>>>>>> As outlined on IRC, I found a problem with the ACLs created on new
>>>>>> FIFOs and frixed that (I think).  However, Cygwin doesn't actually
>>>>>> return the real permissions in stat(), only the constant perms 0666,
>>>>>> kind of like for symlinks.  I didn't have time to look into that yet,
>>>>>> but it would be great if we could fix that, too.
>>>>>
>>>>> I'll take a look if you don't get to it first.
>>>>
>>>> Two quick thoughts, and then I won't have time to think about this any more
>>>> until tomorrow:
>>>>
>>>> First, I wonder why in fstat_fs we're not using the stat handle (i.e., pc.handle()).
>>>
>>> Ignore this.  I was confused.
>>>
>>>> Second, in the call to get_file_attribute in fstat_helper
>>>> (fhandler_disk_file.cc:478), why do we set the first argument to NULL instead of
>>>> using our handle?
>>
>> The handle is a pipe handle, not the file handle, and the permissions
>> on the pipe handle were not reflecting the permissions on the file.
>> The NULL pointer was trying to make sure that the file gets opened
>> for fetching the security descriptor in get_file_sd().
>
> I pushed a fix for the permission problem, but I didn't touch the
> get_file_attribute() call in fstat_helper.  If you think this can
> be further streamlined, go ahead.

I'll take a closer look after I finish the O_PATH implementation for AF_LOCAL
sockets (almost done).

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] Some O_PATH fixes

Ken Brown-6
In reply to this post by Corinna Vinschen-2
On 1/29/2020 9:22 AM, Corinna Vinschen wrote:

> On Jan 29 10:52, Corinna Vinschen wrote:
>> On Jan 29 03:08, Ken Brown wrote:
>>> On 1/28/2020 3:48 PM, Ken Brown wrote:
>>>> On 1/28/2020 2:01 PM, Ken Brown wrote:
>>>>> On 1/28/2020 12:06 PM, Corinna Vinschen wrote:
>>>>>> As outlined on IRC, I found a problem with the ACLs created on new
>>>>>> FIFOs and frixed that (I think).  However, Cygwin doesn't actually
>>>>>> return the real permissions in stat(), only the constant perms 0666,
>>>>>> kind of like for symlinks.  I didn't have time to look into that yet,
>>>>>> but it would be great if we could fix that, too.
>>>>>
>>>>> I'll take a look if you don't get to it first.
>>>>
>>>> Two quick thoughts, and then I won't have time to think about this any more
>>>> until tomorrow:
>>>>
>>>> First, I wonder why in fstat_fs we're not using the stat handle (i.e., pc.handle()).
>>>
>>> Ignore this.  I was confused.
>>>
>>>> Second, in the call to get_file_attribute in fstat_helper
>>>> (fhandler_disk_file.cc:478), why do we set the first argument to NULL instead of
>>>> using our handle?
>>
>> The handle is a pipe handle, not the file handle, and the permissions
>> on the pipe handle were not reflecting the permissions on the file.
>> The NULL pointer was trying to make sure that the file gets opened
>> for fetching the security descriptor in get_file_sd().
>
> I pushed a fix for the permission problem, but I didn't touch the
> get_file_attribute() call in fstat_helper.  If you think this can
> be further streamlined, go ahead.

AFAICT, the handle returned by get_stat_handle() should always be pc.handle(),
not a pipe handle.  Patch on the way.

Ken