Re: [PATCH v3 16/16] win32: replace closesocket() with close() wrapper

[Stefan Berger <stefanb@linux.ibm.com>]

On 2/21/23 07:48, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Use a close() wrapper instead, so that we don't need to worry about
> closesocket() vs close() anymore, let's hope.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 7836fb0be3..29a667ae3d 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -370,39 +370,39 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
>   }
> 
> 
> -#undef closesocket
> -int qemu_closesocket_wrap(int fd)
> +#undef close
> +int qemu_close_wrap(int fd)
>   {
>       int ret;
>       DWORD flags = 0;
> -    SOCKET s = _get_osfhandle(fd);
> +    SOCKET s = INVALID_SOCKET;
> 
> -    if (s == INVALID_SOCKET) {
> -        return -1;
> -    }
> +    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 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 (!SetHandleInformation((HANDLE)s, HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
> +            errno = EACCES;
> +            return -1;
> +        }
>       }
> 
>       ret = close(fd);
> 
> -    if (!SetHandleInformation((HANDLE)s, flags, flags)) {
> +    if (s != INVALID_SOCKET && !SetHandleInformation((HANDLE)s, flags, flags)) {
>           errno = EACCES;
>           return -1;
>       }
> @@ -411,13 +411,15 @@ int qemu_closesocket_wrap(int fd)
>        * close() returns EBADF since we PROTECT_FROM_CLOSE the underlying handle,
>        * but the FD is actually freed
>        */
> -    if (ret < 0 && errno != EBADF) {
> +    if (ret < 0 && (s == INVALID_SOCKET || errno != EBADF)) {
>           return ret;
>       }
> 
> -    ret = closesocket(s);
> -    if (ret < 0) {
> -        errno = socket_error();
> +    if (s != INVALID_SOCKET) {
> +        ret = closesocket(s);
> +        if (ret < 0) {
> +            errno = socket_error();
> +        }
>       }


if (fs_is_socket()) {{
     ...
     close()
     ...
     closesocket()
     ...
} else {
     ...
     close()
     ...
}

would avoid the threading and make this function look a bit simpler.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>