[PATCH v2] libxl__prepare_sockaddr_un

[PATCH v2] libxl__prepare_sockaddr_un

[Olaf Hering]

libxl: remove usage of strncpy from libxl__prepare_sockaddr_un

The runtime check for the length of path already prevents malfunction.
But gcc does not detect this runtime check and reports incorrect
usage of strncpy. Remove the usage of strncpy and work directly with
the calculated path length.

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>
---
 tools/libxl/libxl_utils.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index f360f5e228..40885794c9 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1252,14 +1252,16 @@ int libxl__prepare_sockaddr_un(libxl__gc *gc,
                                struct sockaddr_un *un, const char *path,
                                const char *what)
 {
-    if (sizeof(un->sun_path) <= strlen(path)) {
+    size_t len = strlen(path);
+
+    if (sizeof(un->sun_path) <= len) {
         LOG(ERROR, "UNIX socket path '%s' is too long for %s", path, what);
-        LOG(DEBUG, "Path must be less than %zu bytes", sizeof(un->sun_path));
+        LOG(DEBUG, "Path of len %zu must be less than %zu bytes", len, sizeof(un->sun_path));
         return ERROR_INVAL;
     }
     memset(un, 0, sizeof(struct sockaddr_un));
     un->sun_family = AF_UNIX;
-    strncpy(un->sun_path, path, sizeof(un->sun_path));
+    memcpy(un->sun_path, path, len);
     return 0;
 }
 

Re: [PATCH v2] libxl__prepare_sockaddr_un

[Ian Jackson]

Olaf Hering writes ("[PATCH v2] libxl__prepare_sockaddr_un"):
> libxl: remove usage of strncpy from libxl__prepare_sockaddr_un
> 
> The runtime check for the length of path already prevents malfunction.
> But gcc does not detect this runtime check and reports incorrect
> usage of strncpy. Remove the usage of strncpy and work directly with
> the calculated path length.
> 
> 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

Thanks for working on this.


I found this code a bit confusing:

> -    if (sizeof(un->sun_path) <= strlen(path)) {
> +    size_t len = strlen(path);
> +
> +    if (sizeof(un->sun_path) <= len) {
>          LOG(ERROR, "UNIX socket path '%s' is too long for %s", path, what);
> -        LOG(DEBUG, "Path must be less than %zu bytes", sizeof(un->sun_path));
> +        LOG(DEBUG, "Path of len %zu must be less than %zu bytes", len, sizeof(un->sun_path));
>          return ERROR_INVAL;
>      }
>      memset(un, 0, sizeof(struct sockaddr_un));
>      un->sun_family = AF_UNIX;
> -    strncpy(un->sun_path, path, sizeof(un->sun_path));
> +    memcpy(un->sun_path, path, len);

This does not copy the trailing nul.  The reader must read up to see
the call to memset.  Why do you not use strcpy here ?

Nevertheless, as this is a minor point of style,

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

Re: [PATCH v2] libxl__prepare_sockaddr_un

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 230 bytes --]

Am Tue, 9 Jun 2020 14:00:31 +0100
schrieb Ian Jackson <ian.jackson@citrix.com>:

> Why do you not use strcpy here ?

Either variant of 'cpy' will work in this context. I decided to use memcpy for no specific reason.

Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]