[Qemu-devel] [Bug 1803872] [NEW] gcc 8.2 reports stringop-truncation when building qemu

[Qemu-devel] [Bug 1803872] [NEW] gcc 8.2 reports stringop-truncation when building qemu

[Amir Gonnen]

Public bug reported:

QEMU 3.0

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);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


If this is the intended behavior, please suppress the warning. For example:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-truncation"
    strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
#pragma GCC diagnostic pop

** Affects: qemu
     Importance: Undecided
         Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1803872

Title:
  gcc 8.2 reports stringop-truncation when building qemu

Status in QEMU:
  New

Bug description:
  QEMU 3.0

  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);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  
  If this is the intended behavior, please suppress the warning. For example:

  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wstringop-truncation"
      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  #pragma GCC diagnostic pop

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1803872/+subscriptions

[Qemu-devel] [Bug 1803872] Re: gcc 8.2 reports stringop-truncation when building qemu

[Amir Gonnen]

** Description changed:

  QEMU 3.0
  
  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);
-      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
+      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  
- 
- If this is the intended behavior, please suppress the warning. For example:
+ If this is the intended behavior, please suppress the warning. For
+ example:
  
  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wstringop-truncation"
-     strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
+     strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  #pragma GCC diagnostic pop
+ 
+ This also happens on other sources, for example hw/acpi/core.c, so
+ another option is to suppress it globally on CFLAGS (-Wno-stringop-
+ truncation)

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1803872

Title:
  gcc 8.2 reports stringop-truncation when building qemu

Status in QEMU:
  New

Bug description:
  QEMU 3.0

  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);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  If this is the intended behavior, please suppress the warning. For
  example:

  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wstringop-truncation"
      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  #pragma GCC diagnostic pop

  This also happens on other sources, for example hw/acpi/core.c, so
  another option is to suppress it globally on CFLAGS (-Wno-stringop-
  truncation)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1803872/+subscriptions

[Qemu-devel] [Bug 1803872] Re: [PATCH v2 3/3] migration: Replace strncpy() by strpadcpy(pad='\0')

[elmarco]

On Tue, Dec 18, 2018 at 3:09 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> GCC 8 added a -Wstringop-truncation warning:
>
>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>   bug 81117 is specifically intended to highlight likely unintended
>   uses of the strncpy function that truncate the terminating NUL
>   character from the source string.
>
> This new warning leads to compilation failures:
>
>     CC      migration/global_state.o
>   qemu/migration/global_state.c: In function 'global_state_store_running':
>   qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
>        strncpy((char *)global_state.runstate, state, sizeof(global_state.runstate));
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
>
> The runstate name doesn't require the strings to be NUL-terminated,
> therefore strncpy is the right function to use here.
>
> We could add a #pragma GCC diagnostic ignored "-Wstringop-truncation"
> around, disable the warning globally using -Wno-stringop-truncation,
> but since QEMU provides the strpadcpy() which does the same purpose,
> simply use it to avoid the annoying warning.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  migration/global_state.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 8e8ab5c51e..c7e7618118 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -42,8 +42,8 @@ int global_state_store(void)
>  void global_state_store_running(void)
>  {
>      const char *state = RunState_str(RUN_STATE_RUNNING);
> -    strncpy((char *)global_state.runstate,
> -           state, sizeof(global_state.runstate));
> +    strpadcpy((char *)global_state.runstate,
> +              sizeof(global_state.runstate), state, '\0');
>  }
>
>  bool global_state_received(void)
> --
> 2.17.2
>
>


-- 
Marc-André Lureau

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1803872

Title:
  gcc 8.2 reports stringop-truncation when building qemu

Status in QEMU:
  New

Bug description:
  QEMU 3.0

  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);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  If this is the intended behavior, please suppress the warning. For
  example:

  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wstringop-truncation"
      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  #pragma GCC diagnostic pop

  This also happens on other sources, for example hw/acpi/core.c, so
  another option is to suppress it globally on CFLAGS (-Wno-stringop-
  truncation)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1803872/+subscriptions

[Qemu-devel] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation

[Philippe Mathieu-Daudé]

GCC 8 new warning prevents builds to success since quite some time.
First report on the mailing list is in July 2018:
https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg03723.html

Various intents has been sent to fix this:
- Incorrectly using g_strlcpy()
  https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03705.html
  https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03706.html
- Using assert() and strpadcpy()
  https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03938.html
- Use #pragma GCC diagnostic ignored "-Wstringop-truncation"
  https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html
- adding an inline wrapper with said pragma in there
  https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html
- -Wno-stringop-truncation is the makefile
  https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html
- Use the 'nonstring' attribute
  https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04493.html

This series add the QEMU_NONSTRING definition and use it.

Regards,

Phil.

Philippe Mathieu-Daudé (5):
  qemu/compiler: Define QEMU_NONSTRING
  block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays
  hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays
  migration: Use QEMU_NONSTRING for non NUL-terminated arrays
  migration: Use strnlen() for fixed-size string

 block/sheepdog.c            |  2 +-
 hw/acpi/core.c              |  8 ++++----
 include/hw/acpi/acpi-defs.h |  8 ++++----
 include/qemu/compiler.h     | 15 +++++++++++++++
 migration/global_state.c    |  4 ++--
 5 files changed, 26 insertions(+), 11 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v3 1/5] qemu/compiler: Define QEMU_NONSTRING

[Philippe Mathieu-Daudé]

GCC 8 introduced the -Wstringop-truncation checker to detect truncation by
the strncat and strncpy functions (closely related to -Wstringop-overflow,
which detect buffer overflow by string-modifying functions declared in
<string.h>).

Add the QEMU_NONSTRING macro which checks if the compiler supports this
attribute.

>From the GCC manual [*]:

  The nonstring variable attribute specifies that an object or member
  declaration with type array of char, signed char, or unsigned char,
  or pointer to such a type is intended to store character arrays that
  do not necessarily contain a terminating NUL. This is useful in detecting
  uses of such arrays or pointers with functions that expect NUL-terminated
  strings, and to avoid warnings when such an array or pointer is used as
  an argument to a bounded string manipulation function such as strncpy.

[*] https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/compiler.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 261842beae..2d8f507c73 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -151,6 +151,21 @@
 # define QEMU_ERROR(X)
 #endif
 
+/*
+ * The nonstring variable attribute specifies that an object or member
+ * declaration with type array of char or pointer to char is intended
+ * to store character arrays that do not necessarily contain a terminating
+ * NUL character. This is useful in detecting uses of such arrays or pointers
+ * with functions that expect NUL-terminated strings, and to avoid warnings
+ * when such an array or pointer is used as an argument to a bounded string
+ * manipulation function such as strncpy.
+ */
+#if __has_attribute(nonstring)
+# define QEMU_NONSTRING __attribute__((nonstring))
+#else
+# define QEMU_NONSTRING
+#endif
+
 /* Implement C11 _Generic via GCC builtins.  Example:
  *
  *    QEMU_GENERIC(x, (float, sinf), (long double, sinl), sin) (x)
-- 
2.17.2

[Qemu-devel] [PATCH v3 2/5] block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays

[Philippe Mathieu-Daudé]

GCC 8 added a -Wstringop-truncation warning:

  The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
  bug 81117 is specifically intended to highlight likely unintended
  uses of the strncpy function that truncate the terminating NUL
  character from the source string.

This new warning leads to compilation failures:

    CC      block/sheepdog.o
  qemu/block/sheepdog.c: In function 'find_vdi_name':
  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);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  make: *** [qemu/rules.mak:69: block/sheepdog.o] Error 1

As described previous to the strncpy() calls, the use of strncpy() is
correct here:

    /* 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);

Use the QEMU_NONSTRING attribute, since this array is intended to store
character arrays that do not necessarily contain a terminating NUL.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/sheepdog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0125df9d49..d4ad6b119d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1224,7 +1224,7 @@ 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];
+    QEMU_NONSTRING char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
 
     fd = connect_to_sdog(s, errp);
     if (fd < 0) {
-- 
2.17.2

[Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Philippe Mathieu-Daudé]

GCC 8 added a -Wstringop-truncation warning:

  The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
  bug 81117 is specifically intended to highlight likely unintended
  uses of the strncpy function that truncate the terminating NUL
  character from the source string.

This new warning leads to compilation failures:

    CC      hw/acpi/core.o
  In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5:
  qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
           strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1

Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
strings to be NUL-terminated.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/acpi/core.c              | 8 ++++----
 include/hw/acpi/acpi-defs.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index aafdc61648..f60f750c3d 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -35,14 +35,14 @@
 struct acpi_table_header {
     uint16_t _length;         /* our length, not actual part of the hdr */
                               /* allows easier parsing for fw_cfg clients */
