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

[marcandre . lureau]

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. 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.

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

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().

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);
         }
@@ -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);
     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);
+        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;
+        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;
+    }
+
+    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()! */
+    ret = closesocket(s);
     if (ret < 0) {
         errno = socket_error();
     }
+
     return ret;
 }
 
@@ -400,7 +471,14 @@ int qemu_getsockopt_wrap(int sockfd, int level, int 
optname,
                          void *optval, socklen_t *optlen)
 {
     int ret;
-    ret = getsockopt(sockfd, level, optname, optval, optlen);
+    SOCKET s = _get_osfhandle(sockfd);
+
+    if (s == -1) {
+        errno = EINVAL;
+        return -1;
+    }
+
+    ret = getsockopt(s, level, optname, optval, optlen);
     if (ret < 0) {
         errno = socket_error();
     }
@@ -413,7 +491,13 @@ int qemu_setsockopt_wrap(int sockfd, int level, int 
optname,
                          const void *optval, socklen_t optlen)
 {
     int ret;
-    ret = setsockopt(sockfd, level, optname, optval, optlen);
+    SOCKET s = _get_osfhandle(sockfd);
+
+    if (s == -1) {
+        errno = EINVAL;
+        return -1;
+    }
+    ret = setsockopt(s, level, optname, optval, optlen);
     if (ret < 0) {
         errno = socket_error();
     }
@@ -426,7 +510,13 @@ int qemu_getpeername_wrap(int sockfd, struct sockaddr 
*addr,
                           socklen_t *addrlen)
 {
     int ret;
-    ret = getpeername(sockfd, addr, addrlen);
+    SOCKET s = _get_osfhandle(sockfd);
+
+    if (s == -1) {
+        errno = EINVAL;
+        return -1;
+    }
+    ret = getpeername(s, addr, addrlen);
     if (ret < 0) {
         errno = socket_error();
     }
@@ -439,7 +529,13 @@ int qemu_getsockname_wrap(int sockfd, struct sockaddr 
*addr,
                           socklen_t *addrlen)
 {
     int ret;
-    ret = getsockname(sockfd, addr, addrlen);
+    SOCKET s = _get_osfhandle(sockfd);
+
+    if (s == -1) {
+        errno = EINVAL;
+        return -1;
+    }
+    ret = getsockname(s, addr, addrlen);
     if (ret < 0) {
         errno = socket_error();
     }
@@ -451,7 +547,13 @@ int qemu_getsockname_wrap(int sockfd, struct sockaddr 
*addr,
 ssize_t qemu_send_wrap(int sockfd, const void *buf, size_t len, int flags)
 {
     int ret;
-    ret = send(sockfd, buf, len, flags);
+    SOCKET s = _get_osfhandle(sockfd);
+
+    if (s == -1) {
+        errno = EINVAL;
+        return -1;
+    }
+    ret = send(s, buf, len, flags);
     if (ret < 0) {
         errno = socket_error();
     }
@@ -464,7 +566,13 @@ ssize_t qemu_sendto_wrap(int sockfd, const void *buf, 
size_t len, int flags,
                          const struct sockaddr *addr, socklen_t addrlen)
 {
     int ret;
-    ret = sendto(sockfd, buf, len, flags, addr, addrlen);
+    SOCKET s = _get_osfhandle(sockfd);
+
+    if (s == -1) {
+        errno = EINVAL;
+        return -1;
+    }
+    ret = sendto(s, buf, len, flags, addr, addrlen);
     if (ret < 0) {
         errno = socket_error();
     }
@@ -476,7 +584,13 @@ ssize_t qemu_sendto_wrap(int sockfd, const void *buf, 
size_t len, int flags,
 ssize_t qemu_recv_wrap(int sockfd, void *buf, size_t len, int flags)
 {
     int ret;
-    ret = recv(sockfd, buf, len, flags);
+    SOCKET s = _get_osfhandle(sockfd);
+
+    if (s == -1) {
+        errno = EINVAL;
+        return -1;
+    }
+    ret = recv(s, buf, len, flags);
     if (ret < 0) {
         errno = socket_error();
     }
@@ -489,7 +603,13 @@ ssize_t qemu_recvfrom_wrap(int sockfd, void *buf, size_t 
len, int flags,
                            struct sockaddr *addr, socklen_t *addrlen)
 {
     int ret;
-    ret = recvfrom(sockfd, buf, len, flags, addr, addrlen);
+    SOCKET s = _get_osfhandle(sockfd);
+
+    if (s == -1) {
+        errno = EINVAL;
+        return -1;
+    }
+    ret = recvfrom(s, buf, len, flags, addr, addrlen);
     if (ret < 0) {
         errno = socket_error();
     }
-- 
2.39.1