Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Igor Mammedov <imammedo@redhat.com>]

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