| Message ID | 20230320111412.1516419-2-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix Spice regression on win32 | expand |
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
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) {