Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

On Feb 21 13:04, Ken Brown via Cygwin-developers wrote:
> On 2/20/2021 6:51 PM, Ken Brown via Cygwin-developers wrote:
> > On 2/20/2021 6:32 PM, Ken Brown via Cygwin-developers wrote:
> > > fhandler_socket_local::fstat and many similar methods seem to assume
> > > that the fhandler is based on a socket *file*.  When this is not the
> > > case, they end up using handles inappropriately.  Consider the
> > > following test case, for example.
> > > 
> > > $ cat socket_stat.c
> > > [...]
> > > I haven't been able to find any documentation about what fstat(2)
> > > should do on a socket descriptor, so maybe the difference between
> > > Cygwin and Linux is unimportant.  But the code path followed on
> > > Cygwin definitely seems wrong. fhandler_socket_local::fstat calls
> > > fstat_fs which ends up using the io_handle as though it were a file
> > > handle.

I see what you mean.  This was clearly fat-fingered, probably after
trying to add abstract sockets.

> It seems to me that fstat_fs should only be called if we
> > > have a path_conv handle (which is the case if the fstat call
> > > resulted from a stat call) or if the socket descriptor came from
> > > open with O_PATH set.
> > > 
> > > Similar remarks apply to fstatfvs, fchmod,....  In some of these
> > > cases, like fchmod, POSIX explicitly states that the behavior is
> > > undefined, so maybe we should just fail when there's no underlying
> > > socket file.
> > 
> > Maybe the answer in all these cases is just to revert to the
> > fhandler_socket methods when our fhandler_socket_local is not based on a
> > socket file.
> 
> I think the following should do the job for fstat:
> 
> --- a/winsup/cygwin/fhandler_socket_local.cc
> +++ b/winsup/cygwin/fhandler_socket_local.cc
> @@ -673,11 +673,11 @@ fhandler_socket_local::fcntl (int cmd, intptr_t arg)
>  int __reg2
>  fhandler_socket_local::fstat (struct stat *buf)
>  {
> -  int res;
> +  if (!pc.handle () && !(get_flags () & O_PATH))
> +    /* fstat is not being called on a socket file. */

The comment should probably read "fstat called on a socket"...

> +    return fhandler_socket::fstat (buf);

...plus adding a comment right here:

     /* stat/lstat on a socket file or fstat on a socket opened w/ O_PATH */

> -  if (get_sun_path () && get_sun_path ()[0] == '\0')
> -    return fhandler_socket_wsock::fstat (buf);
> -  res = fhandler_base::fstat_fs (buf);
> +  int res = fhandler_base::fstat_fs (buf);
>    if (!res)
>      {
>        buf->st_mode = (buf->st_mode & ~S_IFMT) | S_IFSOCK;
> 
> With this patch, the output of my testcase looks like this:
> 
> File type:                socket
> I-node number:            6
> dev:                      1966080
> rdev:                     1966200
> Mode:                     140777 (octal)
> Link count:               1
> Ownership:                UID=197609   GID=197121
> Preferred I/O block size: 65536 bytes
> File size:                0 bytes
> Blocks allocated:         0
> Last status change:       Thu Nov 30 19:00:00 2006
> Last file access:         Sun Feb 21 13:03:03 2021
> Last file modification:   Sun Feb 21 13:03:03 2021

Might be a good idea to check the output twice in your testcase, adding
a second fstat call after calling bind, to see if it's still the same
after binding the socket to a file.

> If this patch looks right, I'll look at the other system calls too.

Yeah, I think you're right.  Thanks!


Corinna