Hi On Mon, Feb 20, 2023 at 4:38 PM Markus Armbruster <[email protected]> wrote:
> [email protected] writes: > > > From: Marc-André Lureau <[email protected]> > > > > 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 <[email protected]> > > --- > > 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..
