Our abstract UNIX domain socket support is a mess

Our abstract UNIX domain socket support is a mess

[Markus Armbruster]

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.

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.


Issue#2: we offer abstract sockets even on non-Linux hosts

I expect this to fail with ENOENT elsehwere.  I tested OpenBSD, and
fails that way.  We report this failure like

    Failed to connect socket abc: No such file or directory

Tolerable, although ENOTSUP would be better.

Introspection lies: it has @abstract regardless of host support.  Easy
enough to fix: since Linux provides them since 2.2, 'if':
'defined(CONFIG_LINUX)' should do.

This should also change the error report to

    Parameter 'backend.data.addr.data.abstract' is unexpected

Improvement, I think.


Issue#3: @tight's default is backward in QMP

Incorrect use of the QAPI type in C, plus insufficient test coverage.

Fixable.  Treat 5.1's default as bug.


Final plea: please copy the QAPI schema maintainers on all
schema-changing patches!  We happen to have some interface design
expertise.  Make use of it!  Also, please give us a chance to correct
mistakes while corrections are easy and cheap.

Re: Our abstract UNIX domain socket support is a mess

[Daniel P. Berrangé]

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.

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 "@".


> 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.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Re: Our abstract UNIX domain socket support is a mess

[Eric Blake]

On 10/29/20 11:07 AM, Kevin Wolf wrote:

>>>
>>> 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).
>>>

> 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.

But why two boolean flags for three sensible states (where it is unclear
if the fourth combination that makes no sense is silently accepted or
loudly rejected), instead of a single tri-state-valued enum?

> 
> 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.

Sadly, you are probably right that we are a bit late to be rethinking
what we expose, since 5.1 is released.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: Our abstract UNIX domain socket support is a mess

[Daniel P. Berrangé]

On Thu, Oct 29, 2020 at 01:47:02PM -0500, Eric Blake wrote:
> On 10/29/20 11:07 AM, Kevin Wolf wrote:
> 
> >>>
> >>> 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).
> >>>
> 
> > 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.
> 
> But why two boolean flags for three sensible states (where it is unclear
> if the fourth combination that makes no sense is silently accepted or
> loudly rejected), instead of a single tri-state-valued enum?

This is simply mirroring what  "socat" supports as configuration.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Our abstract UNIX domain socket support is a mess

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> 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:
[...]
>> > 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

Yes.

> 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.

There are no impossible combinations to ignore or reject, and to
document.  Instead, introspection tells the whole story.

Done this way, we could easily support both a (string, bool tight) for
convenience and base64 blob for generality, if we want to.

But I stand by my opinion that the feature is simply not worth its keep.
To make me reconsider, show me actual uses.

> Probably too late now.

It's too late if we decide it is.

>> > 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