On Mon, 9 Dec 2024 13:13:00 +0100
Corinna Vinschen wrote:
> Hi Takashi,
>
> On Dec 8 16:43, Takashi Yano wrote:
> > Previous fhandler_base::fstat_helper() does not assume get_stat_handle()
> > returns NULL. Due to this, access() for network share which has not been
> > authenticated returns 0 (success). This patch add error handling to
> > fhandler_base::fstat_helper() for get_stat_handle() failure.
> >
> > Fixed: 5a0d1edba4b3 [...]
> ^^^^^
> Fixes
>
> > Reviewed-by:
> > Signed-off-by: Takashi Yano <[email protected]>
> > ---
> > winsup/cygwin/fhandler/disk_file.cc | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/winsup/cygwin/fhandler/disk_file.cc
> > b/winsup/cygwin/fhandler/disk_file.cc
> > index 2008fb61b..7c3c805fd 100644
> > --- a/winsup/cygwin/fhandler/disk_file.cc
> > +++ b/winsup/cygwin/fhandler/disk_file.cc
> > @@ -400,6 +400,11 @@ fhandler_base::fstat_helper (struct stat *buf)
> > IO_STATUS_BLOCK st;
> > FILE_COMPRESSION_INFORMATION fci;
> > HANDLE h = get_stat_handle ();
> > + if (h == NULL)
> > + {
> > + __seterrno ();
> > + return -1;
> > + }
> > PFILE_ALL_INFORMATION pfai = pc.fai ();
> > ULONG attributes = pc.file_attributes ();
>
> This introduces a regression from the user perspective.
>
> The underlying fstat functions were meant to return *something*, no
> matter how few information we got, as long as the file exists.
>
> The reason is, for example, that Windows disallows to fetch stat(2)
> information on files you don't have permissions on. For instance,
> pagefile.sys. On POSIX, you don't expect that stat(2) fails for these
> files, even if you can't access them in any other way.
>
> So prior to your patch, ls doesn't fail on pagefile.sys:
>
> $ ls -l /cygdrive/c/pagefile.sys
> -rw-r----- 1 Unknown+User Unknown+Group 2550136832 Dec 1 11:45
> /cygdrive/c/pagefile.sys
>
> The file exists, the stat(2) info is partially available.
>
> After your patch:
>
> $ ls -l /cygdrive/c/pagefile.sys
> ls: cannot access '/cygdrive/c/pagefile.sys': Device or resource busy
Indeed. This seems due to:
$ icacls 'c:\pagefile.sys'
c:\pagefile.sys: The process cannot access the file because it is being used by
another process.
Successfully processed 0 files; Failed processing 1 files
So, it looks very natural to me that stat() fails.
However, it is very annoying that ls /cygdrive/c shows a lot of errors.
$ ls /cygdrive/c
ls: cannot access '/cygdrive/c/Config.Msi': Permission denied
ls: cannot access '/cygdrive/c/DumpStack.log': Permission denied
ls: cannot access '/cygdrive/c/DumpStack.log.tmp': Device or resource busy
ls: cannot access '/cygdrive/c/hiberfil.sys': Device or resource busy
ls: cannot access '/cygdrive/c/pagefile.sys': Device or resource busy
ls: cannot access '/cygdrive/c/swapfile.sys': Device or resource busy
ls: cannot access '/cygdrive/c/System Volume Information': Permission denied
'$GetCurrent' AMD ESD
ProgramData Windows gtk-build vfcompat.dll
'$Recycle.Bin' BOOTNXT Microsoft
Qt appverifUI.dll hiberfil.sys
'$WINDOWS.~BT' Config.Msi 'NVIDIA
Corporation' Recovery bootmgr inetpub
'$WINRE_BACKUP_PARTITION.MARKER' 'Documents and Settings' PerfLogs
Symbols cygwin msys64
'$WinREAgent' DumpStack.log 'Program Files'
'System Volume Information' cygwin64 pagefile.sys
'$Windows.~WS' DumpStack.log.tmp 'Program Files
(x86)' Users etaxSign swapfile.sys
$
> Along these lines, if a share exists and is visible, stat(2) info should
> be available just the same as for pagefile.sys, even if you can't access
> the share otherwise.
This sounds reasonable, so I'd withdraw this patch.
Thanks!
--
Takashi Yano <[email protected]>
