From: "Daniel P. Berrangé" <berrange@redhat.com>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: qemu-devel@nongnu.org, Michael Tokarev <mjt@tls.msk.ru>
Subject: Re: [RFC PATCH] util: split unix socket functions out of qemu-sockets
Date: Wed, 21 May 2025 11:15:01 +0100 [thread overview]
Message-ID: <aC2npW8LB347lwr3@redhat.com> (raw)
In-Reply-To: <20250520165706.3976971-1-alex.bennee@linaro.org>
On Tue, May 20, 2025 at 05:57:06PM +0100, Alex Bennée wrote:
> Since fccb744f41 (gdbstub: Try unlinking the unix socket before
> binding) we use the unix_listen() function from linux-user which
> causes complications when trying to build statically.
>
> Fix this by splitting the unix functions into its own file and doing
> the appropriate tweaks to the headers.
I think I'd be more inclined to just revert the original problem
patch. The original stated problem only seems to need a single
'unlink' call being added in the gdb stub, avoiding the whole
problem with linking. I'm not really a fan of arbitrarily
splitting the up the source files as a workaround.
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
> include/qemu/sockets.h | 1 +
> util/socket-helpers.h | 17 ++++
> util/qemu-sockets.c | 199 +--------------------------------------
> util/unix-sockets.c | 207 +++++++++++++++++++++++++++++++++++++++++
> util/meson.build | 5 +-
> 5 files changed, 231 insertions(+), 198 deletions(-)
> create mode 100644 util/socket-helpers.h
> create mode 100644 util/unix-sockets.c
>
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index c562690d89..578aef13cf 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -65,6 +65,7 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp);
>
> NetworkAddressFamily inet_netfamily(int family);
>
> +/* part of unix-sockets.c */
> int unix_listen(const char *path, Error **errp);
> int unix_connect(const char *path, Error **errp);
>
> diff --git a/util/socket-helpers.h b/util/socket-helpers.h
> new file mode 100644
> index 0000000000..f72925148a
> --- /dev/null
> +++ b/util/socket-helpers.h
> @@ -0,0 +1,17 @@
> +/*
> + * Common helper functions for unix and qemu sockets
> + *
> + * (c) 2008 Gerd Hoffmann <kraxel@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef _SOCKET_HELPERS_H_
> +#define _SOCKET_HELPERS_H_
> +
> +#include "qapi/qapi-visit-sockets.h"
> +
> +int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp);
> +int unix_listen_saddr(UnixSocketAddress *saddr, int num, Error **errp);
> +
> +#endif /* _SOCKET_HELPERS_H_ */
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 77477c1cd5..a5c3515082 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1,5 +1,5 @@
> /*
> - * inet and unix socket functions for qemu
> + * inet socket functions for qemu
> *
> * (c) 2008 Gerd Hoffmann <kraxel@redhat.com>
> *
> @@ -30,6 +30,7 @@
> #include "qapi/qobject-input-visitor.h"
> #include "qapi/qobject-output-visitor.h"
> #include "qemu/cutils.h"
> +#include "socket-helpers.h"
> #include "trace.h"
>
> #ifndef AI_ADDRCONFIG
> @@ -853,202 +854,6 @@ static int vsock_parse(VsockSocketAddress *addr, const char *str,
> }
> #endif /* CONFIG_AF_VSOCK */
>
> -static bool saddr_is_abstract(UnixSocketAddress *saddr)
> -{
> -#ifdef CONFIG_LINUX
> - return saddr->abstract;
> -#else
> - return false;
> -#endif
> -}
> -
> -static bool saddr_is_tight(UnixSocketAddress *saddr)
> -{
> -#ifdef CONFIG_LINUX
> - return !saddr->has_tight || saddr->tight;
> -#else
> - return false;
> -#endif
> -}
> -
> -static int unix_listen_saddr(UnixSocketAddress *saddr,
> - int num,
> - Error **errp)
> -{
> - bool abstract = saddr_is_abstract(saddr);
> - struct sockaddr_un un;
> - int sock, fd;
> - char *pathbuf = NULL;
> - const char *path;
> - size_t pathlen;
> - size_t addrlen;
> -
> - sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
> - if (sock < 0) {
> - error_setg_errno(errp, errno, "Failed to create Unix socket");
> - return -1;
> - }
> -
> - if (saddr->path[0] || abstract) {
> - path = saddr->path;
> - } else {
> - path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX",
> - g_get_tmp_dir());
> - }
> -
> - pathlen = strlen(path);
> - if (pathlen > sizeof(un.sun_path) ||
> - (abstract && pathlen > (sizeof(un.sun_path) - 1))) {
> - error_setg(errp, "UNIX socket path '%s' is too long", path);
> - error_append_hint(errp, "Path must be less than %zu bytes\n",
> - abstract ? sizeof(un.sun_path) - 1 :
> - sizeof(un.sun_path));
> - goto err;
> - }
> -
> - if (pathbuf != NULL) {
> - /*
> - * This dummy fd usage silences the mktemp() insecure warning.
> - * Using mkstemp() doesn't make things more secure here
> - * though. bind() complains about existing files, so we have
> - * to unlink first and thus re-open the race window. The
> - * worst case possible is bind() failing, i.e. a DoS attack.
> - */
> - fd = mkstemp(pathbuf);
> - if (fd < 0) {
> - error_setg_errno(errp, errno,
> - "Failed to make a temporary socket %s", pathbuf);
> - goto err;
> - }
> - close(fd);
> - }
> -
> - if (!abstract && unlink(path) < 0 && errno != ENOENT) {
> - error_setg_errno(errp, errno,
> - "Failed to unlink socket %s", path);
> - goto err;
> - }
> -
> - memset(&un, 0, sizeof(un));
> - un.sun_family = AF_UNIX;
> - addrlen = sizeof(un);
> -
> - if (abstract) {
> - un.sun_path[0] = '\0';
> - memcpy(&un.sun_path[1], path, pathlen);
> - if (saddr_is_tight(saddr)) {
> - addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
> - }
> - } else {
> - memcpy(un.sun_path, path, pathlen);
> - }
> -
> - if (bind(sock, (struct sockaddr *) &un, addrlen) < 0) {
> - error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
> - goto err;
> - }
> - if (listen(sock, num) < 0) {
> - error_setg_errno(errp, errno, "Failed to listen on socket");
> - goto err;
> - }
> -
> - g_free(pathbuf);
> - return sock;
> -
> -err:
> - g_free(pathbuf);
> - close(sock);
> - return -1;
> -}
> -
> -static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
> -{
> - bool abstract = saddr_is_abstract(saddr);
> - struct sockaddr_un un;
> - int sock, rc;
> - size_t pathlen;
> - size_t addrlen;
> -
> - if (saddr->path == NULL) {
> - error_setg(errp, "unix connect: no path specified");
> - return -1;
> - }
> -
> - sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
> - if (sock < 0) {
> - error_setg_errno(errp, errno, "Failed to create socket");
> - return -1;
> - }
> -
> - pathlen = strlen(saddr->path);
> - if (pathlen > sizeof(un.sun_path) ||
> - (abstract && pathlen > (sizeof(un.sun_path) - 1))) {
> - error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
> - error_append_hint(errp, "Path must be less than %zu bytes\n",
> - abstract ? sizeof(un.sun_path) - 1 :
> - sizeof(un.sun_path));
> - goto err;
> - }
> -
> - memset(&un, 0, sizeof(un));
> - un.sun_family = AF_UNIX;
> - addrlen = sizeof(un);
> -
> - if (abstract) {
> - un.sun_path[0] = '\0';
> - memcpy(&un.sun_path[1], saddr->path, pathlen);
> - if (saddr_is_tight(saddr)) {
> - addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
> - }
> - } else {
> - memcpy(un.sun_path, saddr->path, pathlen);
> - }
> - /* connect to peer */
> - do {
> - rc = 0;
> - if (connect(sock, (struct sockaddr *) &un, addrlen) < 0) {
> - rc = -errno;
> - }
> - } while (rc == -EINTR);
> -
> - if (rc < 0) {
> - error_setg_errno(errp, -rc, "Failed to connect to '%s'",
> - saddr->path);
> - goto err;
> - }
> -
> - return sock;
> -
> - err:
> - close(sock);
> - return -1;
> -}
> -
> -/* compatibility wrapper */
> -int unix_listen(const char *str, Error **errp)
> -{
> - UnixSocketAddress *saddr;
> - int sock;
> -
> - saddr = g_new0(UnixSocketAddress, 1);
> - saddr->path = g_strdup(str);
> - sock = unix_listen_saddr(saddr, 1, errp);
> - qapi_free_UnixSocketAddress(saddr);
> - return sock;
> -}
> -
> -int unix_connect(const char *path, Error **errp)
> -{
> - UnixSocketAddress *saddr;
> - int sock;
> -
> - saddr = g_new0(UnixSocketAddress, 1);
> - saddr->path = g_strdup(path);
> - sock = unix_connect_saddr(saddr, errp);
> - qapi_free_UnixSocketAddress(saddr);
> - return sock;
> -}
> -
> char *socket_uri(SocketAddress *addr)
> {
> switch (addr->type) {
> diff --git a/util/unix-sockets.c b/util/unix-sockets.c
> new file mode 100644
> index 0000000000..e271dd3e09
> --- /dev/null
> +++ b/util/unix-sockets.c
> @@ -0,0 +1,207 @@
> +/*
> + * unix socket functions for qemu
> + *
> + * (c) 2008 Gerd Hoffmann <kraxel@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/sockets.h"
> +#include "qapi/error.h"
> +
> +#include "socket-helpers.h"
> +
> +static bool saddr_is_abstract(UnixSocketAddress *saddr)
> +{
> +#ifdef CONFIG_LINUX
> + return saddr->abstract;
> +#else
> + return false;
> +#endif
> +}
> +
> +static bool saddr_is_tight(UnixSocketAddress *saddr)
> +{
> +#ifdef CONFIG_LINUX
> + return !saddr->has_tight || saddr->tight;
> +#else
> + return false;
> +#endif
> +}
> +
> +int unix_listen_saddr(UnixSocketAddress *saddr, int num, Error **errp)
> +{
> + bool abstract = saddr_is_abstract(saddr);
> + struct sockaddr_un un;
> + int sock, fd;
> + char *pathbuf = NULL;
> + const char *path;
> + size_t pathlen;
> + size_t addrlen;
> +
> + sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
> + if (sock < 0) {
> + error_setg_errno(errp, errno, "Failed to create Unix socket");
> + return -1;
> + }
> +
> + if (saddr->path[0] || abstract) {
> + path = saddr->path;
> + } else {
> + path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX",
> + g_get_tmp_dir());
> + }
> +
> + pathlen = strlen(path);
> + if (pathlen > sizeof(un.sun_path) ||
> + (abstract && pathlen > (sizeof(un.sun_path) - 1))) {
> + error_setg(errp, "UNIX socket path '%s' is too long", path);
> + error_append_hint(errp, "Path must be less than %zu bytes\n",
> + abstract ? sizeof(un.sun_path) - 1 :
> + sizeof(un.sun_path));
> + goto err;
> + }
> +
> + if (pathbuf != NULL) {
> + /*
> + * This dummy fd usage silences the mktemp() insecure warning.
> + * Using mkstemp() doesn't make things more secure here
> + * though. bind() complains about existing files, so we have
> + * to unlink first and thus re-open the race window. The
> + * worst case possible is bind() failing, i.e. a DoS attack.
> + */
> + fd = mkstemp(pathbuf);
> + if (fd < 0) {
> + error_setg_errno(errp, errno,
> + "Failed to make a temporary socket %s", pathbuf);
> + goto err;
> + }
> + close(fd);
> + }
> +
> + if (!abstract && unlink(path) < 0 && errno != ENOENT) {
> + error_setg_errno(errp, errno,
> + "Failed to unlink socket %s", path);
> + goto err;
> + }
> +
> + memset(&un, 0, sizeof(un));
> + un.sun_family = AF_UNIX;
> + addrlen = sizeof(un);
> +
> + if (abstract) {
> + un.sun_path[0] = '\0';
> + memcpy(&un.sun_path[1], path, pathlen);
> + if (saddr_is_tight(saddr)) {
> + addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
> + }
> + } else {
> + memcpy(un.sun_path, path, pathlen);
> + }
> +
> + if (bind(sock, (struct sockaddr *) &un, addrlen) < 0) {
> + error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
> + goto err;
> + }
> + if (listen(sock, num) < 0) {
> + error_setg_errno(errp, errno, "Failed to listen on socket");
> + goto err;
> + }
> +
> + g_free(pathbuf);
> + return sock;
> +
> +err:
> + g_free(pathbuf);
> + close(sock);
> + return -1;
> +}
> +
> +int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
> +{
> + bool abstract = saddr_is_abstract(saddr);
> + struct sockaddr_un un;
> + int sock, rc;
> + size_t pathlen;
> + size_t addrlen;
> +
> + if (saddr->path == NULL) {
> + error_setg(errp, "unix connect: no path specified");
> + return -1;
> + }
> +
> + sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
> + if (sock < 0) {
> + error_setg_errno(errp, errno, "Failed to create socket");
> + return -1;
> + }
> +
> + pathlen = strlen(saddr->path);
> + if (pathlen > sizeof(un.sun_path) ||
> + (abstract && pathlen > (sizeof(un.sun_path) - 1))) {
> + error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
> + error_append_hint(errp, "Path must be less than %zu bytes\n",
> + abstract ? sizeof(un.sun_path) - 1 :
> + sizeof(un.sun_path));
> + goto err;
> + }
> +
> + memset(&un, 0, sizeof(un));
> + un.sun_family = AF_UNIX;
> + addrlen = sizeof(un);
> +
> + if (abstract) {
> + un.sun_path[0] = '\0';
> + memcpy(&un.sun_path[1], saddr->path, pathlen);
> + if (saddr_is_tight(saddr)) {
> + addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
> + }
> + } else {
> + memcpy(un.sun_path, saddr->path, pathlen);
> + }
> + /* connect to peer */
> + do {
> + rc = 0;
> + if (connect(sock, (struct sockaddr *) &un, addrlen) < 0) {
> + rc = -errno;
> + }
> + } while (rc == -EINTR);
> +
> + if (rc < 0) {
> + error_setg_errno(errp, -rc, "Failed to connect to '%s'",
> + saddr->path);
> + goto err;
> + }
> +
> + return sock;
> +
> + err:
> + close(sock);
> + return -1;
> +}
> +
> +/* compatibility wrapper */
> +int unix_listen(const char *str, Error **errp)
> +{
> + UnixSocketAddress *saddr;
> + int sock;
> +
> + saddr = g_new0(UnixSocketAddress, 1);
> + saddr->path = g_strdup(str);
> + sock = unix_listen_saddr(saddr, 1, errp);
> + qapi_free_UnixSocketAddress(saddr);
> + return sock;
> +}
> +
> +int unix_connect(const char *path, Error **errp)
> +{
> + UnixSocketAddress *saddr;
> + int sock;
> +
> + saddr = g_new0(UnixSocketAddress, 1);
> + saddr->path = g_strdup(path);
> + sock = unix_connect_saddr(saddr, errp);
> + qapi_free_UnixSocketAddress(saddr);
> + return sock;
> +}
> diff --git a/util/meson.build b/util/meson.build
> index 1adff96ebd..dbcfbc6542 100644
> --- a/util/meson.build
> +++ b/util/meson.build
> @@ -87,7 +87,10 @@ if have_block or have_ga
> util_ss.add(files(f'coroutine-@coroutine_backend@.c'))
> util_ss.add(files('thread-pool.c', 'qemu-timer.c'))
> endif
> -if have_block or have_ga or have_user
> +if have_user
> + util_ss.add(files('unix-sockets.c'))
> +endif
> +if have_block or have_ga
> util_ss.add(files('qemu-sockets.c'))
> endif
> if have_block
> --
> 2.39.5
>
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 :|
next prev parent reply other threads:[~2025-05-21 10:16 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-20 16:57 [RFC PATCH] util: split unix socket functions out of qemu-sockets Alex Bennée
2025-05-21 10:15 ` Daniel P. Berrangé [this message]
2025-05-22 14:36 ` Ilya Leoshkevich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aC2npW8LB347lwr3@redhat.com \
--to=berrange@redhat.com \
--cc=alex.bennee@linaro.org \
--cc=mjt@tls.msk.ru \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.