tests: Bluetooth: Add some more tests for bt_le_create_conn() #78284

rugeGerritsen · 2024-09-11T12:13:54Z

Adds coverage for attempting to connect while already connecting or connecting to a device that already exists.

rugeGerritsen · 2024-09-11T12:17:14Z

tests/bsim/bluetooth/host/central/src/main.c

+
+	TEST_ASSERT(initial_refs >= 1, "Expect to have at least once reference");
+
+	err = bt_conn_le_create(&peer, &create_param, BT_LE_CONN_PARAM_DEFAULT, &conn);


Here the API doesn't protect the user from providing a connection object that still has references to it. I don't think this can be handled with todays implementation unless we change the API to require conn to point to NULL.

I believe this can be a source of subtle bugs.

I agree. This is why testlib (which btw is made to work on target) does this. And I now think it's worth the API break to start enforcing this in core API.

zephyr/tests/bluetooth/common/testlib/include/testlib/conn.h

Lines 30 to 31 in 884a4e5

* @note The reference variable @p connp is required be empty (i.e.

* NULL) on entry.

I don't think it requires an API change.

Isn't to possible to just do something along the lines of

if (ret_conn != NULL && atomic_get((*ret_conn)->ref) != 0) { return -EALREADY; }

isn't that -EALREADY an API change though?

isn't that -EALREADY an API change though?

Yeah, but not a breaking API change IMO, although that is clearly debatable :)

@hermabe , you are right. The code sample was wrong. I updated the code to declare conn as a pointer.

Your point still stands with the updated sample. struct bt_conn *conn; may point to garbage so **ret_conn cannot be used.

When declaring struct bt_conn *conn;, it can happen that it points to something inside acl_conns, right?

Technically, yes if it's uninitialized. IS_ARRAY_ELEMENT should also check whether it's aligned properly, so even if it's within the array, if it's not aligned properly it should still work.

If it happens to actually match a connection object pointer directly, then it should also be OK as we can then treat it as a proper bt_conn pointer and check the reference counter

If it happens to actually match a connection object pointer directly, then it should also be OK as we can then treat it as a proper bt_conn pointer and check the reference counter

I would argue that we shouldn't rely on the undefined initialization value of a pointer.

Lets say the application is configured to support two connections.

int err; struct bt_conn *conn1 = NULL; err = bt_conn_le_create(..., &conn1); /* ... establish connection 1 */ int err; struct bt_conn *conn2; /* By chance this is initialized to conn1 */ err = bt_conn_le_create(..., &conn2); /* This call will now fail */

I assume that is not intended.

If it happens to actually match a connection object pointer directly, then it should also be OK as we can then treat it as a proper bt_conn pointer and check the reference counter

I would argue that we shouldn't rely on the undefined initialization value of a pointer.

Lets say the application is configured to support two connections.

int err; struct bt_conn *conn1 = NULL; err = bt_conn_le_create(..., &conn1); /* ... establish connection 1 */ int err; struct bt_conn *conn2; /* By chance this is initialized to conn1 */ err = bt_conn_le_create(..., &conn2); /* This call will now fail */

I assume that is not intended.

That is a fair point. Ideally it shouldn't be necessary to initialize an [out] pointer, and there's no way to check that at runtime I believe.

The above case would be a very rare instance though.

tests/bsim/bluetooth/host/central/src/dummy_peripheral.c

tests/bsim/bluetooth/host/central/src/main.c

tests/bsim/bluetooth/host/central/src/dummy_peripheral.c

tests/bsim/bluetooth/host/central/src/main.c

Adds coverage for attempting to connect while already connecting or connecting to a device that already exists. Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>

This commit adds a warning and a Kconfig option to `bt_conn_le_create` and `bt_conn_le_create_synced` functions which are meant to warn a user of a potential leakage of an active connection object. This change is implemented due to frequent incorrect use of the connection pointer where a pointer to an existing connection object is overwritten by `bt_conn_le_create` and `bt_conn_le_create_synced` functions which in turns leads to sporadic critical bugs. See zephyrproject-rtos#78284 (comment) for details. The Kconfig option is introduced instead of always returning the error to not affect current implementations. However, it is recommended to keep this option enabled to avoid potential bugs. Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no>

This commit adds a warning and a Kconfig option to `bt_conn_le_create` and `bt_conn_le_create_synced` functions which are meant to warn a user of a potential leakage of an active connection object. This change is implemented due to frequent incorrect use of the connection pointer where a pointer to an existing connection object is overwritten by `bt_conn_le_create` and `bt_conn_le_create_synced` functions which in turns leads to sporadic critical bugs. See zephyrproject-rtos#78284 (comment) for more details. The Kconfig option is introduced instead of always returning the error to not affect current implementations. However, it is recommended to keep this option enabled to avoid potential bugs. Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no>

This commit adds a warning and a Kconfig option to `bt_conn_le_create` and `bt_conn_le_create_synced` functions which are meant to warn a user of a potential leakage of an active connection object. This change is implemented due to frequent incorrect use of the connection pointer where a pointer to an existing connection object is overwritten by `bt_conn_le_create` and `bt_conn_le_create_synced` functions which in turns leads to sporadic critical bugs. See #78284 (comment) for more details. The Kconfig option is introduced instead of always returning the error to not affect current implementations. However, it is recommended to keep this option enabled to avoid potential bugs. Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no>

This commit adds a warning and a Kconfig option to `bt_conn_le_create` and `bt_conn_le_create_synced` functions which are meant to warn a user of a potential leakage of an active connection object. This change is implemented due to frequent incorrect use of the connection pointer where a pointer to an existing connection object is overwritten by `bt_conn_le_create` and `bt_conn_le_create_synced` functions which in turns leads to sporadic critical bugs. See zephyrproject-rtos/zephyr#78284 (comment) for more details. The Kconfig option is introduced instead of always returning the error to not affect current implementations. However, it is recommended to keep this option enabled to avoid potential bugs. Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no> (cherry picked from commit 8acb1cc)

