All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH for-3.1? 0/3] strcpy: fix stringop-truncation warnings
@ 2018-11-20 15:27 Marc-André Lureau
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning Marc-André Lureau
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Marc-André Lureau @ 2018-11-20 15:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-block, Kevin Wolf, Juan Quintela, Max Reitz, Jeff Cody,
	Liu Yuan, Igor Mammedov, Michael S. Tsirkin,
	Dr. David Alan Gilbert, Marc-André Lureau

Hi,

Some of those warnings have already been fixed, others have been
delayed as it could make sense to disable/ignoring the warning, or
write a custom strncpy() function.

In some cases where NUL-ending string is not mandatory (because the
string length is bound in some format or protocol), we can replace
strncpy() with qemu strpadcpy(), so that the destination string is
still NUL-ending in cases where the destination is larger than the
source string.

Some warnings can be shut up with assert() lines in some cases.

Marc-André Lureau (3):
  sheepdog: fix stringop-truncation warning
  migration: fix stringop-truncation warning
  acpi: fix stringop-truncation warnings

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

-- 
2.19.1.708.g4ede3d42df

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning
  2018-11-20 15:27 [Qemu-devel] [PATCH for-3.1? 0/3] strcpy: fix stringop-truncation warnings Marc-André Lureau
@ 2018-11-20 15:27 ` Marc-André Lureau
  2018-11-20 16:56   ` Eric Blake
  2018-11-20 19:35   ` Philippe Mathieu-Daudé
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 2/3] migration: " Marc-André Lureau
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings Marc-André Lureau
  2 siblings, 2 replies; 13+ messages in thread
From: Marc-André Lureau @ 2018-11-20 15:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-block, Kevin Wolf, Juan Quintela, Max Reitz, Jeff Cody,
	Liu Yuan, Igor Mammedov, Michael S. Tsirkin,
	Dr. David Alan Gilbert, Marc-André Lureau

It seems adding an assert is enough to silence GCC.
(sd_parse_snapid_or_tag() g_strlcpy() ensures that we don't get in
that situation)

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/sheepdog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0125df9d49..f8877b611d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1236,6 +1236,7 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
      * don't want the send_req to read uninitialized data.
      */
     strncpy(buf, filename, SD_MAX_VDI_LEN);
+    assert(strlen(tag) < SD_MAX_VDI_TAG_LEN);
     strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
 
     memset(&hdr, 0, sizeof(hdr));
-- 
2.19.1.708.g4ede3d42df

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning
  2018-11-20 15:27 [Qemu-devel] [PATCH for-3.1? 0/3] strcpy: fix stringop-truncation warnings Marc-André Lureau
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning Marc-André Lureau
@ 2018-11-20 15:27 ` Marc-André Lureau
  2018-11-20 17:01   ` Eric Blake
  2018-11-20 19:37   ` Philippe Mathieu-Daudé
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings Marc-André Lureau
  2 siblings, 2 replies; 13+ messages in thread
From: Marc-André Lureau @ 2018-11-20 15:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-block, Kevin Wolf, Juan Quintela, Max Reitz, Jeff Cody,
	Liu Yuan, Igor Mammedov, Michael S. Tsirkin,
	Dr. David Alan Gilbert, Marc-André Lureau

Adding an assert is enough to silence GCC.

~/src/qemu/migration/global_state.c: In function 'global_state_store_running':
~/src/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));
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@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.19.1.708.g4ede3d42df

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings
  2018-11-20 15:27 [Qemu-devel] [PATCH for-3.1? 0/3] strcpy: fix stringop-truncation warnings Marc-André Lureau
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning Marc-André Lureau
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 2/3] migration: " Marc-André Lureau
@ 2018-11-20 15:27 ` Marc-André Lureau
  2018-11-20 17:03   ` Eric Blake
  2018-11-20 19:45   ` Philippe Mathieu-Daudé
  2 siblings, 2 replies; 13+ messages in thread
From: Marc-André Lureau @ 2018-11-20 15:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-block, Kevin Wolf, Juan Quintela, Max Reitz, Jeff Cody,
	Liu Yuan, Igor Mammedov, Michael S. Tsirkin,
	Dr. David Alan Gilbert, Marc-André Lureau

Replace strcpy() that don't mind about having dest not ending with NUL
char by qemu strpadcpy().

Signed-off-by: Marc-André Lureau <marcandre.lureau@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..0a64a23e09 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..c1b4dfbfd9 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.19.1.708.g4ede3d42df

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning Marc-André Lureau
@ 2018-11-20 16:56   ` Eric Blake
  2018-11-20 19:35   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 13+ messages in thread
