flex array: Replace 0 length arrays #95134

ceolin · 2025-08-29T01:04:59Z

Replace 0 length arrays declaration with proper C99 flexible arrays.

cvinayak · 2025-08-29T07:29:01Z

subsys/bluetooth/controller/ll_sw/openisa/lll/pdu_vendor.h

-	union {
-		uint8_t resv[0];
-	} __packed;
+	FLEXIBLE_ARRAY_DECLARE(uint8_t, resv);


I have concern (not verified) that __packed has been removed by the introduction of FLEXIBLE_ARRAY_DECLARE. Is this truly equivalent?

I see prior PRs have already change some in subsys/bluetooth/controller/ll_sw/pdu.h file.

Can we have a __packed variant of this "workaround" define, like FLEXIBLE_ARRAY_DECLARE_PACKED?

Out of curiosity, did __packed for an array of size 0 really do anything?

And even if it wasn't of size 0, since the elements are byte aligned, I don't think __packed would do anything at all.

__packed struct or union when nested need __packed individually every time; besides themselves being packed, the parent struct or union which has __packed still need to include them without which the members are machine word aligned/sized (please run a sample on Cortex-M0 nRF51 and you will hit hardfaults based on how linking has been unlucky... i.e. increasing/decreasing buffer counts will misalign these structs).

This file is not the best example, as the union has only one zero length byte array.

But here :

zephyr/subsys/bluetooth/controller/ll_sw/nordic/lll/pdu_vendor.h

Lines 17 to 25 in e028ecf

struct pdu_data_vnd_octet3 {

union {

uint8_t resv[OCTET3_LEN]; /* nRF specific octet3 required for NRF_CCM use */

#if !defined(CONFIG_BT_CTLR_DATA_LENGTH_CLEAR)

struct pdu_cte_info cte_info; /* BT 5.1 Core spec. CTEInfo storage */

#endif /* !CONFIG_BT_CTLR_DATA_LENGTH_CLEAR */

} __packed;

} __packed;

The union needs __packed so that it take 1 byte and not 4 bytes in size.

It is best to have a FLEXIBLE_ARRAY_DECLARE_PACKED for code that had __packed for the union inside other __packed union/structs... and also where FLEXIBLE_ARRAY_DECLARE added new union/struct into __packed structs/unions. Hence to also correct the other PR that made changes to as here: 1a56b90

There is also zero length array alignment requirements for nRF51: 701d524

I think this example is wrong. With/without packed you will get 4 bytes if CONFIG_BT_CTLR_DATA_LENGTH_CLEAR is not defined. And since this union is the only element of the structure pdu_data_vnd_octet3 that is packed you will get 1 byte size when CONFIG_BT_CTLR_DATA_LENGTH_CLEAR is defined.

If you noticed, I have just changed for this specific use case where the union.

I am wondering if I am missing something here though. Did you get any memory alignment issue ? Printing the struct sizes I get exactly the same thing and the alignment should be correct.

ok... I did a quick diff of .lst file of a sample for BBC microbit board (adding CONFIG_OUTPUT_DISASSEMBLY=y), with and without 56e2b01, and I see no difference. I am ok for now.

cvinayak · 2025-08-29T10:53:52Z

subsys/bluetooth/controller/ll_sw/openisa/lll/pdu_vendor.h

-	union {
-		uint8_t resv[0];
-	} __packed;
+	FLEXIBLE_ARRAY_DECLARE(uint8_t, resv);


__packed struct or union when nested need __packed individually every time; besides themselves being packed, the parent struct or union which has __packed still need to include them without which the members are machine word aligned/sized (please run a sample on Cortex-M0 nRF51 and you will hit hardfaults based on how linking has been unlucky... i.e. increasing/decreasing buffer counts will misalign these structs).

This file is not the best example, as the union has only one zero length byte array.

But here :

zephyr/subsys/bluetooth/controller/ll_sw/nordic/lll/pdu_vendor.h

Lines 17 to 25 in e028ecf

