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

[Michael Tokarev]

31.08.2021 00:38, Michael Tokarev wrote:
...
> > @@ -1345,13 +1345,16 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage 
> > *sa,
> >       SocketAddress *addr;
> >       struct sockaddr_un *su = (struct sockaddr_un *)sa;
> > +    assert(salen >= sizeof(su->sun_family) + 1 &&
> > +           salen <= sizeof(struct sockaddr_un));
> > +
>
> This seems to be wrong and causes this assert in the existing qemu code to fire 
> up.
> I can't say for *sure* but it *seems* the issue is the trailing null terminator
> in the case of abstract sockets on linux where the path name is exactly equal
> to 108 bytes (including the leading \0).
>
> The prob seems to be that socket_local_address() initially allocates
> sockaddr_storage bytes for the getsockname() call - which is larger
> than sizeof(sockaddr_un). So the kernel is able to add the zero terminator,
> and return 111 bytes in salen, not 110 as the size of sockaddr_un.
> And later on the above assert will fire up..

Here's the linux kernel code:

net/unix/af_unix.c
tatic int unix_mkname(struct sockaddr_un *sunaddr, int len, unsigned int *hashp)
{
        *hashp = 0;

        if (len <= sizeof(short) || len > sizeof(*sunaddr))
                return -EINVAL;
        if (!sunaddr || sunaddr->sun_family != AF_UNIX)
                return -EINVAL;
        if (sunaddr->sun_path[0]) {
                /*
                 * This may look like an off by one error but it is a bit more
                 * subtle. 108 is the longest valid AF_UNIX path for a binding.
                 * sun_path[108] doesn't as such exist.  However in kernel space
                 * we are guaranteed that it is a valid memory location in our
                 * kernel address buffer.
                 */
                ((char *)sunaddr)[len] = 0;
                len = strlen(sunaddr->sun_path)+1+sizeof(short);
                return len;
        }
..

Suppose we have sun_path of exactly 108 chars (not counting the trailing
zero terminator), so the total length of the sockaddr is 110 bytes.

After the len = recalculation above, it will be 111 bytes -
 strlen(sun_path)=108 + 1 + sizeof(short)=2 = 111.

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. Currently kernel will only
return +1 byte for non-abstract sockets, but this is an implementation
detail. We asked for it. So we - I think - should account for this +1
byte here.

/mjt