-    char sig[4];              /* ACPI signature (4 ASCII characters) */
+    char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) */
     uint32_t length;          /* Length of table, in bytes, including header */
     uint8_t revision;         /* ACPI Specification minor version # */
     uint8_t checksum;         /* To make sum of entire table == 0 */
-    char oem_id[6];           /* OEM identification */
-    char oem_table_id[8];     /* OEM table identification */
+    char oem_id[6] QEMU_NONSTRING; /* OEM identification */
+    char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */
     uint32_t oem_revision;    /* OEM revision number */
-    char asl_compiler_id[4];  /* ASL compiler vendor ID */
+    char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */
     uint32_t asl_compiler_revision; /* ASL compiler revision number */
 } QEMU_PACKED;
 
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index af8e023968..3bf0bec8ba 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -43,7 +43,7 @@ enum {
 struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
     uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
     uint8_t  checksum;               /* To make sum of struct == 0 */
-    uint8_t  oem_id [6];             /* OEM identification */
+    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */
     uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
     uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
     uint32_t length;                 /* XSDT Length in bytes including hdr */
@@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
     uint32_t length;                 /* Length of table, in bytes, including header */ \
     uint8_t  revision;               /* ACPI Specification minor version # */ \
     uint8_t  checksum;               /* To make sum of entire table == 0 */ \
-    uint8_t  oem_id [6];             /* OEM identification */ \
-    uint8_t  oem_table_id [8];       /* OEM table identification */ \
+    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */ \
+    uint8_t  oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ \
     uint32_t oem_revision;           /* OEM revision number */ \
-    uint8_t  asl_compiler_id [4];    /* ASL compiler vendor ID */ \
+    uint8_t  asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID */ \
     uint32_t asl_compiler_revision;  /* ASL compiler revision number */
 
 
-- 
2.17.2

[Qemu-devel] [PATCH v3 4/5] migration: Use QEMU_NONSTRING for non NUL-terminated arrays

[Philippe Mathieu-Daudé]

GCC 8 added a -Wstringop-truncation warning:

  The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
  bug 81117 is specifically intended to highlight likely unintended
  uses of the strncpy function that truncate the terminating NUL
  character from the source string.

This new warning leads to compilation failures:

    CC      migration/global_state.o
  qemu/migration/global_state.c: In function 'global_state_store_running':
  qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
       strncpy((char *)global_state.runstate, state, sizeof(global_state.runstate));
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1

Use the QEMU_NONSTRING attribute, since this array is intended to store
character arrays that do not necessarily contain a terminating NUL.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 migration/global_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/global_state.c b/migration/global_state.c
index 8e8ab5c51e..6e19333422 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -21,7 +21,7 @@
 
 typedef struct {
     uint32_t size;
-    uint8_t runstate[100];
+    uint8_t runstate[100] QEMU_NONSTRING;
     RunState state;
     bool received;
 } GlobalState;
-- 
2.17.2

[Qemu-devel] [PATCH v3 5/5] migration: Use strnlen() for fixed-size string

[Philippe Mathieu-Daudé]

GCC 8 introduced the -Wstringop-overflow, which detect buffer overflow
by string-modifying functions declared in <string.h>, such strncpy(),
used in global_state_store_running().

Since the global_state.runstate does not necessarily contains a
terminating NUL character, We had to use the QEMU_NONSTRING attribute.

The GCC manual says about the nonstring attribute:

  However, when the array is declared with the attribute the call to
  strlen is diagnosed because when the array doesn’t contain a
  NUL-terminated string the call is undefined. [...]
  In addition, calling strnlen and strndup with such arrays is safe
  provided a suitable bound is specified, and not diagnosed.

GCC indeed found an incorrect use of strlen(), because this array
is loaded by VMSTATE_BUFFER(runstate, GlobalState) then parsed
using qapi_enum_parse which does not get the buffer length.

Use strnlen() which returns sizeof(s->runstate) if the array is not
NUL-terminated.

This fixes:

    CC      migration/global_state.o
  qemu/migration/global_state.c: In function 'global_state_pre_save':
  qemu/migration/global_state.c:109:15: error: 'strlen' argument 1 declared attribute 'nonstring' [-Werror=stringop-overflow=]
       s->size = strlen((char *)s->runstate) + 1;
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~
  qemu/migration/global_state.c:24:13: note: argument 'runstate' declared here
       uint8_t runstate[100] QEMU_NONSTRING;
               ^~~~~~~~
  cc1: all warnings being treated as errors
  make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 migration/global_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/global_state.c b/migration/global_state.c
index 6e19333422..c19030ef62 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
     GlobalState *s = opaque;
 
     trace_migrate_global_state_pre_save((char *)s->runstate);
-    s->size = strlen((char *)s->runstate) + 1;
+    s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;
 
     return 0;
 }
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation

[Philippe Mathieu-Daudé]

On 12/18/18 6:51 PM, Philippe Mathieu-Daudé wrote:
> GCC 8 new warning prevents builds to success since quite some time.
> First report on the mailing list is in July 2018:
> https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg03723.html
> 
> Various intents has been sent to fix this:
> - Incorrectly using g_strlcpy()
>   https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03705.html
>   https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03706.html
> - Using assert() and strpadcpy()
>   https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03938.html

And I forgot to add here:

  This was the approch taken by the previous v2:
  https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04405.html

> - Use #pragma GCC diagnostic ignored "-Wstringop-truncation"
>   https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html
> - adding an inline wrapper with said pragma in there
>   https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html
> - -Wno-stringop-truncation is the makefile
>   https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html
> - Use the 'nonstring' attribute
>   https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04493.html
> 
> This series add the QEMU_NONSTRING definition and use it.
> 
> Regards,
> 
> Phil.

Re: [Qemu-devel] [PATCH v3 1/5] qemu/compiler: Define QEMU_NONSTRING

[Eric Blake]

On 12/18/18 11:51 AM, Philippe Mathieu-Daudé wrote:
> GCC 8 introduced the -Wstringop-truncation checker to detect truncation by
> the strncat and strncpy functions (closely related to -Wstringop-overflow,
> which detect buffer overflow by string-modifying functions declared in
> <string.h>).

This paragraph talks about a new warning checker, but makes no mention 
of an attribute.

> 
> Add the QEMU_NONSTRING macro which checks if the compiler supports this
> attribute.

Thus, "this attribute" has no antecedent; did you forget to add a 
sentence to the previous paragraph, or maybe put the mention of adding 
QEMU_NONSTRING after...

> 
>>From the GCC manual [*]:
> 
>    The nonstring variable attribute specifies that an object or member
>    declaration with type array of char, signed char, or unsigned char,
>    or pointer to such a type is intended to store character arrays that
>    do not necessarily contain a terminating NUL. This is useful in detecting
>    uses of such arrays or pointers with functions that expect NUL-terminated
>    strings, and to avoid warnings when such an array or pointer is used as
>    an argument to a bounded string manipulation function such as strncpy.

...the explanation of how the attribute was added in tandem with the new 
warning checker for silencing specific instances of the warning?

> 
> [*] https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   include/qemu/compiler.h | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 2/5] block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays

[Eric Blake]

