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
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>

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

doc/releases/release-notes-4.0.rst

@@ -123,6 +123,13 @@ Bluetooth
 
   * The host now disconnects from the peer upon ATT timeout.
 
+  * 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

include/zephyr/bluetooth/conn.h

@@ -1336,7 +1336,9 @@ struct bt_conn_le_create_param {
  *  Allows initiate new LE link to remote peer using its address.
  *
  *  The caller gets a new reference to the connection object which must be
- *  released with bt_conn_unref() once done using the object.
+ *  released with bt_conn_unref() once done using the object. If
+ *  @kconfig{CONFIG_BT_CONN_CHECK_NULL_BEFORE_CREATE} is enabled, this function
+ *  will return -EINVAL if dereferenced @p conn is not NULL.
  *
  *  This uses the General Connection Establishment procedure.
  *
@@ -1375,7 +1377,9 @@ struct bt_conn_le_create_synced_param {
  *  with Responses (PAwR) train.
  *
  *  The caller gets a new reference to the connection object which must be
- *  released with bt_conn_unref() once done using the object.
+ *  released with bt_conn_unref() once done using the object. If
+ *  @kconfig{CONFIG_BT_CONN_CHECK_NULL_BEFORE_CREATE} is enabled, this function
+ *  will return -EINVAL if dereferenced @p conn is not NULL.
  *
  *  This uses the Periodic Advertising Connection Procedure.
  *

subsys/bluetooth/host/Kconfig

@@ -343,6 +343,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

subsys/bluetooth/host/conn.c

@@ -3720,6 +3720,23 @@ 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 rule helps application developers prevent leaks of connection references. If
+		 * a bt_conn variable is not null, it presumably holds a reference and must not be
+		 * overwritten. To avoid this warning, initialize the variables to null, and set
+		 * them to null when moving the reference.
+		 */
+		LOG_WRN("*conn should be unreferenced and initialized to NULL");
+
+		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;
@@ -3779,6 +3796,23 @@ 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 rule helps application developers prevent leaks of connection references. If
+		 * a bt_conn variable is not null, it presumably holds a reference and must not be
+		 * overwritten. To avoid this warning, initialize the variables to null, and set
+		 * them to null when moving the reference.
+		 */
+		LOG_WRN("*conn should be unreferenced and initialized to NULL");
+
+		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;