Re: [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)

[Philippe Mathieu-Daudé]

On 3/4/20 1:51 AM, Philippe Mathieu-Daudé wrote:
> Description copied from Linux kernel commit from Gustavo A. R. Silva
> (see [3]):
>
> --v-- description start --v--
>
>    The current codebase makes use of the zero-length array language
>    extension to the C90 standard, but the preferred mechanism to
>    declare variable-length types such as these ones is a flexible
>    array member [1], introduced in C99:
>
>    struct foo {
>        int stuff;
>        struct boo array[];
>    };
>
>    By making use of the mechanism above, we will get a compiler
>    warning in case the flexible array does not occur last in the
>    structure, which will help us prevent some kind of undefined
>    behavior bugs from being unadvertenly introduced [2] to the
>    Linux codebase from now on.
>
> --^-- description end --^--
>
> Do the similar housekeeping in the QEMU codebase (which uses
> C99 since commit 7be41675f7cb).
>
> All these instances of code were found with the help of the
> following Coccinelle script:
>
>    @@
>    identifier s, a;
>    type T;
>    @@
>     struct s {
>        ...
>    -   T a[0];
>    +   T a[];
>    };
>    @@
>    identifier s, a;
>    type T;
>    @@
>     struct s {
>        ...
>    -   T a[0];
>    +   T a[];
>     } QEMU_PACKED;
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
> [3] 
> https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1
>
> Inspired-by: Gustavo A. R. Silva <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>   bsd-user/qemu.h                       |  2 +-
>   contrib/libvhost-user/libvhost-user.h |  2 +-
>   hw/m68k/bootinfo.h                    |  2 +-
>   hw/scsi/srp.h                         |  6 +++---
>   hw/xen/xen_pt.h                       |  2 +-
>   include/hw/acpi/acpi-defs.h           | 12 ++++++------
>   include/hw/arm/smmu-common.h          |  2 +-
>   include/hw/i386/intel_iommu.h         |  3 ++-
>   include/hw/virtio/virtio-iommu.h      |  2 +-
>   include/sysemu/cryptodev.h            |  2 +-
>   include/tcg/tcg.h                     |  2 +-
>   pc-bios/s390-ccw/bootmap.h            |  2 +-
>   pc-bios/s390-ccw/sclp.h               |  2 +-
>   tests/qtest/libqos/ahci.h             |  2 +-
>   block/linux-aio.c                     |  2 +-
>   hw/acpi/nvdimm.c                      |  6 +++---
>   hw/dma/soc_dma.c                      |  2 +-
>   hw/i386/x86.c                         |  2 +-
>   hw/misc/omap_l4.c                     |  2 +-
>   hw/nvram/eeprom93xx.c                 |  2 +-
>   hw/rdma/vmw/pvrdma_qp_ops.c           |  4 ++--
>   hw/usb/dev-network.c                  |  2 +-
>   hw/usb/dev-smartcard-reader.c         |  4 ++--
>   hw/virtio/virtio.c                    |  4 ++--
>   net/queue.c                           |  2 +-
>   25 files changed, 38 insertions(+), 37 deletions(-)
[...]
> diff --git a/hw/scsi/srp.h b/hw/scsi/srp.h
> index d27f31d2d5..54c954badd 100644
> --- a/hw/scsi/srp.h
> +++ b/hw/scsi/srp.h
> @@ -112,7 +112,7 @@ struct srp_direct_buf {
>   struct srp_indirect_buf {
>       struct srp_direct_buf    table_desc;
>       uint32_t                 len;
> -    struct srp_direct_buf    desc_list[0];
> +    struct srp_direct_buf    desc_list[];
>   } QEMU_PACKED;
>   
>   enum {
> @@ -211,7 +211,7 @@ struct srp_cmd {
>       uint8_t    reserved4;
>       uint8_t    add_cdb_len;
>       uint8_t    cdb[16];
> -    uint8_t    add_data[0];
> +    uint8_t    add_data[];
>   } QEMU_PACKED;
>   
>   enum {
> @@ -241,7 +241,7 @@ struct srp_rsp {
>       uint32_t   data_in_res_cnt;
>       uint32_t   sense_data_len;
>       uint32_t   resp_data_len;
> -    uint8_t    data[0];
> +    uint8_t    data[];
>   } QEMU_PACKED;

hw/scsi/spapr_vscsi.c:69:29: error: field 'iu' with variable sized type 
'union viosrp_iu' not at the end of a struct or class is a GNU extension 
[-Werror,-Wgnu-variable-sized-type-not-at-end]
    union viosrp_iu         iu;
                            ^

Yay we found a bug! Thanks Gustavo :)

union srp_iu {
    struct srp_login_req login_req;
    struct srp_login_rsp login_rsp;
    struct srp_login_rej login_rej;
    struct srp_i_logout i_logout;
    struct srp_t_logout t_logout;
    struct srp_tsk_mgmt tsk_mgmt;
    struct srp_cmd cmd;
    struct srp_rsp rsp;
    uint8_t reserved[SRP_MAX_IU_LEN];
};

union viosrp_iu {
    union srp_iu srp;
    union mad_iu mad;
};

typedef struct vscsi_req {
    vscsi_crq               crq;
    union viosrp_iu         iu;

    /* SCSI request tracking */
    SCSIRequest             *sreq;
    uint32_t                qtag; /* qemu tag != srp tag */
    bool                    active;
    bool                    writing;
    bool                    dma_error;
    uint32_t                data_len;
    uint32_t                senselen;
    uint8_t                 sense[SCSI_SENSE_BUF_SIZE];

    /* RDMA related bits */
    uint8_t                 dma_fmt;
    uint16_t                local_desc;
    uint16_t                total_desc;
    uint16_t                cdb_offset;
    uint16_t                cur_desc_num;
    uint16_t                cur_desc_offset;
} vscsi_req;

>   
>   #endif /* SCSI_SRP_H */
[...]