From b08cc97d6ba4250439829a8a1d476064a1cb54da Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:18 +0100 Subject: sockets: Fix default of UnixSocketAddress member @tight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An optional bool member of a QAPI struct can be false, true, or absent. The previous commit demonstrated that socket_listen() and socket_connect() are broken for absent @tight, and indeed QMP chardev- add also defaults absent member @tight to false instead of true. In C, QAPI members are represented by two fields, has_MEMBER and MEMBER. We have: has_MEMBER MEMBER false true false true true true absent false false/ignore When has_MEMBER is false, MEMBER should be set to false on write, and ignored on read. For QMP, the QAPI visitors handle absent @tight by setting both @has_tight and @tight to false. unix_listen_saddr() and unix_connect_saddr() however use @tight only, disregarding @has_tight. This is wrong and means that absent @tight defaults to false whereas it should default to true. The same is true for @has_abstract, though @abstract defaults to false and therefore has the same behavior for all of QMP, HMP and CLI. Fix unix_listen_saddr() and unix_connect_saddr() to check @has_abstract/@has_tight, and to default absent @tight to true. However, this is only half of the story. HMP chardev-add and CLI -chardev so far correctly defaulted @tight to true, but defaults to false again with the above fix for HMP and CLI. In fact, the "tight" and "abstract" options now break completely. Digging deeper, we find that qemu_chr_parse_socket() also ignores @has_tight, leaving it false when it sets @tight. That is also wrong, but the two wrongs cancelled out. Fix qemu_chr_parse_socket() to set @has_tight and @has_abstract; writing testcases for HMP and CLI is left for another day. Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9 Reported-by: Kevin Wolf Reviewed-by: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- util/qemu-sockets.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'util') diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 38f82179b0..3ceaa81226 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -925,7 +925,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, if (saddr->abstract) { un.sun_path[0] = '\0'; memcpy(&un.sun_path[1], path, pathlen); - if (saddr->tight) { + if (!saddr->has_tight || saddr->tight) { addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen; } } else { @@ -985,7 +985,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) if (saddr->abstract) { un.sun_path[0] = '\0'; memcpy(&un.sun_path[1], saddr->path, pathlen); - if (saddr->tight) { + if (!saddr->has_tight || saddr->tight) { addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen; } } else { -- cgit v1.2.3 From 3b14b4ec49a801067da19d6b8469eb1c1911c020 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:19 +0100 Subject: sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket support" neglected to update socket_sockaddr_to_address_unix(). The function returns a non-abstract socket address for abstract sockets (wrong) with a null @path (also wrong; a non-optional QAPI str member must never be null). The null @path is due to confused code going back all the way to commit 17c55decec "sockets: add helpers for creating SocketAddress from a socket". Add the required special case, and simplify the confused code. Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9 Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- util/qemu-sockets.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) (limited to 'util') diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 3ceaa81226..a578c434c2 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -1270,10 +1270,20 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa, addr = g_new0(SocketAddress, 1); addr->type = SOCKET_ADDRESS_TYPE_UNIX; - if (su->sun_path[0]) { - addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path)); +#ifdef CONFIG_LINUX + if (!su->sun_path[0]) { + /* Linux abstract socket */ + addr->u.q_unix.path = g_strndup(su->sun_path + 1, + sizeof(su->sun_path) - 1); + addr->u.q_unix.has_abstract = true; + addr->u.q_unix.abstract = true; + addr->u.q_unix.has_tight = true; + addr->u.q_unix.tight = salen < sizeof(*su); + return addr; } +#endif + addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path)); return addr; } #endif /* WIN32 */ -- cgit v1.2.3 From ef298e3826e574c712d10e38a5f2a3629d6f5e01 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:21 +0100 Subject: sockets: Bypass "replace empty @path" for abstract unix sockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit unix_listen_saddr() replaces empty @path by unique value. It obtains the value by creating and deleting a unique temporary file with mkstemp(). This is racy, as the comment explains. It's also entirely undocumented as far as I can tell. Goes back to commit d247d25f18 "sockets: helper functions for qemu (Gerd Hoffman)", v0.10.0. Since abstract socket addresses have no connection with filesystem pathnames, making them up with mkstemp() seems inappropriate. Bypass the replacement of empty @path. Reviewed-by: Eric Blake Reviewed-by: Paolo Bonzini Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- util/qemu-sockets.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'util') diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index a578c434c2..671717499f 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -877,7 +877,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, return -1; } - if (saddr->path && saddr->path[0]) { + if (saddr->path[0] || saddr->abstract) { path = saddr->path; } else { const char *tmpdir = getenv("TMPDIR"); -- cgit v1.2.3 From 8acefc79deaab1c7ee2ab07b540b0e3edf0f9f47 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:22 +0100 Subject: sockets: Make abstract UnixSocketAddress depend on CONFIG_LINUX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The abstract socket namespace is a non-portable Linux extension. An attempt to use it elsewhere should fail with ENOENT (the abstract address looks like a "" pathname, which does not resolve). We report this failure like Failed to connect socket abc: No such file or directory Tolerable, although ENOTSUP would be better. However, introspection lies: it has @abstract regardless of host support. Easy enough to fix: since Linux provides them since 2.2, 'if': 'defined(CONFIG_LINUX)' should do. The above failure becomes Parameter 'backend.data.addr.data.abstract' is unexpected I consider this an improvement. Reviewed-by: Paolo Bonzini Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- util/qemu-sockets.c | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) (limited to 'util') diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 671717499f..8af0278f15 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -860,10 +860,29 @@ static int vsock_parse(VsockSocketAddress *addr, const char *str, #ifndef _WIN32 +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; @@ -877,7 +896,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, return -1; } - if (saddr->path[0] || saddr->abstract) { + if (saddr->path[0] || abstract) { path = saddr->path; } else { const char *tmpdir = getenv("TMPDIR"); @@ -887,10 +906,10 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, pathlen = strlen(path); if (pathlen > sizeof(un.sun_path) || - (saddr->abstract && pathlen > (sizeof(un.sun_path) - 1))) { + (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", - saddr->abstract ? sizeof(un.sun_path) - 1 : + abstract ? sizeof(un.sun_path) - 1 : sizeof(un.sun_path)); goto err; } @@ -912,7 +931,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, close(fd); } - if (!saddr->abstract && unlink(path) < 0 && errno != ENOENT) { + if (!abstract && unlink(path) < 0 && errno != ENOENT) { error_setg_errno(errp, errno, "Failed to unlink socket %s", path); goto err; @@ -922,10 +941,10 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, un.sun_family = AF_UNIX; addrlen = sizeof(un); - if (saddr->abstract) { + if (abstract) { un.sun_path[0] = '\0'; memcpy(&un.sun_path[1], path, pathlen); - if (!saddr->has_tight || saddr->tight) { + if (saddr_is_tight(saddr)) { addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen; } } else { @@ -952,6 +971,7 @@ err: 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; @@ -970,10 +990,10 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) pathlen = strlen(saddr->path); if (pathlen > sizeof(un.sun_path) || - (saddr->abstract && pathlen > (sizeof(un.sun_path) - 1))) { + (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", - saddr->abstract ? sizeof(un.sun_path) - 1 : + abstract ? sizeof(un.sun_path) - 1 : sizeof(un.sun_path)); goto err; } @@ -982,10 +1002,10 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) un.sun_family = AF_UNIX; addrlen = sizeof(un); - if (saddr->abstract) { + if (abstract) { un.sun_path[0] = '\0'; memcpy(&un.sun_path[1], saddr->path, pathlen); - if (!saddr->has_tight || saddr->tight) { + if (saddr_is_tight(saddr)) { addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen; } } else { -- cgit v1.2.3