QEMU

gcc 8.2 reports stringop-truncation when building qemu

Bug #1803872 reported by Amir Gonnen
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
QEMU
Fix Released
Undecided
Unassigned

Bug Description

QEMU 3.0

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

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

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

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

See original description
Amir Gonnen (amirgon)
description: updated
Revision history for this message
elmarco (marcandre-lureau) wrote : Re: [Qemu-devel] [PATCH v2 3/3] migration: Replace strncpy() by strpadcpy(pad='\0')

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

Reviewed-by: Marc-André Lureau <email address hidden>

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

--
Marc-André Lureau

Revision history for this message
Igor (imammedo) wrote : Re: [Qemu-devel] [PATCH v2 1/3] hw/acpi: Replace strncpy() by strpadcpy(pad='\0')
Download full text (4.7 KiB)

On Tue, 18 Dec 2018 12:03:31 +0100
Philippe Mathieu-Daudé <email address hidden> wrote:

> From: Marc-André Lureau <email address hidden>
>
> 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 <email address hidden>
> Reviewed-by: Eric Blake <email address hidden>
> Reviewed-by: Philippe Mathieu-Daudé <email address hidden>
> [PMD: reword commit subject and description]
> Signed-off-by: Philippe Mathieu-Daudé <email address hidden>

Reviewed-by: Igor Mammedov <email address hidden>

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

Read more...

Revision history for this message
mst (mst-0) wrote : Re: [PATCH v2 0/3] Fix strncpy() warnings for GCC8 new -Wstringop-truncation

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

Revision history for this message
Daniel Berrange (berrange) wrote :

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

Revision history for this message
mst (mst-0) wrote :

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

Revision history for this message
mst (mst-0) 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.

--
MST

Revision history for this message
mst (mst-0) 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?

--
MST

Revision history for this message
mst (mst-0) wrote :

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

Revision history for this message
mst (mst-0) 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?

--
MST

Revision history for this message
Daniel Berrange (berrange) wrote :

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

Revision history for this message
Eric Blake (eblake) wrote : Re: [PATCH v3 1/5] qemu/compiler: Define QEMU_NONSTRING

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

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

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

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

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

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

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

Reviewed-by: Eric Blake <email address hidden>

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

Revision history for this message
Eric Blake (eblake) wrote : Re: [PATCH v3 2/5] block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays

On 12/18/18 11:51 AM, Philippe Mathieu-Daudé wrote:
> GCC 8 added a -Wstringop-truncation warning:
>
> The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
> bug 81117 is specifically intended to highlight likely unintended
> uses of the strncpy function that truncate the terminating NUL
> character from the source string.
>
> This new warning leads to compilation failures:
>
> CC block/sheepdog.o
> qemu/block/sheepdog.c: In function 'find_vdi_name':
> qemu/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
> strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> make: *** [qemu/rules.mak:69: block/sheepdog.o] Error 1
>
> As described previous to the strncpy() calls, the use of strncpy() is
> correct here:
>
> /* This pair of strncpy calls ensures that the buffer is zero-filled,
> * which is desirable since we'll soon be sending those bytes, and
> * don't want the send_req to read uninitialized data.
> */
> strncpy(buf, filename, SD_MAX_VDI_LEN);
> strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>
> Use the QEMU_NONSTRING attribute, since this array is intended to store
> character arrays that do not necessarily contain a terminating NUL.
>
> Suggested-by: Michael S. Tsirkin <email address hidden>
> Signed-off-by: Philippe Mathieu-Daudé <email address hidden>
> ---
> block/sheepdog.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <email address hidden>