On 12/18/18 11:51 AM, Philippe Mathieu-Daudé wrote:
> GCC 8 added a -Wstringop-truncation warning:
> 
>    The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>    bug 81117 is specifically intended to highlight likely unintended
>    uses of the strncpy function that truncate the terminating NUL
>    character from the source string.
> 
> This new warning leads to compilation failures:
> 
>      CC      block/sheepdog.o
>    qemu/block/sheepdog.c: In function 'find_vdi_name':
>    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);
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    make: *** [qemu/rules.mak:69: block/sheepdog.o] Error 1
> 
> As described previous to the strncpy() calls, the use of strncpy() is
> correct here:
> 
>      /* 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);
> 
> Use the QEMU_NONSTRING attribute, since this array is intended to store
> character arrays that do not necessarily contain a terminating NUL.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   block/sheepdog.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 0125df9d49..d4ad6b119d 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1224,7 +1224,7 @@ 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];
> +    QEMU_NONSTRING char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
>   
>       fd = connect_to_sdog(s, errp);
>       if (fd < 0) {
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 1/5] qemu/compiler: Define QEMU_NONSTRING

[Philippe Mathieu-Daudé]

On 12/18/18 7:29 PM, Eric Blake wrote:
> On 12/18/18 11:51 AM, Philippe Mathieu-Daudé wrote:
>> GCC 8 introduced the -Wstringop-truncation checker to detect
>> truncation by
>> the strncat and strncpy functions (closely related to
>> -Wstringop-overflow,
>> which detect buffer overflow by string-modifying functions declared in
>> <string.h>).
> 
> This paragraph talks about a new warning checker, but makes no mention
> of an attribute.
> 
>>
>> Add the QEMU_NONSTRING macro which checks if the compiler supports this
>> attribute.
> 
> Thus, "this attribute" has no antecedent; did you forget to add a
> sentence to the previous paragraph, or maybe put the mention of adding
> QEMU_NONSTRING after...
> 
>>
>>> From the GCC manual [*]:
>>
>>    The nonstring variable attribute specifies that an object or member
>>    declaration with type array of char, signed char, or unsigned char,
>>    or pointer to such a type is intended to store character arrays that
>>    do not necessarily contain a terminating NUL. This is useful in
>> detecting
>>    uses of such arrays or pointers with functions that expect
>> NUL-terminated
>>    strings, and to avoid warnings when such an array or pointer is
>> used as
>>    an argument to a bounded string manipulation function such as strncpy.
> 
> ...the explanation of how the attribute was added in tandem with the new
> warning checker for silencing specific instances of the warning?

Yes... I will rewrite this.

> 
>>
>> [*]
>> https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute
>>
>>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   include/qemu/compiler.h | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[Qemu-devel] [Bug 1803872] Re: [PATCH v3 4/5] migration: Use QEMU_NONSTRING for non NUL-terminated arrays

[Eric Blake]

On 12/18/18 11:51 AM, Philippe Mathieu-Daudé wrote:
> GCC 8 added a -Wstringop-truncation warning:
> 
>    The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>    bug 81117 is specifically intended to highlight likely unintended
>    uses of the strncpy function that truncate the terminating NUL
>    character from the source string.
> 
> This new warning leads to compilation failures:
> 
>      CC      migration/global_state.o
>    qemu/migration/global_state.c: In function 'global_state_store_running':
>    qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
>         strncpy((char *)global_state.runstate, state, sizeof(global_state.runstate));
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
> 
> Use the QEMU_NONSTRING attribute, since this array is intended to store
> character arrays that do not necessarily contain a terminating NUL.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   migration/global_state.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Should this be squashed with 5/5?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1803872

Title:
  gcc 8.2 reports stringop-truncation when building qemu

Status in QEMU:
  New

Bug description:
  QEMU 3.0

  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);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  If this is the intended behavior, please suppress the warning. For
  example:

  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wstringop-truncation"
      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  #pragma GCC diagnostic pop

  This also happens on other sources, for example hw/acpi/core.c, so
  another option is to suppress it globally on CFLAGS (-Wno-stringop-
  truncation)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1803872/+subscriptions

[Qemu-devel] [Bug 1803872] Re: [PATCH v3 5/5] migration: Use strnlen() for fixed-size string

[Eric Blake]

On 12/18/18 11:51 AM, Philippe Mathieu-Daudé wrote:
> GCC 8 introduced the -Wstringop-overflow, which detect buffer overflow
> by string-modifying functions declared in <string.h>, such strncpy(),
> used in global_state_store_running().
> 
> Since the global_state.runstate does not necessarily contains a
> terminating NUL character, We had to use the QEMU_NONSTRING attribute.

s/We/we/

> 
> The GCC manual says about the nonstring attribute:
> 
>    However, when the array is declared with the attribute the call to
>    strlen is diagnosed because when the array doesn’t contain a
>    NUL-terminated string the call is undefined. [...]
>    In addition, calling strnlen and strndup with such arrays is safe
>    provided a suitable bound is specified, and not diagnosed.
> 
> GCC indeed found an incorrect use of strlen(), because this array
> is loaded by VMSTATE_BUFFER(runstate, GlobalState) then parsed
> using qapi_enum_parse which does not get the buffer length.
> 
> Use strnlen() which returns sizeof(s->runstate) if the array is not
> NUL-terminated.
> 
> This fixes:
> 
>      CC      migration/global_state.o
>    qemu/migration/global_state.c: In function 'global_state_pre_save':
>    qemu/migration/global_state.c:109:15: error: 'strlen' argument 1 declared attribute 'nonstring' [-Werror=stringop-overflow=]
>         s->size = strlen((char *)s->runstate) + 1;
>                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>    qemu/migration/global_state.c:24:13: note: argument 'runstate' declared here
>         uint8_t runstate[100] QEMU_NONSTRING;
>                 ^~~~~~~~
>    cc1: all warnings being treated as errors
>    make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   migration/global_state.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 6e19333422..c19030ef62 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
>       GlobalState *s = opaque;
>   
>       trace_migrate_global_state_pre_save((char *)s->runstate);
> -    s->size = strlen((char *)s->runstate) + 1;

The old code sets s->size to the string length + space for the NUL byte 
(by assuming that a NUL byte was present), and accidentally sets it 
beyond the s->runstate array if there was no NUL byte (our existing 
runstate names are shorter than 100 bytes, so this could only happen on 
a malicious stream).

> +    s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;

The new code can still end up setting s->size beyond the array.  Is that 
intended, or would it be better to use strnlen(s->runstate, 
sizeof(s->runstate) - 1) + 1?

Also, as I argued on 4/5, why isn't this squashed in with the patch that 
marks the field NONSTRING?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1803872

Title:
  gcc 8.2 reports stringop-truncation when building qemu

Status in QEMU:
  New

Bug description:
  QEMU 3.0

  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);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  If this is the intended behavior, please suppress the warning. For
  example:

  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wstringop-truncation"
      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  #pragma GCC diagnostic pop

  This also happens on other sources, for example hw/acpi/core.c, so
  another option is to suppress it globally on CFLAGS (-Wno-stringop-
  truncation)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1803872/+subscriptions

Re: [Qemu-devel] [PATCH v3 5/5] migration: Use strnlen() for fixed-size string

[Paolo Bonzini]

On 18/12/18 20:33, Eric Blake wrote:
>> diff --git a/migration/global_state.c b/migration/global_state.c
>> index 6e19333422..c19030ef62 100644
>> --- a/migration/global_state.c
>> +++ b/migration/global_state.c
>> @@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
>>       GlobalState *s = opaque;
>>         trace_migrate_global_state_pre_save((char *)s->runstate);
>> -    s->size = strlen((char *)s->runstate) + 1;
> 
> The old code sets s->size to the string length + space for the NUL byte
> (by assuming that a NUL byte was present), and accidentally sets it
> beyond the s->runstate array if there was no NUL byte (our existing
> runstate names are shorter than 100 bytes, so this could only happen on
> a malicious stream).

It cannot---this is a pre_save hook.  A possible overflow bug exists,
but it is in the call to qapi_enum_parse.

Paolo

[Qemu-devel] [Bug 1803872] Re: [PATCH v3 4/5] migration: Use QEMU_NONSTRING for non NUL-terminated arrays

[Eric Blake]

On 12/18/18 11:51 AM, Philippe Mathieu-Daudé wrote:
> GCC 8 added a -Wstringop-truncation warning:
> 
>    The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>    bug 81117 is specifically intended to highlight likely unintended
>    uses of the strncpy function that truncate the terminating NUL
>    character from the source string.
> 
> This new warning leads to compilation failures:
> 
>      CC      migration/global_state.o
>    qemu/migration/global_state.c: In function 'global_state_store_running':
>    qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
>         strncpy((char *)global_state.runstate, state, sizeof(global_state.runstate));
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
> 
> Use the QEMU_NONSTRING attribute, since this array is intended to store
> character arrays that do not necessarily contain a terminating NUL.

>   typedef struct {
>       uint32_t size;
> -    uint8_t runstate[100];
> +    uint8_t runstate[100] QEMU_NONSTRING;

Since 100 bytes for runstate[] is larger than any string possible in our 
current enum string values, could we instead add an assert that 
strlen(state) < sizeof(global_state.runstate), and then use strpadcpy() 
to make our intent obvious while still shutting up the compiler warning, 
but without having to deal with the fallout of marking runstate as a 
non-string?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1803872

Title:
  gcc 8.2 reports stringop-truncation when building qemu

Status in QEMU:
  New

Bug description:
  QEMU 3.0

  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);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  If this is the intended behavior, please suppress the warning. For
  example:

  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wstringop-truncation"
      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  #pragma GCC diagnostic pop

  This also happens on other sources, for example hw/acpi/core.c, so
  another option is to suppress it globally on CFLAGS (-Wno-stringop-
  truncation)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1803872/+subscriptions

Re: [Qemu-devel] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation

[Michael S. Tsirkin]

On Tue, Dec 18, 2018 at 06:51:17PM +0100, Philippe Mathieu-Daudé wrote:
> GCC 8 new warning prevents builds to success since quite some time.
> First report on the mailing list is in July 2018:
> https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg03723.html
> 
> Various intents has been sent to fix this:
> - Incorrectly using g_strlcpy()
>   https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03705.html
>   https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03706.html
> - Using assert() and strpadcpy()
>   https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03938.html
> - Use #pragma GCC diagnostic ignored "-Wstringop-truncation"
>   https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html
> - adding an inline wrapper with said pragma in there
>   https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html
> - -Wno-stringop-truncation is the makefile
>   https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html
> - Use the 'nonstring' attribute
>   https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04493.html
> 
> This series add the QEMU_NONSTRING definition and use it.
> 
> Regards,
> 
> Phil.


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> Philippe Mathieu-Daudé (5):
>   qemu/compiler: Define QEMU_NONSTRING
>   block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays
>   hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays
>   migration: Use QEMU_NONSTRING for non NUL-terminated arrays
>   migration: Use strnlen() for fixed-size string
> 
>  block/sheepdog.c            |  2 +-
>  hw/acpi/core.c              |  8 ++++----
>  include/hw/acpi/acpi-defs.h |  8 ++++----
>  include/qemu/compiler.h     | 15 +++++++++++++++
>  migration/global_state.c    |  4 ++--
>  5 files changed, 26 insertions(+), 11 deletions(-)
> 
> -- 
> 2.17.2

Re: [Qemu-devel] [PATCH v3 2/5] block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays

[Michael S. Tsirkin]

On Tue, Dec 18, 2018 at 06:51:19PM +0100, Philippe Mathieu-Daudé wrote:
> GCC 8 added a -Wstringop-truncation warning:
> 
>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>   bug 81117 is specifically intended to highlight likely unintended
>   uses of the strncpy function that truncate the terminating NUL
>   character from the source string.
> 
> This new warning leads to compilation failures:
> 
>     CC      block/sheepdog.o
>   qemu/block/sheepdog.c: In function 'find_vdi_name':
>   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);
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   make: *** [qemu/rules.mak:69: block/sheepdog.o] Error 1
> 
> As described previous to the strncpy() calls, the use of strncpy() is
> correct here:
> 
>     /* 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);
> 
> Use the QEMU_NONSTRING attribute, since this array is intended to store
> character arrays that do not necessarily contain a terminating NUL.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/sheepdog.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 0125df9d49..d4ad6b119d 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1224,7 +1224,7 @@ 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];
> +    QEMU_NONSTRING char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];

In case you decide to respin anyway - this would be
a bit nicer as:
	char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN] QEMU_NONSTRING




>      fd = connect_to_sdog(s, errp);
>      if (fd < 0) {
> -- 
> 2.17.2

Re: [Qemu-devel] [PATCH v3 5/5] migration: Use strnlen() for fixed-size string

[Michael S. Tsirkin]

On Tue, Dec 18, 2018 at 06:51:22PM +0100, Philippe Mathieu-Daudé wrote:
> GCC 8 introduced the -Wstringop-overflow, which detect buffer overflow
> by string-modifying functions declared in <string.h>, such strncpy(),
> used in global_state_store_running().
> 
> Since the global_state.runstate does not necessarily contains a
> terminating NUL character, We had to use the QEMU_NONSTRING attribute.
> 
> The GCC manual says about the nonstring attribute:
> 
>   However, when the array is declared with the attribute the call to
>   strlen is diagnosed because when the array doesn’t contain a
>   NUL-terminated string the call is undefined. [...]
>   In addition, calling strnlen and strndup with such arrays is safe
>   provided a suitable bound is specified, and not diagnosed.
> 
> GCC indeed found an incorrect use of strlen(), because this array
> is loaded by VMSTATE_BUFFER(runstate, GlobalState) then parsed
> using qapi_enum_parse which does not get the buffer length.
> 
> Use strnlen() which returns sizeof(s->runstate) if the array is not
> NUL-terminated.
> 
> This fixes:
> 
>     CC      migration/global_state.o
>   qemu/migration/global_state.c: In function 'global_state_pre_save':
>   qemu/migration/global_state.c:109:15: error: 'strlen' argument 1 declared attribute 'nonstring' [-Werror=stringop-overflow=]
>        s->size = strlen((char *)s->runstate) + 1;
>                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   qemu/migration/global_state.c:24:13: note: argument 'runstate' declared here
>        uint8_t runstate[100] QEMU_NONSTRING;
>                ^~~~~~~~
>   cc1: all warnings being treated as errors
>   make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  migration/global_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 6e19333422..c19030ef62 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
>      GlobalState *s = opaque;
>  
>      trace_migrate_global_state_pre_save((char *)s->runstate);
> -    s->size = strlen((char *)s->runstate) + 1;
> +    s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;
>  
>      return 0;

I don't think this works correctly if strnlen returns
sizeof(s->runstate). Which never happens so we probably should
jus add

	assert(e->size is <= sizeof(s->runstate));

But also I think this is not enough, there's a problem in post-load
in the call to qapi_enum_parse. You probably want to force
the last character to 0 there.


>  }
> -- 
> 2.17.2

Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Igor Mammedov]

On Tue, 18 Dec 2018 18:51:20 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> GCC 8 added a -Wstringop-truncation warning:
> 
>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>   bug 81117 is specifically intended to highlight likely unintended
>   uses of the strncpy function that truncate the terminating NUL
>   character from the source string.
> 
> This new warning leads to compilation failures:
> 
>     CC      hw/acpi/core.o
>   In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5:
>   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
>            strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> 
> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
> strings to be NUL-terminated.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/acpi/core.c              | 8 ++++----
>  include/hw/acpi/acpi-defs.h | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index aafdc61648..f60f750c3d 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -35,14 +35,14 @@
>  struct acpi_table_header {
>      uint16_t _length;         /* our length, not actual part of the hdr */
>                                /* allows easier parsing for fw_cfg clients */
> -    char sig[4];              /* ACPI signature (4 ASCII characters) */
> +    char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) */
>      uint32_t length;          /* Length of table, in bytes, including header */
>      uint8_t revision;         /* ACPI Specification minor version # */
>      uint8_t checksum;         /* To make sum of entire table == 0 */
> -    char oem_id[6];           /* OEM identification */
> -    char oem_table_id[8];     /* OEM table identification */
> +    char oem_id[6] QEMU_NONSTRING; /* OEM identification */
> +    char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */
>      uint32_t oem_revision;    /* OEM revision number */
> -    char asl_compiler_id[4];  /* ASL compiler vendor ID */
> +    char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */
>      uint32_t asl_compiler_revision; /* ASL compiler revision number */
>  } QEMU_PACKED;
>  
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index af8e023968..3bf0bec8ba 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -43,7 +43,7 @@ enum {
>  struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
>      uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
>      uint8_t  checksum;               /* To make sum of struct == 0 */
> -    uint8_t  oem_id [6];             /* OEM identification */
> +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */
>      uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
>      uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
>      uint32_t length;                 /* XSDT Length in bytes including hdr */

you'll need to rebase this on top the latest Michael's pull request.
[PULL v2 25/30] hw: arm: Carry RSDP specific data through  AcpiRsdpData
[PULL v2 29/30] hw: acpi: Remove AcpiRsdpDescriptor and fix tests

> @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
>      uint32_t length;                 /* Length of table, in bytes, including header */ \
>      uint8_t  revision;               /* ACPI Specification minor version # */ \
>      uint8_t  checksum;               /* To make sum of entire table == 0 */ \
> -    uint8_t  oem_id [6];             /* OEM identification */ \
> -    uint8_t  oem_table_id [8];       /* OEM table identification */ \
> +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */ \
> +    uint8_t  oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ \
>      uint32_t oem_revision;           /* OEM revision number */ \
> -    uint8_t  asl_compiler_id [4];    /* ASL compiler vendor ID */ \
> +    uint8_t  asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID */ \
>      uint32_t asl_compiler_revision;  /* ASL compiler revision number */
>  
>  

Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Philippe Mathieu-Daudé]