This commit adds a warning and a Kconfig option to `bt_conn_le_create` and `bt_conn_le_create_synced` functions which are meant to warn a user of a potential leakage of an active connection object. This change is implemented due to frequent incorrect use of the connection pointer where a pointer to an existing connection object is overwritten by `bt_conn_le_create` and `bt_conn_le_create_synced` functions which in turns leads to sporadic critical bugs. See zephyrproject-rtos/zephyr#78284 (comment) for more details. The Kconfig option is introduced instead of always returning the error to not affect current implementations. However, it is recommended to keep this option enabled to avoid potential bugs. (cherry picked from commit 8acb1cc) Original-Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no> GitOrigin-RevId: 8acb1cc Cr-Build-Id: 8733565196572304993 Cr-Build-Url: https://cr-buildbucket.appspot.com/build/8733565196572304993 Copybot-Job-Name: zephyr-main-copybot-downstream Change-Id: I6987d4437fa20c66933aa467e78dc9a0bf234d0b Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5944414 Reviewed-by: Fabio Baltieri <fabiobaltieri@google.com> Commit-Queue: Ting Shen <phoenixshen@chromium.org> Tested-by: Ting Shen <phoenixshen@chromium.org> Reviewed-by: Dawid Niedźwiecki <dawidn@google.com>

This commit adds a warning and a Kconfig option to `bt_conn_le_create` and `bt_conn_le_create_synced` functions which are meant to warn a user of a potential leakage of an active connection object. This change is implemented due to frequent incorrect use of the connection pointer where a pointer to an existing connection object is overwritten by `bt_conn_le_create` and `bt_conn_le_create_synced` functions which in turns leads to sporadic critical bugs. See zephyrproject-rtos/zephyr#78284 (comment) for more details. The Kconfig option is introduced instead of always returning the error to not affect current implementations. However, it is recommended to keep this option enabled to avoid potential bugs. Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no> (cherry picked from commit 8acb1cc)

rugeGerritsen commented Sep 11, 2024

View reviewed changes

rugeGerritsen force-pushed the bt-conn-create-tests branch 2 times, most recently from f6719b1 to 90c16c9 Compare September 11, 2024 12:21

rugeGerritsen marked this pull request as ready for review September 11, 2024 14:01

zephyrbot added area: Bluetooth Host platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim area: Bluetooth labels Sep 11, 2024

zephyrbot requested review from aescolar, alwa-nordic, hermabe, jhedberg, jori-nordic, sjanc, Thalley and theob-pro September 11, 2024 14:02

zephyrbot assigned jori-nordic and jhedberg Sep 11, 2024

rugeGerritsen marked this pull request as draft September 11, 2024 14:42

rugeGerritsen force-pushed the bt-conn-create-tests branch from 90c16c9 to f49a095 Compare September 12, 2024 07:53

rugeGerritsen marked this pull request as ready for review September 12, 2024 09:28

Thalley requested changes Sep 12, 2024

View reviewed changes

rugeGerritsen force-pushed the bt-conn-create-tests branch from f49a095 to 4407315 Compare September 12, 2024 14:13

Thalley reviewed Sep 12, 2024

View reviewed changes

tests/bsim/bluetooth/host/central/src/dummy_peripheral.c Outdated Show resolved Hide resolved

tests/bsim/bluetooth/host/central/src/main.c Outdated Show resolved Hide resolved

tests: Bluetooth: Add some more tests for bt_le_create_conn()

526cd4c

Adds coverage for attempting to connect while already connecting or connecting to a device that already exists. Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>

rugeGerritsen force-pushed the bt-conn-create-tests branch from 4407315 to 526cd4c Compare September 17, 2024 06:19

rugeGerritsen requested a review from Thalley September 17, 2024 06:19

Thalley approved these changes Sep 17, 2024

View reviewed changes

jori-nordic approved these changes Sep 17, 2024

View reviewed changes

mmahadevan108 merged commit f6712c4 into zephyrproject-rtos:main Sep 17, 2024
25 checks passed

PavelVPV mentioned this pull request Oct 15, 2024

bluetooth: host: conn: Check if *conn is not NULL #79856

Merged

aescolar mentioned this pull request Oct 22, 2024

Nominate rugeGerritsen as collaborator #80233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Bluetooth: Add some more tests for bt_le_create_conn() #78284

tests: Bluetooth: Add some more tests for bt_le_create_conn() #78284

rugeGerritsen commented Sep 11, 2024

rugeGerritsen Sep 11, 2024 •

edited

Loading

alwa-nordic Sep 11, 2024

Thalley Sep 11, 2024

jori-nordic Sep 12, 2024

Thalley Sep 12, 2024

rugeGerritsen Sep 12, 2024

hermabe Sep 12, 2024

Thalley Sep 12, 2024

rugeGerritsen Sep 12, 2024

Thalley Sep 12, 2024


		TEST_ASSERT(initial_refs >= 1, "Expect to have at least once reference");

		err = bt_conn_le_create(&peer, &create_param, BT_LE_CONN_PARAM_DEFAULT, &conn);

	* @note The reference variable @p connp is required be empty (i.e.
	* NULL) on entry.

tests: Bluetooth: Add some more tests for bt_le_create_conn() #78284

tests: Bluetooth: Add some more tests for bt_le_create_conn() #78284

Conversation

rugeGerritsen commented Sep 11, 2024

rugeGerritsen Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugeGerritsen Sep 11, 2024 •

edited

Loading