Hi On Tue, Aug 31, 2021 at 10:26 PM Michael Tokarev <m...@tls.msk.ru> wrote:
> We test whenever the path of unix-domain socket > address is non-empty and strictly-less than > the length of the path buffer. Both these > conditions are wrong: the socket can be unnamed, > with empty path, or socket can have pathname > null-terminated _after_ the sun_path buffer, > since we provided more room when asking kernel. > > So on one side, allow empty, unnamed sockets > (and adjust the test for abstract socket too - > only do that if the socket is not unnamed), > and on another side, allow path length to be > up to our own maximum, - we have up to size > of sockaddr_storage there. > > While at it, fix the duplication of regular > pathname socket to not require trailing \0 > (since it can be missing for unnamed sockets). > > Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0) > Fixes: http://bugs.debian.org/993145 > Signed-off-by: Michael Tokarev <m...@tls.msk.ru> > Cc: qemu-sta...@nongnu.org > -- > Two questions. > 1. Why do we store the name of the socket to start with? > 2. The code in the abstract socket case should not use > g_strndup but g_memdup instead, since the whole thing > is a blob of the given length, not a \0-terminated string. > --- > util/qemu-sockets.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index f2f3676d1f..7c83d81792 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -1345,13 +1345,20 @@ 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)); > + /* there's a corner case when trailing \0 does not fit into > + * sockaddr_un. Compare length with sizeof(sockaddr_storage), > + * not with sizeof(sockaddr_un), since this is what we actually > + * provide, to ensure we had no truncation and a room for > + * the trailing \0 which we add below. > + * When salen == sizeof(sun_family) it is unnamed socket, > + * and when first byte of sun_path is \0, it is abstract. */ > + assert(salen >= sizeof(su->sun_family) && > + salen <= sizeof(struct sockaddr_storage)); > Seems right to me, however there are some notes in libc bits/socket.h /* Structure large enough to hold any socket address (with the historical exception of AF_UNIX). */ And also this https://idea.popcount.org/2019-12-06-addressing/#fn:sockaddr_storage I must say I feel confused by those comments :) Is it large enough or not?? > addr = g_new0(SocketAddress, 1); > addr->type = SOCKET_ADDRESS_TYPE_UNIX; > #ifdef CONFIG_LINUX > - if (!su->sun_path[0]) { > + if (salen > sizeof(su->sun_family) && !su->sun_path[0]) { > /* Linux abstract socket */ > addr->u.q_unix.path = g_strndup(su->sun_path + 1, > salen - sizeof(su->sun_family) - > 1); > @@ -1363,7 +1370,7 @@ socket_sockaddr_to_address_unix(struct > sockaddr_storage *sa, > } > #endif > > - addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path)); > + addr->u.q_unix.path = g_strndup(su->sun_path, salen - > sizeof(su->sun_family)); > looks good to me otherwise return addr; > } > #endif /* WIN32 */ > -- > 2.30.2 > >
