Re: [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
Re: [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.
Re: [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
Re: [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: [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;
>
