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

[Michael Tokarev]

31.08.2021 12:53, Peter Maydell wrote:
> 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?

It is not about protection really, it is about correctness.
This is actually a grey area. This single trailing \0 byte
depends on the implementation. Please read man 7 unix -
especially the "Pathname sockets" and BUGS sections.


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

Because for abstract sockets (the ones whos name starts with \0
byte) the sun_path is treated as a blob of given length, without
the additional trailing \0, and neither the kernel nor userspace
is trying to add the terminator, while for pathname sockets this
is not the case and someone has to add the trailing \0 somewhere.

> 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);

Because this is the case of abstract socket. Actual name is a blob
starting from the sun_path+1. I dunno if this is correct to use
g_strndup there - will it terminate at first \0 encountered or not,
but this is another question. The difference between this place
and regular pathname socket below is this: addr->u.q_unix.abstract = true;

We have the issue only whith regular traditional unix pathname sockets
whose name is exactly 108 bytes long.  I provided the code from
the kernel in another thread showing how it increases the len by
one in this case and returns it in getsockname() if userspace
provided enough room.

/mjt