In case of truncation no trailing zero will be added to the target
string. Reduce the amount of bytes to copy by one to make sure a
trailing zero always exists.

In file included from /usr/include/string.h:495,
                 from libxl_internal.h:38,
                 from libxl_utils.c:20:
In function 'strncpy',
    inlined from 'libxl__prepare_sockaddr_un' at libxl_utils.c:1262:5:
/usr/include/bits/string_fortified.h:106:10: error: '__builtin_strncpy' specified bound 108 equals destination size [-Werror=stringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---

gcc may not detect the off-by-one error in libxl__prepare_sockaddr_un, fix the strncpy usage anyway.

 tools/libvchan/vchan-socket-proxy.c | 8 ++++----
 tools/libxl/libxl_utils.c           | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
index 13700c5d67..b312f05ca7 100644
--- a/tools/libvchan/vchan-socket-proxy.c
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -140,7 +140,7 @@ static int set_nonblocking(int fd, int nonblocking) {
 static int connect_socket(const char *path_or_fd) {
     int fd;
     char *endptr;
-    struct sockaddr_un addr;
+    struct sockaddr_un addr = {};
 
     fd = strtoll(path_or_fd, &endptr, 0);
     if (*endptr == '\0') {
@@ -153,7 +153,7 @@ static int connect_socket(const char *path_or_fd) {
         return -1;
 
     addr.sun_family = AF_UNIX;
-    strncpy(addr.sun_path, path_or_fd, sizeof(addr.sun_path));
+    strncpy(addr.sun_path, path_or_fd, sizeof(addr.sun_path) - 1);
     if (connect(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
         close(fd);
         return -1;
@@ -167,7 +167,7 @@ static int connect_socket(const char *path_or_fd) {
 static int listen_socket(const char *path_or_fd) {
     int fd;
     char *endptr;
-    struct sockaddr_un addr;
+    struct sockaddr_un addr = {};
 
     fd = strtoll(path_or_fd, &endptr, 0);
     if (*endptr == '\0') {
@@ -180,7 +180,7 @@ static int listen_socket(const char *path_or_fd) {
         return -1;
 
     addr.sun_family = AF_UNIX;
-    strncpy(addr.sun_path, path_or_fd, sizeof(addr.sun_path));
+    strncpy(addr.sun_path, path_or_fd, sizeof(addr.sun_path) - 1);
     if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
         close(fd);
         return -1;
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index f360f5e228..83592e829d 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1259,7 +1259,7 @@ int libxl__prepare_sockaddr_un(libxl__gc *gc,
     }
     memset(un, 0, sizeof(struct sockaddr_un));
     un->sun_family = AF_UNIX;
-    strncpy(un->sun_path, path, sizeof(un->sun_path));
+    strncpy(un->sun_path, path, sizeof(un->sun_path) - 1);
     return 0;
 }
 

Am Mon,  8 Jun 2020 09:28:54 +0200
schrieb Olaf Hering <olaf@aepfle.de>:

> off-by-one error in libxl__prepare_sockaddr_un

There is none, I had read the code backwards...

Olaf

Olaf Hering writes ("Re: [PATCH v1] tools: fix usage of strncpy"):
> Am Mon,  8 Jun 2020 09:28:54 +0200
> schrieb Olaf Hering <olaf@aepfle.de>:
> > off-by-one error in libxl__prepare_sockaddr_un
> 
> There is none, I had read the code backwards...

I have just had the same thoughts but in the opposite order.  That is
at first I thought this was not a problem, but now I think there is.

There are some kernel interfaces where a fixed-size buffer is
provided, and the kernel will tolerate a null-terminated string, but
will in any case not read beyond the end of the buffer.  Anything
involving IFNAMSIZ comes to mind.

But I think sun_path is not one of those.  The manpage I have here
says that to be portable you must null-terminate sun_path.  I know
that there are some implementations where it is possible to pass a
longer path, effectively treating sun_path as a trailing vla.

Looking at your diff, its effect seems to be to ensure
null-termination by truncating overlong paths.

I think the right approach is to return an error, not to silently
truncate.

Ian.

On Mon, Jun 8, 2020 at 7:03 AM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Olaf Hering writes ("Re: [PATCH v1] tools: fix usage of strncpy"):
> > Am Mon,  8 Jun 2020 09:28:54 +0200
> > schrieb Olaf Hering <olaf@aepfle.de>:
> > > off-by-one error in libxl__prepare_sockaddr_un
> >
> > There is none, I had read the code backwards...
>
> I have just had the same thoughts but in the opposite order.  That is
> at first I thought this was not a problem, but now I think there is.
>
> There are some kernel interfaces where a fixed-size buffer is
> provided, and the kernel will tolerate a null-terminated string, but
> will in any case not read beyond the end of the buffer.  Anything
> involving IFNAMSIZ comes to mind.
>
> But I think sun_path is not one of those.  The manpage I have here
> says that to be portable you must null-terminate sun_path.  I know
> that there are some implementations where it is possible to pass a
> longer path, effectively treating sun_path as a trailing vla.
>
> Looking at your diff, its effect seems to be to ensure
> null-termination by truncating overlong paths.
>
> I think the right approach is to return an error, not to silently
> truncate.

I added a length check in this patch:

https://lore.kernel.org/xen-devel/20200525024955.225415-2-jandryuk@gmail.com/

Regards,
Jason

Am Mon, 8 Jun 2020 08:43:50 -0400
schrieb Jason Andryuk <jandryuk@gmail.com>:

> I added a length check in this patch:

gcc will not recognize such runtime checks and will (most likely) complain about the strncpy usage anyway, just as it does now in libxl__prepare_sockaddr_un. While the usage in libxl__prepare_sockaddr_un is fatal due to -Werror, libvchan is apparently built without -Werror.

Olaf

On Jun 8, 2020, at 10:12, Olaf Hering <olaf@aepfle.de> wrote:
> 
> Am Mon, 8 Jun 2020 08:43:50 -0400
> schrieb Jason Andryuk <jandryuk@gmail.com>:
> 
>> I added a length check in this patch:
> 
> gcc will not recognize such runtime checks and will (most likely) complain about the strncpy usage anyway, just as it does now in libxl__prepare_sockaddr_un. While the usage in libxl__prepare_sockaddr_un is fatal due to -Werror, libvchan is apparently built without -Werror.
> 
> Olaf

Is there any reason not to take a patch that builds libvchan with -Werror?

Rich

Am Tue, 9 Jun 2020 12:35:51 -0400
schrieb Rich Persaud <persaur@gmail.com>:

> Is there any reason not to take a patch that builds libvchan with -Werror?

The per-subdirectory settings of -Werror should probably become a global -Werror. Someone has to step up and explore that path.
Bonus points if -Werror could be disabled via configure.

Olaf

On Mon, Jun 8, 2020 at 10:11 AM Olaf Hering <olaf@aepfle.de> wrote:
>
> Am Mon, 8 Jun 2020 08:43:50 -0400
> schrieb Jason Andryuk <jandryuk@gmail.com>:
>
> > I added a length check in this patch:
>
> gcc will not recognize such runtime checks and will (most likely) complain about the strncpy usage anyway, just as it does now in libxl__prepare_sockaddr_un. While the usage in libxl__prepare_sockaddr_un is fatal due to -Werror, libvchan is apparently built without -Werror.

gcc complaining about strncpy after the length check is unfortunate.
Do you know if gcc would complain if strcpy is used?  It would be okay
since we just checked the length.

What version of gcc are you using?  I was using 9.x and it didn't warn
from what I can remember.

Regards,
Jason

Am Tue, 9 Jun 2020 08:33:12 -0400
schrieb Jason Andryuk <jandryuk@gmail.com>:

> What version of gcc are you using?  I was using 9.x and it didn't warn from what I can remember.

This is gcc10 from current Tumbleweed. For libxl strcpy will certainly work because the length check is done prior the copying of data.

Olaf