Re: [Xen-devel] [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)
On 04/03/20 15:12, Philippe Mathieu-Daudé wrote: > I'll send a fix for the dangerous code. > Do you want to drop this series, or only the change in 'struct srp_rsp' > (or in all hw/scsi/srp.h). Actually I guess it makes sense I move the > 'hw/scsi/srp.h' changes with the series cleaning dangerous code. As you prefer, it's not urgent to merge it. Paolo ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)
On 3/4/20 2:44 PM, Paolo Bonzini wrote: On 04/03/20 14:12, Philippe Mathieu-Daudé wrote: 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]; }; It's variable-sized but it's okay as long as the total size doesn't exceed SRP_MAX_IU_LEN. So it's not a bug, but I agree it's a time bomb. Moving the field last should work, but it would still be quite dangerous code. Yeah I reached the same conclusion. I'll send a fix for the dangerous code. Do you want to drop this series, or only the change in 'struct srp_rsp' (or in all hw/scsi/srp.h). Actually I guess it makes sense I move the 'hw/scsi/srp.h' changes with the series cleaning dangerous code. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)
On 04/03/20 14:12, Philippe Mathieu-Daudé wrote: > > 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]; > }; It's variable-sized but it's okay as long as the total size doesn't exceed SRP_MAX_IU_LEN. So it's not a bug, but I agree it's a time bomb. Moving the field last should work, but it would still be quite dangerous code. Paolo ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)
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 Signed-off-by: Philippe Mathieu-Daudé --- 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_buftable_desc; uint32_t len; -struct srp_direct_bufdesc_list[0]; +struct srp_direct_bufdesc_list[]; } QEMU_PACKED; enum { @@ -211,7 +211,7 @@ struct srp_cmd { uint8_treserved4; uint8_tadd_cdb_len; uint8_tcdb[16]; -uint8_tadd_data[0]; +uint8_tadd_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_tdata[0]; +uint8_tdata[]; } 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_tqtag; /* qemu tag != srp tag */ boolactive; boolwriting; booldma_error; uint32_tdata_len; uint32_tsenselen; uint8_t sense[SCSI_SENSE_BUF_SIZE]; /* RDMA related bits */ uint8_t dma_fmt; uint16_tlocal_desc; uint16_ttotal_desc; uint16_tcdb_offset; uint16_tcur_desc_num; uint16_t
Re: [Xen-devel] [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)
On 04.03.20 01:51, 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 > Signed-off-by: Philippe Mathieu-Daudé > --- > 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/bsd-user/qemu.h b/bsd-user/qemu.h > index 09e8aed9c7..f8bb1e5459 100644 > --- a/bsd-user/qemu.h > +++ b/bsd-user/qemu.h > @@ -95,7 +95,7 @@ typedef struct TaskState { > struct sigqueue *first_free; /* first free siginfo queue entry */ > int signal_pending; /* non zero if a signal may be pending */ > > -uint8_t stack[0]; > +uint8_t stack[]; > } __attribute__((aligned(16))) TaskState; > > void init_task_state(TaskState *ts); > diff --git a/contrib/libvhost-user/libvhost-user.h > b/contrib/libvhost-user/libvhost-user.h > index 6fc8000e99..f30394fab6 100644 > --- a/contrib/libvhost-user/libvhost-user.h > +++ b/contrib/libvhost-user/libvhost-user.h > @@ -286,7 +286,7 @@ typedef struct VuVirtqInflight { > uint16_t used_idx; > > /* Used to track the state of each descriptor in descriptor table */ > -VuDescStateSplit desc[0]; > +VuDescStateSplit desc[]; > } VuVirtqInflight; > > typedef struct VuVirtqInflightDesc { > diff --git a/hw/m68k/bootinfo.h b/hw/m68k/bootinfo.h > index 5f8ded2686..c954270aad 100644 > --- a/hw/m68k/bootinfo.h > +++ b/hw/m68k/bootinfo.h > @@ -14,7 +14,7 @@ > struct bi_record { > uint16_t tag;/* tag ID */ > uint16_t size; /* size of record */ > -uint32_t data[0];/* data */ > +uint32_t data[]; /* data */ > }; > > /* machine independent tags */ > 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_buftable_desc; > uint32_t len; > -struct srp_direct_bufdesc_list[0]; > +struct srp_direct_bufdesc_list[]; > } QEMU_PACKED; > > enum { > @@ -211,7 +211,7 @@ struct srp_cmd { > uint8_treserved4; > uint8_tadd_cdb_len; > uint8_tcdb[16]; > -uint8_tadd_data[0]; > +uint8_tadd_data[]; > } QEMU_PACKED; > > enum { > @@ -241,7 +241,7 @@ struct srp_rsp { > uint32_t data_in_res_cnt; >