Le mer. 19 déc. 2018 10:16, Igor Mammedov <imammedo@redhat.com> a écrit :

> On Tue, 18 Dec 2018 18:51:20 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> > GCC 8 added a -Wstringop-truncation warning:
> >
> >   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
> >   bug 81117 is specifically intended to highlight likely unintended
> >   uses of the strncpy function that truncate the terminating NUL
> >   character from the source string.
> >
> > This new warning leads to compilation failures:
> >
> >     CC      hw/acpi/core.o
> >   In function 'acpi_table_install', inlined from 'acpi_table_add' at
> qemu/hw/acpi/core.c:296:5:
> >   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals
> destination size [-Werror=stringop-truncation]
> >            strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> >            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> >
> > Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
> > strings to be NUL-terminated.
> >
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  hw/acpi/core.c              | 8 ++++----
> >  include/hw/acpi/acpi-defs.h | 8 ++++----
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > index aafdc61648..f60f750c3d 100644
> > --- a/hw/acpi/core.c
> > +++ b/hw/acpi/core.c
> > @@ -35,14 +35,14 @@
> >  struct acpi_table_header {
> >      uint16_t _length;         /* our length, not actual part of the hdr
> */
> >                                /* allows easier parsing for fw_cfg
> clients */
> > -    char sig[4];              /* ACPI signature (4 ASCII characters) */
> > +    char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters)
> */
> >      uint32_t length;          /* Length of table, in bytes, including
> header */
> >      uint8_t revision;         /* ACPI Specification minor version # */
> >      uint8_t checksum;         /* To make sum of entire table == 0 */
> > -    char oem_id[6];           /* OEM identification */
> > -    char oem_table_id[8];     /* OEM table identification */
> > +    char oem_id[6] QEMU_NONSTRING; /* OEM identification */
> > +    char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */
> >      uint32_t oem_revision;    /* OEM revision number */
> > -    char asl_compiler_id[4];  /* ASL compiler vendor ID */
> > +    char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */
> >      uint32_t asl_compiler_revision; /* ASL compiler revision number */
> >  } QEMU_PACKED;
> >
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index af8e023968..3bf0bec8ba 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -43,7 +43,7 @@ enum {
> >  struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
> >      uint64_t signature;              /* ACPI signature, contains "RSD
> PTR " */
> >      uint8_t  checksum;               /* To make sum of struct == 0 */
> > -    uint8_t  oem_id [6];             /* OEM identification */
> > +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */
> >      uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
> >      uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT
> */
> >      uint32_t length;                 /* XSDT Length in bytes including
> hdr */
>
> you'll need to rebase this on top the latest Michael's pull request.
> [PULL v2 25/30] hw: arm: Carry RSDP specific data through  AcpiRsdpData
> [PULL v2 29/30] hw: acpi: Remove AcpiRsdpDescriptor and fix tests
>

OK. Can I add your Ack-by then?

> @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
> >      uint32_t length;                 /* Length of table, in bytes,
> including header */ \
> >      uint8_t  revision;               /* ACPI Specification minor
> version # */ \
> >      uint8_t  checksum;               /* To make sum of entire table ==
> 0 */ \
> > -    uint8_t  oem_id [6];             /* OEM identification */ \
> > -    uint8_t  oem_table_id [8];       /* OEM table identification */ \
> > +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */ \
> > +    uint8_t  oem_table_id [8] QEMU_NONSTRING; /* OEM table
> identification */ \
> >      uint32_t oem_revision;           /* OEM revision number */ \
> > -    uint8_t  asl_compiler_id [4];    /* ASL compiler vendor ID */ \
> > +    uint8_t  asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor
> ID */ \
> >      uint32_t asl_compiler_revision;  /* ASL compiler revision number */
> >
> >
>
>

Re: [Qemu-devel] [PATCH v3 2/5] block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays

[Philippe Mathieu-Daudé]

Le mer. 19 déc. 2018 00:09, Michael S. Tsirkin <mst@redhat.com> a écrit :

> On Tue, Dec 18, 2018 at 06:51:19PM +0100, Philippe Mathieu-Daudé wrote:
> > GCC 8 added a -Wstringop-truncation warning:
> >
> >   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
> >   bug 81117 is specifically intended to highlight likely unintended
> >   uses of the strncpy function that truncate the terminating NUL
> >   character from the source string.
> >
> > This new warning leads to compilation failures:
> >
> >     CC      block/sheepdog.o
> >   qemu/block/sheepdog.c: In function 'find_vdi_name':
> >   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);
> >        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   make: *** [qemu/rules.mak:69: block/sheepdog.o] Error 1
> >
> > As described previous to the strncpy() calls, the use of strncpy() is
> > correct here:
> >
> >     /* 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);
> >
> > Use the QEMU_NONSTRING attribute, since this array is intended to store
> > character arrays that do not necessarily contain a terminating NUL.
> >
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  block/sheepdog.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index 0125df9d49..d4ad6b119d 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -1224,7 +1224,7 @@ 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];
> > +    QEMU_NONSTRING char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
>
> In case you decide to respin anyway - this would be
> a bit nicer as:
>         char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN] QEMU_NONSTRING
>

I'll have to, so OK.
(it looked to me more explicit prepended).


>
>
>
> >      fd = connect_to_sdog(s, errp);
> >      if (fd < 0) {
> > --
> > 2.17.2
>

Re: [Qemu-devel] [PATCH v3 5/5] migration: Use strnlen() for fixed-size string

[Philippe Mathieu-Daudé]

Le mer. 19 déc. 2018 00:16, Michael S. Tsirkin <mst@redhat.com> a écrit :

> On Tue, Dec 18, 2018 at 06:51:22PM +0100, Philippe Mathieu-Daudé wrote:
> > GCC 8 introduced the -Wstringop-overflow, which detect buffer overflow
> > by string-modifying functions declared in <string.h>, such strncpy(),
> > used in global_state_store_running().
> >
> > Since the global_state.runstate does not necessarily contains a
> > terminating NUL character, We had to use the QEMU_NONSTRING attribute.
> >
> > The GCC manual says about the nonstring attribute:
> >
> >   However, when the array is declared with the attribute the call to
> >   strlen is diagnosed because when the array doesn’t contain a
> >   NUL-terminated string the call is undefined. [...]
> >   In addition, calling strnlen and strndup with such arrays is safe
> >   provided a suitable bound is specified, and not diagnosed.
> >
> > GCC indeed found an incorrect use of strlen(), because this array
> > is loaded by VMSTATE_BUFFER(runstate, GlobalState) then parsed
> > using qapi_enum_parse which does not get the buffer length.
> >
> > Use strnlen() which returns sizeof(s->runstate) if the array is not
> > NUL-terminated.
> >
> > This fixes:
> >
> >     CC      migration/global_state.o
> >   qemu/migration/global_state.c: In function 'global_state_pre_save':
> >   qemu/migration/global_state.c:109:15: error: 'strlen' argument 1
> declared attribute 'nonstring' [-Werror=stringop-overflow=]
> >        s->size = strlen((char *)s->runstate) + 1;
> >                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   qemu/migration/global_state.c:24:13: note: argument 'runstate'
> declared here
> >        uint8_t runstate[100] QEMU_NONSTRING;
> >                ^~~~~~~~
> >   cc1: all warnings being treated as errors
> >   make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  migration/global_state.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/migration/global_state.c b/migration/global_state.c
> > index 6e19333422..c19030ef62 100644
> > --- a/migration/global_state.c
> > +++ b/migration/global_state.c
> > @@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
> >      GlobalState *s = opaque;
> >
> >      trace_migrate_global_state_pre_save((char *)s->runstate);
> > -    s->size = strlen((char *)s->runstate) + 1;
> > +    s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;
> >
> >      return 0;
>
> I don't think this works correctly if strnlen returns
> sizeof(s->runstate). Which never happens so we probably should
> jus add
>
>         assert(e->size is <= sizeof(s->runstate));
>

OK I'll resend Marc-André previous patch.


> But also I think this is not enough, there's a problem in post-load
> in the call to qapi_enum_parse. You probably want to force
> the last character to 0 there.
>

OK, I'll have a look there.


>
> >  }
> > --
> > 2.17.2
>

Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Igor Mammedov]

On Wed, 19 Dec 2018 10:20:36 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Le mer. 19 déc. 2018 10:16, Igor Mammedov <imammedo@redhat.com> a écrit :
> 
> > On Tue, 18 Dec 2018 18:51:20 +0100
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >  
> > > GCC 8 added a -Wstringop-truncation warning:
> > >
> > >   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
> > >   bug 81117 is specifically intended to highlight likely unintended
> > >   uses of the strncpy function that truncate the terminating NUL
> > >   character from the source string.
> > >
> > > This new warning leads to compilation failures:
> > >
> > >     CC      hw/acpi/core.o
> > >   In function 'acpi_table_install', inlined from 'acpi_table_add' at  
> > qemu/hw/acpi/core.c:296:5:  
> > >   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals  
> > destination size [-Werror=stringop-truncation]  
> > >            strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> > >            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> > >
> > > Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
> > > strings to be NUL-terminated.
> > >
> > > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > ---
> > >  hw/acpi/core.c              | 8 ++++----
> > >  include/hw/acpi/acpi-defs.h | 8 ++++----
> > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > > index aafdc61648..f60f750c3d 100644
> > > --- a/hw/acpi/core.c
> > > +++ b/hw/acpi/core.c
> > > @@ -35,14 +35,14 @@
> > >  struct acpi_table_header {
> > >      uint16_t _length;         /* our length, not actual part of the hdr  
> > */  
> > >                                /* allows easier parsing for fw_cfg  
> > clients */  
> > > -    char sig[4];              /* ACPI signature (4 ASCII characters) */
> > > +    char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters)  
> > */  
> > >      uint32_t length;          /* Length of table, in bytes, including  
> > header */  
> > >      uint8_t revision;         /* ACPI Specification minor version # */
> > >      uint8_t checksum;         /* To make sum of entire table == 0 */
> > > -    char oem_id[6];           /* OEM identification */
> > > -    char oem_table_id[8];     /* OEM table identification */
> > > +    char oem_id[6] QEMU_NONSTRING; /* OEM identification */
> > > +    char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */
> > >      uint32_t oem_revision;    /* OEM revision number */
> > > -    char asl_compiler_id[4];  /* ASL compiler vendor ID */
> > > +    char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */
> > >      uint32_t asl_compiler_revision; /* ASL compiler revision number */
> > >  } QEMU_PACKED;
> > >
> > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > index af8e023968..3bf0bec8ba 100644
> > > --- a/include/hw/acpi/acpi-defs.h
> > > +++ b/include/hw/acpi/acpi-defs.h
> > > @@ -43,7 +43,7 @@ enum {
> > >  struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
> > >      uint64_t signature;              /* ACPI signature, contains "RSD  
> > PTR " */  
> > >      uint8_t  checksum;               /* To make sum of struct == 0 */
> > > -    uint8_t  oem_id [6];             /* OEM identification */
> > > +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */
> > >      uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
> > >      uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT  
> > */  
> > >      uint32_t length;                 /* XSDT Length in bytes including  
> > hdr */
> >
> > you'll need to rebase this on top the latest Michael's pull request.
> > [PULL v2 25/30] hw: arm: Carry RSDP specific data through  AcpiRsdpData
> > [PULL v2 29/30] hw: acpi: Remove AcpiRsdpDescriptor and fix tests
> >  
> 
> OK. Can I add your Ack-by then?
pls note that new AcpiRsdpData has oem_id field that needs the same treatment

with rebase
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> 
> > @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;  
> > >      uint32_t length;                 /* Length of table, in bytes,  
> > including header */ \  
> > >      uint8_t  revision;               /* ACPI Specification minor  
> > version # */ \  
> > >      uint8_t  checksum;               /* To make sum of entire table ==  
> > 0 */ \  
> > > -    uint8_t  oem_id [6];             /* OEM identification */ \
> > > -    uint8_t  oem_table_id [8];       /* OEM table identification */ \
> > > +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */ \
> > > +    uint8_t  oem_table_id [8] QEMU_NONSTRING; /* OEM table  
> > identification */ \  
> > >      uint32_t oem_revision;           /* OEM revision number */ \
> > > -    uint8_t  asl_compiler_id [4];    /* ASL compiler vendor ID */ \
> > > +    uint8_t  asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor  
> > ID */ \  
> > >      uint32_t asl_compiler_revision;  /* ASL compiler revision number */
> > >
> > >  
> >
> >  

Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Andrew Jones]

