Re: [PATCH 08/11] sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets

From: Eric Blake <eblake@redhat.com>
To: Markus Armbruster <armbru@redhat.com>, qemu-devel@nongnu.org
Cc: kwolf@redhat.com, berrange@redhat.com, zxq_yx_007@163.com,
	kraxel@redhat.com, pbonzini@redhat.com,
	marcandre.lureau@redhat.com
Subject: Re: [PATCH 08/11] sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets
Date: Thu, 29 Oct 2020 14:38:37 -0500	[thread overview]
Message-ID: <a30d57a6-2188-12fa-a1e5-e0c015e8f970@redhat.com> (raw)
In-Reply-To: <20201029133833.3450220-9-armbru@redhat.com>

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