Libguestfs - May 2019 - [libnbd PATCH] connect: Better handling of long socket names

Copy various Unix socket handling techniques from nbdkit's nbd plugin:

Silently truncating a socket name rather than issuing an error message
can confuse users.  No need to do an explicit memset if the compiler
does it for us via an initializer.  No need to use strncpy() which
does wasted effort on short names, when we can use memcpy() given that
we already checked length in order to detect truncation.  Linux does
not require a trailing NUL byte in sockaddr_un, so we can allow names
one byte longer.

Not entirely fixed: although the proposed NBD URI document mentions
that URIs can start with a leading (encoded) NUL byte to access the
Linux abstract socket namespace, our use of strlen(sun.sun_path) would
truncate the trailing bytes - but to support abstract sockets in
general, we'd need to add h->unixlen alongside the existing
h->unixsocket.
---
 generator/states-connect.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index a410e34..014f6bb 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -86,15 +86,20 @@
   }

  CONNECT_UNIX.START:
-  struct sockaddr_un sun;
+  struct sockaddr_un sun = { .sun_family = AF_UNIX };
   socklen_t len;
+  size_t socklen;

   assert (h->unixsocket != NULL);

-  sun.sun_family = AF_UNIX;
-  memset (sun.sun_path, 0, sizeof (sun.sun_path));
-  strncpy (sun.sun_path, h->unixsocket, sizeof (sun.sun_path) - 1);
-  len = sizeof (sun.sun_family) + strlen (sun.sun_path) + 1;
+  socklen = strlen (h->unixsocket);
+  if (socklen > sizeof sun.sun_path) {
+    set_error (ENAMETOOLONG, "socket name too long: %s",
h->unixsocket);
+    SET_NEXT_STATE (%.DEAD);
+    return -1;
+  }
+  memcpy (sun.sun_path, h->unixsocket, socklen);
+  len = sizeof sun;

   memcpy (&h->connaddr, &sun, len);
   h->connaddrlen = len;
-- 
2.20.1

On Tue, May 28, 2019 at 03:12:16PM -0500, Eric Blake
wrote:
> Copy various Unix socket handling techniques from nbdkit's nbd plugin:
> 
> Silently truncating a socket name rather than issuing an error message
> can confuse users.  No need to do an explicit memset if the compiler
> does it for us via an initializer.  No need to use strncpy() which
> does wasted effort on short names, when we can use memcpy() given that
> we already checked length in order to detect truncation.  Linux does
> not require a trailing NUL byte in sockaddr_un, so we can allow names
> one byte longer.
> 
> Not entirely fixed: although the proposed NBD URI document mentions
> that URIs can start with a leading (encoded) NUL byte to access the
> Linux abstract socket namespace, our use of strlen(sun.sun_path) would
> truncate the trailing bytes - but to support abstract sockets in
> general, we'd need to add h->unixlen alongside the existing
> h->unixsocket.
> ---
>  generator/states-connect.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/generator/states-connect.c b/generator/states-connect.c
> index a410e34..014f6bb 100644
> --- a/generator/states-connect.c
> +++ b/generator/states-connect.c
> @@ -86,15 +86,20 @@
>    }
> 
>   CONNECT_UNIX.START:
> -  struct sockaddr_un sun;
> +  struct sockaddr_un sun = { .sun_family = AF_UNIX };
>    socklen_t len;
> +  size_t socklen;
> 
>    assert (h->unixsocket != NULL);
> 
> -  sun.sun_family = AF_UNIX;
> -  memset (sun.sun_path, 0, sizeof (sun.sun_path));
> -  strncpy (sun.sun_path, h->unixsocket, sizeof (sun.sun_path) - 1);
> -  len = sizeof (sun.sun_family) + strlen (sun.sun_path) + 1;
> +  socklen = strlen (h->unixsocket);
> +  if (socklen > sizeof sun.sun_path) {
> +    set_error (ENAMETOOLONG, "socket name too long: %s",
h->unixsocket);
> +    SET_NEXT_STATE (%.DEAD);
> +    return -1;
> +  }
> +  memcpy (sun.sun_path, h->unixsocket, socklen);
> +  len = sizeof sun;
> 
>    memcpy (&h->connaddr, &sun, len);
>    h->connaddrlen = len;
> -- 
ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/