From: Eric Blake @ 2018-11-20 16:56 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel
  Cc: Kevin Wolf, qemu-block, Juan Quintela, Jeff Cody,
	Michael S. Tsirkin, Dr. David Alan Gilbert, Max Reitz,
	Igor Mammedov, Liu Yuan

On 11/20/18 9:27 AM, Marc-André Lureau wrote:
> It seems adding an assert is enough to silence GCC.
> (sd_parse_snapid_or_tag() g_strlcpy() ensures that we don't get in
> that situation)
> 
> ~/src/qemu/block/sheepdog.c: In function 'find_vdi_name':
> ~/src/qemu/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
>       strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   block/sheepdog.c | 1 +
>   1 file changed, 1 insertion(+)

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

and safe for 3.1 in my opinion

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 2/3] migration: " Marc-André Lureau
@ 2018-11-20 17:01   ` Eric Blake
  2018-11-20 17:22     ` Dr. David Alan Gilbert
  2018-11-20 19:37   ` Philippe Mathieu-Daudé
  1 sibling, 1 reply; 13+ messages in thread
From: Eric Blake @ 2018-11-20 17:01 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel
  Cc: Kevin Wolf, qemu-block, Juan Quintela, Jeff Cody,
	Michael S. Tsirkin, Dr. David Alan Gilbert, Max Reitz,
	Igor Mammedov, Liu Yuan

On 11/20/18 9:27 AM, Marc-André Lureau wrote:
> Adding an assert is enough to silence GCC.
> 
> ~/src/qemu/migration/global_state.c: In function 'global_state_store_running':
> ~/src/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));
>              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> (alternatively, we could hard-code "running")
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   migration/global_state.c | 1 +
>   1 file changed, 1 insertion(+)

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

I think this is safe for 3.1, but I know the migration code is 
particularly wary of assert()s, even when they are non-triggerable (a 
100-byte buffer at global_state.runstate is big enough for ALL of the 
run states, not just RUN_STATE_RUNNING).

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

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings Marc-André Lureau
@ 2018-11-20 17:03   ` Eric Blake
  2018-11-20 19:45   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 13+ messages in thread
From: Eric Blake @ 2018-11-20 17:03 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel
  Cc: Kevin Wolf, qemu-block, Juan Quintela, Jeff Cody,
	Michael S. Tsirkin, Dr. David Alan Gilbert, Max Reitz,
	Igor Mammedov, Liu Yuan

On 11/20/18 9:27 AM, Marc-André Lureau wrote:
> Replace strcpy() that don't mind about having dest not ending with NUL
> char by qemu strpadcpy().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/acpi/aml-build.c |  6 ++++--
>   hw/acpi/core.c      | 13 +++++++------
>   2 files changed, 11 insertions(+), 8 deletions(-)
> 

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

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning
  2018-11-20 17:01   ` Eric Blake
@ 2018-11-20 17:22     ` Dr. David Alan Gilbert
  2018-11-20 17:24       ` Marc-André Lureau
  0 siblings, 1 reply; 13+ messages in thread
From: Dr. David Alan Gilbert @ 2018-11-20 17:22 UTC (permalink / raw)
  To: Eric Blake
  Cc: Marc-André Lureau, qemu-devel, Kevin Wolf, qemu-block,
	Juan Quintela, Jeff Cody, Michael S. Tsirkin, Max Reitz,
	Igor Mammedov, Liu Yuan

* Eric Blake (eblake@redhat.com) wrote:
> On 11/20/18 9:27 AM, Marc-André Lureau wrote:
> > Adding an assert is enough to silence GCC.
> > 
> > ~/src/qemu/migration/global_state.c: In function 'global_state_store_running':
> > ~/src/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));
> >              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> > 
> > (alternatively, we could hard-code "running")
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   migration/global_state.c | 1 +
> >   1 file changed, 1 insertion(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I think this is safe for 3.1, but I know the migration code is particularly
> wary of assert()s, even when they are non-triggerable (a 100-byte buffer at
> global_state.runstate is big enough for ALL of the run states, not just
> RUN_STATE_RUNNING).

That's OK; the universe would have to be particularly broken to trigger
that one, and it's in no way connected with any state, so it would
trigger on even the most basic tests.

However, I wonder if this fixes the problem on mingw builds - windows
asserts are not marked as noreturn.

Dave

> > 
> > 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));
> >   }
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning
  2018-11-20 17:22     ` Dr. David Alan Gilbert
