Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Until now, a win32 SOCKET handle is often cast to an int file
> descriptor, as this is what other OS use for sockets.

Brief recap, to refamiliarize myself with the way this stuff works under
Windows:

1. Both POSIX and Windows use small integer file descriptors.

2. With POSIX, these are an OS thing.  With Windows, these are a CRT
   thing, wrapping a HANDLE, which is the OS thing.

3. A Windows HANDLE is to be treated as an abstract data type.

4. _get_osfhandle() returns a CRT file descriptor's HANDLE.

5. _open_osfhandle() creates a CRT file descriptor that wraps around a
   HANDLE.

6. Closing a CRT file descriptor also closes the wrapped HANDLE.

7. A Windows SOCKET is also a HANDLE.  Maybe.  I guess.  Docs are
   confusing.

8. There's merry confusion between int, intptr_t, HANDLE, SOCKET, and
   who knows what else.

>                                                       When necessary,
> QEMU eventually queries whether it's a socket with the help of
> fd_is_socket(). However, there is no guarantee of conflict between the
> fd and SOCKET space. Such conflict would have surprising consequences,
> we shouldn't mix them.

True.

However, if conflicts were an issue in practice, conflating the two
wouldn't be so common, don't you think?  File descriptors start at zero.
Perhaps SOCKETs are much bigger when interpreted as int?  Not really
relevant, because:

> Also, it is often forgotten that SOCKET must be closed with
> closesocket(), and not close().

Yup.  After the next patch, we don't have to remember anymore outside
oslib-win32.c, and that's a fairly compelling argument for this patch.

> Instead, let's make the win32 socket wrapper functions return and take a
> file descriptor, and let util/ wrappers do the fd/SOCKET conversion as
> necessary. A bit of adaptation is necessary in io/ as well.
>
> Unfortunately, we can't drop closesocket() usage, despite
> _open_osfhandle() documentation claiming transfer of ownership, testing
> shows bad behaviour if you forget to call closesocket().

I figure this refers to your patch to qemu_closesocket_wrap().  Correct?

