From: Marc-André Lureau <marcandre.lureau@redhat.com>
Until now, a win32 SOCKET handle is often cast to an int file
descriptor, as this is what other OS use for sockets. When necessary,
QEMU eventually queries whether it's a socket with the help of
fd_is_socket(). However, there is no guarantee of conflict between the
fd and SOCKET space. Such conflict would have surprising consequences,
we shouldn't mix them.
Also, it is often forgotten that SOCKET must be closed with
closesocket(), and not close().
Instead, let's make the win32 socket wrapper functions return and take a
file descriptor, and let util/ wrappers do the fd/SOCKET conversion as
necessary. A bit of adaptation is necessary in io/ as well.
Unfortunately, we can't drop closesocket() usage, despite
_open_osfhandle() documentation claiming transfer of ownership, testing
shows bad behaviour if you forget to call closesocket().
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
io/channel-socket.c | 18 +++--
io/channel-watch.c | 17 +++--
util/oslib-win32.c | 164 ++++++++++++++++++++++++++++++++++++++------
3 files changed, 165 insertions(+), 34 deletions(-)
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 2040297d2b..18cc062431 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -426,6 +426,14 @@ static void qio_channel_socket_init(Object *obj)
ioc->fd = -1;
}
+static void wsa_event_clear(int sockfd)
+{
+#ifdef WIN32
+ SOCKET s = _get_osfhandle(sockfd);
+ WSAEventSelect(s, NULL, 0);
+#endif
+}
+
static void qio_channel_socket_finalize(Object *obj)
{
QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
@@ -441,9 +449,7 @@ static void qio_channel_socket_finalize(Object *obj)
err = NULL;
}
}
-#ifdef WIN32
- WSAEventSelect(ioc->fd, NULL, 0);
-#endif
+ wsa_event_clear(ioc->fd);
closesocket(ioc->fd);
ioc->fd = -1;
}
@@ -845,9 +851,7 @@ qio_channel_socket_close(QIOChannel *ioc,
Error *err = NULL;
if (sioc->fd != -1) {
-#ifdef WIN32
- WSAEventSelect(sioc->fd, NULL, 0);
-#endif
+ wsa_event_clear(sioc->fd);
if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
socket_listen_cleanup(sioc->fd, errp);
}
@@ -899,7 +903,7 @@ static void qio_channel_socket_set_aio_fd_handler(QIOChannel *ioc,
void *opaque)
{
QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
- aio_set_fd_handler(ctx, sioc->fd, false,
+ aio_set_fd_handler(ctx, _get_osfhandle(sioc->fd), false,
io_read, io_write, NULL, NULL, opaque);
}
diff --git a/io/channel-watch.c b/io/channel-watch.c
index ad7c568a84..8c1c24008f 100644
--- a/io/channel-watch.c
+++ b/io/channel-watch.c
@@ -19,6 +19,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include "io/channel-watch.h"
typedef struct QIOChannelFDSource QIOChannelFDSource;
@@ -275,15 +276,21 @@ GSource *qio_channel_create_fd_watch(QIOChannel *ioc,
#ifdef CONFIG_WIN32
GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
- int socket,
+ int sockfd,
GIOCondition condition)
{
+ SOCKET s = _get_osfhandle(sockfd);
GSource *source;
QIOChannelSocketSource *ssource;
- WSAEventSelect(socket, ioc->event,
- FD_READ | FD_ACCEPT | FD_CLOSE |
- FD_CONNECT | FD_WRITE | FD_OOB);
+ if (s == -1 ||
+ WSAEventSelect(s, ioc->event,
+ FD_READ | FD_ACCEPT | FD_CLOSE |
+ FD_CONNECT | FD_WRITE | FD_OOB) == SOCKET_ERROR) {
+ g_autofree gchar *emsg = g_win32_error_message(GetLastError());
+ error_printf("error creating socket watch: %s", emsg);
+ return NULL;
+ }
source = g_source_new(&qio_channel_socket_source_funcs,
sizeof(QIOChannelSocketSource));
@@ -293,7 +300,7 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
object_ref(OBJECT(ioc));
ssource->condition = condition;
- ssource->socket = socket;
+ ssource->socket = s;
ssource->revents = 0;
ssource->fd.fd = (gintptr)ioc->event;
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 07ade41800..78fab521cf 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -180,7 +180,8 @@ static int socket_error(void)
void qemu_socket_set_block(int fd)
{
unsigned long opt = 0;
- WSAEventSelect(fd, NULL, 0);
+ SOCKET s = _get_osfhandle(fd);
+ WSAEventSelect(s, NULL, 0);
ioctlsocket(fd, FIONBIO, &opt);
}
@@ -297,7 +298,13 @@ int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
socklen_t addrlen)
{
int ret;
- ret = connect(sockfd, addr, addrlen);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = connect(s, addr, addrlen);
if (ret < 0) {
if (WSAGetLastError() == WSAEWOULDBLOCK) {
errno = EINPROGRESS;
@@ -313,7 +320,13 @@ int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
int qemu_listen_wrap(int sockfd, int backlog)
{
int ret;
- ret = listen(sockfd, backlog);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = listen(s, backlog);
if (ret < 0) {
errno = socket_error();
}
@@ -326,7 +339,13 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
socklen_t addrlen)
{
int ret;
- ret = bind(sockfd, addr, addrlen);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = bind(s, addr, addrlen);
if (ret < 0) {
errno = socket_error();
}
@@ -337,12 +356,22 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
#undef socket
int qemu_socket_wrap(int domain, int type, int protocol)
{
- int ret;
- ret = socket(domain, type, protocol);
- if (ret < 0) {
+ SOCKET s;
+ int fd;
+
+ s = socket(domain, type, protocol);
+ if (s == -1) {
errno = socket_error();
+ return -1;
}
- return ret;
+
+ fd = _open_osfhandle(s, _O_BINARY);
+ if (fd < 0) {
+ closesocket(s);
+ errno = ENOMEM;
+ }
+
+ return fd;
}
@@ -350,10 +379,22 @@ int qemu_socket_wrap(int domain, int type, int protocol)
int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
socklen_t *addrlen)
{
- int ret;
- ret = accept(sockfd, addr, addrlen);
- if (ret < 0) {
+ int ret = -1;
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ s = accept(s, addr, addrlen);
+ if (s == -1) {
errno = socket_error();
+ } else {
+ ret = _open_osfhandle(s, _O_BINARY);
+ if (ret < 0) {
+ closesocket(s);
+ errno = ENOMEM;
+ }
}
return ret;
}
@@ -363,7 +404,13 @@ int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
int qemu_shutdown_wrap(int sockfd, int how)
{
int ret;
- ret = shutdown(sockfd, how);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = shutdown(s, how);
if (ret < 0) {
errno = socket_error();
}
@@ -375,7 +422,13 @@ int qemu_shutdown_wrap(int sockfd, int how)
int qemu_ioctlsocket_wrap(int fd, int req, void *val)
{
int ret;
- ret = ioctlsocket(fd, req, val);
+ SOCKET s = _get_osfhandle(fd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = ioctlsocket(s, req, val);
if (ret < 0) {
errno = socket_error();
}
@@ -387,10 +440,28 @@ int qemu_ioctlsocket_wrap(int fd, int req, void *val)
int qemu_closesocket_wrap(int fd)
{
int ret;
- ret = closesocket(fd);
+ SOCKET s = _get_osfhandle(fd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ /*
+ * close() must be called before closesocket(), otherwise close() returns an
+ * error and sets EBADF.
+ */
+ ret = close(fd);
+ if (ret < 0) {
+ return ret;
+ }
+
+ /* closesocket() is required, event after close()! */
+ ret = closesocket(s);
if (ret < 0) {
errno = socket_error();
}
+
return ret;
}
@@ -400,7 +471,14 @@ int qemu_getsockopt_wrap(int sockfd, int level, int optname,
void *optval, socklen_t *optlen)
{
int ret;
- ret = getsockopt(sockfd, level, optname, optval, optlen);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ ret = getsockopt(s, level, optname, optval, optlen);
if (ret < 0) {
errno = socket_error();
}
@@ -413,7 +491,13 @@ int qemu_setsockopt_wrap(int sockfd, int level, int optname,
const void *optval, socklen_t optlen)
{
int ret;
- ret = setsockopt(sockfd, level, optname, optval, optlen);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = setsockopt(s, level, optname, optval, optlen);
if (ret < 0) {
errno = socket_error();
}
@@ -426,7 +510,13 @@ int qemu_getpeername_wrap(int sockfd, struct sockaddr *addr,
socklen_t *addrlen)
{
int ret;
- ret = getpeername(sockfd, addr, addrlen);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = getpeername(s, addr, addrlen);
if (ret < 0) {
errno = socket_error();
}
@@ -439,7 +529,13 @@ int qemu_getsockname_wrap(int sockfd, struct sockaddr *addr,
socklen_t *addrlen)
{
int ret;
- ret = getsockname(sockfd, addr, addrlen);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = getsockname(s, addr, addrlen);
if (ret < 0) {
errno = socket_error();
}
@@ -451,7 +547,13 @@ int qemu_getsockname_wrap(int sockfd, struct sockaddr *addr,
ssize_t qemu_send_wrap(int sockfd, const void *buf, size_t len, int flags)
{
int ret;
- ret = send(sockfd, buf, len, flags);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = send(s, buf, len, flags);
if (ret < 0) {
errno = socket_error();
}
@@ -464,7 +566,13 @@ ssize_t qemu_sendto_wrap(int sockfd, const void *buf, size_t len, int flags,
const struct sockaddr *addr, socklen_t addrlen)
{
int ret;
- ret = sendto(sockfd, buf, len, flags, addr, addrlen);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = sendto(s, buf, len, flags, addr, addrlen);
if (ret < 0) {
errno = socket_error();
}
@@ -476,7 +584,13 @@ ssize_t qemu_sendto_wrap(int sockfd, const void *buf, size_t len, int flags,
ssize_t qemu_recv_wrap(int sockfd, void *buf, size_t len, int flags)
{
int ret;
- ret = recv(sockfd, buf, len, flags);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = recv(s, buf, len, flags);
if (ret < 0) {
errno = socket_error();
}
@@ -489,7 +603,13 @@ ssize_t qemu_recvfrom_wrap(int sockfd, void *buf, size_t len, int flags,
struct sockaddr *addr, socklen_t *addrlen)
{
int ret;
- ret = recvfrom(sockfd, buf, len, flags, addr, addrlen);
+ SOCKET s = _get_osfhandle(sockfd);
+
+ if (s == -1) {
+ errno = EINVAL;
+ return -1;
+ }
+ ret = recvfrom(s, buf, len, flags, addr, addrlen);
if (ret < 0) {
errno = socket_error();
}
--
2.39.1
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Until now, a win32 SOCKET handle is often cast to an int file
> descriptor, as this is what other OS use for sockets.
Brief recap, to refamiliarize myself with the way this stuff works under
Windows:
1. Both POSIX and Windows use small integer file descriptors.
2. With POSIX, these are an OS thing. With Windows, these are a CRT
thing, wrapping a HANDLE, which is the OS thing.
3. A Windows HANDLE is to be treated as an abstract data type.
4. _get_osfhandle() returns a CRT file descriptor's HANDLE.
5. _open_osfhandle() creates a CRT file descriptor that wraps around a
HANDLE.
6. Closing a CRT file descriptor also closes the wrapped HANDLE.
7. A Windows SOCKET is also a HANDLE. Maybe. I guess. Docs are
confusing.
8. There's merry confusion between int, intptr_t, HANDLE, SOCKET, and
who knows what else.
> When necessary,
> QEMU eventually queries whether it's a socket with the help of
> fd_is_socket(). However, there is no guarantee of conflict between the
> fd and SOCKET space. Such conflict would have surprising consequences,
> we shouldn't mix them.
True.
However, if conflicts were an issue in practice, conflating the two
wouldn't be so common, don't you think? File descriptors start at zero.
Perhaps SOCKETs are much bigger when interpreted as int? Not really
relevant, because:
> Also, it is often forgotten that SOCKET must be closed with
> closesocket(), and not close().
Yup. After the next patch, we don't have to remember anymore outside
oslib-win32.c, and that's a fairly compelling argument for this patch.
> Instead, let's make the win32 socket wrapper functions return and take a
> file descriptor, and let util/ wrappers do the fd/SOCKET conversion as
> necessary. A bit of adaptation is necessary in io/ as well.
>
> Unfortunately, we can't drop closesocket() usage, despite
> _open_osfhandle() documentation claiming transfer of ownership, testing
> shows bad behaviour if you forget to call closesocket().
I figure this refers to your patch to qemu_closesocket_wrap(). Correct?
What bad behavior did you observe in testing?
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> io/channel-socket.c | 18 +++--
> io/channel-watch.c | 17 +++--
> util/oslib-win32.c | 164 ++++++++++++++++++++++++++++++++++++++------
> 3 files changed, 165 insertions(+), 34 deletions(-)
>
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 2040297d2b..18cc062431 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -426,6 +426,14 @@ static void qio_channel_socket_init(Object *obj)
> ioc->fd = -1;
> }
>
> +static void wsa_event_clear(int sockfd)
> +{
> +#ifdef WIN32
> + SOCKET s = _get_osfhandle(sockfd);
> + WSAEventSelect(s, NULL, 0);
> +#endif
> +}
> +
> static void qio_channel_socket_finalize(Object *obj)
> {
> QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
> @@ -441,9 +449,7 @@ static void qio_channel_socket_finalize(Object *obj)
> err = NULL;
> }
> }
> -#ifdef WIN32
> - WSAEventSelect(ioc->fd, NULL, 0);
> -#endif
> + wsa_event_clear(ioc->fd);
> closesocket(ioc->fd);
> ioc->fd = -1;
> }
> @@ -845,9 +851,7 @@ qio_channel_socket_close(QIOChannel *ioc,
> Error *err = NULL;
>
> if (sioc->fd != -1) {
> -#ifdef WIN32
> - WSAEventSelect(sioc->fd, NULL, 0);
> -#endif
> + wsa_event_clear(sioc->fd);
> if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
> socket_listen_cleanup(sioc->fd, errp);
> }
Factoring out wsa_event_clear() could be a separate patch. Observation,
not demand.
> @@ -899,7 +903,7 @@ static void qio_channel_socket_set_aio_fd_handler(QIOChannel *ioc,
> void *opaque)
> {
> QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
> - aio_set_fd_handler(ctx, sioc->fd, false,
> + aio_set_fd_handler(ctx, _get_osfhandle(sioc->fd), false,
> io_read, io_write, NULL, NULL, opaque);
> }
>
> diff --git a/io/channel-watch.c b/io/channel-watch.c
> index ad7c568a84..8c1c24008f 100644
> --- a/io/channel-watch.c
> +++ b/io/channel-watch.c
> @@ -19,6 +19,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "io/channel-watch.h"
>
> typedef struct QIOChannelFDSource QIOChannelFDSource;
> @@ -275,15 +276,21 @@ GSource *qio_channel_create_fd_watch(QIOChannel *ioc,
>
> #ifdef CONFIG_WIN32
> GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
> - int socket,
> + int sockfd,
> GIOCondition condition)
> {
> + SOCKET s = _get_osfhandle(sockfd);
_get_osfhandle() returns a HANDLE as intptr_t. Is a HANDLE that refers
to a socket also a SOCKET? The docs I found so far are confusing...
> GSource *source;
> QIOChannelSocketSource *ssource;
>
> - WSAEventSelect(socket, ioc->event,
> - FD_READ | FD_ACCEPT | FD_CLOSE |
> - FD_CONNECT | FD_WRITE | FD_OOB);
> + if (s == -1 ||
> + WSAEventSelect(s, ioc->event,
> + FD_READ | FD_ACCEPT | FD_CLOSE |
> + FD_CONNECT | FD_WRITE | FD_OOB) == SOCKET_ERROR) {
> + g_autofree gchar *emsg = g_win32_error_message(GetLastError());
> + error_printf("error creating socket watch: %s", emsg);
Uh, why is printing an error appropriate here? Shouldn't we leave error
handling to callers?
Also, does GetLastError() do the right thing after _get_osfhandle()
failure? _get_osfhandle() is documented to set errno...
> + return NULL;
> + }
>
> source = g_source_new(&qio_channel_socket_source_funcs,
> sizeof(QIOChannelSocketSource));
> @@ -293,7 +300,7 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
> object_ref(OBJECT(ioc));
>
> ssource->condition = condition;
> - ssource->socket = socket;
> + ssource->socket = s;
> ssource->revents = 0;
>
> ssource->fd.fd = (gintptr)ioc->event;
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 07ade41800..78fab521cf 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -180,7 +180,8 @@ static int socket_error(void)
> void qemu_socket_set_block(int fd)
> {
> unsigned long opt = 0;
> - WSAEventSelect(fd, NULL, 0);
> + SOCKET s = _get_osfhandle(fd);
> + WSAEventSelect(s, NULL, 0);
> ioctlsocket(fd, FIONBIO, &opt);
> }
>
> @@ -297,7 +298,13 @@ int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
> socklen_t addrlen)
> {
> int ret;
> - ret = connect(sockfd, addr, addrlen);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
_get_osfhandle() is documented to set errno to EBADF in this case. If
true, you change errno from EBADF to EINVAL. Doesn't seem like an
improvement :)
More of the same below, not pointing it out there.
> + return -1;
> + }
> + ret = connect(s, addr, addrlen);
> if (ret < 0) {
> if (WSAGetLastError() == WSAEWOULDBLOCK) {
> errno = EINPROGRESS;
> @@ -313,7 +320,13 @@ int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
> int qemu_listen_wrap(int sockfd, int backlog)
> {
> int ret;
> - ret = listen(sockfd, backlog);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = listen(s, backlog);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -326,7 +339,13 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
> socklen_t addrlen)
> {
> int ret;
> - ret = bind(sockfd, addr, addrlen);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = bind(s, addr, addrlen);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -337,12 +356,22 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
> #undef socket
> int qemu_socket_wrap(int domain, int type, int protocol)
> {
> - int ret;
> - ret = socket(domain, type, protocol);
> - if (ret < 0) {
> + SOCKET s;
> + int fd;
> +
> + s = socket(domain, type, protocol);
> + if (s == -1) {
> errno = socket_error();
> + return -1;
> }
> - return ret;
> +
> + fd = _open_osfhandle(s, _O_BINARY);
> + if (fd < 0) {
> + closesocket(s);
> + errno = ENOMEM;
_open_osfhandle() is not documented to set errno, unlike
_get_osfhandle(). So, okay, I guess.
Similar uses below, also okay.
> + }
> +
> + return fd;
> }
>
>
> @@ -350,10 +379,22 @@ int qemu_socket_wrap(int domain, int type, int protocol)
> int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
> socklen_t *addrlen)
> {
> - int ret;
> - ret = accept(sockfd, addr, addrlen);
> - if (ret < 0) {
> + int ret = -1;
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + s = accept(s, addr, addrlen);
> + if (s == -1) {
> errno = socket_error();
> + } else {
> + ret = _open_osfhandle(s, _O_BINARY);
> + if (ret < 0) {
> + closesocket(s);
> + errno = ENOMEM;
> + }
> }
> return ret;
> }
> @@ -363,7 +404,13 @@ int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
> int qemu_shutdown_wrap(int sockfd, int how)
> {
> int ret;
> - ret = shutdown(sockfd, how);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = shutdown(s, how);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -375,7 +422,13 @@ int qemu_shutdown_wrap(int sockfd, int how)
> int qemu_ioctlsocket_wrap(int fd, int req, void *val)
> {
> int ret;
> - ret = ioctlsocket(fd, req, val);
> + SOCKET s = _get_osfhandle(fd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = ioctlsocket(s, req, val);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -387,10 +440,28 @@ int qemu_ioctlsocket_wrap(int fd, int req, void *val)
> int qemu_closesocket_wrap(int fd)
> {
> int ret;
> - ret = closesocket(fd);
> + SOCKET s = _get_osfhandle(fd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> +
> + /*
> + * close() must be called before closesocket(), otherwise close() returns an
> + * error and sets EBADF.
> + */
> + ret = close(fd);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + /* closesocket() is required, event after close()! */
As you mention in the commit message, this contradicts _open_osfhandle()
documentation, which claims close() is enough. I think the comment here
should mention it, too.
Found in an old Stackoverflow reply:
open() returns CRT file descriptors which is different from the
Win32 handle. You can create a CRT file descriptor using
_open_osfhandle(). But this is not recommened for sockets because
you cannot close the file in a clean way. You either use close()
which will leak the Winsock user-mode state, or closesocket() which
will leak the CRT descriptor.
https://stackoverflow.com/questions/4676256/whats-the-difference-between-socket-and-handle-in-windows
How can we be sure this is not an issue here?
> + ret = closesocket(s);
> if (ret < 0) {
> errno = socket_error();
> }
> +
> return ret;
> }
>
[...]
Hi
On Mon, Feb 20, 2023 at 4:38 PM Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Until now, a win32 SOCKET handle is often cast to an int file
> > descriptor, as this is what other OS use for sockets.
>
> Brief recap, to refamiliarize myself with the way this stuff works under
> Windows:
>
> 1. Both POSIX and Windows use small integer file descriptors.
>
> 2. With POSIX, these are an OS thing. With Windows, these are a CRT
> thing, wrapping a HANDLE, which is the OS thing.
>
> 3. A Windows HANDLE is to be treated as an abstract data type.
>
> 4. _get_osfhandle() returns a CRT file descriptor's HANDLE.
>
> 5. _open_osfhandle() creates a CRT file descriptor that wraps around a
> HANDLE.
>
> 6. Closing a CRT file descriptor also closes the wrapped HANDLE.
>
> 7. A Windows SOCKET is also a HANDLE. Maybe. I guess. Docs are
> confusing.
>
Kind of, but not really. I think a HANDLE is a kind of void*. You need to
be careful using it appropriately with the right functions. Sometime, a
HANDLE can work with generic functions, like ReadFile, but you should not
use a CloseHandle on SOCKET, or registry key..
>
> 8. There's merry confusion between int, intptr_t, HANDLE, SOCKET, and
> who knows what else.
>
indeed
>
> > When necessary,
> > QEMU eventually queries whether it's a socket with the help of
> > fd_is_socket(). However, there is no guarantee of conflict between the
> > fd and SOCKET space. Such conflict would have surprising consequences,
> > we shouldn't mix them.
>
> True.
>
> However, if conflicts were an issue in practice, conflating the two
> wouldn't be so common, don't you think? File descriptors start at zero.
> Perhaps SOCKETs are much bigger when interpreted as int? Not really
> relevant, because:
>
They are usually fairly low integers on my system.
>
> > Also, it is often forgotten that SOCKET must be closed with
> > closesocket(), and not close().
>
> Yup. After the next patch, we don't have to remember anymore outside
> oslib-win32.c, and that's a fairly compelling argument for this patch.
>
> > Instead, let's make the win32 socket wrapper functions return and take a
> > file descriptor, and let util/ wrappers do the fd/SOCKET conversion as
> > necessary. A bit of adaptation is necessary in io/ as well.
> >
> > Unfortunately, we can't drop closesocket() usage, despite
> > _open_osfhandle() documentation claiming transfer of ownership, testing
> > shows bad behaviour if you forget to call closesocket().
>
> I figure this refers to your patch to qemu_closesocket_wrap(). Correct?
>
> What bad behavior did you observe in testing?
>
Weird failures, as if fd and SOCKET were mixed by the CRT.. ex of test:
#include <winsock2.h>
#include <windows.h>
#include <io.h>
#include <assert.h>
#include <stdbool.h>
#include <stdio.h>
static bool
fd_is_socket(int fd)
{
int optval;
int optlen = sizeof(optval);
SOCKET s = _get_osfhandle(fd);
return getsockopt(s, SOL_SOCKET, SO_TYPE, (char *)&optval, &optlen) ==
0;
}
static void
test(void)
{
SOCKET s;
int fd;
char tmp[] = "fooXXXXXX";
s = socket(AF_INET, SOCK_STREAM, 0);
fd = _open_osfhandle(s, 0);
printf("sock: %d\n", fd);
assert(fd_is_socket(fd));
assert(close(fd) == 0);
/* if you comment this, test will fail after a few iterations */
assert(closesocket(s) == 0);
fd = mkstemp(tmp);
printf("tmp: %d\n", fd);
assert(!fd_is_socket(fd));
assert(close(fd) == 0);
}
int main(int argc, char *argv[])
{
int i;
WSADATA wsaData;
WSAStartup(MAKEWORD(2,2), &wsaData);
for (i = 0; i < 40; i++) {
test();
}
return 0;
}
fd_is_socket() is wrong after a few iterations if you forget closesocket().
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > io/channel-socket.c | 18 +++--
> > io/channel-watch.c | 17 +++--
> > util/oslib-win32.c | 164 ++++++++++++++++++++++++++++++++++++++------
> > 3 files changed, 165 insertions(+), 34 deletions(-)
> >
> > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > index 2040297d2b..18cc062431 100644
> > --- a/io/channel-socket.c
> > +++ b/io/channel-socket.c
> > @@ -426,6 +426,14 @@ static void qio_channel_socket_init(Object *obj)
> > ioc->fd = -1;
> > }
> >
> > +static void wsa_event_clear(int sockfd)
> > +{
> > +#ifdef WIN32
> > + SOCKET s = _get_osfhandle(sockfd);
> > + WSAEventSelect(s, NULL, 0);
> > +#endif
> > +}
> > +
> > static void qio_channel_socket_finalize(Object *obj)
> > {
> > QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
> > @@ -441,9 +449,7 @@ static void qio_channel_socket_finalize(Object *obj)
> > err = NULL;
> > }
> > }
> > -#ifdef WIN32
> > - WSAEventSelect(ioc->fd, NULL, 0);
> > -#endif
> > + wsa_event_clear(ioc->fd);
> > closesocket(ioc->fd);
> > ioc->fd = -1;
> > }
> > @@ -845,9 +851,7 @@ qio_channel_socket_close(QIOChannel *ioc,
> > Error *err = NULL;
> >
> > if (sioc->fd != -1) {
> > -#ifdef WIN32
> > - WSAEventSelect(sioc->fd, NULL, 0);
> > -#endif
> > + wsa_event_clear(sioc->fd);
> > if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
> > socket_listen_cleanup(sioc->fd, errp);
> > }
>
> Factoring out wsa_event_clear() could be a separate patch. Observation,
> not demand.
>
ok
>
> > @@ -899,7 +903,7 @@ static void
> qio_channel_socket_set_aio_fd_handler(QIOChannel *ioc,
> > void *opaque)
> > {
> > QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
> > - aio_set_fd_handler(ctx, sioc->fd, false,
> > + aio_set_fd_handler(ctx, _get_osfhandle(sioc->fd), false,
> > io_read, io_write, NULL, NULL, opaque);
> > }
> >
> > diff --git a/io/channel-watch.c b/io/channel-watch.c
> > index ad7c568a84..8c1c24008f 100644
> > --- a/io/channel-watch.c
> > +++ b/io/channel-watch.c
> > @@ -19,6 +19,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qemu/error-report.h"
> > #include "io/channel-watch.h"
> >
> > typedef struct QIOChannelFDSource QIOChannelFDSource;
> > @@ -275,15 +276,21 @@ GSource *qio_channel_create_fd_watch(QIOChannel
> *ioc,
> >
> > #ifdef CONFIG_WIN32
> > GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
> > - int socket,
> > + int sockfd,
> > GIOCondition condition)
> > {
> > + SOCKET s = _get_osfhandle(sockfd);
>
> _get_osfhandle() returns a HANDLE as intptr_t. Is a HANDLE that refers
> to a socket also a SOCKET? The docs I found so far are confusing...
>
yes
>
> > GSource *source;
> > QIOChannelSocketSource *ssource;
> >
> > - WSAEventSelect(socket, ioc->event,
> > - FD_READ | FD_ACCEPT | FD_CLOSE |
> > - FD_CONNECT | FD_WRITE | FD_OOB);
> > + if (s == -1 ||
> > + WSAEventSelect(s, ioc->event,
> > + FD_READ | FD_ACCEPT | FD_CLOSE |
> > + FD_CONNECT | FD_WRITE | FD_OOB) == SOCKET_ERROR)
> {
> > + g_autofree gchar *emsg = g_win32_error_message(GetLastError());
> > + error_printf("error creating socket watch: %s", emsg);
>
> Uh, why is printing an error appropriate here? Shouldn't we leave error
> handling to callers?
>
We could, but we would have to modify callers as well, which can go deep. I
am considering a &error_warn as a first approach (I am working on something
to check other WSA API users). Does that sound reasonable?
> Also, does GetLastError() do the right thing after _get_osfhandle()
> failure? _get_osfhandle() is documented to set errno...
>
>
Indeed, I better use errno.
> > + return NULL;
> > + }
> >
> > source = g_source_new(&qio_channel_socket_source_funcs,
> > sizeof(QIOChannelSocketSource));
> > @@ -293,7 +300,7 @@ GSource *qio_channel_create_socket_watch(QIOChannel
> *ioc,
> > object_ref(OBJECT(ioc));
> >
> > ssource->condition = condition;
> > - ssource->socket = socket;
> > + ssource->socket = s;
> > ssource->revents = 0;
> >
> > ssource->fd.fd = (gintptr)ioc->event;
> > diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> > index 07ade41800..78fab521cf 100644
> > --- a/util/oslib-win32.c
> > +++ b/util/oslib-win32.c
> > @@ -180,7 +180,8 @@ static int socket_error(void)
> > void qemu_socket_set_block(int fd)
> > {
> > unsigned long opt = 0;
> > - WSAEventSelect(fd, NULL, 0);
> > + SOCKET s = _get_osfhandle(fd);
> > + WSAEventSelect(s, NULL, 0);
> > ioctlsocket(fd, FIONBIO, &opt);
> > }
> >
> > @@ -297,7 +298,13 @@ int qemu_connect_wrap(int sockfd, const struct
> sockaddr *addr,
> > socklen_t addrlen)
> > {
> > int ret;
> > - ret = connect(sockfd, addr, addrlen);
> > + SOCKET s = _get_osfhandle(sockfd);
> > +
> > + if (s == -1) {
> > + errno = EINVAL;
>
> _get_osfhandle() is documented to set errno to EBADF in this case. If
> true, you change errno from EBADF to EINVAL. Doesn't seem like an
> improvement :)
>
right
>
> More of the same below, not pointing it out there.
>
> > + return -1;
> > + }
> > + ret = connect(s, addr, addrlen);
> > if (ret < 0) {
> > if (WSAGetLastError() == WSAEWOULDBLOCK) {
> > errno = EINPROGRESS;
> > @@ -313,7 +320,13 @@ int qemu_connect_wrap(int sockfd, const struct
> sockaddr *addr,
> > int qemu_listen_wrap(int sockfd, int backlog)
> > {
> > int ret;
> > - ret = listen(sockfd, backlog);
> > + SOCKET s = _get_osfhandle(sockfd);
> > +
> > + if (s == -1) {
> > + errno = EINVAL;
> > + return -1;
> > + }
> > + ret = listen(s, backlog);
> > if (ret < 0) {
> > errno = socket_error();
> > }
> > @@ -326,7 +339,13 @@ int qemu_bind_wrap(int sockfd, const struct
> sockaddr *addr,
> > socklen_t addrlen)
> > {
> > int ret;
> > - ret = bind(sockfd, addr, addrlen);
> > + SOCKET s = _get_osfhandle(sockfd);
> > +
> > + if (s == -1) {
> > + errno = EINVAL;
> > + return -1;
> > + }
> > + ret = bind(s, addr, addrlen);
> > if (ret < 0) {
> > errno = socket_error();
> > }
> > @@ -337,12 +356,22 @@ int qemu_bind_wrap(int sockfd, const struct
> sockaddr *addr,
> > #undef socket
> > int qemu_socket_wrap(int domain, int type, int protocol)
> > {
> > - int ret;
> > - ret = socket(domain, type, protocol);
> > - if (ret < 0) {
> > + SOCKET s;
> > + int fd;
> > +
> > + s = socket(domain, type, protocol);
> > + if (s == -1) {
> > errno = socket_error();
> > + return -1;
> > }
> > - return ret;
> > +
> > + fd = _open_osfhandle(s, _O_BINARY);
> > + if (fd < 0) {
> > + closesocket(s);
> > + errno = ENOMEM;
>
> _open_osfhandle() is not documented to set errno, unlike
> _get_osfhandle(). So, okay, I guess.
>
> Similar uses below, also okay.
>
> > + }
> > +
> > + return fd;
> > }
> >
> >
> > @@ -350,10 +379,22 @@ int qemu_socket_wrap(int domain, int type, int
> protocol)
> > int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
> > socklen_t *addrlen)
> > {
> > - int ret;
> > - ret = accept(sockfd, addr, addrlen);
> > - if (ret < 0) {
> > + int ret = -1;
> > + SOCKET s = _get_osfhandle(sockfd);
> > +
> > + if (s == -1) {
> > + errno = EINVAL;
> > + return -1;
> > + }
> > + s = accept(s, addr, addrlen);
> > + if (s == -1) {
> > errno = socket_error();
> > + } else {
> > + ret = _open_osfhandle(s, _O_BINARY);
> > + if (ret < 0) {
> > + closesocket(s);
> > + errno = ENOMEM;
> > + }
> > }
> > return ret;
> > }
> > @@ -363,7 +404,13 @@ int qemu_accept_wrap(int sockfd, struct sockaddr
> *addr,
> > int qemu_shutdown_wrap(int sockfd, int how)
> > {
> > int ret;
> > - ret = shutdown(sockfd, how);
> > + SOCKET s = _get_osfhandle(sockfd);
> > +
> > + if (s == -1) {
> > + errno = EINVAL;
> > + return -1;
> > + }
> > + ret = shutdown(s, how);
> > if (ret < 0) {
> > errno = socket_error();
> > }
> > @@ -375,7 +422,13 @@ int qemu_shutdown_wrap(int sockfd, int how)
> > int qemu_ioctlsocket_wrap(int fd, int req, void *val)
> > {
> > int ret;
> > - ret = ioctlsocket(fd, req, val);
> > + SOCKET s = _get_osfhandle(fd);
> > +
> > + if (s == -1) {
> > + errno = EINVAL;
> > + return -1;
> > + }
> > + ret = ioctlsocket(s, req, val);
> > if (ret < 0) {
> > errno = socket_error();
> > }
> > @@ -387,10 +440,28 @@ int qemu_ioctlsocket_wrap(int fd, int req, void
> *val)
> > int qemu_closesocket_wrap(int fd)
> > {
> > int ret;
> > - ret = closesocket(fd);
> > + SOCKET s = _get_osfhandle(fd);
> > +
> > + if (s == -1) {
> > + errno = EINVAL;
> > + return -1;
> > + }
> > +
> > + /*
> > + * close() must be called before closesocket(), otherwise close()
> returns an
> > + * error and sets EBADF.
> > + */
> > + ret = close(fd);
> > + if (ret < 0) {
> > + return ret;
> > + }
> > +
> > + /* closesocket() is required, event after close()! */
>
> As you mention in the commit message, this contradicts _open_osfhandle()
> documentation, which claims close() is enough. I think the comment here
> should mention it, too.
>
> Found in an old Stackoverflow reply:
>
> open() returns CRT file descriptors which is different from the
> Win32 handle. You can create a CRT file descriptor using
> _open_osfhandle(). But this is not recommened for sockets because
> you cannot close the file in a clean way. You either use close()
> which will leak the Winsock user-mode state, or closesocket() which
> will leak the CRT descriptor.
>
>
> https://stackoverflow.com/questions/4676256/whats-the-difference-between-socket-and-handle-in-windows
>
> How can we be sure this is not an issue here?
>
With enough testing (example code above), error checking for close and
closesocket & running the whole QEMU test suite, I hope it's ok..
On 2/20/23 16:29, Marc-André Lureau wrote: >> 7. A Windows SOCKET is also a HANDLE. Maybe. I guess. Docs are >> confusing. >> > Kind of, but not really. I think a HANDLE is a kind of void*. You need to > be careful using it appropriately with the right functions. Sometime, a > HANDLE can work with generic functions, like ReadFile, but you should not > use a CloseHandle on SOCKET, or registry key.. A Windows SOCKET *is* a file HANDLE except it's always in overlapped mode so Windows provides send()/recv() in case you don't want to deal with overlapped mode. But you can use it with ReadFile too (Windows API documentation says that is only true sometimes, but Winsock API is 30 years old and right now you pretty much always can). However, sockets also has some extra information on the side, so you need to close them with closesocket() and CloseHandle() is not enough. The problem is that close() of something opened with _open_osfhandle() *does* do that CloseHandle(), so basically you are closing the handle twice. IIRC there used to be undocumented functions _alloc_osfhnd() and similar, but they don't exist anymore (even Wine does not have them), so we're stuck; unfortunately this is the reason why QEMU is not already doing something like what you have in this patch. Is this a real bug or is it theoretical? Do file descriptor and socket spaces overlap in practice? Paolo
Hi On Tue, Feb 21, 2023 at 12:18 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 2/20/23 16:29, Marc-André Lureau wrote: > >> 7. A Windows SOCKET is also a HANDLE. Maybe. I guess. Docs are > >> confusing. > >> > > Kind of, but not really. I think a HANDLE is a kind of void*. You need to > > be careful using it appropriately with the right functions. Sometime, a > > HANDLE can work with generic functions, like ReadFile, but you should not > > use a CloseHandle on SOCKET, or registry key.. > > A Windows SOCKET *is* a file HANDLE except it's always in overlapped > mode so Windows provides send()/recv() in case you don't want to deal > with overlapped mode. But you can use it with ReadFile too (Windows API > documentation says that is only true sometimes, but Winsock API is 30 > years old and right now you pretty much always can). > > However, sockets also has some extra information on the side, so you > need to close them with closesocket() and CloseHandle() is not enough. > Yeah, the question is "is it safe to call CloseHandle() on a SOCKET, before closesocket()". Testing/error checking seems to say it's okay.. I wouldn't be surprised if internally the CloseHandle() function does something to check if the given handle is a SOCKET and skip it. I wish they would document it.. > The problem is that close() of something opened with _open_osfhandle() > *does* do that CloseHandle(), so basically you are closing the handle > twice. IIRC there used to be undocumented functions _alloc_osfhnd() and > similar, but they don't exist anymore (even Wine does not have them), so > we're stuck; unfortunately this is the reason why QEMU is not already > doing something like what you have in this patch. > > Is this a real bug or is it theoretical? Do file descriptor and socket > spaces overlap in practice? > > Yes it likely can, the first SOCKET value starts at 92 in a simple test. It looks like it may depend on the system number of opened sockets. I think the second big issue is that we have many places where we assume a fd is a fd, and we simply call close() (which would result in CloseHandle, but missing closesocket). sigh, if the CRT would allow us to steal the handle back..
Hi On Tue, Feb 21, 2023 at 1:13 PM Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > Hi > > On Tue, Feb 21, 2023 at 12:18 PM Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 2/20/23 16:29, Marc-André Lureau wrote: >> >> 7. A Windows SOCKET is also a HANDLE. Maybe. I guess. Docs are >> >> confusing. >> >> >> > Kind of, but not really. I think a HANDLE is a kind of void*. You need to >> > be careful using it appropriately with the right functions. Sometime, a >> > HANDLE can work with generic functions, like ReadFile, but you should not >> > use a CloseHandle on SOCKET, or registry key.. >> >> A Windows SOCKET *is* a file HANDLE except it's always in overlapped >> mode so Windows provides send()/recv() in case you don't want to deal >> with overlapped mode. But you can use it with ReadFile too (Windows API >> documentation says that is only true sometimes, but Winsock API is 30 >> years old and right now you pretty much always can). >> >> However, sockets also has some extra information on the side, so you >> need to close them with closesocket() and CloseHandle() is not enough. > > > Yeah, the question is "is it safe to call CloseHandle() on a SOCKET, before closesocket()". Testing/error checking seems to say it's okay.. I wouldn't be surprised if internally the CloseHandle() function does something to check if the given handle is a SOCKET and skip it. I wish they would document it.. > >> >> The problem is that close() of something opened with _open_osfhandle() >> *does* do that CloseHandle(), so basically you are closing the handle >> twice. IIRC there used to be undocumented functions _alloc_osfhnd() and >> similar, but they don't exist anymore (even Wine does not have them), so >> we're stuck; unfortunately this is the reason why QEMU is not already >> doing something like what you have in this patch. >> >> Is this a real bug or is it theoretical? Do file descriptor and socket >> spaces overlap in practice? >> > > Yes it likely can, the first SOCKET value starts at 92 in a simple test. It looks like it may depend on the system number of opened sockets. > > I think the second big issue is that we have many places where we assume a fd is a fd, and we simply call close() (which would result in CloseHandle, but missing closesocket). > > sigh, if the CRT would allow us to steal the handle back.. > I found an interesting option here, using HANDLE_FLAG_PROTECT_FROM_CLOSE: https://github.com/ksmurph1/VulkanConfigurator/blob/986992a8b963a6b271785a77d5efd349b6e6ea4f/src/folly/src/net/detail/SocketFileDescriptorMap.cpp#L36 And also an old KB: https://jeffpar.github.io/kbarchive/kb/185/Q185727/ When _open_osfhandle is used on a socket descriptor, both _close() and closesocket() should be called before exiting. _close() only closes the file handle. closesocket() has to be called as well to close the socket descriptor and clean up the underlying socket object. I'll work on a v3 of the patch/series with the flag trick. -- Marc-André Lureau
On 2/21/23 11:40, Marc-André Lureau wrote: >> Yes it likely can, the first SOCKET value starts at 92 in a simple >> test. It looks like it may depend on the system number of opened >> sockets. >> >> I think the second big issue is that we have many places where we >> assume a fd is a fd, and we simply call close() (which would result >> in CloseHandle, but missing closesocket). >> >> sigh, if the CRT would allow us to steal the handle back.. >> > I found an interesting option here, using HANDLE_FLAG_PROTECT_FROM_CLOSE: > https://github.com/ksmurph1/VulkanConfigurator/blob/986992a8b963a6b271785a77d5efd349b6e6ea4f/src/folly/src/net/detail/SocketFileDescriptorMap.cpp#L36 Wow, that's the ugliest thing ever but it seems to be made for this. :) Paolo
On Mon, Feb 20, 2023 at 07:29:11PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Mon, Feb 20, 2023 at 4:38 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> > marcandre.lureau@redhat.com writes:
> >
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Until now, a win32 SOCKET handle is often cast to an int file
> > > descriptor, as this is what other OS use for sockets.
> > > @@ -275,15 +276,21 @@ GSource *qio_channel_create_fd_watch(QIOChannel
> > *ioc,
> > >
> > > #ifdef CONFIG_WIN32
> > > GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
> > > - int socket,
> > > + int sockfd,
> > > GIOCondition condition)
> > > {
> > > + SOCKET s = _get_osfhandle(sockfd);
> >
> > _get_osfhandle() returns a HANDLE as intptr_t. Is a HANDLE that refers
> > to a socket also a SOCKET? The docs I found so far are confusing...
> >
>
> yes
>
>
> >
> > > GSource *source;
> > > QIOChannelSocketSource *ssource;
> > >
> > > - WSAEventSelect(socket, ioc->event,
> > > - FD_READ | FD_ACCEPT | FD_CLOSE |
> > > - FD_CONNECT | FD_WRITE | FD_OOB);
> > > + if (s == -1 ||
> > > + WSAEventSelect(s, ioc->event,
> > > + FD_READ | FD_ACCEPT | FD_CLOSE |
> > > + FD_CONNECT | FD_WRITE | FD_OOB) == SOCKET_ERROR)
> > {
> > > + g_autofree gchar *emsg = g_win32_error_message(GetLastError());
> > > + error_printf("error creating socket watch: %s", emsg);
> >
> > Uh, why is printing an error appropriate here? Shouldn't we leave error
> > handling to callers?
> >
>
> We could, but we would have to modify callers as well, which can go deep. I
> am considering a &error_warn as a first approach (I am working on something
> to check other WSA API users). Does that sound reasonable?
The caller should also be handling 'NULL' as a return value, as none
of them expect that. They just carry on calling g_source APIs. "Luckily"
glib turns them all into no-ops, so it won't crash, but it also means
the backend is likelyto be non-functional since events won't be
processed.
It isn't clear that there's much of value that a caller can do when it
gets a NULL source either. The context in wich we call this API does
not have error propagation either and its non-trival to add in many
of the callers.
Feels like the realistic choice is between a error_report or an
assert/abort, whether in this method or the caller doesn't make
all that much difference.
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, Feb 13, 2023 at 12:51 AM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Until now, a win32 SOCKET handle is often cast to an int file
> descriptor, as this is what other OS use for sockets. When necessary,
> QEMU eventually queries whether it's a socket with the help of
> fd_is_socket(). However, there is no guarantee of conflict between the
> fd and SOCKET space. Such conflict would have surprising consequences,
> we shouldn't mix them.
>
> Also, it is often forgotten that SOCKET must be closed with
> closesocket(), and not close().
>
> Instead, let's make the win32 socket wrapper functions return and take a
> file descriptor, and let util/ wrappers do the fd/SOCKET conversion as
> necessary. A bit of adaptation is necessary in io/ as well.
>
> Unfortunately, we can't drop closesocket() usage, despite
> _open_osfhandle() documentation claiming transfer of ownership, testing
> shows bad behaviour if you forget to call closesocket().
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> io/channel-socket.c | 18 +++--
> io/channel-watch.c | 17 +++--
> util/oslib-win32.c | 164 ++++++++++++++++++++++++++++++++++++++------
> 3 files changed, 165 insertions(+), 34 deletions(-)
>
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 2040297d2b..18cc062431 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -426,6 +426,14 @@ static void qio_channel_socket_init(Object *obj)
> ioc->fd = -1;
> }
>
> +static void wsa_event_clear(int sockfd)
> +{
> +#ifdef WIN32
> + SOCKET s = _get_osfhandle(sockfd);
> + WSAEventSelect(s, NULL, 0);
> +#endif
> +}
> +
> static void qio_channel_socket_finalize(Object *obj)
> {
> QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
> @@ -441,9 +449,7 @@ static void qio_channel_socket_finalize(Object *obj)
> err = NULL;
> }
> }
> -#ifdef WIN32
> - WSAEventSelect(ioc->fd, NULL, 0);
> -#endif
> + wsa_event_clear(ioc->fd);
> closesocket(ioc->fd);
> ioc->fd = -1;
> }
> @@ -845,9 +851,7 @@ qio_channel_socket_close(QIOChannel *ioc,
> Error *err = NULL;
>
> if (sioc->fd != -1) {
> -#ifdef WIN32
> - WSAEventSelect(sioc->fd, NULL, 0);
> -#endif
> + wsa_event_clear(sioc->fd);
> if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
> socket_listen_cleanup(sioc->fd, errp);
> }
> @@ -899,7 +903,7 @@ static void qio_channel_socket_set_aio_fd_handler(QIOChannel *ioc,
> void *opaque)
> {
> QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
> - aio_set_fd_handler(ctx, sioc->fd, false,
> + aio_set_fd_handler(ctx, _get_osfhandle(sioc->fd), false,
My bad, this breaks compilation on !win32, fixing. (ah, if only it was
a gitlab MR with CI..)
> io_read, io_write, NULL, NULL, opaque);
> }
>
> diff --git a/io/channel-watch.c b/io/channel-watch.c
> index ad7c568a84..8c1c24008f 100644
> --- a/io/channel-watch.c
> +++ b/io/channel-watch.c
> @@ -19,6 +19,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "io/channel-watch.h"
>
> typedef struct QIOChannelFDSource QIOChannelFDSource;
> @@ -275,15 +276,21 @@ GSource *qio_channel_create_fd_watch(QIOChannel *ioc,
>
> #ifdef CONFIG_WIN32
> GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
> - int socket,
> + int sockfd,
> GIOCondition condition)
> {
> + SOCKET s = _get_osfhandle(sockfd);
> GSource *source;
> QIOChannelSocketSource *ssource;
>
> - WSAEventSelect(socket, ioc->event,
> - FD_READ | FD_ACCEPT | FD_CLOSE |
> - FD_CONNECT | FD_WRITE | FD_OOB);
> + if (s == -1 ||
> + WSAEventSelect(s, ioc->event,
> + FD_READ | FD_ACCEPT | FD_CLOSE |
> + FD_CONNECT | FD_WRITE | FD_OOB) == SOCKET_ERROR) {
> + g_autofree gchar *emsg = g_win32_error_message(GetLastError());
> + error_printf("error creating socket watch: %s", emsg);
> + return NULL;
> + }
>
> source = g_source_new(&qio_channel_socket_source_funcs,
> sizeof(QIOChannelSocketSource));
> @@ -293,7 +300,7 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
> object_ref(OBJECT(ioc));
>
> ssource->condition = condition;
> - ssource->socket = socket;
> + ssource->socket = s;
> ssource->revents = 0;
>
> ssource->fd.fd = (gintptr)ioc->event;
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 07ade41800..78fab521cf 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -180,7 +180,8 @@ static int socket_error(void)
> void qemu_socket_set_block(int fd)
> {
> unsigned long opt = 0;
> - WSAEventSelect(fd, NULL, 0);
> + SOCKET s = _get_osfhandle(fd);
> + WSAEventSelect(s, NULL, 0);
> ioctlsocket(fd, FIONBIO, &opt);
> }
>
> @@ -297,7 +298,13 @@ int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
> socklen_t addrlen)
> {
> int ret;
> - ret = connect(sockfd, addr, addrlen);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = connect(s, addr, addrlen);
> if (ret < 0) {
> if (WSAGetLastError() == WSAEWOULDBLOCK) {
> errno = EINPROGRESS;
> @@ -313,7 +320,13 @@ int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
> int qemu_listen_wrap(int sockfd, int backlog)
> {
> int ret;
> - ret = listen(sockfd, backlog);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = listen(s, backlog);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -326,7 +339,13 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
> socklen_t addrlen)
> {
> int ret;
> - ret = bind(sockfd, addr, addrlen);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = bind(s, addr, addrlen);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -337,12 +356,22 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
> #undef socket
> int qemu_socket_wrap(int domain, int type, int protocol)
> {
> - int ret;
> - ret = socket(domain, type, protocol);
> - if (ret < 0) {
> + SOCKET s;
> + int fd;
> +
> + s = socket(domain, type, protocol);
> + if (s == -1) {
> errno = socket_error();
> + return -1;
> }
> - return ret;
> +
> + fd = _open_osfhandle(s, _O_BINARY);
> + if (fd < 0) {
> + closesocket(s);
> + errno = ENOMEM;
> + }
> +
> + return fd;
> }
>
>
> @@ -350,10 +379,22 @@ int qemu_socket_wrap(int domain, int type, int protocol)
> int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
> socklen_t *addrlen)
> {
> - int ret;
> - ret = accept(sockfd, addr, addrlen);
> - if (ret < 0) {
> + int ret = -1;
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + s = accept(s, addr, addrlen);
> + if (s == -1) {
> errno = socket_error();
> + } else {
> + ret = _open_osfhandle(s, _O_BINARY);
> + if (ret < 0) {
> + closesocket(s);
> + errno = ENOMEM;
> + }
> }
> return ret;
> }
> @@ -363,7 +404,13 @@ int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
> int qemu_shutdown_wrap(int sockfd, int how)
> {
> int ret;
> - ret = shutdown(sockfd, how);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = shutdown(s, how);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -375,7 +422,13 @@ int qemu_shutdown_wrap(int sockfd, int how)
> int qemu_ioctlsocket_wrap(int fd, int req, void *val)
> {
> int ret;
> - ret = ioctlsocket(fd, req, val);
> + SOCKET s = _get_osfhandle(fd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = ioctlsocket(s, req, val);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -387,10 +440,28 @@ int qemu_ioctlsocket_wrap(int fd, int req, void *val)
> int qemu_closesocket_wrap(int fd)
> {
> int ret;
> - ret = closesocket(fd);
> + SOCKET s = _get_osfhandle(fd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> +
> + /*
> + * close() must be called before closesocket(), otherwise close() returns an
> + * error and sets EBADF.
> + */
> + ret = close(fd);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + /* closesocket() is required, event after close()! */
> + ret = closesocket(s);
> if (ret < 0) {
> errno = socket_error();
> }
> +
> return ret;
> }
>
> @@ -400,7 +471,14 @@ int qemu_getsockopt_wrap(int sockfd, int level, int optname,
> void *optval, socklen_t *optlen)
> {
> int ret;
> - ret = getsockopt(sockfd, level, optname, optval, optlen);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> +
> + ret = getsockopt(s, level, optname, optval, optlen);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -413,7 +491,13 @@ int qemu_setsockopt_wrap(int sockfd, int level, int optname,
> const void *optval, socklen_t optlen)
> {
> int ret;
> - ret = setsockopt(sockfd, level, optname, optval, optlen);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = setsockopt(s, level, optname, optval, optlen);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -426,7 +510,13 @@ int qemu_getpeername_wrap(int sockfd, struct sockaddr *addr,
> socklen_t *addrlen)
> {
> int ret;
> - ret = getpeername(sockfd, addr, addrlen);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = getpeername(s, addr, addrlen);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -439,7 +529,13 @@ int qemu_getsockname_wrap(int sockfd, struct sockaddr *addr,
> socklen_t *addrlen)
> {
> int ret;
> - ret = getsockname(sockfd, addr, addrlen);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = getsockname(s, addr, addrlen);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -451,7 +547,13 @@ int qemu_getsockname_wrap(int sockfd, struct sockaddr *addr,
> ssize_t qemu_send_wrap(int sockfd, const void *buf, size_t len, int flags)
> {
> int ret;
> - ret = send(sockfd, buf, len, flags);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = send(s, buf, len, flags);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -464,7 +566,13 @@ ssize_t qemu_sendto_wrap(int sockfd, const void *buf, size_t len, int flags,
> const struct sockaddr *addr, socklen_t addrlen)
> {
> int ret;
> - ret = sendto(sockfd, buf, len, flags, addr, addrlen);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = sendto(s, buf, len, flags, addr, addrlen);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -476,7 +584,13 @@ ssize_t qemu_sendto_wrap(int sockfd, const void *buf, size_t len, int flags,
> ssize_t qemu_recv_wrap(int sockfd, void *buf, size_t len, int flags)
> {
> int ret;
> - ret = recv(sockfd, buf, len, flags);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = recv(s, buf, len, flags);
> if (ret < 0) {
> errno = socket_error();
> }
> @@ -489,7 +603,13 @@ ssize_t qemu_recvfrom_wrap(int sockfd, void *buf, size_t len, int flags,
> struct sockaddr *addr, socklen_t *addrlen)
> {
> int ret;
> - ret = recvfrom(sockfd, buf, len, flags, addr, addrlen);
> + SOCKET s = _get_osfhandle(sockfd);
> +
> + if (s == -1) {
> + errno = EINVAL;
> + return -1;
> + }
> + ret = recvfrom(s, buf, len, flags, addr, addrlen);
> if (ret < 0) {
> errno = socket_error();
> }
> --
> 2.39.1
>
>
--
Marc-André Lureau
© 2016 - 2026 Red Hat, Inc.