On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote:
> GCC 8 added a -Wstringop-truncation warning:
> 
>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>   bug 81117 is specifically intended to highlight likely unintended
>   uses of the strncpy function that truncate the terminating NUL
>   character from the source string.
> 
> This new warning leads to compilation failures:
> 
>     CC      hw/acpi/core.o
>   In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5:
>   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
>            strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> 
> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
> strings to be NUL-terminated.

Aren't we always starting with zero-initialized structures in ACPI code?
If so, then we should be able to change the strncpy's to memcpy's.

Thanks,
drew

> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/acpi/core.c              | 8 ++++----
>  include/hw/acpi/acpi-defs.h | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index aafdc61648..f60f750c3d 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -35,14 +35,14 @@
>  struct acpi_table_header {
>      uint16_t _length;         /* our length, not actual part of the hdr */
>                                /* allows easier parsing for fw_cfg clients */
> -    char sig[4];              /* ACPI signature (4 ASCII characters) */
> +    char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) */
>      uint32_t length;          /* Length of table, in bytes, including header */
>      uint8_t revision;         /* ACPI Specification minor version # */
>      uint8_t checksum;         /* To make sum of entire table == 0 */
> -    char oem_id[6];           /* OEM identification */
> -    char oem_table_id[8];     /* OEM table identification */
> +    char oem_id[6] QEMU_NONSTRING; /* OEM identification */
> +    char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */
>      uint32_t oem_revision;    /* OEM revision number */
> -    char asl_compiler_id[4];  /* ASL compiler vendor ID */
> +    char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */
>      uint32_t asl_compiler_revision; /* ASL compiler revision number */
>  } QEMU_PACKED;
>  
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index af8e023968..3bf0bec8ba 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -43,7 +43,7 @@ enum {
>  struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
>      uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
>      uint8_t  checksum;               /* To make sum of struct == 0 */
> -    uint8_t  oem_id [6];             /* OEM identification */
> +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */
>      uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
>      uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
>      uint32_t length;                 /* XSDT Length in bytes including hdr */
> @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
>      uint32_t length;                 /* Length of table, in bytes, including header */ \
>      uint8_t  revision;               /* ACPI Specification minor version # */ \
>      uint8_t  checksum;               /* To make sum of entire table == 0 */ \
> -    uint8_t  oem_id [6];             /* OEM identification */ \
> -    uint8_t  oem_table_id [8];       /* OEM table identification */ \
> +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */ \
> +    uint8_t  oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ \
>      uint32_t oem_revision;           /* OEM revision number */ \
> -    uint8_t  asl_compiler_id [4];    /* ASL compiler vendor ID */ \
> +    uint8_t  asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID */ \
>      uint32_t asl_compiler_revision;  /* ASL compiler revision number */
>  
>  
> -- 
> 2.17.2
> 
> 

Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Philippe Mathieu-Daudé]

