Hi On Tue, Aug 31, 2021 at 2:32 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
> 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. > 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 > > -- Marc-André Lureau
