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

[Qemu-devel] [PATCH v4 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

Since v3:
- patch 1: make sens of description (eblake)
- patch 2: append QEMU_NONSTRING instead of prepending it (mst)
- patch 3: rebased (imammedo), intented
- patch 4: replaced by Marc-André first attempt, improved doc
- patch 5: add assert() and NUL-terminate the buffer (mst)

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
  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.

Marc-André Lureau (1):
  migration: Fix stringop-truncation warning

Philippe Mathieu-Daudé (4):
  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 strnlen() for fixed-size string

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

-- 
2.17.2

[Qemu-devel] [PATCH v4 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>).

In tandem of -Wstringop-truncation, the "nonstring" attribute was added:

  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.

  From the GCC manual: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute

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

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v4: reordered the commit description to make sens (eblake)

Note this trigger the following checkpatch warning (patchew):

  WARNING: architecture specific defines should be avoided
  #50: FILE: include/qemu/compiler.h:163:
  +#if __has_attribute(nonstring)
---
 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

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

[Richard Henderson]

On 12/29/18 4:33 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>).
> 
> In tandem of -Wstringop-truncation, the "nonstring" attribute was added:
> 
>   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.
> 
>   From the GCC manual: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute
> 
> Add the QEMU_NONSTRING macro which checks if the compiler supports this
> attribute.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

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

[Thomas Huth]

On 2018-12-28 18:33, 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>).
> 
> In tandem of -Wstringop-truncation, the "nonstring" attribute was added:
> 
>   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.
> 
>   From the GCC manual: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute
> 
> Add the QEMU_NONSTRING macro which checks if the compiler supports this
> attribute.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH v4 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-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..5cd9618432 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];
+    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

