Re: Our abstract UNIX domain socket support is a mess

[Kevin Wolf]

Am 29.10.2020 um 15:02 hat Daniel P. Berrangé geschrieben:
> On Wed, Oct 28, 2020 at 01:41:06PM +0100, Markus Armbruster wrote:
> > Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> > support" (v5.1) has made a mess.
> > 
> > First, what are abstract UNIX domain sockets?  They use a separate
> > "abstract" namespace which is independent of the filesystem.  This is a
> > non-portable Linux extension.  See unix(7).
> > 
> > Traditional pathname sockets have a zero-terminated string in
> > sockaddr_un member sun_path.  Abstract sockets have a binary blob
> > starting with a 0 byte.  Works, because the empty string is not a valid
> > pathname.
> > 
> > This interface is a hack.  The leading 0 byte is ugly.  The size of the
> > blob is given by the socklen_t argument passed with the struct
> > sockaddr_un.  People use strings, then forget to either cut the length
> > to the size of the string, or pad to the blob to sizeof(sockaddr_un),
> > and do it consistently.
> > 
> > QEMU's interface is differently messy.
> > 
> > Our equivalent to struct sockaddr_un is QAPI type UnixSocketAddress:
> > 
> >     { 'struct': 'UnixSocketAddress',
> >       'data': {
> >         'path': 'str' }
> > 
> > @path corresponds to sockaddr_un member sun_path.  sun_family = AF_UNIX
> > and socklen_t sizeof(sockaddr_un) are implied.
> > 
> > We didn't repurpose @path for abstract sockets like the Linux kernel did
> > with sun_path.  Instead, we added a flag @abstract (default false).
> > When it's on, we make a binary blob by prefixing @path with a 0 byte,
> > and pad it with more 0 bytes.
> > 
> > We added a second flag @tight (default true) to optionally cut the
> > socklen_t to the end of the string (the terminating 0 byte is not
> > included).
> > 
> > Six revisions were posted, and the QAPI maintainers were cc'ed on none
> > of them.  I spotted @tight in the pull request by chance, and found it
> > odd.  I inquired, and Daniel explained:
> > 
> >     In the abstract namespace the length of the socket is critical
> >     information. ie
> > 
> >        "\0foo" (length == 4,  tight=true)
> > 
> >     is a completely different socket from
> > 
> >        "\0foo\0..repeated...\0" (length == sizeof(sun_path), tight=false)
> > 
> >     In theory you can have any length in between those extremes,
> >     each being a different socket, but in practice no one is that
> >     insane. Apps either use the full length, or the minimal
> >     length. The "tight" terminology is copied from "socat" which
> >     uses this same option name
> > 
> >     Message-ID: <87imgqr8g9.fsf@dusky.pond.sub.org>
> >     https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg05812.html
> > 
> > I did not press further, or dig deeper.  I should have.  I did only when
> > Kevin reported @tight's default is backward in QMP.  I'm not liking what
> > I found.
> > 
> > 
> > Issue#1: our interface is differently ugly, for no good reason
> > 
> > Like the Linux kernel, we also appropriate existing @path for abstract
> > sockets.  With less excuse, though; we could have created a neater
> > interface, easily.
> > 
> > Unlike the Linux kernel, we don't do blobs.  In other words, our variant
> > of the hack is not general.
> 
> The Linux kernel interface is low level and not the way any userspace
> application exposes the use of abstract sockets. No one wants to
> specify an abstract socket by listing all 108 characters with many
> trailing nuls. It would be insane to do this.
> 
> There are two ways userspace apps expose abstract socket config.
> 
> Either using a leading "@" as a magic substitute for NUL and not
> supporting a coibfigurable way to distinguish truncated vs full
> length, just define the desired behaviour for their app. THis is
> what dbus does to denote its abstract socket paths.

Using magic characters in strings to distinguish different types of
objects is always wrong in QAPI. If we interpreted leading '@' this way,
you wouldn't be able to specify a relative filename starting with '@'
any more.

> Or, just or by having  explicit flags "abstract" and "tight" to
> control the behaviour.  The latter is what 'socat' does to allow
> use of abstract sockets.
> 
> For QEMU the former approach gives broad interoperabiltiy with
> userspace applications, so made more sense than using magic "@".

Boolean flags to distinguish different types are better than parsing
strings, but still not optimal. Documentation like "only matters for
abstract sockets" is another hint that we're treating things the same
that aren't the same.

The proper way to distinguish two different types is unions. So I think
the ideal interface would be another SocketAddress variant that could
then also use base64 instead of str to represent arbitrary blobs, like
Markus suggested below.

Probably too late now.

> > Elsewhere in QMP, we use base64 for blobs.
> > 
> > Aside: QMP can do embedded 0 bytes.  It represents them as overlong
> > UTF-8 “\xC0\x80", though.
> > 
> > Not sure the interface is worth fixing now.  Abstract sockets are niche.
> > In my opinion, we should've said no.
> 
> The interface doesn't need fixing - the way it is represented in
> QEMU is much saner than the low level struct sockaddr_un representation
> used to talk to the kernel, and is common with other userspace apps.
> 
> The use case is to enable interoperability with other apps that use
> an abstract socket.

Kevin