What bad behavior did you observe in testing?

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  io/channel-socket.c |  18 +++--
>  io/channel-watch.c  |  17 +++--
>  util/oslib-win32.c  | 164 ++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 165 insertions(+), 34 deletions(-)
>
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 2040297d2b..18cc062431 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -426,6 +426,14 @@ static void qio_channel_socket_init(Object *obj)
>      ioc->fd = -1;
>  }
>  
> +static void wsa_event_clear(int sockfd)
> +{
> +#ifdef WIN32
> +    SOCKET s = _get_osfhandle(sockfd);
> +    WSAEventSelect(s, NULL, 0);
> +#endif
> +}
> +
>  static void qio_channel_socket_finalize(Object *obj)
>  {
>      QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
> @@ -441,9 +449,7 @@ static void qio_channel_socket_finalize(Object *obj)
>                  err = NULL;
>              }
>          }
> -#ifdef WIN32
> -        WSAEventSelect(ioc->fd, NULL, 0);
> -#endif
> +        wsa_event_clear(ioc->fd);
>          closesocket(ioc->fd);
>          ioc->fd = -1;
>      }
> @@ -845,9 +851,7 @@ qio_channel_socket_close(QIOChannel *ioc,
>      Error *err = NULL;
>  
>      if (sioc->fd != -1) {
> -#ifdef WIN32
> -        WSAEventSelect(sioc->fd, NULL, 0);
> -#endif
> +        wsa_event_clear(sioc->fd);
>          if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
>              socket_listen_cleanup(sioc->fd, errp);
>          }

Factoring out wsa_event_clear() could be a separate patch.  Observation,
not demand.

> @@ -899,7 +903,7 @@ static void 
> qio_channel_socket_set_aio_fd_handler(QIOChannel *ioc,
>                                                    void *opaque)
>  {
>      QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
> -    aio_set_fd_handler(ctx, sioc->fd, false,
> +    aio_set_fd_handler(ctx, _get_osfhandle(sioc->fd), false,
>                         io_read, io_write, NULL, NULL, opaque);
>  }
>  
> diff --git a/io/channel-watch.c b/io/channel-watch.c
> index ad7c568a84..8c1c24008f 100644
> --- a/io/channel-watch.c
> +++ b/io/channel-watch.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "io/channel-watch.h"
>  
>  typedef struct QIOChannelFDSource QIOChannelFDSource;
> @@ -275,15 +276,21 @@ GSource *qio_channel_create_fd_watch(QIOChannel *ioc,
>  
>  #ifdef CONFIG_WIN32
>  GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
> -                                         int socket,
> +                                         int sockfd,
>                                           GIOCondition condition)
>  {
> +    SOCKET s = _get_osfhandle(sockfd);

_get_osfhandle() returns a HANDLE as intptr_t.  Is a HANDLE that refers
to a socket also a SOCKET?  The docs I found so far are confusing...

>      GSource *source;
>      QIOChannelSocketSource *ssource;
>  
> -    WSAEventSelect(socket, ioc->event,
> -                   FD_READ | FD_ACCEPT | FD_CLOSE |
> -                   FD_CONNECT | FD_WRITE | FD_OOB);
> +    if (s == -1 ||
> +        WSAEventSelect(s, ioc->event,
> +                       FD_READ | FD_ACCEPT | FD_CLOSE |
> +                       FD_CONNECT | FD_WRITE | FD_OOB) == SOCKET_ERROR) {
> +        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
> +        error_printf("error creating socket watch: %s", emsg);

Uh, why is printing an error appropriate here?  Shouldn't we leave error
handling to callers?

Also, does GetLastError() do the right thing after _get_osfhandle()
failure?  _get_osfhandle() is documented to set errno...

> +        return NULL;
> +    }
>  
>      source = g_source_new(&qio_channel_socket_source_funcs,
>                            sizeof(QIOChannelSocketSource));
> @@ -293,7 +300,7 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
>      object_ref(OBJECT(ioc));
>  
>      ssource->condition = condition;
> -    ssource->socket = socket;
> +    ssource->socket = s;
>      ssource->revents = 0;
>  
>      ssource->fd.fd = (gintptr)ioc->event;
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 07ade41800..78fab521cf 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -180,7 +180,8 @@ static int socket_error(void)
>  void qemu_socket_set_block(int fd)
>  {
>      unsigned long opt = 0;
> -    WSAEventSelect(fd, NULL, 0);
> +    SOCKET s = _get_osfhandle(fd);
> +    WSAEventSelect(s, NULL, 0);
>      ioctlsocket(fd, FIONBIO, &opt);
>  }
>  
> @@ -297,7 +298,13 @@ int qemu_connect_wrap(int sockfd, const struct sockaddr 
> *addr,
>                        socklen_t addrlen)
>  {
>      int ret;
> -    ret = connect(sockfd, addr, addrlen);
> +    SOCKET s = _get_osfhandle(sockfd);
> +
> +    if (s == -1) {
> +        errno = EINVAL;

_get_osfhandle() is documented to set errno to EBADF in this case.  If
true, you change errno from EBADF to EINVAL.  Doesn't seem like an
improvement :)

More of the same below, not pointing it out there.

> +        return -1;
> +    }
> +    ret = connect(s, addr, addrlen);
>      if (ret < 0) {
>          if (WSAGetLastError() == WSAEWOULDBLOCK) {
>              errno = EINPROGRESS;
> @@ -313,7 +320,13 @@ int qemu_connect_wrap(int sockfd, const struct sockaddr 
> *addr,
>  int qemu_listen_wrap(int sockfd, int backlog)
>  {
>      int ret;
> -    ret = listen(sockfd, backlog);
> +    SOCKET s = _get_osfhandle(sockfd);
> +
> +    if (s == -1) {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +    ret = listen(s, backlog);
>      if (ret < 0) {
>          errno = socket_error();
>      }
> @@ -326,7 +339,13 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr 
> *addr,
>                     socklen_t addrlen)
>  {
>      int ret;
> -    ret = bind(sockfd, addr, addrlen);
> +    SOCKET s = _get_osfhandle(sockfd);
> +
> +    if (s == -1) {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +    ret = bind(s, addr, addrlen);
>      if (ret < 0) {
>          errno = socket_error();
>      }
> @@ -337,12 +356,22 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr 
> *addr,
>  #undef socket
>  int qemu_socket_wrap(int domain, int type, int protocol)
>  {
> -    int ret;
> -    ret = socket(domain, type, protocol);
> -    if (ret < 0) {
> +    SOCKET s;
> +    int fd;
> +
> +    s = socket(domain, type, protocol);
> +    if (s == -1) {
>          errno = socket_error();
> +        return -1;
>      }
> -    return ret;
> +
> +    fd = _open_osfhandle(s, _O_BINARY);
> +    if (fd < 0) {
> +        closesocket(s);
> +        errno = ENOMEM;

_open_osfhandle() is not documented to set errno, unlike
_get_osfhandle().  So, okay, I guess.

Similar uses below, also okay.

> +    }
> +
> +    return fd;
>  }
>  
>  
> @@ -350,10 +379,22 @@ int qemu_socket_wrap(int domain, int type, int protocol)
>  int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
>                       socklen_t *addrlen)
>  {
> -    int ret;
> -    ret = accept(sockfd, addr, addrlen);
> -    if (ret < 0) {
> +    int ret = -1;
> +    SOCKET s = _get_osfhandle(sockfd);
> +
> +    if (s == -1) {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +    s = accept(s, addr, addrlen);
> +    if (s == -1) {
>          errno = socket_error();
> +    } else {
> +        ret = _open_osfhandle(s, _O_BINARY);
> +        if (ret < 0) {
> +            closesocket(s);
> +            errno = ENOMEM;
> +        }
>      }
>      return ret;
>  }
> @@ -363,7 +404,13 @@ int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
>  int qemu_shutdown_wrap(int sockfd, int how)
>  {
>      int ret;
> -    ret = shutdown(sockfd, how);
> +    SOCKET s = _get_osfhandle(sockfd);
> +
> +    if (s == -1) {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +    ret = shutdown(s, how);
>      if (ret < 0) {
>          errno = socket_error();
>      }
> @@ -375,7 +422,13 @@ int qemu_shutdown_wrap(int sockfd, int how)
>  int qemu_ioctlsocket_wrap(int fd, int req, void *val)
>  {
>      int ret;
> -    ret = ioctlsocket(fd, req, val);
> +    SOCKET s = _get_osfhandle(fd);
> +
> +    if (s == -1) {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +    ret = ioctlsocket(s, req, val);
>      if (ret < 0) {
>          errno = socket_error();
>      }
> @@ -387,10 +440,28 @@ int qemu_ioctlsocket_wrap(int fd, int req, void *val)
>  int qemu_closesocket_wrap(int fd)
>  {
>      int ret;
> -    ret = closesocket(fd);
> +    SOCKET s = _get_osfhandle(fd);
> +
> +    if (s == -1) {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
> +    /*
> +     * close() must be called before closesocket(), otherwise close() 
> returns an
> +     * error and sets EBADF.
> +     */
> +    ret = close(fd);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* closesocket() is required, event after close()! */

As you mention in the commit message, this contradicts _open_osfhandle()
documentation, which claims close() is enough.  I think the comment here
should mention it, too.

Found in an old Stackoverflow reply:

    open() returns CRT file descriptors which is different from the
    Win32 handle. You can create a CRT file descriptor using
    _open_osfhandle(). But this is not recommened for sockets because
    you cannot close the file in a clean way. You either use close()
    which will leak the Winsock user-mode state, or closesocket() which
    will leak the CRT descriptor.

https://stackoverflow.com/questions/4676256/whats-the-difference-between-socket-and-handle-in-windows

How can we be sure this is not an issue here?

> +    ret = closesocket(s);
>      if (ret < 0) {
>          errno = socket_error();
>      }
> +
>      return ret;
>  }
>  

[...]