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

[Daniel P . Berrangé]

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 :|