[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/3] win32: add qemu_close_socket_osfhandle()
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v2 1/3] win32: add qemu_close_socket_osfhandle() |
Date: |
Mon, 20 Mar 2023 13:52:55 +0000 |
User-agent: |
Mutt/2.2.9 (2022-11-12) |
On Mon, Mar 20, 2023 at 05:46:01PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Mon, Mar 20, 2023 at 5:42 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
> > On Mon, Mar 20, 2023 at 05:36:41PM +0400, marcandre.lureau@redhat.com
> > wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Close the given file descriptor, but returns the underlying SOCKET.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > > include/sysemu/os-win32.h | 15 ++++++--
> > > util/oslib-win32.c | 75 +++++++++++++++++++++------------------
> > > 2 files changed, 53 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> > > index e2849f88ab..15c296e0eb 100644
> > > --- a/include/sysemu/os-win32.h
> > > +++ b/include/sysemu/os-win32.h
> > > @@ -171,10 +171,21 @@ bool qemu_socket_select(int sockfd, WSAEVENT
> > hEventObject,
> > >
> > > bool qemu_socket_unselect(int sockfd, Error **errp);
> > >
> > > -/* We wrap all the sockets functions so that we can
> > > - * set errno based on WSAGetLastError()
> > > +/* We wrap all the sockets functions so that we can set errno based on
> > > + * WSAGetLastError(), and use file-descriptors instead of SOCKET.
> > > */
> > >
> > > +/*
> > > + * qemu_close_socket_osfhandle:
> > > + * @fd: a file descriptor associated with a SOCKET
> > > + *
> > > + * Close only the C run-time file descriptor, leave the SOCKET opened.
> > > + *
> > > + * Returns zero on success. On error, -1 is returned, and errno is set
> > to
> > > + * indicate the error.
> > > + */
> > > +int qemu_close_socket_osfhandle(int fd);
> > > +
> > > #undef close
> > > #define close qemu_close_wrap
> > > int qemu_close_wrap(int fd);
> > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> > > index 16f8a67f7e..a98638729a 100644
> > > --- a/util/oslib-win32.c
> > > +++ b/util/oslib-win32.c
> > > @@ -479,40 +479,27 @@ int qemu_bind_wrap(int sockfd, const struct
> > sockaddr *addr,
> > > return ret;
> > > }
> > >
> > > -
> > > #undef close
> > > -int qemu_close_wrap(int fd)
> > > +int qemu_close_socket_osfhandle(int fd)
> > > {
> > > - int ret;
> > > + SOCKET s = _get_osfhandle(fd);
> > > DWORD flags = 0;
> > > - SOCKET s = INVALID_SOCKET;
> > > -
> > > - if (fd_is_socket(fd)) {
> > > - s = _get_osfhandle(fd);
> > > -
> > > - /*
> > > - * If we were to just call _close on the descriptor, it would
> > close the
> > > - * HANDLE, but it wouldn't free any of the resources associated
> > to the
> > > - * SOCKET, and we can't call _close after calling closesocket,
> > because
> > > - * closesocket has already closed the HANDLE, and _close would
> > attempt to
> > > - * close the HANDLE again, resulting in a double free. We can
> > however
> > > - * protect the HANDLE from actually being closed long enough to
> > close the
> > > - * file descriptor, then close the socket itself.
> > > - */
> > > - if (!GetHandleInformation((HANDLE)s, &flags)) {
> > > - errno = EACCES;
> > > - return -1;
> > > - }
> > >
> > > - if (!SetHandleInformation((HANDLE)s,
> > HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
> > > - errno = EACCES;
> > > - return -1;
> > > - }
> > > + /*
> > > + * If we were to just call _close on the descriptor, it would close
> > the
> > > + * HANDLE, but it wouldn't free any of the resources associated to
> > the
> > > + * SOCKET, and we can't call _close after calling closesocket,
> > because
> > > + * closesocket has already closed the HANDLE, and _close would
> > attempt to
> > > + * close the HANDLE again, resulting in a double free. We can
> > however
> > > + * protect the HANDLE from actually being closed long enough to
> > close the
> > > + * file descriptor, then close the socket itself.
> > > + */
> > > + if (!GetHandleInformation((HANDLE)s, &flags)) {
> > > + errno = EACCES;
> > > + return -1;
> > > }
> > >
> > > - ret = close(fd);
> > > -
> > > - if (s != INVALID_SOCKET && !SetHandleInformation((HANDLE)s, flags,
> > flags)) {
> > > + if (!SetHandleInformation((HANDLE)s,
> > HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
> > > errno = EACCES;
> > > return -1;
> > > }
> > > @@ -521,15 +508,33 @@ int qemu_close_wrap(int fd)
> > > * close() returns EBADF since we PROTECT_FROM_CLOSE the underlying
> > handle,
> > > * but the FD is actually freed
> > > */
> > > - if (ret < 0 && (s == INVALID_SOCKET || errno != EBADF)) {
> > > - return ret;
> > > + if (close(fd) < 0 && errno != EBADF) {
> > > + return -1;
> > > }
> > >
> > > - if (s != INVALID_SOCKET) {
> > > - ret = closesocket(s);
> > > - if (ret < 0) {
> > > - errno = socket_error();
> > > - }
> > > + if (!SetHandleInformation((HANDLE)s, flags, flags)) {
> > > + errno = EACCES;
> > > + return -1;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int qemu_close_wrap(int fd)
> > > +{
> > > + SOCKET s = INVALID_SOCKET;
> > > + int ret = -1;
> > > +
> > > + if (!fd_is_socket(fd)) {
> > > + return close(fd);
> > > + }
> > > +
> > > + s = _get_osfhandle(fd);
> > > + qemu_close_socket_osfhandle(fd);
> > > +
> > > + ret = closesocket(s);
> > > + if (ret < 0) {
> > > + errno = socket_error();
> > > }
> >
> > Shouldn't the closesocket() and return check be wrapped in
> >
> > if (s != INVALID_SOCKET) { .... }
> >
> >
> We shouldn't get there since fd_is_socket().
Oh right, yes, fd_is_socket() will evaluate false if the 'fd' is
invalid. eg qemu_clsoe_wrap(-1)
> > as the old code had that conditional invokation of closesocket() ?
> >
>
> The v1 code could actually leak a SOCKET. This version should be a bit
> better, if the FD is already closed for example, we still close the SOCKET.
>
> Open to ideas to improve it.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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 :|