On Tue, 31 Aug 2021 at 11:17, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 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.
Yes, I know about that. Why are we assert()ing ? Our
implementation here doesn't care whether the struct
we're passed is exactly the size of a sockaddr_un,
a bit bigger than it, or 5 bytes bigger. We're not going
to crash or misbehave if the caller passes us in an oversized
buffer.
The minimal len check seems appropriate, since the function accesses at least the first X bytes (3 I suppose).
While at it I probably added an upper bound that I thought made sense (the size of sockaddr_un), but I did wrong.
But now, I also think we can remove the upper bound check.
> > 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.
Ah, I hadn't realized about the abstract-sockets case. Thanks.
-- PMM