[Qemu-devel] sheepdog build warning

[Qemu-devel] sheepdog build warning

[Michael S. Tsirkin]

mingw32 build on fedora fails with this warning:

/scm/qemu/block/sheepdog.c: In function 'find_vdi_name':
/scm/qemu/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);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/scm/qemu/rules.mak:69: block/sheepdog.o] Error 1
make: *** Waiting for unfinished jobs....


Reading the code one sees it's working as intended:


static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
                         uint32_t snapid, const char *tag, uint32_t *vid,
                         bool lock, Error **errp)
{
    int ret, fd;
    SheepdogVdiReq hdr;
    SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
    unsigned int wlen, rlen = 0;
    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);


.....

}



so this seems to be the case of GCC developers deciding that
strncpy is simply a bad API and a correct use of it should be
warned against.

I propose either

1. simply adding

#pragma GCC diagnostic ignored "-Wstringop-truncation"

in osdep.

2. adding an inline wrapper with said pragma in there.

3. -Wno-stringop-truncation is the makefile

Thoughts?

-- 
MST

Re: [Qemu-devel] sheepdog build warning

[Michael S. Tsirkin]

On Mon, Dec 17, 2018 at 11:03:11PM -0500, Michael S. Tsirkin wrote:
> mingw32 build on fedora fails with this warning:
> 
> /scm/qemu/block/sheepdog.c: In function 'find_vdi_name':
> /scm/qemu/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);
>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make: *** [/scm/qemu/rules.mak:69: block/sheepdog.o] Error 1
> make: *** Waiting for unfinished jobs....
> 
> 
> Reading the code one sees it's working as intended:
> 
> 
> static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
>                          uint32_t snapid, const char *tag, uint32_t *vid,
>                          bool lock, Error **errp)
> {
>     int ret, fd;
>     SheepdogVdiReq hdr;
>     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
>     unsigned int wlen, rlen = 0;
>     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);
> 
> 
> .....
> 
> }
> 
> 
> 
> so this seems to be the case of GCC developers deciding that
> strncpy is simply a bad API and a correct use of it should be
> warned against.
> 
> I propose either
> 
> 1. simply adding
> 
> #pragma GCC diagnostic ignored "-Wstringop-truncation"
> 
> in osdep.
> 
> 2. adding an inline wrapper with said pragma in there.
> 
> 3. -Wno-stringop-truncation is the makefile
> 
> Thoughts?
> 
> -- 


So here's approach 2. However I note that a newer gcc 8.2.1
does not give this warning. Maybe detect at configure time
and suppress (option 3)?



diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 3bf48bcdec..64d8258529 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -134,6 +134,22 @@ extern int daemon(int, int);
 #define assert(x)  g_assert(x)
 #endif
 