struct pdu_data_vnd_octet3 {

union {

uint8_t resv[OCTET3_LEN]; /* nRF specific octet3 required for NRF_CCM use */

#if !defined(CONFIG_BT_CTLR_DATA_LENGTH_CLEAR)

struct pdu_cte_info cte_info; /* BT 5.1 Core spec. CTEInfo storage */

#endif /* !CONFIG_BT_CTLR_DATA_LENGTH_CLEAR */

} __packed;

} __packed;

The union needs __packed so that it take 1 byte and not 4 bytes in size.

It is best to have a FLEXIBLE_ARRAY_DECLARE_PACKED for code that had __packed for the union inside other __packed union/structs... and also where FLEXIBLE_ARRAY_DECLARE added new union/struct into __packed structs/unions. Hence to also correct the other PR that made changes to as here: 1a56b90

There is also zero length array alignment requirements for nRF51: 701d524

cvinayak · 2025-08-29T11:06:29Z

tests/bluetooth/tester/src/btp/btp_ots.h

 #define BTP_OTS_READ_SUPPORTED_COMMANDS	0x01
 struct btp_ots_read_supported_commands_rp {
-	uint8_t data[0];
+	FLEXIBLE_ARRAY_DECLARE(uint8_t, data);


Did you use a script to do data[0] changes?

Below name[0] is not using FLEXIBLE_ARRAY_DECLARE :-)

Please revert to using uint8_t data[]; instead.

