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

[Qemu-devel] [PATCH v2 0/3] 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

This series replace the strncpy() calls by strpadcpy() which seemed
to me the saniest option.

Regards,

Phil.

Marc-André Lureau (1):
  hw/acpi: Replace strncpy() by strpadcpy(pad='\0')

Philippe Mathieu-Daudé (2):
  block/sheepdog: Replace strncpy() by strpadcpy(pad='\0')
  migration: Replace strncpy() by strpadcpy(pad='\0')

 block/sheepdog.c         |  6 +++---
 hw/acpi/aml-build.c      |  6 ++++--
 hw/acpi/core.c           | 13 +++++++------
 migration/global_state.c |  4 ++--
 4 files changed, 16 insertions(+), 13 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v2 1/3] hw/acpi: Replace strncpy() by strpadcpy(pad='\0')

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

The ACPI tables don'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: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[PMD: reword commit subject and description]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/acpi/aml-build.c |  6 ++++--
 hw/acpi/core.c      | 13 +++++++------
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 1e43cd736d..397833462a 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -24,6 +24,7 @@
 #include "hw/acpi/aml-build.h"
 #include "qemu/bswap.h"
 #include "qemu/bitops.h"
+#include "qemu/cutils.h"
 #include "sysemu/numa.h"
 
 static GArray *build_alloc_array(void)
