[Qemu-devel] [PATCH] sheepdog: Replace strncpy() by g_strlcpy()

[Qemu-devel] [PATCH] sheepdog: Replace strncpy() by g_strlcpy()

[Philippe Mathieu-Daudé]

Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.

Replace the strncpy() calls by g_strlcpy() to avoid the following warning:

  block/sheepdog.c: In function 'find_vdi_name':
  block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals
  destination size [-Werror=stringop-truncation]
       strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html

 block/sheepdog.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index b229a664d9..5dc3d0c39e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1224,19 +1224,19 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
     SheepdogVdiReq hdr;
     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
     unsigned int wlen, rlen = 0;
-    char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
+    /* Ensures that the buffer is zero-filled,
+     * which is desirable since we'll soon be sending those bytes, and
+     * don't want the send_req to read uninitialized data.
+     */
+    char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN] = { };
 
     fd = connect_to_sdog(s, errp);
     if (fd < 0) {
         return fd;
     }
 
-    /* This pair of strncpy calls ensures that the buffer is zero-filled,
-     * which is desirable since we'll soon be sending those bytes, and
-     * don't want the send_req to read uninitialized data.
-     */
-    strncpy(buf, filename, SD_MAX_VDI_LEN);
-    strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
+    g_strlcpy(buf, filename, SD_MAX_VDI_LEN);
+    g_strlcpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
 
     memset(&hdr, 0, sizeof(hdr));
     if (lock) {
-- 
2.18.0

Re: [Qemu-devel] [PATCH] sheepdog: Replace strncpy() by g_strlcpy()

[David Hildenbrand]

On 18.08.2018 04:56, Philippe Mathieu-Daudé wrote:
> Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.
> 
> Replace the strncpy() calls by g_strlcpy() to avoid the following warning:
> 
>   block/sheepdog.c: In function 'find_vdi_name':
>   block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals
>   destination size [-Werror=stringop-truncation]
>        strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html
> 
>  block/sheepdog.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index b229a664d9..5dc3d0c39e 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1224,19 +1224,19 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
>      SheepdogVdiReq hdr;
>      SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
>      unsigned int wlen, rlen = 0;
> -    char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
> +    /* Ensures that the buffer is zero-filled,
> +     * which is desirable since we'll soon be sending those bytes, and
> +     * don't want the send_req to read uninitialized data.
> +     */

I this really necessary? The two g_strlcpy should still fill the whole
buffer, no?

> +    char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN] = { };
>  
>      fd = connect_to_sdog(s, errp);
>      if (fd < 0) {
>          return fd;
>      }
>  
> -    /* This pair of strncpy calls ensures that the buffer is zero-filled,
> -     * which is desirable since we'll soon be sending those bytes, and
> -     * don't want the send_req to read uninitialized data.
> -     */
> -    strncpy(buf, filename, SD_MAX_VDI_LEN);
> -    strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
> +    g_strlcpy(buf, filename, SD_MAX_VDI_LEN);
> +    g_strlcpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>  
>      memset(&hdr, 0, sizeof(hdr));
>      if (lock) {
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH] sheepdog: Replace strncpy() by g_strlcpy()

[Paolo Bonzini]

On 18/08/2018 04:56, Philippe Mathieu-Daudé wrote:
> Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.
> 
> Replace the strncpy() calls by g_strlcpy() to avoid the following warning:
> 
>   block/sheepdog.c: In function 'find_vdi_name':
>   block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals
>   destination size [-Werror=stringop-truncation]
>        strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html
> 
>  block/sheepdog.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index b229a664d9..5dc3d0c39e 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1224,19 +1224,19 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
>      SheepdogVdiReq hdr;
>      SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
>      unsigned int wlen, rlen = 0;
> -    char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
> +    /* Ensures that the buffer is zero-filled,
> +     * which is desirable since we'll soon be sending those bytes, and
> +     * don't want the send_req to read uninitialized data.
> +     */
> +    char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN] = { };
>  
>      fd = connect_to_sdog(s, errp);
>      if (fd < 0) {
>          return fd;
>      }
>  
> -    /* This pair of strncpy calls ensures that the buffer is zero-filled,
> -     * which is desirable since we'll soon be sending those bytes, and
> -     * don't want the send_req to read uninitialized data.
> -     */
> -    strncpy(buf, filename, SD_MAX_VDI_LEN);
> -    strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
> +    g_strlcpy(buf, filename, SD_MAX_VDI_LEN);
> +    g_strlcpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>  
>      memset(&hdr, 0, sizeof(hdr));
>      if (lock) {
> 

The protocol doesn't require (as far as I can see) the strings to be
NULL-terminated, therefore strncpy is the right function to use here.

However, we should have a check on the length of filename and tag, so
that no truncation is done.  This applies to both strncpy and g_strlcpy.

Indeed I find g_strlcpy to be harmful because it encourages a style
where truncations happen silently.  There are very few cases where
silent truncation is the right thing to do, and in several cases where
you _have to have_ fixed-size buffers, those buffers are sent on the
wire---and then g_strlcpy is wrong, while strncpy is just as good as
memset+strlen+memcpy (and shorter).

Paolo

Re: [Qemu-devel] [PATCH] sheepdog: Replace strncpy() by g_strlcpy()

[David Hildenbrand]

On 20.08.2018 15:28, Paolo Bonzini wrote:
> On 18/08/2018 04:56, Philippe Mathieu-Daudé wrote:
>> Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.
>>
>> Replace the strncpy() calls by g_strlcpy() to avoid the following warning:
>>
>>   block/sheepdog.c: In function 'find_vdi_name':
>>   block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals
>>   destination size [-Werror=stringop-truncation]
>>        strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html
>>
>>  block/sheepdog.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index b229a664d9..5dc3d0c39e 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -1224,19 +1224,19 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
>>      SheepdogVdiReq hdr;
>>      SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
>>      unsigned int wlen, rlen = 0;
>> -    char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
>> +    /* Ensures that the buffer is zero-filled,
>> +     * which is desirable since we'll soon be sending those bytes, and
>> +     * don't want the send_req to read uninitialized data.
>> +     */
>> +    char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN] = { };
>>  
>>      fd = connect_to_sdog(s, errp);
>>      if (fd < 0) {
>>          return fd;
>>      }
>>  
>> -    /* This pair of strncpy calls ensures that the buffer is zero-filled,
>> -     * which is desirable since we'll soon be sending those bytes, and
>> -     * don't want the send_req to read uninitialized data.
>> -     */
>> -    strncpy(buf, filename, SD_MAX_VDI_LEN);
>> -    strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>> +    g_strlcpy(buf, filename, SD_MAX_VDI_LEN);
>> +    g_strlcpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>>  
>>      memset(&hdr, 0, sizeof(hdr));
>>      if (lock) {
>>
> 
> The protocol doesn't require (as far as I can see) the strings to be
> NULL-terminated, therefore strncpy is the right function to use here.
> 
> However, we should have a check on the length of filename and tag, so
> that no truncation is done.  This applies to both strncpy and g_strlcpy.
> 
> Indeed I find g_strlcpy to be harmful because it encourages a style
> where truncations happen silently.  There are very few cases where
> silent truncation is the right thing to do, and in several cases where
> you _have to have_ fixed-size buffers, those buffers are sent on the
> wire---and then g_strlcpy is wrong, while strncpy is just as good as
> memset+strlen+memcpy (and shorter).

Yes, convinced, let's disable the warning.

> 
> Paolo
> 


-- 

Thanks,

David / dhildenb