Hi Drew,

On 12/19/18 11:10 AM, Andrew Jones wrote:
> On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote:
>> GCC 8 added a -Wstringop-truncation warning:
>>
>>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>>   bug 81117 is specifically intended to highlight likely unintended
>>   uses of the strncpy function that truncate the terminating NUL
>>   character from the source string.
>>
>> This new warning leads to compilation failures:
>>
>>     CC      hw/acpi/core.o
>>   In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5:
>>   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
>>            strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
>>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
>>
>> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
>> strings to be NUL-terminated.
> 
> Aren't we always starting with zero-initialized structures in ACPI code?
> If so, then we should be able to change the strncpy's to memcpy's.

The first call zero-initializes, but then we call realloc():

    /* We won't fail from here on. Initialize / extend the globals. */
    if (acpi_tables == NULL) {
        acpi_tables_len = sizeof(uint16_t);
        acpi_tables = g_malloc0(acpi_tables_len);
    }

    acpi_tables = g_realloc(acpi_tables, acpi_tables_len +
                                         ACPI_TABLE_PFX_SIZE +
                                         sizeof dfl_hdr + body_size);

    ext_hdr = (struct acpi_table_header *)(acpi_tables +
                                           acpi_tables_len);

So memcpy() isn't enough.

I can resend the previous patch which uses strpadcpy() if you prefer,
Igor already reviewed it:

