| Message ID | 20181218175122.3229-4-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix strncpy() warnings for GCC8 new -Wstringop-truncation | expand |
On Tue, 18 Dec 2018 18:51:20 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > GCC 8 added a -Wstringop-truncation warning: > > The -Wstringop-truncation warning added in GCC 8.0 via r254630 for > bug 81117 is specifically intended to highlight likely unintended > uses of the strncpy function that truncate the terminating NUL > character from the source string. > > This new warning leads to compilation failures: > > CC hw/acpi/core.o > In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5: > qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation] > strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1 > > Use the QEMU_NONSTRING attribute, since ACPI tables don't require the > strings to be NUL-terminated. > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/acpi/core.c | 8 ++++---- > include/hw/acpi/acpi-defs.h | 8 ++++---- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/hw/acpi/core.c b/hw/acpi/core.c > index aafdc61648..f60f750c3d 100644 > --- a/hw/acpi/core.c > +++ b/hw/acpi/core.c > @@ -35,14 +35,14 @@ > struct acpi_table_header { > uint16_t _length; /* our length, not actual part of the hdr */ > /* allows easier parsing for fw_cfg clients */ > - char sig[4]; /* ACPI signature (4 ASCII characters) */ > + char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) */ > uint32_t length; /* Length of table, in bytes, including header */ > uint8_t revision; /* ACPI Specification minor version # */ > uint8_t checksum; /* To make sum of entire table == 0 */ > - char oem_id[6]; /* OEM identification */ > - char oem_table_id[8]; /* OEM table identification */ > + char oem_id[6] QEMU_NONSTRING; /* OEM identification */ > + char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */ > uint32_t oem_revision; /* OEM revision number */ > - char asl_compiler_id[4]; /* ASL compiler vendor ID */ > + char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */ > uint32_t asl_compiler_revision; /* ASL compiler revision number */ > } QEMU_PACKED; > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index af8e023968..3bf0bec8ba 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -43,7 +43,7 @@ enum { > struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ > uint64_t signature; /* ACPI signature, contains "RSD PTR " */ > uint8_t checksum; /* To make sum of struct == 0 */ > - uint8_t oem_id [6]; /* OEM identification */ > + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ > uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */ > uint32_t rsdt_physical_address; /* 32-bit physical address of RSDT */ > uint32_t length; /* XSDT Length in bytes including hdr */ you'll need to rebase this on top the latest Michael's pull request. [PULL v2 25/30] hw: arm: Carry RSDP specific data through AcpiRsdpData [PULL v2 29/30] hw: acpi: Remove AcpiRsdpDescriptor and fix tests > @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor; > uint32_t length; /* Length of table, in bytes, including header */ \ > uint8_t revision; /* ACPI Specification minor version # */ \ > uint8_t checksum; /* To make sum of entire table == 0 */ \ > - uint8_t oem_id [6]; /* OEM identification */ \ > - uint8_t oem_table_id [8]; /* OEM table identification */ \ > + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ \ > + uint8_t oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ \ > uint32_t oem_revision; /* OEM revision number */ \ > - uint8_t asl_compiler_id [4]; /* ASL compiler vendor ID */ \ > + uint8_t asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID */ \ > uint32_t asl_compiler_revision; /* ASL compiler revision number */ > >
Le mer. 19 déc. 2018 10:16, Igor Mammedov <imammedo@redhat.com> a écrit : > On Tue, 18 Dec 2018 18:51:20 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > GCC 8 added a -Wstringop-truncation warning: > > > > The -Wstringop-truncation warning added in GCC 8.0 via r254630 for > > bug 81117 is specifically intended to highlight likely unintended > > uses of the strncpy function that truncate the terminating NUL > > character from the source string. > > > > This new warning leads to compilation failures: > > > > CC hw/acpi/core.o > > In function 'acpi_table_install', inlined from 'acpi_table_add' at > qemu/hw/acpi/core.c:296:5: > > qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals > destination size [-Werror=stringop-truncation] > > strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1 > > > > Use the QEMU_NONSTRING attribute, since ACPI tables don't require the > > strings to be NUL-terminated. > > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > --- > > hw/acpi/core.c | 8 ++++---- > > include/hw/acpi/acpi-defs.h | 8 ++++---- > > 2 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/hw/acpi/core.c b/hw/acpi/core.c > > index aafdc61648..f60f750c3d 100644 > > --- a/hw/acpi/core.c > > +++ b/hw/acpi/core.c > > @@ -35,14 +35,14 @@ > > struct acpi_table_header { > > uint16_t _length; /* our length, not actual part of the hdr > */ > > /* allows easier parsing for fw_cfg > clients */ > > - char sig[4]; /* ACPI signature (4 ASCII characters) */ > > + char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) > */ > > uint32_t length; /* Length of table, in bytes, including > header */ > > uint8_t revision; /* ACPI Specification minor version # */ > > uint8_t checksum; /* To make sum of entire table == 0 */ > > - char oem_id[6]; /* OEM identification */ > > - char oem_table_id[8]; /* OEM table identification */ > > + char oem_id[6] QEMU_NONSTRING; /* OEM identification */ > > + char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */ > > uint32_t oem_revision; /* OEM revision number */ > > - char asl_compiler_id[4]; /* ASL compiler vendor ID */ > > + char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */ > > uint32_t asl_compiler_revision; /* ASL compiler revision number */ > > } QEMU_PACKED; > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > > index af8e023968..3bf0bec8ba 100644 > > --- a/include/hw/acpi/acpi-defs.h > > +++ b/include/hw/acpi/acpi-defs.h > > @@ -43,7 +43,7 @@ enum { > > struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ > > uint64_t signature; /* ACPI signature, contains "RSD > PTR " */ > > uint8_t checksum; /* To make sum of struct == 0 */ > > - uint8_t oem_id [6]; /* OEM identification */ > > + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ > > uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */ > > uint32_t rsdt_physical_address; /* 32-bit physical address of RSDT > */ > > uint32_t length; /* XSDT Length in bytes including > hdr */ > > you'll need to rebase this on top the latest Michael's pull request. > [PULL v2 25/30] hw: arm: Carry RSDP specific data through AcpiRsdpData > [PULL v2 29/30] hw: acpi: Remove AcpiRsdpDescriptor and fix tests > OK. Can I add your Ack-by then? > @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor; > > uint32_t length; /* Length of table, in bytes, > including header */ \ > > uint8_t revision; /* ACPI Specification minor > version # */ \ > > uint8_t checksum; /* To make sum of entire table == > 0 */ \ > > - uint8_t oem_id [6]; /* OEM identification */ \ > > - uint8_t oem_table_id [8]; /* OEM table identification */ \ > > + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ \ > > + uint8_t oem_table_id [8] QEMU_NONSTRING; /* OEM table > identification */ \ > > uint32_t oem_revision; /* OEM revision number */ \ > > - uint8_t asl_compiler_id [4]; /* ASL compiler vendor ID */ \ > > + uint8_t asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor > ID */ \ > > uint32_t asl_compiler_revision; /* ASL compiler revision number */ > > > > > >
On Wed, 19 Dec 2018 10:20:36 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Le mer. 19 déc. 2018 10:16, Igor Mammedov <imammedo@redhat.com> a écrit : > > > On Tue, 18 Dec 2018 18:51:20 +0100 > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > > > GCC 8 added a -Wstringop-truncation warning: > > > > > > The -Wstringop-truncation warning added in GCC 8.0 via r254630 for > > > bug 81117 is specifically intended to highlight likely unintended > > > uses of the strncpy function that truncate the terminating NUL > > > character from the source string. > > > > > > This new warning leads to compilation failures: > > > > > > CC hw/acpi/core.o > > > In function 'acpi_table_install', inlined from 'acpi_table_add' at > > qemu/hw/acpi/core.c:296:5: > > > qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals > > destination size [-Werror=stringop-truncation] > > > strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1 > > > > > > Use the QEMU_NONSTRING attribute, since ACPI tables don't require the > > > strings to be NUL-terminated. > > > > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > --- > > > hw/acpi/core.c | 8 ++++---- > > > include/hw/acpi/acpi-defs.h | 8 ++++---- > > > 2 files changed, 8 insertions(+), 8 deletions(-) > > > > > > diff --git a/hw/acpi/core.c b/hw/acpi/core.c > > > index aafdc61648..f60f750c3d 100644 > > > --- a/hw/acpi/core.c > > > +++ b/hw/acpi/core.c > > > @@ -35,14 +35,14 @@ > > > struct acpi_table_header { > > > uint16_t _length; /* our length, not actual part of the hdr > > */ > > > /* allows easier parsing for fw_cfg > > clients */ > > > - char sig[4]; /* ACPI signature (4 ASCII characters) */ > > > + char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) > > */ > > > uint32_t length; /* Length of table, in bytes, including > > header */ > > > uint8_t revision; /* ACPI Specification minor version # */ > > > uint8_t checksum; /* To make sum of entire table == 0 */ > > > - char oem_id[6]; /* OEM identification */ > > > - char oem_table_id[8]; /* OEM table identification */ > > > + char oem_id[6] QEMU_NONSTRING; /* OEM identification */ > > > + char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */ > > > uint32_t oem_revision; /* OEM revision number */ > > > - char asl_compiler_id[4]; /* ASL compiler vendor ID */ > > > + char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */ > > > uint32_t asl_compiler_revision; /* ASL compiler revision number */ > > > } QEMU_PACKED; > > > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > > > index af8e023968..3bf0bec8ba 100644 > > > --- a/include/hw/acpi/acpi-defs.h > > > +++ b/include/hw/acpi/acpi-defs.h > > > @@ -43,7 +43,7 @@ enum { > > > struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ > > > uint64_t signature; /* ACPI signature, contains "RSD > > PTR " */ > > > uint8_t checksum; /* To make sum of struct == 0 */ > > > - uint8_t oem_id [6]; /* OEM identification */ > > > + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ > > > uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */ > > > uint32_t rsdt_physical_address; /* 32-bit physical address of RSDT > > */ > > > uint32_t length; /* XSDT Length in bytes including > > hdr */ > > > > you'll need to rebase this on top the latest Michael's pull request. > > [PULL v2 25/30] hw: arm: Carry RSDP specific data through AcpiRsdpData > > [PULL v2 29/30] hw: acpi: Remove AcpiRsdpDescriptor and fix tests > > > > OK. Can I add your Ack-by then? pls note that new AcpiRsdpData has oem_id field that needs the same treatment with rebase Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor; > > > uint32_t length; /* Length of table, in bytes, > > including header */ \ > > > uint8_t revision; /* ACPI Specification minor > > version # */ \ > > > uint8_t checksum; /* To make sum of entire table == > > 0 */ \ > > > - uint8_t oem_id [6]; /* OEM identification */ \ > > > - uint8_t oem_table_id [8]; /* OEM table identification */ \ > > > + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ \ > > > + uint8_t oem_table_id [8] QEMU_NONSTRING; /* OEM table > > identification */ \ > > > uint32_t oem_revision; /* OEM revision number */ \ > > > - uint8_t asl_compiler_id [4]; /* ASL compiler vendor ID */ \ > > > + uint8_t asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor > > ID */ \ > > > uint32_t asl_compiler_revision; /* ASL compiler revision number */ > > > > > > > > > >
On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote: > GCC 8 added a -Wstringop-truncation warning: > > The -Wstringop-truncation warning added in GCC 8.0 via r254630 for > bug 81117 is specifically intended to highlight likely unintended > uses of the strncpy function that truncate the terminating NUL > character from the source string. > > This new warning leads to compilation failures: > > CC hw/acpi/core.o > In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5: > qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation] > strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1 > > Use the QEMU_NONSTRING attribute, since ACPI tables don't require the > strings to be NUL-terminated. Aren't we always starting with zero-initialized structures in ACPI code? If so, then we should be able to change the strncpy's to memcpy's. Thanks, drew > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/acpi/core.c | 8 ++++---- > include/hw/acpi/acpi-defs.h | 8 ++++---- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/hw/acpi/core.c b/hw/acpi/core.c > index aafdc61648..f60f750c3d 100644 > --- a/hw/acpi/core.c > +++ b/hw/acpi/core.c > @@ -35,14 +35,14 @@ > struct acpi_table_header { > uint16_t _length; /* our length, not actual part of the hdr */ > /* allows easier parsing for fw_cfg clients */ > - char sig[4]; /* ACPI signature (4 ASCII characters) */ > + char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) */ > uint32_t length; /* Length of table, in bytes, including header */ > uint8_t revision; /* ACPI Specification minor version # */ > uint8_t checksum; /* To make sum of entire table == 0 */ > - char oem_id[6]; /* OEM identification */ > - char oem_table_id[8]; /* OEM table identification */ > + char oem_id[6] QEMU_NONSTRING; /* OEM identification */ > + char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */ > uint32_t oem_revision; /* OEM revision number */ > - char asl_compiler_id[4]; /* ASL compiler vendor ID */ > + char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */ > uint32_t asl_compiler_revision; /* ASL compiler revision number */ > } QEMU_PACKED; > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index af8e023968..3bf0bec8ba 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -43,7 +43,7 @@ enum { > struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ > uint64_t signature; /* ACPI signature, contains "RSD PTR " */ > uint8_t checksum; /* To make sum of struct == 0 */ > - uint8_t oem_id [6]; /* OEM identification */ > + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ > uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */ > uint32_t rsdt_physical_address; /* 32-bit physical address of RSDT */ > uint32_t length; /* XSDT Length in bytes including hdr */ > @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor; > uint32_t length; /* Length of table, in bytes, including header */ \ > uint8_t revision; /* ACPI Specification minor version # */ \ > uint8_t checksum; /* To make sum of entire table == 0 */ \ > - uint8_t oem_id [6]; /* OEM identification */ \ > - uint8_t oem_table_id [8]; /* OEM table identification */ \ > + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ \ > + uint8_t oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ \ > uint32_t oem_revision; /* OEM revision number */ \ > - uint8_t asl_compiler_id [4]; /* ASL compiler vendor ID */ \ > + uint8_t asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID */ \ > uint32_t asl_compiler_revision; /* ASL compiler revision number */ > > > -- > 2.17.2 > >
Hi Drew, On 12/19/18 11:10 AM, Andrew Jones wrote: > On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote: >> GCC 8 added a -Wstringop-truncation warning: >> >> The -Wstringop-truncation warning added in GCC 8.0 via r254630 for >> bug 81117 is specifically intended to highlight likely unintended >> uses of the strncpy function that truncate the terminating NUL >> character from the source string. >> >> This new warning leads to compilation failures: >> >> CC hw/acpi/core.o >> In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5: >> qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation] >> strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1 >> >> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the >> strings to be NUL-terminated. > > Aren't we always starting with zero-initialized structures in ACPI code? > If so, then we should be able to change the strncpy's to memcpy's. The first call zero-initializes, but then we call realloc(): /* We won't fail from here on. Initialize / extend the globals. */ if (acpi_tables == NULL) { acpi_tables_len = sizeof(uint16_t); acpi_tables = g_malloc0(acpi_tables_len); } acpi_tables = g_realloc(acpi_tables, acpi_tables_len + ACPI_TABLE_PFX_SIZE + sizeof dfl_hdr + body_size); ext_hdr = (struct acpi_table_header *)(acpi_tables + acpi_tables_len); So memcpy() isn't enough. I can resend the previous patch which uses strpadcpy() if you prefer, Igor already reviewed it: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04406.html >> >> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/acpi/core.c | 8 ++++---- >> include/hw/acpi/acpi-defs.h | 8 ++++---- >> 2 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/hw/acpi/core.c b/hw/acpi/core.c >> index aafdc61648..f60f750c3d 100644 >> --- a/hw/acpi/core.c >> +++ b/hw/acpi/core.c >> @@ -35,14 +35,14 @@ >> struct acpi_table_header { >> uint16_t _length; /* our length, not actual part of the hdr */ >> /* allows easier parsing for fw_cfg clients */ >> - char sig[4]; /* ACPI signature (4 ASCII characters) */ >> + char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) */ >> uint32_t length; /* Length of table, in bytes, including header */ >> uint8_t revision; /* ACPI Specification minor version # */ >> uint8_t checksum; /* To make sum of entire table == 0 */ >> - char oem_id[6]; /* OEM identification */ >> - char oem_table_id[8]; /* OEM table identification */ >> + char oem_id[6] QEMU_NONSTRING; /* OEM identification */ >> + char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */ >> uint32_t oem_revision; /* OEM revision number */ >> - char asl_compiler_id[4]; /* ASL compiler vendor ID */ >> + char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */ >> uint32_t asl_compiler_revision; /* ASL compiler revision number */ >> } QEMU_PACKED; >> >> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h >> index af8e023968..3bf0bec8ba 100644 >> --- a/include/hw/acpi/acpi-defs.h >> +++ b/include/hw/acpi/acpi-defs.h >> @@ -43,7 +43,7 @@ enum { >> struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ >> uint64_t signature; /* ACPI signature, contains "RSD PTR " */ >> uint8_t checksum; /* To make sum of struct == 0 */ >> - uint8_t oem_id [6]; /* OEM identification */ >> + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ >> uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */ >> uint32_t rsdt_physical_address; /* 32-bit physical address of RSDT */ >> uint32_t length; /* XSDT Length in bytes including hdr */ >> @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor; >> uint32_t length; /* Length of table, in bytes, including header */ \ >> uint8_t revision; /* ACPI Specification minor version # */ \ >> uint8_t checksum; /* To make sum of entire table == 0 */ \ >> - uint8_t oem_id [6]; /* OEM identification */ \ >> - uint8_t oem_table_id [8]; /* OEM table identification */ \ >> + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ \ >> + uint8_t oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ \ >> uint32_t oem_revision; /* OEM revision number */ \ >> - uint8_t asl_compiler_id [4]; /* ASL compiler vendor ID */ \ >> + uint8_t asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID */ \ >> uint32_t asl_compiler_revision; /* ASL compiler revision number */ >> >> >> -- >> 2.17.2 >> >>
On Wed, Dec 19, 2018 at 01:43:40PM +0100, Philippe Mathieu-Daudé wrote: > Hi Drew, > > On 12/19/18 11:10 AM, Andrew Jones wrote: > > On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote: > >> GCC 8 added a -Wstringop-truncation warning: > >> > >> The -Wstringop-truncation warning added in GCC 8.0 via r254630 for > >> bug 81117 is specifically intended to highlight likely unintended > >> uses of the strncpy function that truncate the terminating NUL > >> character from the source string. > >> > >> This new warning leads to compilation failures: > >> > >> CC hw/acpi/core.o > >> In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5: > >> qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation] > >> strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); > >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1 > >> > >> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the > >> strings to be NUL-terminated. > > > > Aren't we always starting with zero-initialized structures in ACPI code? > > If so, then we should be able to change the strncpy's to memcpy's. > > The first call zero-initializes, but then we call realloc(): > > /* We won't fail from here on. Initialize / extend the globals. */ > if (acpi_tables == NULL) { > acpi_tables_len = sizeof(uint16_t); > acpi_tables = g_malloc0(acpi_tables_len); > } > > acpi_tables = g_realloc(acpi_tables, acpi_tables_len + > ACPI_TABLE_PFX_SIZE + > sizeof dfl_hdr + body_size); > > ext_hdr = (struct acpi_table_header *)(acpi_tables + > acpi_tables_len); > > So memcpy() isn't enough. Ah, thanks. > > I can resend the previous patch which uses strpadcpy() if you prefer, > Igor already reviewed it: > > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04406.html > I do like strpadcpy() better, but I'm not going to lose sleep about this either way it goes. Thanks, drew
On Wed, 19 Dec 2018 14:00:37 +0100 Andrew Jones <drjones@redhat.com> wrote: > On Wed, Dec 19, 2018 at 01:43:40PM +0100, Philippe Mathieu-Daudé wrote: > > Hi Drew, > > > > On 12/19/18 11:10 AM, Andrew Jones wrote: > > > On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote: > > >> GCC 8 added a -Wstringop-truncation warning: > > >> > > >> The -Wstringop-truncation warning added in GCC 8.0 via r254630 for > > >> bug 81117 is specifically intended to highlight likely unintended > > >> uses of the strncpy function that truncate the terminating NUL > > >> character from the source string. > > >> > > >> This new warning leads to compilation failures: > > >> > > >> CC hw/acpi/core.o > > >> In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5: > > >> qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation] > > >> strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); > > >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > >> make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1 > > >> > > >> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the > > >> strings to be NUL-terminated. > > > > > > Aren't we always starting with zero-initialized structures in ACPI code? > > > If so, then we should be able to change the strncpy's to memcpy's. > > > > The first call zero-initializes, but then we call realloc(): > > > > /* We won't fail from here on. Initialize / extend the globals. */ > > if (acpi_tables == NULL) { > > acpi_tables_len = sizeof(uint16_t); > > acpi_tables = g_malloc0(acpi_tables_len); > > } > > > > acpi_tables = g_realloc(acpi_tables, acpi_tables_len + > > ACPI_TABLE_PFX_SIZE + > > sizeof dfl_hdr + body_size); > > > > ext_hdr = (struct acpi_table_header *)(acpi_tables + > > acpi_tables_len); > > > > So memcpy() isn't enough. > > Ah, thanks. > > > > > I can resend the previous patch which uses strpadcpy() if you prefer, > > Igor already reviewed it: > > > > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04406.html > > > > I do like strpadcpy() better, but I'm not going to lose sleep about > this either way it goes. I'm ok with both ways, but v2 consensus was to use QEMU_NONSTRING if I got it right > > Thanks, > drew
On Thu, Dec 20, 2018 at 4:18 PM Igor Mammedov <imammedo@redhat.com> wrote: > > On Wed, 19 Dec 2018 14:00:37 +0100 > Andrew Jones <drjones@redhat.com> wrote: > > > On Wed, Dec 19, 2018 at 01:43:40PM +0100, Philippe Mathieu-Daudé wrote: > > > Hi Drew, > > > > > > On 12/19/18 11:10 AM, Andrew Jones wrote: > > > > On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote: > > > >> GCC 8 added a -Wstringop-truncation warning: > > > >> > > > >> The -Wstringop-truncation warning added in GCC 8.0 via r254630 for > > > >> bug 81117 is specifically intended to highlight likely unintended > > > >> uses of the strncpy function that truncate the terminating NUL > > > >> character from the source string. > > > >> > > > >> This new warning leads to compilation failures: > > > >> > > > >> CC hw/acpi/core.o > > > >> In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5: > > > >> qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation] > > > >> strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); > > > >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > >> make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1 > > > >> > > > >> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the > > > >> strings to be NUL-terminated. > > > > > > > > Aren't we always starting with zero-initialized structures in ACPI code? > > > > If so, then we should be able to change the strncpy's to memcpy's. > > > > > > The first call zero-initializes, but then we call realloc(): > > > > > > /* We won't fail from here on. Initialize / extend the globals. */ > > > if (acpi_tables == NULL) { > > > acpi_tables_len = sizeof(uint16_t); > > > acpi_tables = g_malloc0(acpi_tables_len); > > > } > > > > > > acpi_tables = g_realloc(acpi_tables, acpi_tables_len + > > > ACPI_TABLE_PFX_SIZE + > > > sizeof dfl_hdr + body_size); > > > > > > ext_hdr = (struct acpi_table_header *)(acpi_tables + > > > acpi_tables_len); > > > > > > So memcpy() isn't enough. > > > > Ah, thanks. > > > > > > > > I can resend the previous patch which uses strpadcpy() if you prefer, > > > Igor already reviewed it: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04406.html > > > > > > > I do like strpadcpy() better, but I'm not going to lose sleep about > > this either way it goes. > I'm ok with both ways, but v2 consensus was to use QEMU_NONSTRING if I got it right Yes, MST recommended it because this attribute is clever than strpadcpy().
diff --git a/hw/acpi/core.c b/hw/acpi/core.c index aafdc61648..f60f750c3d 100644 --- a/hw/acpi/core.c +++ b/hw/acpi/core.c @@ -35,14 +35,14 @@ struct acpi_table_header { uint16_t _length; /* our length, not actual part of the hdr */ /* allows easier parsing for fw_cfg clients */ - char sig[4]; /* ACPI signature (4 ASCII characters) */ + char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) */ uint32_t length; /* Length of table, in bytes, including header */ uint8_t revision; /* ACPI Specification minor version # */ uint8_t checksum; /* To make sum of entire table == 0 */ - char oem_id[6]; /* OEM identification */ - char oem_table_id[8]; /* OEM table identification */ + char oem_id[6] QEMU_NONSTRING; /* OEM identification */ + char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */ uint32_t oem_revision; /* OEM revision number */ - char asl_compiler_id[4]; /* ASL compiler vendor ID */ + char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */ uint32_t asl_compiler_revision; /* ASL compiler revision number */ } QEMU_PACKED; diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h index af8e023968..3bf0bec8ba 100644 --- a/include/hw/acpi/acpi-defs.h +++ b/include/hw/acpi/acpi-defs.h @@ -43,7 +43,7 @@ enum { struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ uint64_t signature; /* ACPI signature, contains "RSD PTR " */ uint8_t checksum; /* To make sum of struct == 0 */ - uint8_t oem_id [6]; /* OEM identification */ + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */ uint32_t rsdt_physical_address; /* 32-bit physical address of RSDT */ uint32_t length; /* XSDT Length in bytes including hdr */ @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor; uint32_t length; /* Length of table, in bytes, including header */ \ uint8_t revision; /* ACPI Specification minor version # */ \ uint8_t checksum; /* To make sum of entire table == 0 */ \ - uint8_t oem_id [6]; /* OEM identification */ \ - uint8_t oem_table_id [8]; /* OEM table identification */ \ + uint8_t oem_id [6] QEMU_NONSTRING; /* OEM identification */ \ + uint8_t oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ \ uint32_t oem_revision; /* OEM revision number */ \ - uint8_t asl_compiler_id [4]; /* ASL compiler vendor ID */ \ + uint8_t asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID */ \ uint32_t asl_compiler_revision; /* ASL compiler revision number */
GCC 8 added a -Wstringop-truncation warning: The -Wstringop-truncation warning added in GCC 8.0 via r254630 for bug 81117 is specifically intended to highlight likely unintended uses of the strncpy function that truncate the terminating NUL character from the source string. This new warning leads to compilation failures: CC hw/acpi/core.o In function 'acpi_table_install', inlined from 'acpi_table_add' at qemu/hw/acpi/core.c:296:5: qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation] strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1 Use the QEMU_NONSTRING attribute, since ACPI tables don't require the strings to be NUL-terminated. Suggested-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/acpi/core.c | 8 ++++---- include/hw/acpi/acpi-defs.h | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-)