+/*
+ * GCC 8.0 declared war on strncpy. Admittedly it's a tricky interface
+ * with unintuitive semantics, but we use it widely.
+ */
+#if defined(__GNUC__) && __GNUC__ >= 8
+static inline void qemu_strncpy(char *to, const char *from, int n)
+{
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstringop-truncation"
+    strncpy(to, from, n);
+#pragma GCC diagnostic pop
+}
+
+#define strncpy qemu_strncpy
+#endif
+
 /*
  * According to waitpid man page:
  * WCOREDUMP

Re: [Qemu-devel] sheepdog build warning

[Philippe Mathieu-Daudé]

Hi Michael,

On 12/18/18 5:16 AM, Michael S. Tsirkin wrote:
> On Mon, Dec 17, 2018 at 11:03:11PM -0500, Michael S. Tsirkin wrote:
>> mingw32 build on fedora fails with this warning:
>>
>> /scm/qemu/block/sheepdog.c: In function 'find_vdi_name':
>> /scm/qemu/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);
>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> cc1: all warnings being treated as errors
>> make: *** [/scm/qemu/rules.mak:69: block/sheepdog.o] Error 1
>> make: *** Waiting for unfinished jobs....
>>
>>
>> Reading the code one sees it's working as intended:
>>
>>
>> static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
>>                          uint32_t snapid, const char *tag, uint32_t *vid,
>>                          bool lock, Error **errp)
>> {
>>     int ret, fd;
>>     SheepdogVdiReq hdr;
>>     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
>>     unsigned int wlen, rlen = 0;
>>     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);
>>
>>
>> .....
>>
>> }
>>
>>
>>
>> so this seems to be the case of GCC developers deciding that
>> strncpy is simply a bad API and a correct use of it should be
>> warned against.
>>
>> I propose either
>>
>> 1. simply adding
>>
>> #pragma GCC diagnostic ignored "-Wstringop-truncation"
>>
>> in osdep.
>>
>> 2. adding an inline wrapper with said pragma in there.
>>
>> 3. -Wno-stringop-truncation is the makefile
>>
>> Thoughts?
>>
>> -- 
> 
> 
> So here's approach 2. However I note that a newer gcc 8.2.1
> does not give this warning. Maybe detect at configure time
> and suppress (option 3)?

I prepared patches yesterday to fix this issue, I'll send to the list
soon. I used another approach, replacing strncpy() by strpadcpy(pad=0).

> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 3bf48bcdec..64d8258529 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -134,6 +134,22 @@ extern int daemon(int, int);
>  #define assert(x)  g_assert(x)
>  #endif
>  
> +/*
> + * GCC 8.0 declared war on strncpy. Admittedly it's a tricky interface
> + * with unintuitive semantics, but we use it widely.
> + */
> +#if defined(__GNUC__) && __GNUC__ >= 8
> +static inline void qemu_strncpy(char *to, const char *from, int n)
> +{
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wstringop-truncation"
> +    strncpy(to, from, n);
> +#pragma GCC diagnostic pop
> +}
> +
> +#define strncpy qemu_strncpy
> +#endif
> +
>  /*
>   * According to waitpid man page:
>   * WCOREDUMP
> 

Re: [Qemu-devel] sheepdog build warning

[Daniel P. Berrangé]

On Mon, Dec 17, 2018 at 11:16:05PM -0500, Michael S. Tsirkin wrote:
> On Mon, Dec 17, 2018 at 11:03:11PM -0500, Michael S. Tsirkin wrote:
> > mingw32 build on fedora fails with this warning:
> > 
> > /scm/qemu/block/sheepdog.c: In function 'find_vdi_name':
> > /scm/qemu/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);
> >      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> > make: *** [/scm/qemu/rules.mak:69: block/sheepdog.o] Error 1
> > make: *** Waiting for unfinished jobs....
> > 
> > 
> > Reading the code one sees it's working as intended:
> > 
> > 
> > static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
> >                          uint32_t snapid, const char *tag, uint32_t *vid,
> >                          bool lock, Error **errp)
> > {
> >     int ret, fd;
> >     SheepdogVdiReq hdr;
> >     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
> >     unsigned int wlen, rlen = 0;
> >     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);
> > 
> > 
> > .....
> > 
> > }
> > 
> > 
> > 
> > so this seems to be the case of GCC developers deciding that
> > strncpy is simply a bad API and a correct use of it should be
> > warned against.
> > 
> > I propose either
> > 
> > 1. simply adding
> > 
> > #pragma GCC diagnostic ignored "-Wstringop-truncation"
> > 
> > in osdep.
> > 
> > 2. adding an inline wrapper with said pragma in there.
> > 
> > 3. -Wno-stringop-truncation is the makefile
> > 
> > Thoughts?
> > 
> > -- 
> 
> 
> So here's approach 2. However I note that a newer gcc 8.2.1
> does not give this warning. Maybe detect at configure time
> and suppress (option 3)?

I'm not 

> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 3bf48bcdec..64d8258529 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -134,6 +134,22 @@ extern int daemon(int, int);
>  #define assert(x)  g_assert(x)
>  #endif
>  
> +/*
> + * GCC 8.0 declared war on strncpy. Admittedly it's a tricky interface
> + * with unintuitive semantics, but we use it widely.
> + */
> +#if defined(__GNUC__) && __GNUC__ >= 8
> +static inline void qemu_strncpy(char *to, const char *from, int n)
> +{
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wstringop-truncation"
> +    strncpy(to, from, n);
> +#pragma GCC diagnostic pop
> +}
> +
> +#define strncpy qemu_strncpy
> +#endif

I don't think we should disable the warning for all uses of strncpy
as that defeats the protection this warning offers for bad usage.

IMHO, just push/pop the warning pragma inline in the sheepdog code
which triggers it.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|