@ 2018-11-20 17:24       ` Marc-André Lureau
  2018-11-20 17:25         ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 13+ messages in thread
From: Marc-André Lureau @ 2018-11-20 17:24 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: Blake, Eric, qemu-devel, Wolf, Kevin, qemu-block, Juan Quintela,
	Jeff Cody, Michael S . Tsirkin, Max Reitz, Igor Mammedov, unix,
	namei

Hi

On Tue, Nov 20, 2018 at 9:22 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Eric Blake (eblake@redhat.com) wrote:
> > On 11/20/18 9:27 AM, Marc-André Lureau wrote:
> > > Adding an assert is enough to silence GCC.
> > >
> > > ~/src/qemu/migration/global_state.c: In function 'global_state_store_running':
> > > ~/src/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));
> > >              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > cc1: all warnings being treated as errors
> > >
> > > (alternatively, we could hard-code "running")
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >   migration/global_state.c | 1 +
> > >   1 file changed, 1 insertion(+)
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> >
> > I think this is safe for 3.1, but I know the migration code is particularly
> > wary of assert()s, even when they are non-triggerable (a 100-byte buffer at
> > global_state.runstate is big enough for ALL of the run states, not just
> > RUN_STATE_RUNNING).
>
> That's OK; the universe would have to be particularly broken to trigger
> that one, and it's in no way connected with any state, so it would
> trigger on even the most basic tests.
>
> However, I wonder if this fixes the problem on mingw builds - windows
> asserts are not marked as noreturn.

On f29, with mingw64-gcc-8.2.0-3.fc29.x86_64, it fixes the warning.

>
> Dave
>
> > >
> > > 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));
> > >   }
> > >
> >
> > --
> > Eric Blake, Principal Software Engineer
> > Red Hat, Inc.           +1-919-301-3266
> > Virtualization:  qemu.org | libvirt.org
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning
  2018-11-20 17:24       ` Marc-André Lureau
@ 2018-11-20 17:25         ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 13+ messages in thread
From: Dr. David Alan Gilbert @ 2018-11-20 17:25 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: Blake, Eric, qemu-devel, Wolf, Kevin, qemu-block, Juan Quintela,
	Jeff Cody, Michael S . Tsirkin, Max Reitz, Igor Mammedov, unix,
	namei

* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> Hi
> 
> On Tue, Nov 20, 2018 at 9:22 PM Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Eric Blake (eblake@redhat.com) wrote:
> > > On 11/20/18 9:27 AM, Marc-André Lureau wrote:
> > > > Adding an assert is enough to silence GCC.
> > > >
> > > > ~/src/qemu/migration/global_state.c: In function 'global_state_store_running':
> > > > ~/src/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));
> > > >              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > cc1: all warnings being treated as errors
> > > >
> > > > (alternatively, we could hard-code "running")
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > >   migration/global_state.c | 1 +
> > > >   1 file changed, 1 insertion(+)
> > >
> > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > >
> > > I think this is safe for 3.1, but I know the migration code is particularly
> > > wary of assert()s, even when they are non-triggerable (a 100-byte buffer at
> > > global_state.runstate is big enough for ALL of the run states, not just
> > > RUN_STATE_RUNNING).
> >
> > That's OK; the universe would have to be particularly broken to trigger
> > that one, and it's in no way connected with any state, so it would
> > trigger on even the most basic tests.
> >
> > However, I wonder if this fixes the problem on mingw builds - windows
> > asserts are not marked as noreturn.
> 
> On f29, with mingw64-gcc-8.2.0-3.fc29.x86_64, it fixes the warning.

OK, fine.


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

Dave