https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04406.html

>>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/acpi/core.c              | 8 ++++----
>>  include/hw/acpi/acpi-defs.h | 8 ++++----
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
>> index aafdc61648..f60f750c3d 100644
>> --- a/hw/acpi/core.c
>> +++ b/hw/acpi/core.c
>> @@ -35,14 +35,14 @@
>>  struct acpi_table_header {
>>      uint16_t _length;         /* our length, not actual part of the hdr */
>>                                /* allows easier parsing for fw_cfg clients */
>> -    char sig[4];              /* ACPI signature (4 ASCII characters) */
>> +    char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) */
>>      uint32_t length;          /* Length of table, in bytes, including header */
>>      uint8_t revision;         /* ACPI Specification minor version # */
>>      uint8_t checksum;         /* To make sum of entire table == 0 */
>> -    char oem_id[6];           /* OEM identification */
>> -    char oem_table_id[8];     /* OEM table identification */
>> +    char oem_id[6] QEMU_NONSTRING; /* OEM identification */
>> +    char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */
>>      uint32_t oem_revision;    /* OEM revision number */
>> -    char asl_compiler_id[4];  /* ASL compiler vendor ID */
>> +    char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */
>>      uint32_t asl_compiler_revision; /* ASL compiler revision number */
>>  } QEMU_PACKED;
>>  
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index af8e023968..3bf0bec8ba 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -43,7 +43,7 @@ enum {
>>  struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
>>      uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
>>      uint8_t  checksum;               /* To make sum of struct == 0 */
>> -    uint8_t  oem_id [6];             /* OEM identification */
>> +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */
>>      uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
>>      uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
>>      uint32_t length;                 /* XSDT Length in bytes including hdr */
>> @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
>>      uint32_t length;                 /* Length of table, in bytes, including header */ \
>>      uint8_t  revision;               /* ACPI Specification minor version # */ \
>>      uint8_t  checksum;               /* To make sum of entire table == 0 */ \
>> -    uint8_t  oem_id [6];             /* OEM identification */ \
>> -    uint8_t  oem_table_id [8];       /* OEM table identification */ \
>> +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */ \
>> +    uint8_t  oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ \
>>      uint32_t oem_revision;           /* OEM revision number */ \
>> -    uint8_t  asl_compiler_id [4];    /* ASL compiler vendor ID */ \
>> +    uint8_t  asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID */ \
>>      uint32_t asl_compiler_revision;  /* ASL compiler revision number */
>>  
>>  
>> -- 
>> 2.17.2
>>
>>

Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Andrew Jones]

On Wed, Dec 19, 2018 at 01:43:40PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Drew,
> 
> On 12/19/18 11:10 AM, Andrew Jones wrote:
> > On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote:
> >> GCC 8 added a -Wstringop-truncation warning:
> >>
> >>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
> >>   bug 81117 is specifically intended to highlight likely unintended
> >>   uses of the strncpy function that truncate the terminating NUL
> >>   character from the source string.
> >>
> >> This new warning leads to compilation failures:
> >>
> >>     CC      hw/acpi/core.o
> >>   In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5:
> >>   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
> >>            strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> >>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> >>
> >> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
> >> strings to be NUL-terminated.
> > 
> > Aren't we always starting with zero-initialized structures in ACPI code?
> > If so, then we should be able to change the strncpy's to memcpy's.
> 
> The first call zero-initializes, but then we call realloc():
> 
>     /* We won't fail from here on. Initialize / extend the globals. */
>     if (acpi_tables == NULL) {
>         acpi_tables_len = sizeof(uint16_t);
>         acpi_tables = g_malloc0(acpi_tables_len);
>     }
> 
>     acpi_tables = g_realloc(acpi_tables, acpi_tables_len +
>                                          ACPI_TABLE_PFX_SIZE +
>                                          sizeof dfl_hdr + body_size);
> 
>     ext_hdr = (struct acpi_table_header *)(acpi_tables +
>                                            acpi_tables_len);
> 
> So memcpy() isn't enough.

Ah, thanks.

> 
> I can resend the previous patch which uses strpadcpy() if you prefer,
> Igor already reviewed it:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04406.html
>

I do like strpadcpy() better, but I'm not going to lose sleep about
this either way it goes.

Thanks,
drew 

Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Igor Mammedov]

On Wed, 19 Dec 2018 14:00:37 +0100
Andrew Jones <drjones@redhat.com> wrote:

