Re: [PATCH v2] libxl__prepare_sockaddr_un

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>

Ian.