On Tue, 31 Aug 2021 at 11:17, Michael Tokarev <m...@tls.msk.ru> wrote: > > 31.08.2021 12:53, Peter Maydell wrote: > > On Mon, 30 Aug 2021 at 23:30, Michael Tokarev <m...@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. > > 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