> >
> > Dave
> >
> > > >
> > > > 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));
> > > >   }
> > > >
> > >
> > > --
> > > Eric Blake, Principal Software Engineer
> > > Red Hat, Inc.           +1-919-301-3266
> > > Virtualization:  qemu.org | libvirt.org
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning Marc-André Lureau
  2018-11-20 16:56   ` Eric Blake
@ 2018-11-20 19:35   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-11-20 19:35 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel
  Cc: Kevin Wolf, qemu-block, Juan Quintela, Jeff Cody,
	Michael S. Tsirkin, Dr. David Alan Gilbert, Max Reitz,
	Igor Mammedov, Liu Yuan

On 20/11/18 16:27, Marc-André Lureau wrote:
> It seems adding an assert is enough to silence GCC.
> (sd_parse_snapid_or_tag() g_strlcpy() ensures that we don't get in
> that situation)
> 
> ~/src/qemu/block/sheepdog.c: In function 'find_vdi_name':
> ~/src/qemu/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
>       strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 

Fixes: https://bugs.launchpad.net/bugs/1803872

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   block/sheepdog.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 0125df9d49..f8877b611d 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1236,6 +1236,7 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
>        * don't want the send_req to read uninitialized data.
>        */
>       strncpy(buf, filename, SD_MAX_VDI_LEN);
> +    assert(strlen(tag) < SD_MAX_VDI_TAG_LEN);
>       strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>   
>       memset(&hdr, 0, sizeof(hdr));
> 

I tried to fix this warning this way:

-    char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
+    struct {
+        char vdi[SD_MAX_VDI_LEN];
+        char vdi_tag[SD_MAX_VDI_TAG_LEN];
+    } buf;

...

-    strncpy(buf, filename, SD_MAX_VDI_LEN);
-    strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
+    strncpy(buf.vdi, filename, SD_MAX_VDI_LEN);
+    strncpy(buf.vdi_tag, tag, SD_MAX_VDI_TAG_LEN);

but your patch is simpler.

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

Thanks,

Phil.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 2/3] migration: " Marc-André Lureau
  2018-11-20 17:01   ` Eric Blake
@ 2018-11-20 19:37   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-11-20 19:37 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel
  Cc: Kevin Wolf, qemu-block, Juan Quintela, Jeff Cody,
	Michael S. Tsirkin, Dr. David Alan Gilbert, Max Reitz,
	Igor Mammedov, Liu Yuan

On 20/11/18 16:27, Marc-André Lureau wrote:
> Adding an assert is enough to silence GCC.
> 
> ~/src/qemu/migration/global_state.c: In function 'global_state_store_running':
> ~/src/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));
>              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> (alternatively, we could hard-code "running")
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-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));
>   }
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings
  2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings Marc-André Lureau
  2018-11-20 17:03   ` Eric Blake
@ 2018-11-20 19:45   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-11-20 19:45 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel
  Cc: Kevin Wolf, qemu-block, Juan Quintela, Jeff Cody,
	Michael S. Tsirkin, Dr. David Alan Gilbert, Max Reitz,
	Igor Mammedov, Liu Yuan

On 20/11/18 16:27, Marc-André Lureau wrote:
> Replace strcpy() that don't mind about having dest not ending with NUL
> char by qemu strpadcpy().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@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..0a64a23e09 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..c1b4dfbfd9 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) {
> 

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

Sounds like a good idea. However we don't this particular one for 3.1, 
does it silents a warning?

Regards,

Phil.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-11-20 19:45 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-20 15:27 [Qemu-devel] [PATCH for-3.1? 0/3] strcpy: fix stringop-truncation warnings Marc-André Lureau
2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning Marc-André Lureau
2018-11-20 16:56   ` Eric Blake
2018-11-20 19:35   ` Philippe Mathieu-Daudé
2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 2/3] migration: " Marc-André Lureau
2018-11-20 17:01   ` Eric Blake
2018-11-20 17:22     ` Dr. David Alan Gilbert
2018-11-20 17:24       ` Marc-André Lureau
2018-11-20 17:25         ` Dr. David Alan Gilbert
2018-11-20 19:37   ` Philippe Mathieu-Daudé
2018-11-20 15:27 ` [Qemu-devel] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings Marc-André Lureau
2018-11-20 17:03   ` Eric Blake
2018-11-20 19:45   ` Philippe Mathieu-Daudé

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.