On Tue, Aug 31, 2021 at 04:32:36PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Tue, Aug 31, 2021 at 3:00 AM Michael Tokarev <m...@tls.msk.ru> wrote:
>
> > Linux kernel can return size of af_unix socket to be
> > one byte larger than sockaddr_un structure - adding
> > the trailing zero byte.
> >
> > Signed-off-by: Michael Tokarev <m...@tls.msk.ru>
> > Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
> > Cc: qemu-sta...@nongnu.org
> >
> > 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);
> >
> >
> Looks right, but we may want to drop the upper bound check altogether. I
> thought the path must always fit the sockaddr_un, but since that's not the
> case it's only harmful here.
Yeah, I don't think either old or new code are good here. The assert
is validating Linux semantics here, other platforms don't add the
extra NUL
Also later in the same method the string copy is dubious too:
addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path));
The man page says that to take account of non-portable semantics
wrt trailing NULs, the path length should be calculated using:
strnlen(addr.sun_path, salen - offsetof(sockaddr_un, sun_path))
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
