| Message ID | 20201029133833.3450220-9-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | sockets: Attempt to drain the abstract socket swamp | expand |
On 29/10/20 14:38, Markus Armbruster wrote: > + /* 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 = !su->sun_path[sizeof(su->sun_path) - 1]; > + return addr; I think this should be addr->u.q_unit.tight = salen < sizeof(*su); Paolo
On 10/29/20 8:38 AM, Markus Armbruster wrote: > 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 > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > util/qemu-sockets.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index c802d5aa0a..801c5e3957 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -1264,10 +1264,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 = !su->sun_path[sizeof(su->sun_path) - 1]; This is questionable - how can you tell from the last byte whether the name was created as tight or not? > + return addr; > } > +#endif > > + addr->u.q_unix.path = g_strdup(su->sun_path); This is wrong on at least Linux, where su->sun_path need not be NUL-terminated (allowing file-system Unix sockets to have one more byte in their name); you need the strndup that you replaced above, in order avoid reading beyond the end of the array. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Paolo Bonzini <pbonzini@redhat.com> writes: > On 29/10/20 14:38, Markus Armbruster wrote: >> + /* 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 = !su->sun_path[sizeof(su->sun_path) - 1]; >> + return addr; > > I think this should be > > addr->u.q_unit.tight = salen < sizeof(*su); > > Paolo You're right, my code is wrong. The case "@path just fits" is ambiguous: @tight doesn't matter then. Your code arbitrarily picks tight=false then. Picking the default tight=true would perhaps be a bit nicer. Not worth complicating the code.
Eric Blake <eblake@redhat.com> writes: > On 10/29/20 8:38 AM, Markus Armbruster wrote: >> 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 >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> util/qemu-sockets.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c >> index c802d5aa0a..801c5e3957 100644 >> --- a/util/qemu-sockets.c >> +++ b/util/qemu-sockets.c >> @@ -1264,10 +1264,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 = !su->sun_path[sizeof(su->sun_path) - 1]; > > This is questionable - how can you tell from the last byte whether the > name was created as tight or not? I plead temporary insanity. See my reply to Paolo. >> + return addr; >> } >> +#endif >> >> + addr->u.q_unix.path = g_strdup(su->sun_path); > > This is wrong on at least Linux, where su->sun_path need not be > NUL-terminated (allowing file-system Unix sockets to have one more byte > in their name); Out of curiosity: is this usage portable? I tried man pages and SUS, no luck. > you need the strndup that you replaced above, in order > avoid reading beyond the end of the array. You're right. Prone to allocate a bit more than necessary (always sizeof(su->sun_path) + 1 bytes), but that doesn't matter.
On 10/30/20 4:04 AM, Markus Armbruster wrote: >>> + addr->u.q_unix.path = g_strdup(su->sun_path); >> >> This is wrong on at least Linux, where su->sun_path need not be >> NUL-terminated (allowing file-system Unix sockets to have one more byte >> in their name); > > Out of curiosity: is this usage portable? I tried man pages and SUS, no > luck. On Linux, 'man 7 unix' says: > BUGS > When binding a socket to an address, Linux is one of the implementa‐ > tions that appends a null terminator if none is supplied in sun_path. > In most cases this is unproblematic: when the socket address is > retrieved, it will be one byte longer than that supplied when the > socket was bound. However, there is one case where confusing behavior > can result: if 108 non-null bytes are supplied when a socket is bound, > then the addition of the null terminator takes the length of the path‐ > name beyond sizeof(sun_path). Consequently, when retrieving the socket > address (for example, via accept(2)), if the input addrlen argument for > the retrieving call is specified as sizeof(struct sockaddr_un), then > the returned address structure won't have a null terminator in > sun_path. > > In addition, some implementations don't require a null terminator when > binding a socket (the addrlen argument is used to determine the length > of sun_path) and when the socket address is retrieved on these imple‐ > mentations, there is no null terminator in sun_path. along with advice on using strnlen and/or overallocation to handle various cases in a cleaner manner, and the caveat that if you always use a name smaller than sun_path you can avoid the tricky code (at the expense of one byte less in your namespace). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index c802d5aa0a..801c5e3957 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -1264,10 +1264,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 = !su->sun_path[sizeof(su->sun_path) - 1]; + return addr; } +#endif + addr->u.q_unix.path = g_strdup(su->sun_path); return addr; } #endif /* WIN32 */
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 Signed-off-by: Markus Armbruster <armbru@redhat.com> --- util/qemu-sockets.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)