* [PATCH 0/4] win32: do not mix SOCKET and fd space
@ 2023-02-12 20:49 marcandre.lureau
2023-02-12 20:49 ` [PATCH 1/4] tests: use closesocket() marcandre.lureau
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: marcandre.lureau @ 2023-02-12 20:49 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Thomas Huth,
Jason Wang, qemu-arm, Stefan Berger, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Hi,
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 can fix this by using FDs only.
After fixing a few missed closesocket(), this patch series makes the win32
socket API wrappers take FDs. It finally get rid of closesocket() usage by using
a close() wrapper instead. (note that fdopen/fclose would not be enough either
to close the underlying socket appropriately)
Marc-André Lureau (4):
tests: use closesocket()
io: use closesocket()
win32: stop mixing SOCKET and file descriptor space
win32: replace closesocket() with close() wrapper
include/sysemu/os-posix.h | 1 -
include/sysemu/os-win32.h | 8 +-
backends/tpm/tpm_emulator.c | 6 +-
crypto/afalg.c | 6 +-
hw/hyperv/syndbg.c | 4 +-
io/channel-socket.c | 22 +++--
io/channel-watch.c | 17 ++--
net/dgram.c | 14 +--
net/socket.c | 22 ++---
tests/qtest/libqtest.c | 8 +-
tests/qtest/microbit-test.c | 2 +-
tests/qtest/netdev-socket.c | 10 +--
tests/unit/socket-helpers.c | 2 +-
util/oslib-win32.c | 169 ++++++++++++++++++++++++++++++------
util/qemu-sockets.c | 22 ++---
15 files changed, 221 insertions(+), 92 deletions(-)
--
2.39.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/4] tests: use closesocket()
2023-02-12 20:49 [PATCH 0/4] win32: do not mix SOCKET and fd space marcandre.lureau
@ 2023-02-12 20:49 ` marcandre.lureau
2023-02-13 8:03 ` Thomas Huth
2023-02-12 20:49 ` [PATCH 2/4] io: " marcandre.lureau
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: marcandre.lureau @ 2023-02-12 20:49 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Thomas Huth,
Jason Wang, qemu-arm, Stefan Berger, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Because they are actually sockets...
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
tests/unit/socket-helpers.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/unit/socket-helpers.c b/tests/unit/socket-helpers.c
index eecadf3a3c..914b3aa0cf 100644
--- a/tests/unit/socket-helpers.c
+++ b/tests/unit/socket-helpers.c
@@ -117,13 +117,13 @@ static int socket_can_bind_connect(const char *hostname, int family)
cleanup:
if (afd != -1) {
- close(afd);
+ closesocket(afd);
}
if (cfd != -1) {
- close(cfd);
+ closesocket(cfd);
}
if (lfd != -1) {
- close(lfd);
+ closesocket(lfd);
}
if (res) {
freeaddrinfo(res);
--
2.39.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/4] io: use closesocket()
2023-02-12 20:49 [PATCH 0/4] win32: do not mix SOCKET and fd space marcandre.lureau
2023-02-12 20:49 ` [PATCH 1/4] tests: use closesocket() marcandre.lureau
@ 2023-02-12 20:49 ` marcandre.lureau
2023-02-13 8:04 ` Thomas Huth
2023-02-12 20:49 ` [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space marcandre.lureau
2023-02-12 20:49 ` [PATCH 4/4] win32: replace closesocket() with close() wrapper marcandre.lureau
3 siblings, 1 reply; 15+ messages in thread
From: marcandre.lureau @ 2023-02-12 20:49 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Thomas Huth,
Jason Wang, qemu-arm, Stefan Berger, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Because they are actually sockets...
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
io/channel-socket.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 7aca84f61a..2040297d2b 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -159,7 +159,7 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
trace_qio_channel_socket_connect_complete(ioc, fd);
if (qio_channel_socket_set_fd(ioc, fd, errp) < 0) {
- close(fd);
+ closesocket(fd);
return -1;
}
@@ -233,7 +233,7 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
trace_qio_channel_socket_listen_complete(ioc, fd);
if (qio_channel_socket_set_fd(ioc, fd, errp) < 0) {
- close(fd);
+ closesocket(fd);
return -1;
}
qio_channel_set_feature(QIO_CHANNEL(ioc), QIO_CHANNEL_FEATURE_LISTEN);
@@ -310,7 +310,7 @@ int qio_channel_socket_dgram_sync(QIOChannelSocket *ioc,
trace_qio_channel_socket_dgram_complete(ioc, fd);
if (qio_channel_socket_set_fd(ioc, fd, errp) < 0) {
- close(fd);
+ closesocket(fd);
return -1;
}
--
2.39.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space
2023-02-12 20:49 [PATCH 0/4] win32: do not mix SOCKET and fd space marcandre.lureau
2023-02-12 20:49 ` [PATCH 1/4] tests: use closesocket() marcandre.lureau
2023-02-12 20:49 ` [PATCH 2/4] io: " marcandre.lureau
@ 2023-02-12 20:49 ` marcandre.lureau
2023-02-20 11:27 ` Marc-André Lureau
2023-02-20 12:38 ` Markus Armbruster
2023-02-12 20:49 ` [PATCH 4/4] win32: replace closesocket() with close() wrapper marcandre.lureau
3 siblings, 2 replies; 15+ messages in thread
From: marcandre.lureau @ 2023-02-12 20:49 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Thomas Huth,
Jason Wang, qemu-arm, Stefan Berger, Marc-André Lureau
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/4] win32: replace closesocket() with close() wrapper
2023-02-12 20:49 [PATCH 0/4] win32: do not mix SOCKET and fd space marcandre.lureau
` (2 preceding siblings ...)
2023-02-12 20:49 ` [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space marcandre.lureau
@ 2023-02-12 20:49 ` marcandre.lureau
3 siblings, 0 replies; 15+ messages in thread
From: marcandre.lureau @ 2023-02-12 20:49 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Thomas Huth,
Jason Wang, qemu-arm, Stefan Berger, Marc-André Lureau
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>
---
include/sysemu/os-posix.h | 1 -
include/sysemu/os-win32.h | 8 ++++----
backends/tpm/tpm_emulator.c | 6 +++---
crypto/afalg.c | 6 +++---
hw/hyperv/syndbg.c | 4 ++--
io/channel-socket.c | 10 +++++-----
net/dgram.c | 14 +++++++-------
net/socket.c | 22 +++++++++++-----------
tests/qtest/libqtest.c | 8 ++++----
tests/qtest/microbit-test.c | 2 +-
tests/qtest/netdev-socket.c | 10 +++++-----
tests/unit/socket-helpers.c | 8 ++++----
util/oslib-win32.c | 21 ++++++++++-----------
util/qemu-sockets.c | 22 +++++++++++-----------
14 files changed, 70 insertions(+), 72 deletions(-)
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 58de7c994d..4c29b72941 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -51,7 +51,6 @@ void os_daemonize(void);
void os_setup_post(void);
int os_mlock(void);
-#define closesocket(s) close(s)
#define ioctlsocket(s, r, v) ioctl(s, r, v)
int os_set_daemonize(bool d);
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 5b38c7bd04..c5008538ec 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -148,6 +148,10 @@ static inline void qemu_funlockfile(FILE *f)
* set errno based on WSAGetLastError()
*/
+#undef close
+#define close qemu_close_wrap
+int qemu_close_wrap(int fd);
+
#undef connect
#define connect qemu_connect_wrap
int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
@@ -179,10 +183,6 @@ int qemu_shutdown_wrap(int sockfd, int how);
#define ioctlsocket qemu_ioctlsocket_wrap
int qemu_ioctlsocket_wrap(int fd, int req, void *val);
-#undef closesocket
-#define closesocket qemu_closesocket_wrap
-int qemu_closesocket_wrap(int fd);
-
#undef getsockopt
#define getsockopt qemu_getsockopt_wrap
int qemu_getsockopt_wrap(int sockfd, int level, int optname,
diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 67e7b212e3..0695febb35 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -574,13 +574,13 @@ static int tpm_emulator_prepare_data_fd(TPMEmulator *tpm_emu)
goto err_exit;
}
- closesocket(fds[1]);
+ close(fds[1]);
return 0;
err_exit:
- closesocket(fds[0]);
- closesocket(fds[1]);
+ close(fds[0]);
+ close(fds[1]);
return -1;
}
diff --git a/crypto/afalg.c b/crypto/afalg.c
index 10046bb0ae..348301e703 100644
--- a/crypto/afalg.c
+++ b/crypto/afalg.c
@@ -59,7 +59,7 @@ qcrypto_afalg_socket_bind(const char *type, const char *name,
if (bind(sbind, (const struct sockaddr *)&salg, sizeof(salg)) != 0) {
error_setg_errno(errp, errno, "Failed to bind socket");
- closesocket(sbind);
+ close(sbind);
return -1;
}
@@ -105,11 +105,11 @@ void qcrypto_afalg_comm_free(QCryptoAFAlg *afalg)
}
if (afalg->tfmfd != -1) {
- closesocket(afalg->tfmfd);
+ close(afalg->tfmfd);
}
if (afalg->opfd != -1) {
- closesocket(afalg->opfd);
+ close(afalg->opfd);
}
g_free(afalg);
diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
index 16d04cfdc6..4d5ac71f03 100644
--- a/hw/hyperv/syndbg.c
+++ b/hw/hyperv/syndbg.c
@@ -340,7 +340,7 @@ static void hv_syndbg_realize(DeviceState *dev, Error **errp)
syndbg->servaddr.sin_family = AF_INET;
if (connect(syndbg->socket, (struct sockaddr *)&syndbg->servaddr,
sizeof(syndbg->servaddr)) < 0) {
- closesocket(syndbg->socket);
+ close(syndbg->socket);
error_setg(errp, "%s failed to connect to socket", TYPE_HV_SYNDBG);
return;
}
@@ -357,7 +357,7 @@ static void hv_syndbg_unrealize(DeviceState *dev)
if (syndbg->socket > 0) {
qemu_set_fd_handler(syndbg->socket, NULL, NULL, NULL);
- closesocket(syndbg->socket);
+ close(syndbg->socket);
}
}
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 18cc062431..db5f25d780 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -159,7 +159,7 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
trace_qio_channel_socket_connect_complete(ioc, fd);
if (qio_channel_socket_set_fd(ioc, fd, errp) < 0) {
- closesocket(fd);
+ close(fd);
return -1;
}
@@ -233,7 +233,7 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
trace_qio_channel_socket_listen_complete(ioc, fd);
if (qio_channel_socket_set_fd(ioc, fd, errp) < 0) {
- closesocket(fd);
+ close(fd);
return -1;
}
qio_channel_set_feature(QIO_CHANNEL(ioc), QIO_CHANNEL_FEATURE_LISTEN);
@@ -310,7 +310,7 @@ int qio_channel_socket_dgram_sync(QIOChannelSocket *ioc,
trace_qio_channel_socket_dgram_complete(ioc, fd);
if (qio_channel_socket_set_fd(ioc, fd, errp) < 0) {
- closesocket(fd);
+ close(fd);
return -1;
}
@@ -450,7 +450,7 @@ static void qio_channel_socket_finalize(Object *obj)
}
}
wsa_event_clear(ioc->fd);
- closesocket(ioc->fd);
+ close(ioc->fd);
ioc->fd = -1;
}
}
@@ -856,7 +856,7 @@ qio_channel_socket_close(QIOChannel *ioc,
socket_listen_cleanup(sioc->fd, errp);
}
- if (closesocket(sioc->fd) < 0) {
+ if (close(sioc->fd) < 0) {
sioc->fd = -1;
error_setg_errno(&err, errno, "Unable to close socket");
error_propagate(errp, err);
diff --git a/net/dgram.c b/net/dgram.c
index 9f7bf38376..48f653bceb 100644
--- a/net/dgram.c
+++ b/net/dgram.c
@@ -230,7 +230,7 @@ static int net_dgram_mcast_create(struct sockaddr_in *mcastaddr,
return fd;
fail:
if (fd >= 0) {
- closesocket(fd);
+ close(fd);
}
return -1;
}
@@ -352,7 +352,7 @@ static int net_dgram_mcast_init(NetClientState *peer,
if (convert_host_port(saddr, local->u.inet.host, local->u.inet.port,
errp) < 0) {
g_free(saddr);
- closesocket(fd);
+ close(fd);
return -1;
}
@@ -360,14 +360,14 @@ static int net_dgram_mcast_init(NetClientState *peer,
if (saddr->sin_addr.s_addr == 0) {
error_setg(errp, "can't setup multicast destination address");
g_free(saddr);
- closesocket(fd);
+ close(fd);
return -1;
}
/* clone dgram socket */
newfd = net_dgram_mcast_create(saddr, NULL, errp);
if (newfd < 0) {
g_free(saddr);
- closesocket(fd);
+ close(fd);
return -1;
}
/* clone newfd to fd, close newfd */
@@ -494,14 +494,14 @@ int net_init_dgram(const Netdev *netdev, const char *name,
if (ret < 0) {
error_setg_errno(errp, errno,
"can't set socket option SO_REUSEADDR");
- closesocket(fd);
+ close(fd);
return -1;
}
ret = bind(fd, (struct sockaddr *)&laddr_in, sizeof(laddr_in));
if (ret < 0) {
error_setg_errno(errp, errno, "can't bind ip=%s to socket",
inet_ntoa(laddr_in.sin_addr));
- closesocket(fd);
+ close(fd);
return -1;
}
qemu_socket_set_nonblock(fd);
@@ -548,7 +548,7 @@ int net_init_dgram(const Netdev *netdev, const char *name,
if (ret < 0) {
error_setg_errno(errp, errno, "can't bind unix=%s to socket",
laddr_un.sun_path);
- closesocket(fd);
+ close(fd);
return -1;
}
qemu_socket_set_nonblock(fd);
diff --git a/net/socket.c b/net/socket.c
index 2fc5696755..ba6e5b0b00 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -172,7 +172,7 @@ static void net_socket_send(void *opaque)
if (s->listen_fd != -1) {
qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
}
- closesocket(s->fd);
+ close(s->fd);
s->fd = -1;
net_socket_rs_init(&s->rs, net_socket_rs_finalize, false);
@@ -299,7 +299,7 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr,
return fd;
fail:
if (fd >= 0)
- closesocket(fd);
+ close(fd);
return -1;
}
@@ -314,7 +314,7 @@ static void net_socket_cleanup(NetClientState *nc)
}
if (s->listen_fd != -1) {
qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
- closesocket(s->listen_fd);
+ close(s->listen_fd);
s->listen_fd = -1;
}
}
@@ -399,7 +399,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
return s;
err:
- closesocket(fd);
+ close(fd);
return NULL;
}
@@ -456,7 +456,7 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
(socklen_t *)&optlen)< 0) {
error_setg(errp, "can't get socket option SO_TYPE");
- closesocket(fd);
+ close(fd);
return NULL;
}
switch(so_type) {
@@ -468,7 +468,7 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
default:
error_setg(errp, "socket type=%d for fd=%d must be either"
" SOCK_DGRAM or SOCK_STREAM", so_type, fd);
- closesocket(fd);
+ close(fd);
}
return NULL;
}
@@ -526,13 +526,13 @@ static int net_socket_listen_init(NetClientState *peer,
if (ret < 0) {
error_setg_errno(errp, errno, "can't bind ip=%s to socket",
inet_ntoa(saddr.sin_addr));
- closesocket(fd);
+ close(fd);
return -1;
}
ret = listen(fd, 0);
if (ret < 0) {
error_setg_errno(errp, errno, "can't listen on socket");
- closesocket(fd);
+ close(fd);
return -1;
}
@@ -579,7 +579,7 @@ static int net_socket_connect_init(NetClientState *peer,
break;
} else {
error_setg_errno(errp, errno, "can't connect socket");
- closesocket(fd);
+ close(fd);
return -1;
}
} else {
@@ -671,14 +671,14 @@ static int net_socket_udp_init(NetClientState *peer,
if (ret < 0) {
error_setg_errno(errp, errno,
"can't set socket option SO_REUSEADDR");
- closesocket(fd);
+ close(fd);
return -1;
}
ret = bind(fd, (struct sockaddr *)&laddr, sizeof(laddr));
if (ret < 0) {
error_setg_errno(errp, errno, "can't bind ip=%s to socket",
inet_ntoa(laddr.sin_addr));
- closesocket(fd);
+ close(fd);
return -1;
}
qemu_socket_set_nonblock(fd);
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index d658222a19..983ae18ca5 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -124,7 +124,7 @@ static int socket_accept(int sock)
(void *)&timeout, sizeof(timeout))) {
fprintf(stderr, "%s failed to set SO_RCVTIMEO: %s\n",
__func__, strerror(errno));
- closesocket(sock);
+ close(sock);
return -1;
}
@@ -135,7 +135,7 @@ static int socket_accept(int sock)
if (ret == -1) {
fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
}
- closesocket(sock);
+ close(sock);
return ret;
}
@@ -546,8 +546,8 @@ void qtest_quit(QTestState *s)
qtest_remove_abrt_handler(s);
qtest_kill_qemu(s);
- closesocket(s->fd);
- closesocket(s->qmp_fd);
+ close(s->fd);
+ close(s->qmp_fd);
g_string_free(s->rx, true);
for (GList *it = s->pending_events; it != NULL; it = it->next) {
diff --git a/tests/qtest/microbit-test.c b/tests/qtest/microbit-test.c
index 4bc267020b..6022a92b6a 100644
--- a/tests/qtest/microbit-test.c
+++ b/tests/qtest/microbit-test.c
@@ -107,7 +107,7 @@ static void test_nrf51_uart(void)
g_assert_true(recv(sock_fd, s, 10, 0) == 5);
g_assert_true(memcmp(s, "world", 5) == 0);
- closesocket(sock_fd);
+ close(sock_fd);
qtest_quit(qts);
}
diff --git a/tests/qtest/netdev-socket.c b/tests/qtest/netdev-socket.c
index 6ba256e173..9b5c52a272 100644
--- a/tests/qtest/netdev-socket.c
+++ b/tests/qtest/netdev-socket.c
@@ -95,7 +95,7 @@ static int inet_get_free_port_multiple(int nb, int *port, bool ipv6)
nb = i;
for (i = 0; i < nb; i++) {
- closesocket(sock[i]);
+ close(sock[i]);
}
return nb;
@@ -262,8 +262,8 @@ static void test_stream_fd(void)
qtest_quit(qts1);
qtest_quit(qts0);
- closesocket(sock[0]);
- closesocket(sock[1]);
+ close(sock[0]);
+ close(sock[1]);
}
#endif
@@ -388,8 +388,8 @@ static void test_dgram_fd(void)
qtest_quit(qts1);
qtest_quit(qts0);
- closesocket(sv[0]);
- closesocket(sv[1]);
+ close(sv[0]);
+ close(sv[1]);
}
#endif
diff --git a/tests/unit/socket-helpers.c b/tests/unit/socket-helpers.c
index 914b3aa0cf..6de27baee2 100644
--- a/tests/unit/socket-helpers.c
+++ b/tests/unit/socket-helpers.c
@@ -117,13 +117,13 @@ static int socket_can_bind_connect(const char *hostname, int family)
cleanup:
if (afd != -1) {
- closesocket(afd);
+ close(afd);
}
if (cfd != -1) {
- closesocket(cfd);
+ close(cfd);
}
if (lfd != -1) {
- closesocket(lfd);
+ close(lfd);
}
if (res) {
freeaddrinfo(res);
@@ -160,7 +160,7 @@ void socket_check_afunix_support(bool *has_afunix)
int fd;
fd = socket(PF_UNIX, SOCK_STREAM, 0);
- closesocket(fd);
+ close(fd);
#ifdef _WIN32
*has_afunix = (fd != (int)INVALID_SOCKET);
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 78fab521cf..a99f38e7ff 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -436,17 +436,15 @@ int qemu_ioctlsocket_wrap(int fd, int req, void *val)
}
-#undef closesocket
-int qemu_closesocket_wrap(int fd)
+#undef close
+int qemu_close_wrap(int fd)
{
+ SOCKET s = INVALID_SOCKET;
int ret;
- SOCKET s = _get_osfhandle(fd);
- if (s == -1) {
- errno = EINVAL;
- return -1;
+ if (fd_is_socket(fd)) {
+ s = _get_osfhandle(fd);
}
-
/*
* close() must be called before closesocket(), otherwise close() returns an
* error and sets EBADF.
@@ -456,10 +454,11 @@ int qemu_closesocket_wrap(int fd)
return ret;
}
- /* closesocket() is required, event after close()! */
- ret = closesocket(s);
- if (ret < 0) {
- errno = socket_error();
+ if (s != INVALID_SOCKET) {
+ ret = closesocket(s);
+ if (ret < 0) {
+ errno = socket_error();
+ }
}
return ret;
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 6538859b87..c06a4dce77 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -326,7 +326,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
* recover from this situation, so we need to recreate the
* socket to allow bind attempts for subsequent ports:
*/
- closesocket(slisten);
+ close(slisten);
slisten = -1;
}
}
@@ -337,7 +337,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
listen_failed:
saved_errno = errno;
if (slisten >= 0) {
- closesocket(slisten);
+ close(slisten);
}
freeaddrinfo(res);
errno = saved_errno;
@@ -380,7 +380,7 @@ static int inet_connect_addr(const InetSocketAddress *saddr,
if (rc < 0) {
error_setg_errno(errp, errno, "Failed to connect to '%s:%s'",
saddr->host, saddr->port);
- closesocket(sock);
+ close(sock);
return -1;
}
@@ -483,7 +483,7 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
if (ret < 0) {
error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
- closesocket(sock);
+ close(sock);
return -1;
}
}
@@ -580,7 +580,7 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
err:
if (sock != -1) {
- closesocket(sock);
+ close(sock);
}
if (local) {
freeaddrinfo(local);
@@ -777,7 +777,7 @@ static int vsock_connect_addr(const VsockSocketAddress *vaddr,
if (rc < 0) {
error_setg_errno(errp, errno, "Failed to connect to '%s:%s'",
vaddr->cid, vaddr->port);
- closesocket(sock);
+ close(sock);
return -1;
}
@@ -814,13 +814,13 @@ static int vsock_listen_saddr(VsockSocketAddress *vaddr,
if (bind(slisten, (const struct sockaddr *)&svm, sizeof(svm)) != 0) {
error_setg_errno(errp, errno, "Failed to bind socket");
- closesocket(slisten);
+ close(slisten);
return -1;
}
if (listen(slisten, num) != 0) {
error_setg_errno(errp, errno, "Failed to listen on socket");
- closesocket(slisten);
+ close(slisten);
return -1;
}
return slisten;
@@ -978,7 +978,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
err:
g_free(pathbuf);
- closesocket(sock);
+ close(sock);
return -1;
}
@@ -1041,7 +1041,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
return sock;
err:
- closesocket(sock);
+ close(sock);
return -1;
}
@@ -1238,7 +1238,7 @@ int socket_listen(SocketAddress *addr, int num, Error **errp)
*/
if (listen(fd, num) != 0) {
error_setg_errno(errp, errno, "Failed to listen on fd socket");
- closesocket(fd);
+ close(fd);
return -1;
}
break;
--
2.39.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] tests: use closesocket()
2023-02-12 20:49 ` [PATCH 1/4] tests: use closesocket() marcandre.lureau
@ 2023-02-13 8:03 ` Thomas Huth
0 siblings, 0 replies; 15+ messages in thread
From: Thomas Huth @ 2023-02-13 8:03 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Jason Wang, qemu-arm,
Stefan Berger
On 12/02/2023 21.49, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Because they are actually sockets...
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> tests/unit/socket-helpers.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tests/unit/socket-helpers.c b/tests/unit/socket-helpers.c
> index eecadf3a3c..914b3aa0cf 100644
> --- a/tests/unit/socket-helpers.c
> +++ b/tests/unit/socket-helpers.c
> @@ -117,13 +117,13 @@ static int socket_can_bind_connect(const char *hostname, int family)
>
> cleanup:
> if (afd != -1) {
> - close(afd);
> + closesocket(afd);
> }
> if (cfd != -1) {
> - close(cfd);
> + closesocket(cfd);
> }
> if (lfd != -1) {
> - close(lfd);
> + closesocket(lfd);
> }
> if (res) {
> freeaddrinfo(res);
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] io: use closesocket()
2023-02-12 20:49 ` [PATCH 2/4] io: " marcandre.lureau
@ 2023-02-13 8:04 ` Thomas Huth
0 siblings, 0 replies; 15+ messages in thread
From: Thomas Huth @ 2023-02-13 8:04 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Jason Wang, qemu-arm,
Stefan Berger
On 12/02/2023 21.49, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Because they are actually sockets...
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> io/channel-socket.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 7aca84f61a..2040297d2b 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -159,7 +159,7 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
>
> trace_qio_channel_socket_connect_complete(ioc, fd);
> if (qio_channel_socket_set_fd(ioc, fd, errp) < 0) {
> - close(fd);
> + closesocket(fd);
> return -1;
> }
>
> @@ -233,7 +233,7 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
>
> trace_qio_channel_socket_listen_complete(ioc, fd);
> if (qio_channel_socket_set_fd(ioc, fd, errp) < 0) {
> - close(fd);
> + closesocket(fd);
> return -1;
> }
> qio_channel_set_feature(QIO_CHANNEL(ioc), QIO_CHANNEL_FEATURE_LISTEN);
> @@ -310,7 +310,7 @@ int qio_channel_socket_dgram_sync(QIOChannelSocket *ioc,
>
> trace_qio_channel_socket_dgram_complete(ioc, fd);
> if (qio_channel_socket_set_fd(ioc, fd, errp) < 0) {
> - close(fd);
> + closesocket(fd);
> return -1;
> }
>
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space
2023-02-12 20:49 ` [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space marcandre.lureau
@ 2023-02-20 11:27 ` Marc-André Lureau
2023-02-20 12:38 ` Markus Armbruster
1 sibling, 0 replies; 15+ messages in thread
From: Marc-André Lureau @ 2023-02-20 11:27 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Thomas Huth,
Jason Wang, qemu-arm, Stefan Berger
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space
2023-02-12 20:49 ` [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space marcandre.lureau
2023-02-20 11:27 ` Marc-André Lureau
@ 2023-02-20 12:38 ` Markus Armbruster
2023-02-20 15:29 ` Marc-André Lureau
1 sibling, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2023-02-20 12:38 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Thomas Huth,
Jason Wang, qemu-arm, Stefan Berger
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;
> }
>
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space
2023-02-20 12:38 ` Markus Armbruster
@ 2023-02-20 15:29 ` Marc-André Lureau
2023-02-20 15:58 ` Daniel P. Berrangé
2023-02-21 8:18 ` Paolo Bonzini
0 siblings, 2 replies; 15+ messages in thread
From: Marc-André Lureau @ 2023-02-20 15:29 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Thomas Huth,
Jason Wang, qemu-arm, Stefan Berger
[-- Attachment #1: Type: text/plain, Size: 15003 bytes --]
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..
[-- Attachment #2: Type: text/html, Size: 20085 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space
2023-02-20 15:29 ` Marc-André Lureau
@ 2023-02-20 15:58 ` Daniel P. Berrangé
2023-02-21 8:18 ` Paolo Bonzini
1 sibling, 0 replies; 15+ messages in thread
From: Daniel P. Berrangé @ 2023-02-20 15:58 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Markus Armbruster, qemu-devel, Peter Maydell, Stefan Weil,
Paolo Bonzini, Joel Stanley, Laurent Vivier, Thomas Huth,
Jason Wang, qemu-arm, Stefan Berger
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 :|
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space
2023-02-20 15:29 ` Marc-André Lureau
2023-02-20 15:58 ` Daniel P. Berrangé
@ 2023-02-21 8:18 ` Paolo Bonzini
2023-02-21 9:12 ` Marc-André Lureau
1 sibling, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2023-02-21 8:18 UTC (permalink / raw)
To: Marc-André Lureau, Markus Armbruster
Cc: qemu-devel, Peter Maydell, Stefan Weil, Daniel P. Berrangé,
Joel Stanley, Laurent Vivier, Thomas Huth, Jason Wang, qemu-arm,
Stefan Berger
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space
2023-02-21 8:18 ` Paolo Bonzini
@ 2023-02-21 9:12 ` Marc-André Lureau
2023-02-21 10:40 ` Marc-André Lureau
0 siblings, 1 reply; 15+ messages in thread
From: Marc-André Lureau @ 2023-02-21 9:12 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Markus Armbruster, qemu-devel, Peter Maydell, Stefan Weil,
Daniel P. Berrangé, Joel Stanley, Laurent Vivier,
Thomas Huth, Jason Wang, qemu-arm, Stefan Berger
[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]
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..
[-- Attachment #2: Type: text/html, Size: 2990 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space
2023-02-21 9:12 ` Marc-André Lureau
@ 2023-02-21 10:40 ` Marc-André Lureau
2023-02-21 10:52 ` Paolo Bonzini
0 siblings, 1 reply; 15+ messages in thread
From: Marc-André Lureau @ 2023-02-21 10:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Markus Armbruster, qemu-devel, Peter Maydell, Stefan Weil,
Daniel P. Berrangé, Joel Stanley, Laurent Vivier,
Thomas Huth, Jason Wang, qemu-arm, Stefan Berger
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space
2023-02-21 10:40 ` Marc-André Lureau
@ 2023-02-21 10:52 ` Paolo Bonzini
0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2023-02-21 10:52 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Markus Armbruster, qemu-devel, Peter Maydell, Stefan Weil,
Daniel P. Berrangé, Joel Stanley, Laurent Vivier,
Thomas Huth, Jason Wang, qemu-arm, Stefan Berger
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-02-21 10:52 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-12 20:49 [PATCH 0/4] win32: do not mix SOCKET and fd space marcandre.lureau
2023-02-12 20:49 ` [PATCH 1/4] tests: use closesocket() marcandre.lureau
2023-02-13 8:03 ` Thomas Huth
2023-02-12 20:49 ` [PATCH 2/4] io: " marcandre.lureau
2023-02-13 8:04 ` Thomas Huth
2023-02-12 20:49 ` [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space marcandre.lureau
2023-02-20 11:27 ` Marc-André Lureau
2023-02-20 12:38 ` Markus Armbruster
2023-02-20 15:29 ` Marc-André Lureau
2023-02-20 15:58 ` Daniel P. Berrangé
2023-02-21 8:18 ` Paolo Bonzini
2023-02-21 9:12 ` Marc-André Lureau
2023-02-21 10:40 ` Marc-André Lureau
2023-02-21 10:52 ` Paolo Bonzini
2023-02-12 20:49 ` [PATCH 4/4] win32: replace closesocket() with close() wrapper marcandre.lureau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).