IIRC C++ does not allow empty structs (i.e. struct with only uint8_t data[];, so that's why we need to use FLEXIBLE_ARRAY_DECLARE if a struct only contains a flexible array

@cvinayak it is exactly what @Thalley mentioned. I don't bother to change it if that is not a problem.

IIRC C++ does not allow empty structs

@Thalley this is not documented in FLEXIBLE_ARRAY_DECLARE as a workaround for.

* Since C99, flexible arrays are part of the C standard, but for historical * reasons many places still use an older GNU extension that is declare * zero length arrays. * * Although zero length arrays are flexible arrays, we can't blindly * replace [0] with [] because of some syntax limitations. This macro * workaround these limitations.

@ceolin I am not happy polluting the source with workarounds (more of Zephyr macro magics to make code convoluted, and "steep learning curve" when it comes to Zephyr code base), as clearly the documentation conveys that the macro is a workaround.

Well, for many reasons, redesigns/refactoring increasing CPU usages, compiler instruction re-ordering, bugs included, nRF51 support is broken over the times now (#74345).

@cvinayak I see your point, unfortunately I don't see a better way to do it, at least not while we are supporting C++. Also, this macro is needed when using flexible arrays in unions.

There is always a risk of introducing issues for sure, but there are many benefits as well. Doing these changes I found some places where flexible arrays were not in the end of the structure and more importantly it will allow us to enable new compilation options to check some type out-of-bound accesses that is the majority cause of security issues.

cvinayak · 2025-08-30T04:33:44Z

Convinced for now, that the changes (did not check this particular PR changes, but only with 56e2b01) do not show any difference in generated code for a bbc microbit board sample with the Bluetooth Controller.

@thedjnK Cool experiment as part of this PR: #95191

My request changes can be dismissed (when conflicts are resolved in this PR).

0 length array is a GNU extension. Use proper C99 flexible array. Signed-off-by: Flavio Ceolin <[email protected]>

sonarqubecloud · 2025-09-03T04:52:05Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ceolin · 2025-09-03T05:19:37Z

@jhedberg @cvinayak @Thalley would you mind take another look ?

Thalley · 2025-09-03T07:02:36Z

@jhedberg @cvinayak @Thalley would you mind take another look ?

At least for me, it's easier if you just press the "Re-request review" button, so that it will show up in "Review requests" tab on Github :)

No concerns from my side.

zephyrbot added area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests area: Bluetooth Controller area: Bluetooth Audio area: Bluetooth Classic Bluetooth Classic (BR/EDR) labels Aug 29, 2025

github-project-automation bot added this to Bluetooth LE Audio Aug 29, 2025

github-project-automation bot moved this to In Review in Bluetooth LE Audio Aug 29, 2025

zephyrbot added the area: Bluetooth label Aug 29, 2025

zephyrbot requested review from Thalley, asbjornsabo, babrsn, carlescufi, cvinayak, erbr-ot, fredrikdanebjer, jhedberg, jthm-ot, larsgk, niym-ot, ppryga, sjanc, thoh-ot and wopu-ot August 29, 2025 01:06

zephyrbot assigned sjanc Aug 29, 2025

jhedberg previously approved these changes Aug 29, 2025

View reviewed changes

cvinayak previously requested changes Aug 29, 2025

View reviewed changes

Thalley previously approved these changes Aug 29, 2025

View reviewed changes

cvinayak reviewed Aug 29, 2025

View reviewed changes

ceolin added 2 commits September 2, 2025 21:37

bt: audio: Use proper flexible array

0b41495

0 length array is a GNU extension. Use proper C99 flexible array. Signed-off-by: Flavio Ceolin <[email protected]>

lib: heap: Use proper flexible array

4e046c1

0 length array is a GNU extension. Use proper C99 flexible array. Signed-off-by: Flavio Ceolin <[email protected]>

ceolin added 5 commits September 2, 2025 21:37

bt: controller/util: Use proper flexible array

0ca4f46

0 length array is a GNU extension. Use proper C99 flexible array. Signed-off-by: Flavio Ceolin <[email protected]>

tests: bt/tester: Use proper flexible array

9be698b

0 length array is a GNU extension. Use proper C99 flexible array. Signed-off-by: Flavio Ceolin <[email protected]>

bt: host/classic: Use proper flexible array

6d3965f

0 length array is a GNU extension. Use proper C99 flexible array. Signed-off-by: Flavio Ceolin <[email protected]>

bt: controller/openisa: Use proper flexible array

fe60526

0 length array is a GNU extension. Use proper C99 flexible array. Signed-off-by: Flavio Ceolin <[email protected]>

bt: host/classic: Use proper flexible array

0df79cf

0 length array is a GNU extension. Use proper C99 flexible array. Signed-off-by: Flavio Ceolin <[email protected]>

ceolin dismissed stale reviews from Thalley and jhedberg via 0df79cf September 3, 2025 04:47

ceolin force-pushed the security/flex-array branch from e028ecf to 0df79cf Compare September 3, 2025 04:47

Thalley approved these changes Sep 3, 2025

View reviewed changes

jhedberg approved these changes Sep 3, 2025

View reviewed changes

jhedberg assigned jhedberg and alwa-nordic Sep 3, 2025

jhedberg merged commit 9fdbe5f into zephyrproject-rtos:main Sep 3, 2025
36 of 39 checks passed

github-project-automation bot moved this from In Review to Done in Bluetooth LE Audio Sep 3, 2025

jhedberg mentioned this pull request Oct 7, 2025

bluetooth: hci: userchan: Improve RX allocation handling #97091

Merged

	struct pdu_data_vnd_octet3 {
	union {
	uint8_t resv[OCTET3_LEN]; /* nRF specific octet3 required for NRF_CCM use */

	#if !defined(CONFIG_BT_CTLR_DATA_LENGTH_CLEAR)
	struct pdu_cte_info cte_info; /* BT 5.1 Core spec. CTEInfo storage */
	#endif /* !CONFIG_BT_CTLR_DATA_LENGTH_CLEAR */
	} __packed;
	} __packed;

flex array: Replace 0 length arrays #95134

flex array: Replace 0 length arrays #95134

Uh oh!

Conversation

ceolin commented Aug 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvinayak Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvinayak Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvinayak Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvinayak commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Sep 3, 2025

Quality Gate passed

Uh oh!

ceolin commented Sep 3, 2025

Uh oh!

Thalley commented Sep 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cvinayak Aug 29, 2025 •

edited

Loading

cvinayak Aug 29, 2025 •

edited

Loading

cvinayak Aug 29, 2025 •

edited

Loading

cvinayak commented Aug 30, 2025 •

edited

Loading