| Message ID | 20200225002746.GA26789@embeddedor (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a1b7714b72fd2ba5dead01b465a9e37863051eff |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [next] wireless: realtek: Replace zero-length array with flexible-array member | expand |
On 2/24/20 7:27 PM, Gustavo A. R. Silva wrote: > 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][2], > 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 > inadvertently introduced[3] to the codebase from now on. > > Also, notice that, dynamic memory allocations won't be affected by > this change: > > "Flexible array members have incomplete type, and so the sizeof operator > may not be applied. As a quirk of the original implementation of > zero-length arrays, sizeof evaluates to zero."[1] > > This issue was found with the help of Coccinelle. Hi Gustavo, I really don't think this improves the code in any way for the drivers you are modifying. If we really want to address this corner case, it seems like fixing the compiler to address [0] arrays the same as [] arrays is the right solution. Cheers, Jes
"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes: > 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][2], > 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 > inadvertently introduced[3] to the codebase from now on. Preferred by who exactly?
"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote: > 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][2], > 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 > inadvertently introduced[3] to the codebase from now on. > > Also, notice that, dynamic memory allocations won't be affected by > this change: > > "Flexible array members have incomplete type, and so the sizeof operator > may not be applied. As a quirk of the original implementation of > zero-length arrays, sizeof evaluates to zero."[1] > > This issue was found with the help of Coccinelle. > > [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html > [2] https://github.com/KSPP/linux/issues/21 > [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour") > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> Patch applied to wireless-drivers-next.git, thanks. a1b7714b72fd wireless: realtek: Replace zero-length array with flexible-array member
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h index 6598c8d786ea..440d164443bc 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h @@ -627,7 +627,7 @@ struct rtl8xxxu_firmware_header { u32 reserved4; u32 reserved5; - u8 data[0]; + u8 data[]; }; /* diff --git a/drivers/net/wireless/realtek/rtlwifi/wifi.h b/drivers/net/wireless/realtek/rtlwifi/wifi.h index 1cff9f07c9e9..13421cf2d201 100644 --- a/drivers/net/wireless/realtek/rtlwifi/wifi.h +++ b/drivers/net/wireless/realtek/rtlwifi/wifi.h @@ -1051,13 +1051,13 @@ struct rtl_hdr_3addr { u8 addr2[ETH_ALEN]; u8 addr3[ETH_ALEN]; __le16 seq_ctl; - u8 payload[0]; + u8 payload[]; } __packed; struct rtl_info_element { u8 id; u8 len; - u8 data[0]; + u8 data[]; } __packed; struct rtl_probe_rsp { @@ -1068,7 +1068,7 @@ struct rtl_probe_rsp { /*SSID, supported rates, FH params, DS params, * CF params, IBSS params, TIM (if beacon), RSN */ - struct rtl_info_element info_element[0]; + struct rtl_info_element info_element[]; } __packed; /*LED related.*/ diff --git a/drivers/net/wireless/realtek/rtw88/fw.h b/drivers/net/wireless/realtek/rtw88/fw.h index ccd27bd45775..414827800a5f 100644 --- a/drivers/net/wireless/realtek/rtw88/fw.h +++ b/drivers/net/wireless/realtek/rtw88/fw.h @@ -36,7 +36,7 @@ enum rtw_c2h_cmd_id_ext { struct rtw_c2h_cmd { u8 id; u8 seq; - u8 payload[0]; + u8 payload[]; } __packed; enum rtw_rsvd_packet_type { diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h index c074cef22120..46c0ebceb177 100644 --- a/drivers/net/wireless/realtek/rtw88/main.h +++ b/drivers/net/wireless/realtek/rtw88/main.h @@ -1641,7 +1641,7 @@ struct rtw_dev { struct rtw_wow_param wow; /* hci related data, must be last */ - u8 priv[0] __aligned(sizeof(void *)); + u8 priv[] __aligned(sizeof(void *)); }; #include "hci.h"
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][2], 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 inadvertently introduced[3] to the codebase from now on. Also, notice that, dynamic memory allocations won't be affected by this change: "Flexible array members have incomplete type, and so the sizeof operator may not be applied. As a quirk of the original implementation of zero-length arrays, sizeof evaluates to zero."[1] This issue was found with the help of Coccinelle. [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html [2] https://github.com/KSPP/linux/issues/21 [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour") Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 2 +- drivers/net/wireless/realtek/rtlwifi/wifi.h | 6 +++--- drivers/net/wireless/realtek/rtw88/fw.h | 2 +- drivers/net/wireless/realtek/rtw88/main.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-)