| Message ID | 20201029133833.3450220-12-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | sockets: Attempt to drain the abstract socket swamp | expand |
On 10/29/20 8:38 AM, Markus Armbruster wrote: > 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. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > +++ b/qapi/sockets.json > @@ -74,18 +74,20 @@ > # Captures a socket address in the local ("Unix socket") namespace. > # > # @path: filesystem path to use > -# @tight: pass a socket address length confined to the minimum length of the > -# abstract string, rather than the full sockaddr_un record length > -# (only matters for abstract sockets, default true). (Since 5.1) > -# @abstract: whether this is an abstract address, default false. (Since 5.1) > +# @abstract: if true, this is a Linux abstract socket address. @path > +# will be prefixed by a null byte, and optionally padded > +# with null bytes. Defaults to false. (Since 5.1) > +# @tight: if false, pad an abstract socket address with enough null > +# bytes to make it fill struct sockaddr_un member sun_path. > +# Defaults to true. (Since 5.1) Do we need to mention that @tight is ignored (or even make it an error) if @abstract is false? > # > # Since: 1.3 > ## > { 'struct': 'UnixSocketAddress', > 'data': { > 'path': 'str', > - '*tight': 'bool', > - '*abstract': 'bool' } } > + '*tight': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' }, > + '*abstract': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' } } } So we document @abstract before @tight, but declare them in reverse order. I guess our doc generator doesn't care? > > ## > # @VsockSocketAddress: > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index dc1cf86ecf..1d2b2efb13 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix) > break; > case SOCKET_ADDRESS_TYPE_UNIX: > { > +#ifdef CONFIG_LINUX > UnixSocketAddress *sa = &s->addr->u.q_unix; > +#endif > > return g_strdup_printf("%sunix:%s%s%s%s", prefix, > s->addr->u.q_unix.path, Why did we need the #ifdef above, which means we can't we use sa here? > +#ifdef CONFIG_LINUX > sa->has_abstract && sa->abstract I hate mid-()-expression #ifdefs. If g_strdup_printf() were itself a macro expansion, things break. Can you come up with a saner way of writing this? > ? ",abstract" : "", > sa->has_tight && sa->tight > ? ",tight" : "", > +#else > + "", "", > +#endif > s->is_listen ? ",server" : ""); I suggest: const char *tight = "", *abstract = ""; UnixSocketAddress *sa = &s->addr->u.q_unix; #ifdef CONFIG_LINUX if (sa->has_abstract && sa->abstract) { abstract = ",abstract"; if (sa->has_tight && sa->tight) { tight = ",tight"; } } #endif return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path, abstract, tight, s->is_listen ? ", server" : ""); > +++ b/util/qemu-sockets.c > @@ -854,10 +854,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; Should this also look at abstract? > +#else > + return false; > +#endif > +} > + Is it any easier to split the patch, first into the introduction of saddr_is_* and adjusting all clients, and second into adding the 'if' to the QAPI declaration? But the idea makes sense.
Eric Blake <eblake@redhat.com> writes: > On 10/29/20 8:38 AM, Markus Armbruster wrote: >> 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. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- > >> +++ b/qapi/sockets.json >> @@ -74,18 +74,20 @@ >> # Captures a socket address in the local ("Unix socket") namespace. >> # >> # @path: filesystem path to use >> -# @tight: pass a socket address length confined to the minimum length of the >> -# abstract string, rather than the full sockaddr_un record length >> -# (only matters for abstract sockets, default true). (Since 5.1) >> -# @abstract: whether this is an abstract address, default false. (Since 5.1) >> +# @abstract: if true, this is a Linux abstract socket address. @path >> +# will be prefixed by a null byte, and optionally padded >> +# with null bytes. Defaults to false. (Since 5.1) >> +# @tight: if false, pad an abstract socket address with enough null >> +# bytes to make it fill struct sockaddr_un member sun_path. >> +# Defaults to true. (Since 5.1) > > Do we need to mention that @tight is ignored (or even make it an error) > if @abstract is false? We could make presence of @tight an error unless @abstract is true. But again, this series aims for less wrong, not for less ugly. For me, the description "if false, pad an abstract socket address..." implies "no effect when the socket address is not abstract". If you'd like to suggest a clearer phrasing, go right ahead. >> # >> # Since: 1.3 >> ## >> { 'struct': 'UnixSocketAddress', >> 'data': { >> 'path': 'str', >> - '*tight': 'bool', >> - '*abstract': 'bool' } } >> + '*tight': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' }, >> + '*abstract': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' } } } > > So we document @abstract before @tight, but declare them in reverse > order. I guess our doc generator doesn't care? It doesn't. I flipped them in the comment for readability, but neglected to flop the code. Flipping them now. >> >> ## >> # @VsockSocketAddress: >> diff --git a/chardev/char-socket.c b/chardev/char-socket.c >> index dc1cf86ecf..1d2b2efb13 100644 >> --- a/chardev/char-socket.c >> +++ b/chardev/char-socket.c >> @@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix) >> break; >> case SOCKET_ADDRESS_TYPE_UNIX: >> { >> +#ifdef CONFIG_LINUX >> UnixSocketAddress *sa = &s->addr->u.q_unix; >> +#endif >> >> return g_strdup_printf("%sunix:%s%s%s%s", prefix, >> s->addr->u.q_unix.path, > > Why did we need the #ifdef above, which means we can't we use sa here? Tunnel vision. I'll simplify. >> +#ifdef CONFIG_LINUX >> sa->has_abstract && sa->abstract > > I hate mid-()-expression #ifdefs. If g_strdup_printf() were itself a > macro expansion, things break. Can you come up with a saner way of > writing this? > >> ? ",abstract" : "", >> sa->has_tight && sa->tight >> ? ",tight" : "", >> +#else >> + "", "", >> +#endif >> s->is_listen ? ",server" : ""); > > I suggest: > > const char *tight = "", *abstract = ""; > UnixSocketAddress *sa = &s->addr->u.q_unix; > > #ifdef CONFIG_LINUX > if (sa->has_abstract && sa->abstract) { > abstract = ",abstract"; > if (sa->has_tight && sa->tight) { > tight = ",tight"; > } > } > #endif > > return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path, > abstract, tight, > s->is_listen ? ", server" : ""); I don't care either way, so I'm taking yours. >> +++ b/util/qemu-sockets.c >> @@ -854,10 +854,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; > > Should this also look at abstract? It's used in just two places, both guarded by if (abstract). I added the helpers only because the code creating a struct sockaddr_un is duplicated, and de-duplication is too hard to bother due to the (racy) "if path="" pick one" feature. >> +#else >> + return false; >> +#endif >> +} >> + > > Is it any easier to split the patch, first into the introduction of > saddr_is_* and adjusting all clients, and second into adding the 'if' to > the QAPI declaration? I doubt it. But If you guys think it makes the patch easier to understand, I'll gladly do it. > But the idea makes sense. Thanks!
diff --git a/qapi/sockets.json b/qapi/sockets.json index c0c640a5b0..db4019306a 100644 --- a/qapi/sockets.json +++ b/qapi/sockets.json @@ -74,18 +74,20 @@ # Captures a socket address in the local ("Unix socket") namespace. # # @path: filesystem path to use -# @tight: pass a socket address length confined to the minimum length of the -# abstract string, rather than the full sockaddr_un record length -# (only matters for abstract sockets, default true). (Since 5.1) -# @abstract: whether this is an abstract address, default false. (Since 5.1) +# @abstract: if true, this is a Linux abstract socket address. @path +# will be prefixed by a null byte, and optionally padded +# with null bytes. Defaults to false. (Since 5.1) +# @tight: if false, pad an abstract socket address with enough null +# bytes to make it fill struct sockaddr_un member sun_path. +# Defaults to true. (Since 5.1) # # Since: 1.3 ## { 'struct': 'UnixSocketAddress', 'data': { 'path': 'str', - '*tight': 'bool', - '*abstract': 'bool' } } + '*tight': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' }, + '*abstract': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' } } } ## # @VsockSocketAddress: diff --git a/chardev/char-socket.c b/chardev/char-socket.c index dc1cf86ecf..1d2b2efb13 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix) break; case SOCKET_ADDRESS_TYPE_UNIX: { +#ifdef CONFIG_LINUX UnixSocketAddress *sa = &s->addr->u.q_unix; +#endif return g_strdup_printf("%sunix:%s%s%s%s", prefix, s->addr->u.q_unix.path, +#ifdef CONFIG_LINUX sa->has_abstract && sa->abstract ? ",abstract" : "", sa->has_tight && sa->tight ? ",tight" : "", +#else + "", "", +#endif s->is_listen ? ",server" : ""); break; } @@ -1394,8 +1400,10 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, const char *host = qemu_opt_get(opts, "host"); const char *port = qemu_opt_get(opts, "port"); const char *fd = qemu_opt_get(opts, "fd"); +#ifdef CONFIG_LINUX bool tight = qemu_opt_get_bool(opts, "tight", true); bool abstract = qemu_opt_get_bool(opts, "abstract", false); +#endif SocketAddressLegacy *addr; ChardevSocket *sock; @@ -1447,10 +1455,12 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, addr->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX; q_unix = addr->u.q_unix.data = g_new0(UnixSocketAddress, 1); q_unix->path = g_strdup(path); +#ifdef CONFIG_LINUX q_unix->has_tight = true; q_unix->tight = tight; q_unix->has_abstract = true; q_unix->abstract = abstract; +#endif } else if (host) { addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET; addr->u.inet.data = g_new(InetSocketAddress, 1); diff --git a/chardev/char.c b/chardev/char.c index 78553125d3..aa4282164a 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -928,6 +928,7 @@ QemuOptsList qemu_chardev_opts = { },{ .name = "logappend", .type = QEMU_OPT_BOOL, +#ifdef CONFIG_LINUX },{ .name = "tight", .type = QEMU_OPT_BOOL, @@ -935,6 +936,7 @@ QemuOptsList qemu_chardev_opts = { },{ .name = "abstract", .type = QEMU_OPT_BOOL, +#endif }, { /* end of list */ } }, diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index 7ecf95579b..67486055ed 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -229,7 +229,7 @@ static void test_socket_fd_pass_num_nocli(void) } #endif -#ifdef __linux__ +#ifdef CONFIG_LINUX #define ABSTRACT_SOCKET_VARIANTS 3 @@ -326,7 +326,8 @@ static void test_socket_unix_abstract(void) g_free(addr.u.q_unix.path); } -#endif + +#endif /* CONFIG_LINUX */ int main(int argc, char **argv) { @@ -368,7 +369,7 @@ int main(int argc, char **argv) #endif } -#ifdef __linux__ +#ifdef CONFIG_LINUX g_test_add_func("/util/socket/unix-abstract", test_socket_unix_abstract); #endif diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 18c8de8cdb..f8553976e6 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -854,10 +854,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; @@ -871,7 +890,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"); @@ -881,10 +900,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; } @@ -906,7 +925,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; @@ -916,10 +935,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 { @@ -946,6 +965,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; @@ -964,10 +984,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; } @@ -976,10 +996,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 {
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. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qapi/sockets.json | 14 ++++++++------ chardev/char-socket.c | 10 ++++++++++ chardev/char.c | 2 ++ tests/test-util-sockets.c | 7 ++++--- util/qemu-sockets.c | 40 +++++++++++++++++++++++++++++---------- 5 files changed, 54 insertions(+), 19 deletions(-)