> On Wed, Dec 19, 2018 at 01:43:40PM +0100, Philippe Mathieu-Daudé wrote:
> > Hi Drew,
> > 
> > On 12/19/18 11:10 AM, Andrew Jones wrote:
> > > On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote:
> > >> GCC 8 added a -Wstringop-truncation warning:
> > >>
> > >>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
> > >>   bug 81117 is specifically intended to highlight likely unintended
> > >>   uses of the strncpy function that truncate the terminating NUL
> > >>   character from the source string.
> > >>
> > >> This new warning leads to compilation failures:
> > >>
> > >>     CC      hw/acpi/core.o
> > >>   In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5:
> > >>   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
> > >>            strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> > >>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> > >>
> > >> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
> > >> strings to be NUL-terminated.
> > > 
> > > Aren't we always starting with zero-initialized structures in ACPI code?
> > > If so, then we should be able to change the strncpy's to memcpy's.
> > 
> > The first call zero-initializes, but then we call realloc():
> > 
> >     /* We won't fail from here on. Initialize / extend the globals. */
> >     if (acpi_tables == NULL) {
> >         acpi_tables_len = sizeof(uint16_t);
> >         acpi_tables = g_malloc0(acpi_tables_len);
> >     }
> > 
> >     acpi_tables = g_realloc(acpi_tables, acpi_tables_len +
> >                                          ACPI_TABLE_PFX_SIZE +
> >                                          sizeof dfl_hdr + body_size);
> > 
> >     ext_hdr = (struct acpi_table_header *)(acpi_tables +
> >                                            acpi_tables_len);
> > 
> > So memcpy() isn't enough.
> 
> Ah, thanks.
> 
> > 
> > I can resend the previous patch which uses strpadcpy() if you prefer,
> > Igor already reviewed it:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04406.html
> >
> 
> I do like strpadcpy() better, but I'm not going to lose sleep about
> this either way it goes.
I'm ok with both ways, but v2 consensus was to use QEMU_NONSTRING if I got it right

> 
> Thanks,
> drew 

Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Philippe Mathieu-Daudé]

On Thu, Dec 20, 2018 at 4:18 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 19 Dec 2018 14:00:37 +0100
> Andrew Jones <drjones@redhat.com> wrote:
>
> > On Wed, Dec 19, 2018 at 01:43:40PM +0100, Philippe Mathieu-Daudé wrote:
> > > Hi Drew,
> > >
> > > On 12/19/18 11:10 AM, Andrew Jones wrote:
> > > > On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote:
> > > >> GCC 8 added a -Wstringop-truncation warning:
> > > >>
> > > >>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
> > > >>   bug 81117 is specifically intended to highlight likely unintended
> > > >>   uses of the strncpy function that truncate the terminating NUL
> > > >>   character from the source string.
> > > >>
> > > >> This new warning leads to compilation failures:
> > > >>
> > > >>     CC      hw/acpi/core.o
> > > >>   In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5:
> > > >>   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
> > > >>            strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> > > >>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >>   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> > > >>
> > > >> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
> > > >> strings to be NUL-terminated.
> > > >
> > > > Aren't we always starting with zero-initialized structures in ACPI code?
> > > > If so, then we should be able to change the strncpy's to memcpy's.
> > >
> > > The first call zero-initializes, but then we call realloc():
> > >
> > >     /* We won't fail from here on. Initialize / extend the globals. */
> > >     if (acpi_tables == NULL) {
> > >         acpi_tables_len = sizeof(uint16_t);
> > >         acpi_tables = g_malloc0(acpi_tables_len);
> > >     }
> > >
> > >     acpi_tables = g_realloc(acpi_tables, acpi_tables_len +
> > >                                          ACPI_TABLE_PFX_SIZE +
> > >                                          sizeof dfl_hdr + body_size);
> > >
> > >     ext_hdr = (struct acpi_table_header *)(acpi_tables +
> > >                                            acpi_tables_len);
> > >
> > > So memcpy() isn't enough.
> >
> > Ah, thanks.
> >
> > >
> > > I can resend the previous patch which uses strpadcpy() if you prefer,
> > > Igor already reviewed it:
> > >
> > > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04406.html
> > >
> >
> > I do like strpadcpy() better, but I'm not going to lose sleep about
> > this either way it goes.
> I'm ok with both ways, but v2 consensus was to use QEMU_NONSTRING if I got it right

Yes, MST recommended it because this attribute is clever than strpadcpy().

Re: [Qemu-devel] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation

[no-reply]

Patchew URL: https://patchew.org/QEMU/20181218175122.3229-1-philmd@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20181218175122.3229-1-philmd@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
df72d7a migration: Use strnlen() for fixed-size string
18211fe migration: Use QEMU_NONSTRING for non NUL-terminated arrays
f5a08ba hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays
7d31f0c block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays
5ac217b qemu/compiler: Define QEMU_NONSTRING

=== OUTPUT BEGIN ===
Checking PATCH 1/5: qemu/compiler: Define QEMU_NONSTRING...
WARNING: architecture specific defines should be avoided
#50: FILE: include/qemu/compiler.h:163:
+#if __has_attribute(nonstring)

total: 0 errors, 1 warnings, 21 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/5: block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays...
Checking PATCH 3/5: hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays...
ERROR: space prohibited before open square bracket '['
#64: FILE: include/hw/acpi/acpi-defs.h:46:
+    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */

WARNING: Block comments use a leading /* on a separate line
#74: FILE: include/hw/acpi/acpi-defs.h:65:
+    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */ \

ERROR: space prohibited before open square bracket '['
#74: FILE: include/hw/acpi/acpi-defs.h:65:
+    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */ \

WARNING: Block comments use a leading /* on a separate line
#75: FILE: include/hw/acpi/acpi-defs.h:66:
+    uint8_t  oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ \

ERROR: space prohibited before open square bracket '['
#75: FILE: include/hw/acpi/acpi-defs.h:66:
+    uint8_t  oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ \

WARNING: Block comments use a leading /* on a separate line
#78: FILE: include/hw/acpi/acpi-defs.h:68:
+    uint8_t  asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID */ \

ERROR: space prohibited before open square bracket '['
#78: FILE: include/hw/acpi/acpi-defs.h:68:
+    uint8_t  asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID */ \

total: 4 errors, 3 warnings, 39 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/5: migration: Use QEMU_NONSTRING for non NUL-terminated arrays...
Checking PATCH 5/5: migration: Use strnlen() for fixed-size string...
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20181218175122.3229-1-philmd@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v3 4/5] migration: Use QEMU_NONSTRING for non NUL-terminated arrays

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> GCC 8 added a -Wstringop-truncation warning:
> 
>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>   bug 81117 is specifically intended to highlight likely unintended
>   uses of the strncpy function that truncate the terminating NUL
>   character from the source string.
> 
> This new warning leads to compilation failures:
> 
>     CC      migration/global_state.o
>   qemu/migration/global_state.c: In function 'global_state_store_running':
>   qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
>        strncpy((char *)global_state.runstate, state, sizeof(global_state.runstate));
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
> 
> Use the QEMU_NONSTRING attribute, since this array is intended to store
> character arrays that do not necessarily contain a terminating NUL.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  migration/global_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 8e8ab5c51e..6e19333422 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -21,7 +21,7 @@
>  
>  typedef struct {
>      uint32_t size;
> -    uint8_t runstate[100];
> +    uint8_t runstate[100] QEMU_NONSTRING;

Hmm; global_state_post_load needs to be fixed for this;  it
uses s->runsate and ends up passing it to both a trace
and a qapi_enum_parse - so it's really treating it as a string.
That code is unsafe anyway since it's assuming the received
runstate would be terminated.

Dave

>      RunState state;
>      bool received;
>  } GlobalState;
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [Bug 1803872] Re: gcc 8.2 reports stringop-truncation when building qemu

[Thomas Huth]

** Changed in: qemu
       Status: Fix Committed => Fix Released

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1803872

Title:
  gcc 8.2 reports stringop-truncation when building qemu

Status in QEMU:
  Fix Released

Bug description:
  QEMU 3.0

  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);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  If this is the intended behavior, please suppress the warning. For
  example:

  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wstringop-truncation"
      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  #pragma GCC diagnostic pop

  This also happens on other sources, for example hw/acpi/core.c, so
  another option is to suppress it globally on CFLAGS (-Wno-stringop-
  truncation)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1803872/+subscriptions