[Qemu-devel] [PATCH v4 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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v4: rebased

Note this triggers the following checkpatch error (patchew):

  ERROR: space prohibited before open square bracket '['
  #64: FILE: include/hw/acpi/acpi-defs.h:43:
  +    uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */
---
 hw/acpi/core.c              | 12 ++++++++----
 include/hw/acpi/acpi-defs.h | 13 ++++++++-----
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index d6f0709691..47877c0ec1 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -35,14 +35,18 @@
 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 5021cb9e79..17f72e9553 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -41,8 +41,8 @@ enum {
 };
 
 typedef struct AcpiRsdpData {
-    uint8_t oem_id[6]; /* OEM identification */
-    uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
+    uint8_t oem_id[6] QEMU_NONSTRING; /* OEM identification */
+    uint8_t revision;                 /* Must be 0 for 1.0, 2 for 2.0 */
 
     unsigned *rsdt_tbl_offset;
     unsigned *xsdt_tbl_offset;
@@ -57,10 +57,13 @@ typedef struct AcpiRsdpData {
     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 v4 4/5] migration: Fix stringop-truncation warning

[Philippe Mathieu-Daudé]

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

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

Adding an assert is enough to silence GCC.

(alternatively, we could hard-code "running")

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[PMD: More verbose commit message]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 migration/global_state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/global_state.c b/migration/global_state.c
index 8e8ab5c51e..01805c567a 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -42,6 +42,7 @@ int global_state_store(void)
 void global_state_store_running(void)
 {
     const char *state = RunState_str(RUN_STATE_RUNNING);
+    assert(strlen(state) < sizeof(global_state.runstate));
     strncpy((char *)global_state.runstate,
            state, sizeof(global_state.runstate));
 }
-- 
2.17.2

[Qemu-devel] [PATCH v4 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().

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, assert the size is within range, and enforce the array
to be NUL-terminated to avoid an overflow in qapi_enum_parse().

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

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

diff --git a/migration/global_state.c b/migration/global_state.c
index 01805c567a..4f060a6dbd 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -89,6 +89,16 @@ static int global_state_post_load(void *opaque, int version_id)
     s->received = true;
     trace_migrate_global_state_post_load(runstate);
 
+    if (strnlen((char *)s->runstate,
+                sizeof(s->runstate)) == sizeof(s->runstate)) {
+        /* This condition should never happen during migration, because
+         * all runstate names are shorter than 100 bytes (the size of
+         * s->runstate). However, a malicious stream could overflow
+         * the qapi_enum_parse() call, so we force the last character
+         * to a NUL byte.
+         */
+        s->runstate[sizeof(s->runstate) - 1] = '\0';
+    }
     r = qapi_enum_parse(&RunState_lookup, runstate, -1, &local_err);
 
     if (r == -1) {
@@ -107,7 +117,8 @@ 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;
+    assert(s->size <= sizeof(s->runstate));
 
     return 0;
 }
-- 
2.17.2

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

[Richard Henderson]

On 12/29/18 4:33 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().
> 
> 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, assert the size is within range, and enforce the array
> to be NUL-terminated to avoid an overflow in qapi_enum_parse().
> 
> 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
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  migration/global_state.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

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

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) 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().
> 
> 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, assert the size is within range, and enforce the array
> to be NUL-terminated to avoid an overflow in qapi_enum_parse().
> 
> 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
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/global_state.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 01805c567a..4f060a6dbd 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -89,6 +89,16 @@ static int global_state_post_load(void *opaque, int version_id)
>      s->received = true;
>      trace_migrate_global_state_post_load(runstate);
>  
> +    if (strnlen((char *)s->runstate,
> +                sizeof(s->runstate)) == sizeof(s->runstate)) {
> +        /* This condition should never happen during migration, because
> +         * all runstate names are shorter than 100 bytes (the size of
> +         * s->runstate). However, a malicious stream could overflow
> +         * the qapi_enum_parse() call, so we force the last character
> +         * to a NUL byte.
> +         */
> +        s->runstate[sizeof(s->runstate) - 1] = '\0';
> +    }
>      r = qapi_enum_parse(&RunState_lookup, runstate, -1, &local_err);
>  
>      if (r == -1) {
> @@ -107,7 +117,8 @@ 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;
> +    assert(s->size <= sizeof(s->runstate));
>  
>      return 0;
>  }
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[no-reply]

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



Hi,

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

Type: series
Message-id: 20181228173356.15359-1-philmd@redhat.com
Subject: [Qemu-devel] [PATCH v4 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'
7afec15 migration: Use strnlen() for fixed-size string
3f0d878 migration: Fix stringop-truncation warning
f6b1b52 hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays
16e5ce8 block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays
f4ac58a qemu/compiler: Define QEMU_NONSTRING

=== OUTPUT BEGIN ===
Checking PATCH 1/5: qemu/compiler: Define QEMU_NONSTRING...
WARNING: architecture specific defines should be avoided
#53: 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 '['
#82: FILE: include/hw/acpi/acpi-defs.h:60:
+    uint8_t  oem_id [6] \

WARNING: Block comments use a leading /* on a separate line
#83: FILE: include/hw/acpi/acpi-defs.h:61:
+                 QEMU_NONSTRING;     /* OEM identification */ \

ERROR: space prohibited before open square bracket '['
#84: FILE: include/hw/acpi/acpi-defs.h:62:
+    uint8_t  oem_table_id [8] \

WARNING: Block comments use a leading /* on a separate line
#85: FILE: include/hw/acpi/acpi-defs.h:63:
+                 QEMU_NONSTRING;     /* OEM table identification */ \

ERROR: space prohibited before open square bracket '['
#88: FILE: include/hw/acpi/acpi-defs.h:65:
+    uint8_t  asl_compiler_id [4] \

WARNING: Block comments use a leading /* on a separate line
#89: FILE: include/hw/acpi/acpi-defs.h:66:
+                 QEMU_NONSTRING;     /* ASL compiler vendor ID */ \

total: 3 errors, 3 warnings, 48 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: Fix stringop-truncation warning...
Checking PATCH 5/5: migration: Use strnlen() for fixed-size string...
WARNING: Block comments use a leading /* on a separate line
#49: FILE: migration/global_state.c:94:
+        /* This condition should never happen during migration, because

total: 0 errors, 1 warnings, 25 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.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20181228173356.15359-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 v4 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation

[Philippe Mathieu-Daudé]

On 1/2/19 4:21 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20181228173356.15359-1-philmd@redhat.com/
[...]> === OUTPUT BEGIN ===
> Checking PATCH 1/5: qemu/compiler: Define QEMU_NONSTRING...
> WARNING: architecture specific defines should be avoided
> #53: FILE: include/qemu/compiler.h:163:
> +#if __has_attribute(nonstring)

I assume this one is OK, this is consistent with the file.

> Checking PATCH 3/5: hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays...
> ERROR: space prohibited before open square bracket '['
> #82: FILE: include/hw/acpi/acpi-defs.h:60:
> +    uint8_t  oem_id [6] \

I wanted to respect the maintainer previous indentation, but I'll fix.

> 
> WARNING: Block comments use a leading /* on a separate line
> #83: FILE: include/hw/acpi/acpi-defs.h:61:
> +                 QEMU_NONSTRING;     /* OEM identification */ \

Eh this is a multi-line macro... I don't think we can split this
comment. I will keep it that way.

> Checking PATCH 5/5: migration: Use strnlen() for fixed-size string...
> WARNING: Block comments use a leading /* on a separate line
> #49: FILE: migration/global_state.c:94:
> +        /* This condition should never happen during migration, because

Well, this is not a comment describing a function but within a code
block. I find it cleaner this way, but matter of taste, so I'll fix.

Regards,

Phil.