>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 0125df9d49..d4ad6b119d 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1224,7 +1224,7 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
> SheepdogVdiReq hdr;
> SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
> unsigned int wlen, rlen = 0;
> - char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
> + QEMU_NONSTRING char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
>
> fd = connect_to_sdog(s, errp);
> if (fd < 0) {
>

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

Revision history for this message
Eric Blake (eblake) wrote : Re: [PATCH v3 4/5] migration: Use QEMU_NONSTRING for non NUL-terminated arrays

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

Should this be squashed with 5/5?

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

Revision history for this message
Eric Blake (eblake) wrote : Re: [PATCH v3 5/5] migration: Use strnlen() for fixed-size string

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

s/We/we/

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

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

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

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

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

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

Revision history for this message
Eric Blake (eblake) wrote : Re: [PATCH v3 4/5] migration: Use QEMU_NONSTRING for non NUL-terminated arrays

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

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

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

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

Revision history for this message
mst (mst-0) wrote : Re: [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation

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

Reviewed-by: Michael S. Tsirkin <email address hidden>

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

Revision history for this message
mst (mst-0) wrote : Re: [PATCH v3 2/5] block/sheepdog: Use QEMU_NONSTRING for non NUL-terminated arrays

On Tue, Dec 18, 2018 at 06:51:19PM +0100, Philippe Mathieu-Daudé wrote:
> GCC 8 added a -Wstringop-truncation warning:
>
> The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
> bug 81117 is specifically intended to highlight likely unintended
> uses of the strncpy function that truncate the terminating NUL
> character from the source string.
>
> This new warning leads to compilation failures:
>
> CC block/sheepdog.o
> qemu/block/sheepdog.c: In function 'find_vdi_name':
> qemu/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
> strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> make: *** [qemu/rules.mak:69: block/sheepdog.o] Error 1
>
> As described previous to the strncpy() calls, the use of strncpy() is
> correct here:
>
> /* This pair of strncpy calls ensures that the buffer is zero-filled,
> * which is desirable since we'll soon be sending those bytes, and
> * don't want the send_req to read uninitialized data.
> */
> strncpy(buf, filename, SD_MAX_VDI_LEN);
> strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>
> Use the QEMU_NONSTRING attribute, since this array is intended to store
> character arrays that do not necessarily contain a terminating NUL.
>
> Suggested-by: Michael S. Tsirkin <email address hidden>
> Signed-off-by: Philippe Mathieu-Daudé <email address hidden>
> ---
> block/sheepdog.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 0125df9d49..d4ad6b119d 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1224,7 +1224,7 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
> SheepdogVdiReq hdr;
> SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
> unsigned int wlen, rlen = 0;
> - char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
> + QEMU_NONSTRING char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];

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

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

Revision history for this message
mst (mst-0) wrote : Re: [PATCH v3 5/5] migration: Use strnlen() for fixed-size string

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

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

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

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

> }
> --
> 2.17.2

Revision history for this message
Igor (imammedo) wrote : Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays
Download full text (4.4 KiB)

On Tue, 18 Dec 2018 18:51:20 +0100
Philippe Mathieu-Daudé <email address hidden> wrote:

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

Read more...

Revision history for this message
Igor (imammedo) wrote :
Download full text (5.1 KiB)

On Wed, 19 Dec 2018 10:20:36 +0100
Philippe Mathieu-Daudé <email address hidden> wrote:

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

Read more...

Revision history for this message
Igor (imammedo) wrote :

On Wed, 19 Dec 2018 14:00:37 +0100
Andrew Jones <email address hidden> wrote:

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

>
> Thanks,
> drew

Revision history for this message
Dr. David Alan Gilbert (dgilbert-h) wrote : Re: [PATCH v3 4/5] migration: Use QEMU_NONSTRING for non NUL-terminated arrays

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

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

Dave

> RunState state;
> bool received;
> } GlobalState;
> --
> 2.17.2
>
--
Dr. David Alan Gilbert / <email address hidden> / Manchester, UK

Revision history for this message
Peter Maydell (pmaydell) wrote :

We think we've fixed all the GCC 8.2 stringop-truncation warnings now, so current QEMU git master and the 4.0 rc1 we've just tagged should both build cleanly. Please reopen the bug if we missed one somehow.

Changed in qemu:
status: New → Fix Committed
Thomas Huth (th-huth)
Changed in qemu:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.