* [PATCH 1/3] win32: add qemu_close_to_socket()
2023-03-20 11:14 [PATCH 0/3] Fix Spice regression on win32 marcandre.lureau
@ 2023-03-20 11:14 ` marcandre.lureau
2023-03-20 12:10 ` Daniel P. Berrangé
2023-03-20 11:14 ` [PATCH 2/3] ui/spice: fix SOCKET handling regression marcandre.lureau
2023-03-20 11:14 ` [PATCH 3/3] ui/dbus: fix passing SOCKET to GSocket API & leak marcandre.lureau
2 siblings, 1 reply; 7+ messages in thread
From: marcandre.lureau @ 2023-03-20 11:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Stefan Weil, berrange, Gerd Hoffmann
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 | 1 +
util/oslib-win32.c | 68 +++++++++++++++++++++------------------
2 files changed, 38 insertions(+), 31 deletions(-)
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index e2849f88ab..95cba6b284 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -174,6 +174,7 @@ bool qemu_socket_unselect(int sockfd, Error **errp);
/* We wrap all the sockets functions so that we can
* set errno based on WSAGetLastError()
*/
+SOCKET qemu_close_to_socket(int fd);
#undef close
#define close qemu_close_wrap
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 16f8a67f7e..e37276b414 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -479,52 +479,58 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
return ret;
}
-
#undef close
-int qemu_close_wrap(int fd)
+SOCKET qemu_close_to_socket(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 INVALID_SOCKET;
}
- 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;
+ return INVALID_SOCKET;
}
/*
* 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 INVALID_SOCKET;
+ }
+
+ if (!SetHandleInformation((HANDLE)s, flags, flags)) {
+ errno = EACCES;
+ return INVALID_SOCKET;
+ }
+
+ return s;
+}
+
+int qemu_close_wrap(int fd)
+{
+ SOCKET s = INVALID_SOCKET;
+ int ret = -1;
+
+ if (!fd_is_socket(fd)) {
+ return close(fd);
}
+ s = qemu_close_to_socket(fd);
+
if (s != INVALID_SOCKET) {
ret = closesocket(s);
if (ret < 0) {
--
2.39.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] win32: add qemu_close_to_socket()
2023-03-20 11:14 ` [PATCH 1/3] win32: add qemu_close_to_socket() marcandre.lureau
@ 2023-03-20 12:10 ` Daniel P. Berrangé
0 siblings, 0 replies; 7+ messages in thread
From: Daniel P. Berrangé @ 2023-03-20 12:10 UTC (permalink / raw)
To: marcandre.lureau; +Cc: qemu-devel, Stefan Weil, Gerd Hoffmann
On Mon, Mar 20, 2023 at 03:14:10PM +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 | 1 +
> util/oslib-win32.c | 68 +++++++++++++++++++++------------------
> 2 files changed, 38 insertions(+), 31 deletions(-)
>
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index e2849f88ab..95cba6b284 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -174,6 +174,7 @@ bool qemu_socket_unselect(int sockfd, Error **errp);
> /* We wrap all the sockets functions so that we can
> * set errno based on WSAGetLastError()
> */
> +SOCKET qemu_close_to_socket(int fd);
Took me a while to understand what this function is actually doing.
IIUC, it assumes 'fd' was previously created by _open_osfhandle,
and close this OSF handle, leaving the SOCKET still open.
Could we call this one 'qemu_close_socket_osfhandle' and also
add a comment describing what this does.
>
> #undef close
> #define close qemu_close_wrap
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 16f8a67f7e..e37276b414 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -479,52 +479,58 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
> return ret;
> }
>
> -
> #undef close
> -int qemu_close_wrap(int fd)
> +SOCKET qemu_close_to_socket(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 INVALID_SOCKET;
> }
>
> - 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;
> + return INVALID_SOCKET;
> }
>
> /*
> * 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 INVALID_SOCKET;
> + }
> +
> + if (!SetHandleInformation((HANDLE)s, flags, flags)) {
> + errno = EACCES;
> + return INVALID_SOCKET;
> + }
> +
> + return s;
> +}
> +
> +int qemu_close_wrap(int fd)
> +{
> + SOCKET s = INVALID_SOCKET;
> + int ret = -1;
> +
> + if (!fd_is_socket(fd)) {
> + return close(fd);
> }
>
> + s = qemu_close_to_socket(fd);
> +
> if (s != INVALID_SOCKET) {
> ret = closesocket(s);
> if (ret < 0) {
> --
> 2.39.2
>
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 :|
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] ui/spice: fix SOCKET handling regression
2023-03-20 11:14 [PATCH 0/3] Fix Spice regression on win32 marcandre.lureau
2023-03-20 11:14 ` [PATCH 1/3] win32: add qemu_close_to_socket() marcandre.lureau
@ 2023-03-20 11:14 ` marcandre.lureau
2023-03-20 12:10 ` Daniel P. Berrangé
2023-03-20 11:14 ` [PATCH 3/3] ui/dbus: fix passing SOCKET to GSocket API & leak marcandre.lureau
2 siblings, 1 reply; 7+ messages in thread
From: marcandre.lureau @ 2023-03-20 11:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Stefan Weil, berrange, Gerd Hoffmann
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>
---
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..e84ebe4a3f 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_to_socket(csock);
+#endif
if (tls) {
return spice_server_add_ssl_client(spice_server, csock, skipauth);
} else {
--
2.39.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/3] ui/spice: fix SOCKET handling regression
2023-03-20 11:14 ` [PATCH 2/3] ui/spice: fix SOCKET handling regression marcandre.lureau
@ 2023-03-20 12:10 ` Daniel P. Berrangé
0 siblings, 0 replies; 7+ messages in thread
From: Daniel P. Berrangé @ 2023-03-20 12:10 UTC (permalink / raw)
To: marcandre.lureau; +Cc: qemu-devel, Stefan Weil, Gerd Hoffmann
On Mon, Mar 20, 2023 at 03:14:11PM +0400, marcandre.lureau@redhat.com wrote:
> 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>
> ---
> ui/spice-core.c | 29 +++++++++++++++++++++++++++--
> 1 file changed, 27 insertions(+), 2 deletions(-)
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 :|
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] ui/dbus: fix passing SOCKET to GSocket API & leak
2023-03-20 11:14 [PATCH 0/3] Fix Spice regression on win32 marcandre.lureau
2023-03-20 11:14 ` [PATCH 1/3] win32: add qemu_close_to_socket() marcandre.lureau
2023-03-20 11:14 ` [PATCH 2/3] ui/spice: fix SOCKET handling regression marcandre.lureau
@ 2023-03-20 11:14 ` marcandre.lureau
2023-03-20 12:17 ` Daniel P. Berrangé
2 siblings, 1 reply; 7+ messages in thread
From: marcandre.lureau @ 2023-03-20 11:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Stefan Weil, berrange, Gerd Hoffmann
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 | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/ui/dbus.c b/ui/dbus.c
index 0513de9918..5389ac493f 100644
--- a/ui/dbus.c
+++ b/ui/dbus.c
@@ -304,9 +304,17 @@ dbus_display_add_client(int csock, Error **errp)
g_cancellable_cancel(dbus_display->add_client_cancellable);
}
+#ifdef WIN32
+ csock = qemu_close_to_socket(csock);
+#endif
socket = g_socket_new_from_fd(csock, &err);
if (!socket) {
error_setg(errp, "Failed to setup D-Bus socket: %s", err->message);
+#ifdef WIN32
+ closesocket(csock);
+#else
+ close(csock);
+#endif
return false;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 3/3] ui/dbus: fix passing SOCKET to GSocket API & leak
2023-03-20 11:14 ` [PATCH 3/3] ui/dbus: fix passing SOCKET to GSocket API & leak marcandre.lureau
@ 2023-03-20 12:17 ` Daniel P. Berrangé
0 siblings, 0 replies; 7+ messages in thread
From: Daniel P. Berrangé @ 2023-03-20 12:17 UTC (permalink / raw)
To: marcandre.lureau; +Cc: qemu-devel, Stefan Weil, Gerd Hoffmann
On Mon, Mar 20, 2023 at 03:14:12PM +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 | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/ui/dbus.c b/ui/dbus.c
> index 0513de9918..5389ac493f 100644
> --- a/ui/dbus.c
> +++ b/ui/dbus.c
> @@ -304,9 +304,17 @@ dbus_display_add_client(int csock, Error **errp)
> g_cancellable_cancel(dbus_display->add_client_cancellable);
> }
>
> +#ifdef WIN32
> + csock = qemu_close_to_socket(csock);
> +#endif
Happens to work because 'int' and 'SOCKET' are the same, but I feel
like this is confusing to overload one variable for two different
purposes. This confusing code pattern is the only reason why the
method in patch 1 needs to return SOCKET.
> socket = g_socket_new_from_fd(csock, &err);
> if (!socket) {
> error_setg(errp, "Failed to setup D-Bus socket: %s", err->message);
> +#ifdef WIN32
> + closesocket(csock);
> +#else
> + close(csock);
> +#endif
IMHO it would be clearer to write it as
#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 ...
}
#ifdef WIN32
/* socket owns the SOCKET handle now, so release our osf handle */
qemu_close_socket_osfhandle(csock);
#endif
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 :|
^ permalink raw reply [flat|nested] 7+ messages in thread