[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
participants (2)
-
Eric Blake -
Richard W.M. Jones