[PATCH v2 0/3] Fix Spice regression on win32

[PATCH v2 0/3] Fix Spice regression on win32

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

v2: (suggestions from Daniel Berrange)
- change function qemu_close_socket_osfhandle()
- add some documentation for it
- simplify a bit the dbus-related code

Marc-André Lureau (3):
  win32: add qemu_close_socket_osfhandle()
  ui/spice: fix SOCKET handling regression
  ui/dbus: fix passing SOCKET to GSocket API & leak

 include/sysemu/os-win32.h | 15 ++++++--
 ui/dbus.c                 |  9 +++++
 ui/spice-core.c           | 29 +++++++++++++--
 util/oslib-win32.c        | 75 +++++++++++++++++++++------------------
 4 files changed, 89 insertions(+), 39 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] win32: add qemu_close_socket_osfhandle()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Close the given file descriptor, but returns the underlying SOCKET.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/os-win32.h | 15 ++++++--
 util/oslib-win32.c        | 75 +++++++++++++++++++++------------------
 2 files changed, 53 insertions(+), 37 deletions(-)

diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index e2849f88ab..15c296e0eb 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -171,10 +171,21 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
 
 bool qemu_socket_unselect(int sockfd, Error **errp);
 
-/* We wrap all the sockets functions so that we can
- * set errno based on WSAGetLastError()
+/* We wrap all the sockets functions so that we can set errno based on
+ * WSAGetLastError(), and use file-descriptors instead of SOCKET.
  */
 
+/*
+ * qemu_close_socket_osfhandle:
+ * @fd: a file descriptor associated with a SOCKET
+ *
+ * Close only the C run-time file descriptor, leave the SOCKET opened.
+ *
+ * Returns zero on success. On error, -1 is returned, and errno is set to
+ * indicate the error.
+ */
+int qemu_close_socket_osfhandle(int fd);
+
 #undef close
 #define close qemu_close_wrap
 int qemu_close_wrap(int fd);
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 16f8a67f7e..a98638729a 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -479,40 +479,27 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
     return ret;
 }
 
-
 #undef close
-int qemu_close_wrap(int fd)
+int qemu_close_socket_osfhandle(int fd)
 {
-    int ret;
+    SOCKET s = _get_osfhandle(fd);
     DWORD flags = 0;
-    SOCKET s = INVALID_SOCKET;
-
-    if (fd_is_socket(fd)) {
-        s = _get_osfhandle(fd);
-
-        /*
-         * If we were to just call _close on the descriptor, it would close the
-         * HANDLE, but it wouldn't free any of the resources associated to the
-         * SOCKET, and we can't call _close after calling closesocket, because
-         * closesocket has already closed the HANDLE, and _close would attempt to
-         * close the HANDLE again, resulting in a double free. We can however
-         * protect the HANDLE from actually being closed long enough to close the
-         * file descriptor, then close the socket itself.
-         */
-        if (!GetHandleInformation((HANDLE)s, &flags)) {
-            errno = EACCES;
-            return -1;
-        }
 
-        if (!SetHandleInformation((HANDLE)s, HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
-            errno = EACCES;
-            return -1;
-        }
+    /*
+     * If we were to just call _close on the descriptor, it would close the
+     * HANDLE, but it wouldn't free any of the resources associated to the
+     * SOCKET, and we can't call _close after calling closesocket, because
+     * closesocket has already closed the HANDLE, and _close would attempt to
+     * close the HANDLE again, resulting in a double free. We can however
+     * protect the HANDLE from actually being closed long enough to close the
+     * file descriptor, then close the socket itself.
+     */
+    if (!GetHandleInformation((HANDLE)s, &flags)) {
+        errno = EACCES;
+        return -1;
     }
 
-    ret = close(fd);
-
-    if (s != INVALID_SOCKET && !SetHandleInformation((HANDLE)s, flags, flags)) {
+    if (!SetHandleInformation((HANDLE)s, HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
         errno = EACCES;
         return -1;
     }
@@ -521,15 +508,33 @@ int qemu_close_wrap(int fd)
      * close() returns EBADF since we PROTECT_FROM_CLOSE the underlying handle,
      * but the FD is actually freed
      */
-    if (ret < 0 && (s == INVALID_SOCKET || errno != EBADF)) {
-        return ret;
+    if (close(fd) < 0 && errno != EBADF) {
+        return -1;
     }
 
-    if (s != INVALID_SOCKET) {
-        ret = closesocket(s);
-        if (ret < 0) {
-            errno = socket_error();
-        }
+    if (!SetHandleInformation((HANDLE)s, flags, flags)) {
+        errno = EACCES;
+        return -1;
+    }
+
+    return 0;
+}
+
+int qemu_close_wrap(int fd)
+{
+    SOCKET s = INVALID_SOCKET;
+    int ret = -1;
+
+    if (!fd_is_socket(fd)) {
+        return close(fd);
+    }
+
+    s = _get_osfhandle(fd);
+    qemu_close_socket_osfhandle(fd);
+
+    ret = closesocket(s);
+    if (ret < 0) {
+        errno = socket_error();
     }
 
     return ret;
-- 
2.39.2

[PATCH v2 2/3] ui/spice: fix SOCKET handling regression

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Spice uses SOCKET on win32, but QEMU now uses file-descriptors.

Fixes "8.0.0rc0 Regression: spicy windows doesn't open":
https://gitlab.com/qemu-project/qemu/-/issues/1549

Fixes: commit abe34282b ("win32: avoid mixing SOCKET and file descriptor space")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/spice-core.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index b05c830086..67cfd3ca9c 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -90,13 +90,23 @@ struct SpiceWatch {
 static void watch_read(void *opaque)
 {
     SpiceWatch *watch = opaque;
-    watch->func(watch->fd, SPICE_WATCH_EVENT_READ, watch->opaque);
+    int fd = watch->fd;
+
+#ifdef WIN32
+    fd = _get_osfhandle(fd);
+#endif
+    watch->func(fd, SPICE_WATCH_EVENT_READ, watch->opaque);
 }
 
 static void watch_write(void *opaque)
 {
     SpiceWatch *watch = opaque;
-    watch->func(watch->fd, SPICE_WATCH_EVENT_WRITE, watch->opaque);
+    int fd = watch->fd;
+
+#ifdef WIN32
+    fd = _get_osfhandle(fd);
+#endif
+    watch->func(fd, SPICE_WATCH_EVENT_WRITE, watch->opaque);
 }
 
 static void watch_update_mask(SpiceWatch *watch, int event_mask)
@@ -117,6 +127,14 @@ static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *
 {
     SpiceWatch *watch;
 
+#ifdef WIN32
+    fd = _open_osfhandle(fd, _O_BINARY);
+    if (fd < 0) {
+        error_setg_win32(&error_warn, WSAGetLastError(), "Couldn't associate a FD with the SOCKET");
+        return NULL;
+    }
+#endif
+
     watch = g_malloc0(sizeof(*watch));
     watch->fd     = fd;
     watch->func   = func;
@@ -129,6 +147,10 @@ static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *
 static void watch_remove(SpiceWatch *watch)
 {
     qemu_set_fd_handler(watch->fd, NULL, NULL, NULL);
+#ifdef WIN32
+    /* SOCKET is owned by spice */
+    qemu_close_to_socket(watch->fd);
+#endif
     g_free(watch);
 }
 
@@ -908,6 +930,9 @@ static int qemu_spice_set_pw_expire(time_t expires)
 
 static int qemu_spice_display_add_client(int csock, int skipauth, int tls)
 {
+#ifdef WIN32
+    csock = qemu_close_socket_osfhandle(csock);
+#endif
     if (tls) {
         return spice_server_add_ssl_client(spice_server, csock, skipauth);
     } else {
-- 
2.39.2

[PATCH v2 3/3] ui/dbus: fix passing SOCKET to GSocket API & leak

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

-display dbus is not currently available to win32 users, so it's not
considered a regression.

Note also the close() leak fix in case of error.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ui/dbus.c b/ui/dbus.c
index 0513de9918..b9e9698503 100644
--- a/ui/dbus.c
+++ b/ui/dbus.c
@@ -304,11 +304,20 @@ dbus_display_add_client(int csock, Error **errp)
         g_cancellable_cancel(dbus_display->add_client_cancellable);
     }
 
+#ifdef WIN32
+    socket = g_socket_new_from_fd(_get_osfhandle(csock), &err);
+#else
     socket = g_socket_new_from_fd(csock, &err);
+#endif
     if (!socket) {
         error_setg(errp, "Failed to setup D-Bus socket: %s", err->message);
+        close(csock);
         return false;
     }
+#ifdef WIN32
+    /* socket owns the SOCKET handle now, so release our osf handle */
+    qemu_close_socket_osfhandle(csock);
+#endif
 
     conn = g_socket_connection_factory_create_connection(socket);
 
-- 
2.39.2

Re: [PATCH v2 1/3] win32: add qemu_close_socket_osfhandle()

[Daniel P. Berrangé]

On Mon, Mar 20, 2023 at 05:36:41PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Close the given file descriptor, but returns the underlying SOCKET.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/sysemu/os-win32.h | 15 ++++++--
>  util/oslib-win32.c        | 75 +++++++++++++++++++++------------------
>  2 files changed, 53 insertions(+), 37 deletions(-)
> 
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index e2849f88ab..15c296e0eb 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -171,10 +171,21 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>  
>  bool qemu_socket_unselect(int sockfd, Error **errp);
>  
> -/* We wrap all the sockets functions so that we can
> - * set errno based on WSAGetLastError()
> +/* We wrap all the sockets functions so that we can set errno based on
> + * WSAGetLastError(), and use file-descriptors instead of SOCKET.
>   */
>  
> +/*
> + * qemu_close_socket_osfhandle:
> + * @fd: a file descriptor associated with a SOCKET
> + *
> + * Close only the C run-time file descriptor, leave the SOCKET opened.
> + *
> + * Returns zero on success. On error, -1 is returned, and errno is set to
> + * indicate the error.
> + */
> +int qemu_close_socket_osfhandle(int fd);
> +
>  #undef close
>  #define close qemu_close_wrap
>  int qemu_close_wrap(int fd);
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 16f8a67f7e..a98638729a 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -479,40 +479,27 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
>      return ret;
>  }
>  
> -
>  #undef close
> -int qemu_close_wrap(int fd)
> +int qemu_close_socket_osfhandle(int fd)
>  {
> -    int ret;
> +    SOCKET s = _get_osfhandle(fd);
>      DWORD flags = 0;
> -    SOCKET s = INVALID_SOCKET;
> -
> -    if (fd_is_socket(fd)) {
> -        s = _get_osfhandle(fd);
> -
> -        /*
> -         * If we were to just call _close on the descriptor, it would close the
> -         * HANDLE, but it wouldn't free any of the resources associated to the
> -         * SOCKET, and we can't call _close after calling closesocket, because
> -         * closesocket has already closed the HANDLE, and _close would attempt to
> -         * close the HANDLE again, resulting in a double free. We can however
> -         * protect the HANDLE from actually being closed long enough to close the
> -         * file descriptor, then close the socket itself.
> -         */
> -        if (!GetHandleInformation((HANDLE)s, &flags)) {
> -            errno = EACCES;
> -            return -1;
> -        }
>  
> -        if (!SetHandleInformation((HANDLE)s, HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
> -            errno = EACCES;
> -            return -1;
> -        }
> +    /*
> +     * If we were to just call _close on the descriptor, it would close the
> +     * HANDLE, but it wouldn't free any of the resources associated to the
> +     * SOCKET, and we can't call _close after calling closesocket, because
> +     * closesocket has already closed the HANDLE, and _close would attempt to
> +     * close the HANDLE again, resulting in a double free. We can however
> +     * protect the HANDLE from actually being closed long enough to close the
> +     * file descriptor, then close the socket itself.
> +     */
> +    if (!GetHandleInformation((HANDLE)s, &flags)) {
> +        errno = EACCES;
> +        return -1;
>      }
>  
> -    ret = close(fd);
> -
> -    if (s != INVALID_SOCKET && !SetHandleInformation((HANDLE)s, flags, flags)) {
> +    if (!SetHandleInformation((HANDLE)s, HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
>          errno = EACCES;
>          return -1;
>      }
> @@ -521,15 +508,33 @@ int qemu_close_wrap(int fd)
>       * close() returns EBADF since we PROTECT_FROM_CLOSE the underlying handle,
>       * but the FD is actually freed
>       */
> -    if (ret < 0 && (s == INVALID_SOCKET || errno != EBADF)) {
> -        return ret;
> +    if (close(fd) < 0 && errno != EBADF) {
> +        return -1;
>      }
>  
> -    if (s != INVALID_SOCKET) {
> -        ret = closesocket(s);
> -        if (ret < 0) {
> -            errno = socket_error();
> -        }
> +    if (!SetHandleInformation((HANDLE)s, flags, flags)) {
> +        errno = EACCES;
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +int qemu_close_wrap(int fd)
> +{
> +    SOCKET s = INVALID_SOCKET;
> +    int ret = -1;
> +
> +    if (!fd_is_socket(fd)) {
> +        return close(fd);
> +    }
> +
> +    s = _get_osfhandle(fd);
> +    qemu_close_socket_osfhandle(fd);
> +
> +    ret = closesocket(s);
> +    if (ret < 0) {
> +        errno = socket_error();
>      }

Shouldn't the closesocket() and return check be wrapped in

   if (s != INVALID_SOCKET) { .... }

as the old code had that conditional invokation of closesocket() ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 3/3] ui/dbus: fix passing SOCKET to GSocket API & leak

[Daniel P. Berrangé]

On Mon, Mar 20, 2023 at 05:36:43PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> -display dbus is not currently available to win32 users, so it's not
> considered a regression.
> 
> Note also the close() leak fix in case of error.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/dbus.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/3] win32: add qemu_close_socket_osfhandle()

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 5817 bytes --]

Hi

On Mon, Mar 20, 2023 at 5:42 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Mar 20, 2023 at 05:36:41PM +0400, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Close the given file descriptor, but returns the underlying SOCKET.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/sysemu/os-win32.h | 15 ++++++--
> >  util/oslib-win32.c        | 75 +++++++++++++++++++++------------------
> >  2 files changed, 53 insertions(+), 37 deletions(-)
> >
> > diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> > index e2849f88ab..15c296e0eb 100644
> > --- a/include/sysemu/os-win32.h
> > +++ b/include/sysemu/os-win32.h
> > @@ -171,10 +171,21 @@ bool qemu_socket_select(int sockfd, WSAEVENT
> hEventObject,
> >
> >  bool qemu_socket_unselect(int sockfd, Error **errp);
> >
> > -/* We wrap all the sockets functions so that we can
> > - * set errno based on WSAGetLastError()
> > +/* We wrap all the sockets functions so that we can set errno based on
> > + * WSAGetLastError(), and use file-descriptors instead of SOCKET.
> >   */
> >
> > +/*
> > + * qemu_close_socket_osfhandle:
> > + * @fd: a file descriptor associated with a SOCKET
> > + *
> > + * Close only the C run-time file descriptor, leave the SOCKET opened.
> > + *
> > + * Returns zero on success. On error, -1 is returned, and errno is set
> to
> > + * indicate the error.
> > + */
> > +int qemu_close_socket_osfhandle(int fd);
> > +
> >  #undef close
> >  #define close qemu_close_wrap
> >  int qemu_close_wrap(int fd);
> > diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> > index 16f8a67f7e..a98638729a 100644
> > --- a/util/oslib-win32.c
> > +++ b/util/oslib-win32.c
> > @@ -479,40 +479,27 @@ int qemu_bind_wrap(int sockfd, const struct
> sockaddr *addr,
> >      return ret;
> >  }
> >
> > -
> >  #undef close
> > -int qemu_close_wrap(int fd)
> > +int qemu_close_socket_osfhandle(int fd)
> >  {
> > -    int ret;
> > +    SOCKET s = _get_osfhandle(fd);
> >      DWORD flags = 0;
> > -    SOCKET s = INVALID_SOCKET;
> > -
> > -    if (fd_is_socket(fd)) {
> > -        s = _get_osfhandle(fd);
> > -
> > -        /*
> > -         * If we were to just call _close on the descriptor, it would
> close the
> > -         * HANDLE, but it wouldn't free any of the resources associated
> to the
> > -         * SOCKET, and we can't call _close after calling closesocket,
> because
> > -         * closesocket has already closed the HANDLE, and _close would
> attempt to
> > -         * close the HANDLE again, resulting in a double free. We can
> however
> > -         * protect the HANDLE from actually being closed long enough to
> close the
> > -         * file descriptor, then close the socket itself.
> > -         */
> > -        if (!GetHandleInformation((HANDLE)s, &flags)) {
> > -            errno = EACCES;
> > -            return -1;
> > -        }
> >
> > -        if (!SetHandleInformation((HANDLE)s,
> HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
> > -            errno = EACCES;
> > -            return -1;
> > -        }
> > +    /*
> > +     * If we were to just call _close on the descriptor, it would close
> the
> > +     * HANDLE, but it wouldn't free any of the resources associated to
> the
> > +     * SOCKET, and we can't call _close after calling closesocket,
> because
> > +     * closesocket has already closed the HANDLE, and _close would
> attempt to
> > +     * close the HANDLE again, resulting in a double free. We can
> however
> > +     * protect the HANDLE from actually being closed long enough to
> close the
> > +     * file descriptor, then close the socket itself.
> > +     */
> > +    if (!GetHandleInformation((HANDLE)s, &flags)) {
> > +        errno = EACCES;
> > +        return -1;
> >      }
> >
> > -    ret = close(fd);
> > -
> > -    if (s != INVALID_SOCKET && !SetHandleInformation((HANDLE)s, flags,
> flags)) {
> > +    if (!SetHandleInformation((HANDLE)s,
> HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
> >          errno = EACCES;
> >          return -1;
> >      }
> > @@ -521,15 +508,33 @@ int qemu_close_wrap(int fd)
> >       * close() returns EBADF since we PROTECT_FROM_CLOSE the underlying
> handle,
> >       * but the FD is actually freed
> >       */
> > -    if (ret < 0 && (s == INVALID_SOCKET || errno != EBADF)) {
> > -        return ret;
> > +    if (close(fd) < 0 && errno != EBADF) {
> > +        return -1;
> >      }
> >
> > -    if (s != INVALID_SOCKET) {
> > -        ret = closesocket(s);
> > -        if (ret < 0) {
> > -            errno = socket_error();
> > -        }
> > +    if (!SetHandleInformation((HANDLE)s, flags, flags)) {
> > +        errno = EACCES;
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +int qemu_close_wrap(int fd)
> > +{
> > +    SOCKET s = INVALID_SOCKET;
> > +    int ret = -1;
> > +
> > +    if (!fd_is_socket(fd)) {
> > +        return close(fd);
> > +    }
> > +
> > +    s = _get_osfhandle(fd);
> > +    qemu_close_socket_osfhandle(fd);
> > +
> > +    ret = closesocket(s);
> > +    if (ret < 0) {
> > +        errno = socket_error();
> >      }
>
> Shouldn't the closesocket() and return check be wrapped in
>
>    if (s != INVALID_SOCKET) { .... }
>
>
We shouldn't get there since fd_is_socket().


> as the old code had that conditional invokation of closesocket() ?
>

The v1 code could actually leak a SOCKET. This version should be a bit
better, if the FD is already closed for example, we still close the SOCKET.

Open to ideas to improve it.

thanks

[-- Attachment #2: Type: text/html, Size: 7553 bytes --]

Re: [PATCH v2 1/3] win32: add qemu_close_socket_osfhandle()

[Daniel P. Berrangé]

On Mon, Mar 20, 2023 at 05:46:01PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 20, 2023 at 5:42 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Mon, Mar 20, 2023 at 05:36:41PM +0400, marcandre.lureau@redhat.com
> > wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Close the given file descriptor, but returns the underlying SOCKET.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  include/sysemu/os-win32.h | 15 ++++++--
> > >  util/oslib-win32.c        | 75 +++++++++++++++++++++------------------
> > >  2 files changed, 53 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> > > index e2849f88ab..15c296e0eb 100644
> > > --- a/include/sysemu/os-win32.h
> > > +++ b/include/sysemu/os-win32.h
> > > @@ -171,10 +171,21 @@ bool qemu_socket_select(int sockfd, WSAEVENT
> > hEventObject,
> > >
> > >  bool qemu_socket_unselect(int sockfd, Error **errp);
> > >
> > > -/* We wrap all the sockets functions so that we can
> > > - * set errno based on WSAGetLastError()
> > > +/* We wrap all the sockets functions so that we can set errno based on
> > > + * WSAGetLastError(), and use file-descriptors instead of SOCKET.
> > >   */
> > >
> > > +/*
> > > + * qemu_close_socket_osfhandle:
> > > + * @fd: a file descriptor associated with a SOCKET
> > > + *
> > > + * Close only the C run-time file descriptor, leave the SOCKET opened.
> > > + *
> > > + * Returns zero on success. On error, -1 is returned, and errno is set
> > to
> > > + * indicate the error.
> > > + */
> > > +int qemu_close_socket_osfhandle(int fd);
> > > +
> > >  #undef close
> > >  #define close qemu_close_wrap
> > >  int qemu_close_wrap(int fd);
> > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> > > index 16f8a67f7e..a98638729a 100644
> > > --- a/util/oslib-win32.c
> > > +++ b/util/oslib-win32.c
> > > @@ -479,40 +479,27 @@ int qemu_bind_wrap(int sockfd, const struct
> > sockaddr *addr,
> > >      return ret;
> > >  }
> > >
> > > -
> > >  #undef close
> > > -int qemu_close_wrap(int fd)
> > > +int qemu_close_socket_osfhandle(int fd)
> > >  {
> > > -    int ret;
> > > +    SOCKET s = _get_osfhandle(fd);
> > >      DWORD flags = 0;
> > > -    SOCKET s = INVALID_SOCKET;
> > > -
> > > -    if (fd_is_socket(fd)) {
> > > -        s = _get_osfhandle(fd);
> > > -
> > > -        /*
> > > -         * If we were to just call _close on the descriptor, it would
> > close the
> > > -         * HANDLE, but it wouldn't free any of the resources associated
> > to the
> > > -         * SOCKET, and we can't call _close after calling closesocket,
> > because
> > > -         * closesocket has already closed the HANDLE, and _close would
> > attempt to
> > > -         * close the HANDLE again, resulting in a double free. We can
> > however
> > > -         * protect the HANDLE from actually being closed long enough to
> > close the
> > > -         * file descriptor, then close the socket itself.
> > > -         */
> > > -        if (!GetHandleInformation((HANDLE)s, &flags)) {
> > > -            errno = EACCES;
> > > -            return -1;
> > > -        }
> > >
> > > -        if (!SetHandleInformation((HANDLE)s,
> > HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
> > > -            errno = EACCES;
> > > -            return -1;
> > > -        }
> > > +    /*
> > > +     * If we were to just call _close on the descriptor, it would close
> > the
> > > +     * HANDLE, but it wouldn't free any of the resources associated to
> > the
> > > +     * SOCKET, and we can't call _close after calling closesocket,
> > because
> > > +     * closesocket has already closed the HANDLE, and _close would
> > attempt to
> > > +     * close the HANDLE again, resulting in a double free. We can
> > however
> > > +     * protect the HANDLE from actually being closed long enough to
> > close the
> > > +     * file descriptor, then close the socket itself.
> > > +     */
> > > +    if (!GetHandleInformation((HANDLE)s, &flags)) {
> > > +        errno = EACCES;
> > > +        return -1;
> > >      }
> > >
> > > -    ret = close(fd);
> > > -
> > > -    if (s != INVALID_SOCKET && !SetHandleInformation((HANDLE)s, flags,
> > flags)) {
> > > +    if (!SetHandleInformation((HANDLE)s,
> > HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
> > >          errno = EACCES;
> > >          return -1;
> > >      }
> > > @@ -521,15 +508,33 @@ int qemu_close_wrap(int fd)
> > >       * close() returns EBADF since we PROTECT_FROM_CLOSE the underlying
> > handle,
> > >       * but the FD is actually freed
> > >       */
> > > -    if (ret < 0 && (s == INVALID_SOCKET || errno != EBADF)) {
> > > -        return ret;
> > > +    if (close(fd) < 0 && errno != EBADF) {
> > > +        return -1;
> > >      }
> > >
> > > -    if (s != INVALID_SOCKET) {
> > > -        ret = closesocket(s);
> > > -        if (ret < 0) {
> > > -            errno = socket_error();
> > > -        }
> > > +    if (!SetHandleInformation((HANDLE)s, flags, flags)) {
> > > +        errno = EACCES;
> > > +        return -1;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +int qemu_close_wrap(int fd)
> > > +{
> > > +    SOCKET s = INVALID_SOCKET;
> > > +    int ret = -1;
> > > +
> > > +    if (!fd_is_socket(fd)) {
> > > +        return close(fd);
> > > +    }
> > > +
> > > +    s = _get_osfhandle(fd);
> > > +    qemu_close_socket_osfhandle(fd);
> > > +
> > > +    ret = closesocket(s);
> > > +    if (ret < 0) {
> > > +        errno = socket_error();
> > >      }
> >
> > Shouldn't the closesocket() and return check be wrapped in
> >
> >    if (s != INVALID_SOCKET) { .... }
> >
> >
> We shouldn't get there since fd_is_socket().

Oh right, yes,  fd_is_socket() will evaluate false if the 'fd' is
invalid. eg qemu_clsoe_wrap(-1)

> > as the old code had that conditional invokation of closesocket() ?
> >
> 
> The v1 code could actually leak a SOCKET. This version should be a bit
> better, if the FD is already closed for example, we still close the SOCKET.
> 
> Open to ideas to improve it.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|