Libguestfs - Apr 2018 - [nbdkit PATCH] nbd: Fix gcc warning and off-by-one in socket name length

gcc 8 gripes (when using './configure --enable-gcc-warnings'):
nbd.c: In function 'nbd_open':
nbd.c:470:3: error: 'strncpy' specified bound 108 equals destination
size [-Werror=stringop-truncation] strncpy (sock.sun_path, sockname, sizeof
(sock.sun_path));

The warning is a false positive, given that we currently reject
names >= sizeof(sock.sun_path), and thus we are only ever copying
in a name that will include a trailing NUL.  However, note that
Linux permits a socket name to use the full width of sun_path (for
shorter names, you must either provide a trailing NUL or pass
something smaller than sizeof(struct sockaddr_un) to connect();
but for the full-length name, a trailing NUL is not required).

So, relax our off-by-one length restriction (we now permit the user
to pass a 108-byte socket name, instead of a limit of 107), at which
point the gcc complaint is no longer a false positive (we could
indeed be copying a string without its trailing NUL, even though
we know it works), then shut up gcc by using memcpy() instead of
strncpy(), relying on our earlier zero-initialization for supplying
any needed trailing NUL.  For convencience, we stick with the
simpler sizeof(struct sockaddr_un) instead of passing exact lengths
to connect().

[strncpy() is seldom the right function to use, because it does
not NUL-terminate on overflow, yet writes a full size bytes even
when the input string is shorter.  Initializing sockaddr_un is
one of the few places where it actually does what you want - too
bad newer gcc is now rendering even valid uses of strncpy as a
source of complaints]

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/nbd/nbd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index 51de178..a4a1f12 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -100,7 +100,7 @@ nbd_config_complete (void)
     nbdkit_error ("you must supply the socket=<SOCKNAME> parameter
after the plugin name on the command line");
     return -1;
   }
-  if (strlen (sockname) >= sizeof sock.sun_path) {
+  if (strlen (sockname) > sizeof sock.sun_path) {
     nbdkit_error ("socket file name too large");
     return -1;
   }
@@ -467,7 +467,8 @@ nbd_open (int readonly)
     nbdkit_error ("socket: %m");
     return NULL;
   }
-  strncpy (sock.sun_path, sockname, sizeof (sock.sun_path));
+  /* We already validated length during nbd_config_complete */
+  memcpy (sock.sun_path, sockname, strlen (sockname));
   if (connect (h->fd, (const struct sockaddr *) &sock, sizeof sock) <
0) {
     nbdkit_error ("connect: %m");
     goto err;
-- 
2.14.3

On Thu, Apr 05, 2018 at 10:27:10AM -0500, Eric Blake
wrote:
> gcc 8 gripes (when using './configure --enable-gcc-warnings'):
> nbd.c: In function 'nbd_open':
> nbd.c:470:3: error: 'strncpy' specified bound 108 equals
destination size [-Werror=stringop-truncation] strncpy (sock.sun_path, sockname,
sizeof (sock.sun_path));
> 
> The warning is a false positive, given that we currently reject
> names >= sizeof(sock.sun_path), and thus we are only ever copying
> in a name that will include a trailing NUL.  However, note that
> Linux permits a socket name to use the full width of sun_path (for
> shorter names, you must either provide a trailing NUL or pass
> something smaller than sizeof(struct sockaddr_un) to connect();
> but for the full-length name, a trailing NUL is not required).
> 
> So, relax our off-by-one length restriction (we now permit the user
> to pass a 108-byte socket name, instead of a limit of 107), at which
> point the gcc complaint is no longer a false positive (we could
> indeed be copying a string without its trailing NUL, even though
> we know it works), then shut up gcc by using memcpy() instead of
> strncpy(), relying on our earlier zero-initialization for supplying
> any needed trailing NUL.  For convencience, we stick with the
> simpler sizeof(struct sockaddr_un) instead of passing exact lengths
> to connect().
> 
> [strncpy() is seldom the right function to use, because it does
> not NUL-terminate on overflow, yet writes a full size bytes even
> when the input string is shorter.  Initializing sockaddr_un is
> one of the few places where it actually does what you want - too
> bad newer gcc is now rendering even valid uses of strncpy as a
> source of complaints]
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/nbd/nbd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
> index 51de178..a4a1f12 100644
> --- a/plugins/nbd/nbd.c
> +++ b/plugins/nbd/nbd.c
> @@ -100,7 +100,7 @@ nbd_config_complete (void)
>      nbdkit_error ("you must supply the socket=<SOCKNAME>
parameter after the plugin name on the command line");
>      return -1;
>    }
> -  if (strlen (sockname) >= sizeof sock.sun_path) {
> +  if (strlen (sockname) > sizeof sock.sun_path) {
>      nbdkit_error ("socket file name too large");
>      return -1;
>    }
> @@ -467,7 +467,8 @@ nbd_open (int readonly)
>      nbdkit_error ("socket: %m");
>      return NULL;
>    }
> -  strncpy (sock.sun_path, sockname, sizeof (sock.sun_path));
> +  /* We already validated length during nbd_config_complete */
> +  memcpy (sock.sun_path, sockname, strlen (sockname));
>    if (connect (h->fd, (const struct sockaddr *) &sock, sizeof sock)
< 0) {
>      nbdkit_error ("connect: %m");
>      goto err;
> -- 
> 2.14.3
Thanks Eric, tested with gcc-8.0.1-0.19.fc28.x86_64 and it works for me.

ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW