| Message ID | 20230320133643.1618437-2-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix Spice regression on win32 | expand |
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
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
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;