@@ -1532,13 +1533,14 @@ build_header(BIOSLinker *linker, GArray *table_data,
     h->revision = rev;
 
     if (oem_id) {
-        strncpy((char *)h->oem_id, oem_id, sizeof h->oem_id);
+        strpadcpy((char *)h->oem_id, sizeof h->oem_id, oem_id, '\0');
     } else {
         memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
     }
 
     if (oem_table_id) {
-        strncpy((char *)h->oem_table_id, oem_table_id, sizeof(h->oem_table_id));
+        strpadcpy((char *)h->oem_table_id, sizeof(h->oem_table_id),
+                  oem_table_id, '\0');
     } else {
         memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
         memcpy(h->oem_table_id + 4, sig, 4);
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index aafdc61648..6e8f4e5713 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -31,6 +31,7 @@
 #include "qapi/qapi-visit-misc.h"
 #include "qemu/error-report.h"
 #include "qemu/option.h"
+#include "qemu/cutils.h"
 
 struct acpi_table_header {
     uint16_t _length;         /* our length, not actual part of the hdr */
@@ -181,7 +182,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
     ext_hdr->_length = cpu_to_le16(acpi_payload_size);
 
     if (hdrs->has_sig) {
-        strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
+        strpadcpy(ext_hdr->sig, sizeof ext_hdr->sig, hdrs->sig, '\0');
         ++changed_fields;
     }
 
@@ -200,12 +201,12 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
     ext_hdr->checksum = 0;
 
     if (hdrs->has_oem_id) {
-        strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
+        strpadcpy(ext_hdr->oem_id, sizeof ext_hdr->oem_id, hdrs->oem_id, '\0');
         ++changed_fields;
     }
     if (hdrs->has_oem_table_id) {
-        strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
-                sizeof ext_hdr->oem_table_id);
+        strpadcpy(ext_hdr->oem_table_id, sizeof ext_hdr->oem_table_id,
+                  hdrs->oem_table_id, '\0');
         ++changed_fields;
     }
     if (hdrs->has_oem_rev) {
@@ -213,8 +214,8 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
         ++changed_fields;
     }
     if (hdrs->has_asl_compiler_id) {
-        strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
-                sizeof ext_hdr->asl_compiler_id);
+        strpadcpy(ext_hdr->asl_compiler_id, sizeof ext_hdr->asl_compiler_id,
+                  hdrs->asl_compiler_id, '\0');
         ++changed_fields;
     }
     if (hdrs->has_asl_compiler_rev) {
-- 
2.17.2

[Qemu-devel] [PATCH v2 2/3] block/sheepdog: Replace strncpy() by strpadcpy(pad='\0')

[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);

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.

Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1803872
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/sheepdog.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0125df9d49..72e1aef6ea 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1231,12 +1231,12 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
         return fd;
     }
 
-    /* This pair of strncpy calls ensures that the buffer is zero-filled,
+    /* This pair of strpadcpy 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);
+    strpadcpy(buf, SD_MAX_VDI_LEN, filename, '\0');
+    strpadcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, tag, '\0');
 
     memset(&hdr, 0, sizeof(hdr));
     if (lock) {
-- 
2.17.2

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

[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

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

Re: [Qemu-devel] [PATCH v2 1/3] hw/acpi: Replace strncpy() by strpadcpy(pad='\0')

[Philippe Mathieu-Daudé]

On 12/18/18 12:03 PM, Philippe Mathieu-Daudé wrote:
> 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      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
> 
> The ACPI tables don'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: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> [PMD: reword commit subject and description]

I forgot to also add "Use '\0' instead of 0".

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

v1: https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03941.html

>  hw/acpi/aml-build.c |  6 ++++--
>  hw/acpi/core.c      | 13 +++++++------
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 1e43cd736d..397833462a 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -24,6 +24,7 @@
>  #include "hw/acpi/aml-build.h"
>  #include "qemu/bswap.h"
>  #include "qemu/bitops.h"
> +#include "qemu/cutils.h"
>  #include "sysemu/numa.h"
>  
>  static GArray *build_alloc_array(void)
> @@ -1532,13 +1533,14 @@ build_header(BIOSLinker *linker, GArray *table_data,
>      h->revision = rev;
>  
>      if (oem_id) {
> -        strncpy((char *)h->oem_id, oem_id, sizeof h->oem_id);
> +        strpadcpy((char *)h->oem_id, sizeof h->oem_id, oem_id, '\0');
>      } else {
>          memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
>      }
>  
>      if (oem_table_id) {
> -        strncpy((char *)h->oem_table_id, oem_table_id, sizeof(h->oem_table_id));
> +        strpadcpy((char *)h->oem_table_id, sizeof(h->oem_table_id),
> +                  oem_table_id, '\0');
>      } else {
>          memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
>          memcpy(h->oem_table_id + 4, sig, 4);
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index aafdc61648..6e8f4e5713 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -31,6 +31,7 @@
>  #include "qapi/qapi-visit-misc.h"
>  #include "qemu/error-report.h"
>  #include "qemu/option.h"
> +#include "qemu/cutils.h"
>  
>  struct acpi_table_header {
>      uint16_t _length;         /* our length, not actual part of the hdr */
> @@ -181,7 +182,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
>      ext_hdr->_length = cpu_to_le16(acpi_payload_size);
>  
>      if (hdrs->has_sig) {
> -        strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> +        strpadcpy(ext_hdr->sig, sizeof ext_hdr->sig, hdrs->sig, '\0');
>          ++changed_fields;
>      }
>  
> @@ -200,12 +201,12 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
>      ext_hdr->checksum = 0;
>  
>      if (hdrs->has_oem_id) {
> -        strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
> +        strpadcpy(ext_hdr->oem_id, sizeof ext_hdr->oem_id, hdrs->oem_id, '\0');
>          ++changed_fields;
>      }
>      if (hdrs->has_oem_table_id) {
> -        strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
> -                sizeof ext_hdr->oem_table_id);
> +        strpadcpy(ext_hdr->oem_table_id, sizeof ext_hdr->oem_table_id,
> +                  hdrs->oem_table_id, '\0');
>          ++changed_fields;
>      }
>      if (hdrs->has_oem_rev) {
> @@ -213,8 +214,8 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
>          ++changed_fields;
>      }
>      if (hdrs->has_asl_compiler_id) {
> -        strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
> -                sizeof ext_hdr->asl_compiler_id);
> +        strpadcpy(ext_hdr->asl_compiler_id, sizeof ext_hdr->asl_compiler_id,
> +                  hdrs->asl_compiler_id, '\0');
>          ++changed_fields;
>      }
>      if (hdrs->has_asl_compiler_rev) {
> 

Re: [Qemu-devel] [PATCH v2 1/3] hw/acpi: Replace strncpy() by strpadcpy(pad='\0')

[Igor Mammedov]

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

> 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      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
> 
> The ACPI tables don'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: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> [PMD: reword commit subject and description]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

> ---
>  hw/acpi/aml-build.c |  6 ++++--
>  hw/acpi/core.c      | 13 +++++++------
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 1e43cd736d..397833462a 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -24,6 +24,7 @@
>  #include "hw/acpi/aml-build.h"
>  #include "qemu/bswap.h"
>  #include "qemu/bitops.h"
> +#include "qemu/cutils.h"
>  #include "sysemu/numa.h"
>  
>  static GArray *build_alloc_array(void)
> @@ -1532,13 +1533,14 @@ build_header(BIOSLinker *linker, GArray *table_data,
>      h->revision = rev;
>  
>      if (oem_id) {
> -        strncpy((char *)h->oem_id, oem_id, sizeof h->oem_id);
> +        strpadcpy((char *)h->oem_id, sizeof h->oem_id, oem_id, '\0');
>      } else {
>          memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
>      }
>  
>      if (oem_table_id) {
> -        strncpy((char *)h->oem_table_id, oem_table_id, sizeof(h->oem_table_id));
> +        strpadcpy((char *)h->oem_table_id, sizeof(h->oem_table_id),
> +                  oem_table_id, '\0');
>      } else {
>          memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
>          memcpy(h->oem_table_id + 4, sig, 4);
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index aafdc61648..6e8f4e5713 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -31,6 +31,7 @@
>  #include "qapi/qapi-visit-misc.h"
>  #include "qemu/error-report.h"
>  #include "qemu/option.h"
> +#include "qemu/cutils.h"
>  
>  struct acpi_table_header {
>      uint16_t _length;         /* our length, not actual part of the hdr */
> @@ -181,7 +182,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
>      ext_hdr->_length = cpu_to_le16(acpi_payload_size);
>  
>      if (hdrs->has_sig) {
> -        strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> +        strpadcpy(ext_hdr->sig, sizeof ext_hdr->sig, hdrs->sig, '\0');
>          ++changed_fields;
>      }
>  
> @@ -200,12 +201,12 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
>      ext_hdr->checksum = 0;
>  
>      if (hdrs->has_oem_id) {
> -        strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
> +        strpadcpy(ext_hdr->oem_id, sizeof ext_hdr->oem_id, hdrs->oem_id, '\0');
>          ++changed_fields;
>      }
>      if (hdrs->has_oem_table_id) {
> -        strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
> -                sizeof ext_hdr->oem_table_id);
> +        strpadcpy(ext_hdr->oem_table_id, sizeof ext_hdr->oem_table_id,
> +                  hdrs->oem_table_id, '\0');
>          ++changed_fields;
>      }
>      if (hdrs->has_oem_rev) {
> @@ -213,8 +214,8 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
>          ++changed_fields;
>      }
>      if (hdrs->has_asl_compiler_id) {
> -        strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
> -                sizeof ext_hdr->asl_compiler_id);
> +        strpadcpy(ext_hdr->asl_compiler_id, sizeof ext_hdr->asl_compiler_id,
> +                  hdrs->asl_compiler_id, '\0');
>          ++changed_fields;
>      }
>      if (hdrs->has_asl_compiler_rev) {

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

[Michael S. Tsirkin]

On Tue, Dec 18, 2018 at 12:03:30PM +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
> 
> This series replace the strncpy() calls by strpadcpy() which seemed
> to me the saniest option.
> 
> Regards,
> 
> Phil.

Do you happen to know why does it build fine with
Gcc 8.2.1?

Reading the GCC manual it seems that
there is a "nostring" attribute that means
"might not be 0 terminated".
I think we should switch to that which fixes the warning
but also warns if someone tries to misuse these
as C-strings.

Seems to be a better option, does it not?


> Marc-André Lureau (1):
>   hw/acpi: Replace strncpy() by strpadcpy(pad='\0')
> 
> Philippe Mathieu-Daudé (2):
>   block/sheepdog: Replace strncpy() by strpadcpy(pad='\0')
>   migration: Replace strncpy() by strpadcpy(pad='\0')
> 
>  block/sheepdog.c         |  6 +++---
>  hw/acpi/aml-build.c      |  6 ++++--
>  hw/acpi/core.c           | 13 +++++++------
>  migration/global_state.c |  4 ++--
>  4 files changed, 16 insertions(+), 13 deletions(-)
> 
> -- 
> 2.17.2

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

[Daniel P. Berrangé]

On Tue, Dec 18, 2018 at 09:31:00AM -0500, Michael S. Tsirkin wrote:
> On Tue, Dec 18, 2018 at 12:03:30PM +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
> > 
> > This series replace the strncpy() calls by strpadcpy() which seemed
> > to me the saniest option.
> > 
> > Regards,
> > 
> > Phil.
> 
> Do you happen to know why does it build fine with
> Gcc 8.2.1?
> 
> Reading the GCC manual it seems that
> there is a "nostring" attribute that means

typo - its "nonstring"

> "might not be 0 terminated".
> I think we should switch to that which fixes the warning
> but also warns if someone tries to misuse these
> as C-strings.
> 
> Seems to be a better option, does it not?

Yes, it does look best as gcc manual explicitly suggests "nonstring"
as the way to stop this strncpy warning.


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

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

[Michael S. Tsirkin]

On Tue, Dec 18, 2018 at 09:31:00AM -0500, Michael S. Tsirkin wrote:
> On Tue, Dec 18, 2018 at 12:03:30PM +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
> > 
> > This series replace the strncpy() calls by strpadcpy() which seemed
> > to me the saniest option.
> > 
> > Regards,
> > 
> > Phil.
> 
> Do you happen to know why does it build fine with
> Gcc 8.2.1?
> 
> Reading the GCC manual it seems that
> there is a "nostring" attribute

Sorry that should be "nonstring".


> that means
> "might not be 0 terminated".
> I think we should switch to that which fixes the warning
> but also warns if someone tries to misuse these
> as C-strings.
> 
> Seems to be a better option, does it not?
> 

Also maybe we can make strpadcpy check for the nonstring
attribute too somehow?
Or did gcc just hardcode the strncpy name?

> > Marc-André Lureau (1):
> >   hw/acpi: Replace strncpy() by strpadcpy(pad='\0')
> > 
> > Philippe Mathieu-Daudé (2):
> >   block/sheepdog: Replace strncpy() by strpadcpy(pad='\0')
> >   migration: Replace strncpy() by strpadcpy(pad='\0')
> > 
> >  block/sheepdog.c         |  6 +++---
> >  hw/acpi/aml-build.c      |  6 ++++--
> >  hw/acpi/core.c           | 13 +++++++------
> >  migration/global_state.c |  4 ++--
> >  4 files changed, 16 insertions(+), 13 deletions(-)
> > 
> > -- 
> > 2.17.2

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

[Paolo Bonzini]

On 18/12/18 15:31, Michael S. Tsirkin wrote:
> Do you happen to know why does it build fine with
> Gcc 8.2.1?
> 
> Reading the GCC manual it seems that
> there is a "nostring" attribute that means
> "might not be 0 terminated".
> I think we should switch to that which fixes the warning
> but also warns if someone tries to misuse these
> as C-strings.
> 
> Seems to be a better option, does it not?
> 
> 

Using strpadcpy is clever and self-documenting, though.  We have it
already, so why not use it.

Paolo

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

[Michael S. Tsirkin]

On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote:
> On 18/12/18 15:31, Michael S. Tsirkin wrote:
> > Do you happen to know why does it build fine with
> > Gcc 8.2.1?
> > 
> > Reading the GCC manual it seems that
> > there is a "nostring" attribute that means
> > "might not be 0 terminated".
> > I think we should switch to that which fixes the warning
> > but also warns if someone tries to misuse these
> > as C-strings.
> > 
> > Seems to be a better option, does it not?
> > 
> > 
> 
> Using strpadcpy is clever and self-documenting, though.  We have it
> already, so why not use it.
> 
> Paolo

The advantage of nonstring is that it will catch attempts to
use these fields with functions that expect a 0 terminated string.

strpadcpy will instead just silence the warning.


-- 
MST

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

[Paolo Bonzini]

On 18/12/18 15:54, Michael S. Tsirkin wrote:
> On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote:
>> On 18/12/18 15:31, Michael S. Tsirkin wrote:
>>> Do you happen to know why does it build fine with
>>> Gcc 8.2.1?
>>>
>>> Reading the GCC manual it seems that
>>> there is a "nostring" attribute that means
>>> "might not be 0 terminated".
>>> I think we should switch to that which fixes the warning
>>> but also warns if someone tries to misuse these
>>> as C-strings.
>>>
>>> Seems to be a better option, does it not?
>>>
>>>
>>
>> Using strpadcpy is clever and self-documenting, though.  We have it
>> already, so why not use it.
>>
>> Paolo
> 
> The advantage of nonstring is that it will catch attempts to
> use these fields with functions that expect a 0 terminated string.
> 
> strpadcpy will instead just silence the warning.

Ah, I see.  We could also do both, that's a matter of taste.

Paolo

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

[Philippe Mathieu-Daudé]

On 12/18/18 3:54 PM, Michael S. Tsirkin wrote:
> On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote:
>> On 18/12/18 15:31, Michael S. Tsirkin wrote:
>>> Do you happen to know why does it build fine with
>>> Gcc 8.2.1?
>>>
>>> Reading the GCC manual it seems that
>>> there is a "nostring" attribute that means
>>> "might not be 0 terminated".
>>> I think we should switch to that which fixes the warning
>>> but also warns if someone tries to misuse these
>>> as C-strings.
>>>
>>> Seems to be a better option, does it not?
>>>
>>>
>>
>> Using strpadcpy is clever and self-documenting, though.  We have it
>> already, so why not use it.
>>
>> Paolo
> 
> The advantage of nonstring is that it will catch attempts to
> use these fields with functions that expect a 0 terminated string.
> 
> strpadcpy will instead just silence the warning.

migration/global_state.c:109:15: error: 'strlen' argument 1 declared
attribute 'nonstring' [-Werror=stringop-overflow=]
     s->size = strlen((char *)s->runstate) + 1;
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~

GCC won... It is true this strlen() is buggy, indeed s->runstate might
be not NUL-terminated.

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

[Michael S. Tsirkin]

On Tue, Dec 18, 2018 at 05:38:17PM +0100, Paolo Bonzini wrote:
> On 18/12/18 15:54, Michael S. Tsirkin wrote:
> > On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote:
> >> On 18/12/18 15:31, Michael S. Tsirkin wrote:
> >>> Do you happen to know why does it build fine with
> >>> Gcc 8.2.1?
> >>>
> >>> Reading the GCC manual it seems that
> >>> there is a "nostring" attribute that means
> >>> "might not be 0 terminated".
> >>> I think we should switch to that which fixes the warning
> >>> but also warns if someone tries to misuse these
> >>> as C-strings.
> >>>
> >>> Seems to be a better option, does it not?
> >>>
> >>>
> >>
> >> Using strpadcpy is clever and self-documenting, though.  We have it
> >> already, so why not use it.
> >>
> >> Paolo
> > 
> > The advantage of nonstring is that it will catch attempts to
> > use these fields with functions that expect a 0 terminated string.
> > 
> > strpadcpy will instead just silence the warning.
> 
> Ah, I see.  We could also do both, that's a matter of taste.
> 
> Paolo

Do you happen to know how to make gcc check the buffer size
for strpadcpy? Is the name strncpy just hard-coded?

-- 
MST

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

[Michael S. Tsirkin]

On Tue, Dec 18, 2018 at 05:55:27PM +0100, Philippe Mathieu-Daudé wrote:
> On 12/18/18 3:54 PM, Michael S. Tsirkin wrote:
> > On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote:
> >> On 18/12/18 15:31, Michael S. Tsirkin wrote:
> >>> Do you happen to know why does it build fine with
> >>> Gcc 8.2.1?
> >>>
> >>> Reading the GCC manual it seems that
> >>> there is a "nostring" attribute that means
> >>> "might not be 0 terminated".
> >>> I think we should switch to that which fixes the warning
> >>> but also warns if someone tries to misuse these
> >>> as C-strings.
> >>>
> >>> Seems to be a better option, does it not?
> >>>
> >>>
> >>
> >> Using strpadcpy is clever and self-documenting, though.  We have it
> >> already, so why not use it.
> >>
> >> Paolo
> > 
> > The advantage of nonstring is that it will catch attempts to
> > use these fields with functions that expect a 0 terminated string.
> > 
> > strpadcpy will instead just silence the warning.
> 
> migration/global_state.c:109:15: error: 'strlen' argument 1 declared
> attribute 'nonstring' [-Werror=stringop-overflow=]
>      s->size = strlen((char *)s->runstate) + 1;
>                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> GCC won... It is true this strlen() is buggy, indeed s->runstate might
> be not NUL-terminated.


Ooh nice. I smell some CVE fixes coming from this effort.


-- 
MST

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

[Paolo Bonzini]

On 18/12/18 17:55, Philippe Mathieu-Daudé wrote:
>> strpadcpy will instead just silence the warning.
> migration/global_state.c:109:15: error: 'strlen' argument 1 declared
> attribute 'nonstring' [-Werror=stringop-overflow=]
>      s->size = strlen((char *)s->runstate) + 1;
>                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> GCC won... It is true this strlen() is buggy, indeed s->runstate might
> be not NUL-terminated.

No, runstate is declared as an array of 100 bytes, which are more than
enough.  It's ugly code but not buggy.

Paolo

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

[Michael S. Tsirkin]

On Tue, Dec 18, 2018 at 06:12:05PM +0100, Paolo Bonzini wrote:
> On 18/12/18 17:55, Philippe Mathieu-Daudé wrote:
> >> strpadcpy will instead just silence the warning.
> > migration/global_state.c:109:15: error: 'strlen' argument 1 declared
> > attribute 'nonstring' [-Werror=stringop-overflow=]
> >      s->size = strlen((char *)s->runstate) + 1;
> >                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > GCC won... It is true this strlen() is buggy, indeed s->runstate might
> > be not NUL-terminated.
> 
> No, runstate is declared as an array of 100 bytes, which are more than
> enough.  It's ugly code but not buggy.
> 
> Paolo

Yes ... but it is loaded using
        VMSTATE_BUFFER(runstate, GlobalState),
and parsed using qapi_enum_parse which does not get
the buffer length.

So unless we are lucky there's a buffer overrun
on a remote/file input here.

Seems buggy to me - what am I missing?

-- 
MST

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

[Daniel P. Berrangé]

On Tue, Dec 18, 2018 at 12:03:34PM -0500, Michael S. Tsirkin wrote:
> On Tue, Dec 18, 2018 at 05:38:17PM +0100, Paolo Bonzini wrote:
> > On 18/12/18 15:54, Michael S. Tsirkin wrote:
> > > On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote:
> > >> On 18/12/18 15:31, Michael S. Tsirkin wrote:
> > >>> Do you happen to know why does it build fine with
> > >>> Gcc 8.2.1?
> > >>>
> > >>> Reading the GCC manual it seems that
> > >>> there is a "nostring" attribute that means
> > >>> "might not be 0 terminated".
> > >>> I think we should switch to that which fixes the warning
> > >>> but also warns if someone tries to misuse these
> > >>> as C-strings.
> > >>>
> > >>> Seems to be a better option, does it not?
> > >>>
> > >>>
> > >>
> > >> Using strpadcpy is clever and self-documenting, though.  We have it
> > >> already, so why not use it.
> > >>
> > >> Paolo
> > > 
> > > The advantage of nonstring is that it will catch attempts to
> > > use these fields with functions that expect a 0 terminated string.
> > > 
> > > strpadcpy will instead just silence the warning.
> > 
> > Ah, I see.  We could also do both, that's a matter of taste.
> > 
> > Paolo
> 
> Do you happen to know how to make gcc check the buffer size
> for strpadcpy? Is the name strncpy just hard-coded?

GCC provides  strncpy as a builtin function and its warning only
checks its builtin.

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

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

[Paolo Bonzini]

On 18/12/18 18:17, Michael S. Tsirkin wrote:
> On Tue, Dec 18, 2018 at 06:12:05PM +0100, Paolo Bonzini wrote:
>> On 18/12/18 17:55, Philippe Mathieu-Daudé wrote:
>>>> strpadcpy will instead just silence the warning.
>>> migration/global_state.c:109:15: error: 'strlen' argument 1 declared
>>> attribute 'nonstring' [-Werror=stringop-overflow=]
>>>      s->size = strlen((char *)s->runstate) + 1;
>>>                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> GCC won... It is true this strlen() is buggy, indeed s->runstate might
>>> be not NUL-terminated.
>>
>> No, runstate is declared as an array of 100 bytes, which are more than
>> enough.  It's ugly code but not buggy.
>>
>> Paolo
> 
> Yes ... but it is loaded using
>         VMSTATE_BUFFER(runstate, GlobalState),
> and parsed using qapi_enum_parse which does not get
> the buffer length.
> 
> So unless we are lucky there's a buffer overrun
> on a remote/file input here.
> 
> Seems buggy to me - what am I missing?

Yup.   I think we're lucky twice though.  First, the state field stops
the runaway qapi_enum_parse.  Second, in any case worst case it's a segv
on migration.  This is a bug but not a CVE.

Paolo