Re: [PATCH v2] qemu-sockets: add abstract UNIX domain socket support

[Daniel P . Berrangé]

Adding Eric & Markus for QAPI modelling questions

On Thu, Apr 23, 2020 at 11:56:40AM +0800, xiaoqiang zhao wrote:
> unix_connect_saddr now support abstract address type
> 
> By default qemu does not support abstract UNIX domain
> socket address. Add this ability to make qemu handy
> when abstract address is needed.

Was that a specific app you're using with QEMU that needs this ?

> Abstract address is marked by prefixing the address name with a '@'.

For full support of the abstract namespace we would ned to allow
the "sun_path" to contain an arbitrary mix of NULs and non-NULs
characters, and allow connect() @addrlen to be an arbitrary size.

This patch only allows a single initial NUL, and reqiures @addrlen
to be the full size of sun_path, padding with trailing NULs. This
limitation is impossible to lift with QEMU's current approach to
UNIX sockets, as it relies on passing around a NULL terminated
string, so there's no way to have embedded NULs. Since there's
no explicit length, we have to chooose between forcing the full
sun_path size as @addrlen, or forcing the string length as the
@addrlen value.

IIUC, socat makes the latter decision by default, but has a
flag to switch to the former.

  [man socat]
  unix-tightsocklen=[0|1]
  On  socket  operations,  pass  a  socket  address  length that does not
  include the whole struct sockaddr_un record but (besides  other  compo‐
  nents)  only  the  relevant  part  of  the filename or abstract string.
  Default is 1.
  [/man]

This actually is supported for both abstract and non-abstract
sockets, though IIUC this doesn't make a semantic difference
for non-abstract sockets.

The point is we have four possible combinations

 NON-ABSTRACT + FULL SIZE
 NON-ABSTRACT + MINIMAL SIZE  (default)
 ABSTRACT + FULL SIZE
 ABSTRACT + MINIMAL SIZE  (default)

With your patch doing the latter, it means QEMU supports
only two combinations

  NON+ABSTRACT + FULL SIZE
  ABSTRACT + MINIMAL SIZE

and also can't use "@somerealpath" for a non-abstract
socket, though admittedly this is unlikely.

Socat uses a special option to request use of abstract
sockets. eg ABSTRACT:somepath, and automatically adds
the leading NUL, so there's no need for a special "@"
character. This means that UNIX:@somepath still resolves
to a filesystem path and not a abstract socket path.

Finally, the patch as only added support for connect()
not listen(). I think if QEMU wants to support abstract
sockets we must do both, and also have unit tests added
to tests/test-util-sockets.c


The question is whether we're ok with this simple
approach in QEMU, or should do a full approach with
more explicit modelling.

ie should we change QAPI thus:

{ 'struct': 'UnixSocketAddress',
  'data': {
    'path': 'str',
    'tight': 'bool',
    'abstract': 'bool' } }

where 'tight' is a flag indicating whether to set @addrlen
to the minimal string length, or the maximum sun_path length.
And 'abstract' indicates that we automagically add a leading
NUL.

This would *not* allow for NULs in the middle of path,
but I'm not so bothered about that, since I can't see that
being widely used. If we really did need that it could be
added via a 'base64': 'bool' flag, to indicate that @path
is base64 encoded and thus may contain NULs

>From a CLI POV, this could be mapped to QAPI thus

 *  -chardev unix:somepath

      @path==somepath
      @tight==false
      @abstract==false

 *  -chardev unix:somepath,tight

      @path==somepath
      @tight==true
      @abstract==false

 *  -chardev unix-abstract:somepath

      @path==somepath
      @tight==false
      @abstract==true

 *  -chardev unix-abstract:somepath,tight

      @path==somepath
      @tight==true
      @abstract==true


> 
> Signed-off-by: xiaoqiang zhao <address@hidden>
> ---
>  util/qemu-sockets.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index bcc06d0e01..7ba9c497ab 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -939,6 +939,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, 
> Error **errp)
>      struct sockaddr_un un;
>      int sock, rc;
>      size_t pathlen;
> +    socklen_t serverlen;
>  
>      if (saddr->path == NULL) {
>          error_setg(errp, "unix connect: no path specified");
> @@ -963,10 +964,17 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, 
> Error **errp)
>      un.sun_family = AF_UNIX;
>      memcpy(un.sun_path, saddr->path, pathlen);
>  
> +    if (saddr->path[0] == '@') {
> +        un.sun_path[0] = '\0';
> +        serverlen = pathlen + offsetof(struct sockaddr_un, sun_path);
> +    } else {
> +        serverlen = sizeof(un);
> +    }
> +
>      /* connect to peer */
>      do {
>          rc = 0;
> -        if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) {
> +        if (connect(sock, (struct sockaddr *) &un, serverlen) < 0) {
>              rc = -errno;
>          }
>      } while (rc == -EINTR);
> -- 
> 2.17.1
> 
> 

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