From 57d7fc76e8c96948e76df32399dd49590ac6d5c9 Mon Sep 17 00:00:00 2001 From: Pavel Vasilyev Date: Wed, 9 Oct 2024 14:50:43 +0200 Subject: [PATCH] bluetooth: host: conn: Check if *conn is not NULL 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 https://github.com/zephyrproject-rtos/zephyr/pull/78284#discussion_r1754304535 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 --- .../bluetooth/api/connection_mgmt.rst | 8 +++++ doc/releases/release-notes-4.0.rst | 7 ++++ subsys/bluetooth/host/Kconfig | 11 +++++++ subsys/bluetooth/host/conn.c | 32 +++++++++++++++++++ 4 files changed, 58 insertions(+) diff --git a/doc/connectivity/bluetooth/api/connection_mgmt.rst b/doc/connectivity/bluetooth/api/connection_mgmt.rst index 3a3d8d02a8278f2..1b7ea2788c3f291 100644 --- a/doc/connectivity/bluetooth/api/connection_mgmt.rst +++ b/doc/connectivity/bluetooth/api/connection_mgmt.rst @@ -15,6 +15,14 @@ longer period of time, since this ensures that the object remains valid :c:func:`bt_conn_unref` API is to be used when releasing a reference to a connection. +A common mistake is to forget unreleasing a reference to a connection +object created by functions :c:func:`bt_conn_le_create` and +:c:func:`bt_conn_le_create_synced`. To protect against this, use the +:kconfig:option:`CONFIG_BT_CONN_CHECK_NULL_BEFORE_CREATE` Kconfig option, +which forces these functions return an error if the connection pointer +passed to them is not NULL. This helps to spot such issues and avoid +sporadic bugs caused by not releasing the connection object. + An application may track connections by registering a :c:struct:`bt_conn_cb` struct using the :c:func:`bt_conn_cb_register` or :c:macro:`BT_CONN_CB_DEFINE` APIs. This struct lets the application diff --git a/doc/releases/release-notes-4.0.rst b/doc/releases/release-notes-4.0.rst index a00d66cb9bd0444..ad3aa1965c606fc 100644 --- a/doc/releases/release-notes-4.0.rst +++ b/doc/releases/release-notes-4.0.rst @@ -108,6 +108,13 @@ Bluetooth * Added API :c:func:`bt_gatt_get_uatt_mtu` to get current Unenhanced ATT MTU of a given connection (experimental). + * Added a warning to :c:func:`bt_conn_le_create` and :c:func:`bt_conn_le_create_synced` if + the connection pointer passed as an argument is not NULL. + + * Added Kconfig option :kconfig:option:`CONFIG_BT_CONN_CHECK_NULL_BEFORE_CREATE` to enforce + :c:func:`bt_conn_le_create` and :c:func:`bt_conn_le_create_synced` return an error if the + connection pointer passed as an argument is not NULL. + * HCI Drivers Boards & SoC Support diff --git a/subsys/bluetooth/host/Kconfig b/subsys/bluetooth/host/Kconfig index 05214f224b13306..d7da58d7def8b89 100644 --- a/subsys/bluetooth/host/Kconfig +++ b/subsys/bluetooth/host/Kconfig @@ -314,6 +314,17 @@ config BT_CONN_PARAM_ANY min and max connection intervals in order to verify whether the desired parameters have been established in the connection. +config BT_CONN_CHECK_NULL_BEFORE_CREATE + bool "Check if *conn is NULL when creating a connection" + help + Enable this option to ensure that bt_conn_le_create and + bt_conn_le_create_synced return an error if *conn is not initialized + to NULL. This option is recommended to use to catch programming + errors where the application reuses the connection pointer of an + active connection object without dereferencing it. Without + dereferencing, the connection object stays alive which can lead to an + unpredictable behavior. + config BT_USER_PHY_UPDATE bool "User control of PHY Update Procedure" depends on BT_PHY_UPDATE diff --git a/subsys/bluetooth/host/conn.c b/subsys/bluetooth/host/conn.c index 229f7adf0a473b3..8e1da4d95aed585 100644 --- a/subsys/bluetooth/host/conn.c +++ b/subsys/bluetooth/host/conn.c @@ -3674,6 +3674,22 @@ int bt_conn_le_create(const bt_addr_le_t *peer, const struct bt_conn_le_create_p struct bt_conn *conn; int err; + CHECKIF(ret_conn == NULL) { + return -EINVAL; + } + + CHECKIF(*ret_conn != NULL) { + /** This warning is to inform the user that the connection reference is not NULL and + * it may lead to a potential connection object leakage. + */ + LOG_WRN("*conn should be unreferenced and initialized to NULL before passing to" + " %s", __func__); + + if (IS_ENABLED(CONFIG_BT_CONN_CHECK_NULL_BEFORE_CREATE)) { + return -EINVAL; + } + } + err = conn_le_create_common_checks(peer, conn_param); if (err) { return err; @@ -3733,6 +3749,22 @@ int bt_conn_le_create_synced(const struct bt_le_ext_adv *adv, struct bt_conn *conn; int err; + CHECKIF(ret_conn == NULL) { + return -EINVAL; + } + + CHECKIF(*ret_conn != NULL) { + /** This warning is to inform the user that the connection reference is not NULL and + * it may lead to a potential connection object leakage. + */ + LOG_WRN("*conn should be unreferenced and initialized to NULL before passing to" + " %s", __func__); + + if (IS_ENABLED(CONFIG_BT_CONN_CHECK_NULL_BEFORE_CREATE)) { + return -EINVAL; + } + } + err = conn_le_create_common_checks(synced_param->peer, conn_param); if (err) { return err;