[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] util: fix abstract socket path copy
From: |
Peter Maydell |
Subject: |
Re: [PATCH] util: fix abstract socket path copy |
Date: |
Tue, 31 Aug 2021 10:53:53 +0100 |
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