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
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 :|
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
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 :|
© 2016 - 2026 Red Hat, Inc.