Re: [PATCH] util: fix abstract socket path copy

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 30 Aug 2021 at 23:30, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 31.08.2021 01:06, Michael Tokarev wrote:
> ...
> > And this is the value used to be returned in the getsockname/getpeername
> > calls.
> >
> > So this has nothing to do with socket being abstract or not. We asked for
> > larger storage for the sockaddr structure, and the kernel was able to build
> > one for us, including the trailing \0 byte.

> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index f2f3676d1f..83926dc2bc 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,8 +1345,9 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>       SocketAddress *addr;
>       struct sockaddr_un *su = (struct sockaddr_un *)sa;
>
> +    /* kernel might have added \0 terminator to non-abstract socket */
>       assert(salen >= sizeof(su->sun_family) + 1 &&
> -           salen <= sizeof(struct sockaddr_un));
> +           salen <= sizeof(struct sockaddr_un) + su->sun_path[0] ? 1 : 0);

Q: Why are we imposing an upper limit on salen anyway?
We need the lower limit because salen is supposed to include
the whole of the 'struct sockaddr_un' and we assume that.
But what's the upper limit here protecting?

Q2: why does our required upper limit change depending on whether
there happens to be a string in the sun_path array or not ?

Q3: when we copy the sun_path, why do we skip the first character?

        addr->u.q_unix.path = g_strndup(su->sun_path + 1,
                                        salen - sizeof(su->sun_family) - 1);

-- PMM