[BlueZ 00/15] Fix a number of static analysis issues #2

[BlueZ 00/15] Fix a number of static analysis issues #2

[Bastien Nocera]

"main: Simplify variable assignment" makes a come back, moving out the
single special-case out of the function.

For "shared/gatt-client: Fix uninitialised variable usage", please verify
that this default value is correct.

Happy to iterate on any patches you feel are suboptimal. Note that the
only patches that received any sort of real-world testing are the one
mentioned above and the gdbus one.

Cheers

Bastien Nocera (15):
  main: Simplify variable assignment
  shared/ecc: Fix uninitialised variable usage
  shared/gatt-client: Fix uninitialised variable usage
  tools/mesh-cfgclient: Fix uninitialised variable usage
  test-runner: Remove unused envp
  test-runner: Fix uninitialised variable usage
  test-runner: Fix uninitialised variable usage
  shared/bap: Fix possible use-after-free
  isotest: Fix bad free
  test-runner: Fix fd leak on failure
  isotest: Fix string size expectations
  mgmt-tester: Fix non-nul-terminated string
  gdbus: Check sprintf retval
  shared/bap: Fix memory leak in error path
  android/handsfree: Check sprintf retval

 android/handsfree.c      | 14 +++++++++---
 gdbus/watch.c            | 46 +++++++++++++++++++++++++++++-----------
 src/main.c               | 19 +++++++++--------
 src/shared/bap.c         | 19 ++++++++++++-----
 src/shared/ecc.c         |  2 ++
 src/shared/gatt-client.c |  2 +-
 tools/isotest.c          | 20 +++++++++++++----
 tools/mesh-cfgclient.c   |  3 +--
 tools/mgmt-tester.c      |  8 +++++--
 tools/test-runner.c      | 15 +++++++++----
 10 files changed, 106 insertions(+), 42 deletions(-)

-- 
2.44.0

[BlueZ 01/15] main: Simplify variable assignment

[Bastien Nocera]

Error: RESOURCE_LEAK (CWE-772): [#def39] [important]
bluez-5.75/src/main.c:425:2: alloc_fn: Storage is returned from allocation function "g_key_file_get_string".
bluez-5.75/src/main.c:425:2: var_assign: Assigning: "tmp" = storage returned from "g_key_file_get_string(config, group, key, &err)".
bluez-5.75/src/main.c:433:2: noescape: Assuming resource "tmp" is not freed or pointed-to as ellipsis argument to "btd_debug".
bluez-5.75/src/main.c:440:2: leaked_storage: Variable "tmp" going out of scope leaks the storage it points to.
438|	}
439|
440|->	return true;
441|   }
442|
---
 src/main.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/main.c b/src/main.c
index ac840d684f6d..f6369a20d879 100644
--- a/src/main.c
+++ b/src/main.c
@@ -420,9 +420,10 @@ static bool parse_config_string(GKeyFile *config, const char *group,
 					const char *key, char **val)
 {
 	GError *err = NULL;
-	char *tmp;
 
-	tmp = g_key_file_get_string(config, group, key, &err);
+	g_return_val_if_fail(val, false);
+
+	*val = g_key_file_get_string(config, group, key, &err);
 	if (err) {
 		if (err->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND)
 			DBG("%s", err->message);
@@ -430,12 +431,7 @@ static bool parse_config_string(GKeyFile *config, const char *group,
 		return false;
 	}
 
-	DBG("%s.%s = %s", group, key, tmp);
-
-	if (val) {
-		g_free(*val);
-		*val = tmp;
-	}
+	DBG("%s.%s = %s", group, key, *val);
 
 	return true;
 }
@@ -1005,7 +1001,12 @@ static void parse_secure_conns(GKeyFile *config)
 
 static void parse_general(GKeyFile *config)
 {
-	parse_config_string(config, "General", "Name", &btd_opts.name);
+	char *str = NULL;
+
+	if (parse_config_string(config, "General", "Name", &str)) {
+		g_free(btd_opts.name);
+		btd_opts.name = str;
+	}
 	parse_config_hex(config, "General", "Class", &btd_opts.class);
 	parse_config_u32(config, "General", "DiscoverableTimeout",
 						&btd_opts.discovto,
-- 
2.44.0

[BlueZ 02/15] shared/ecc: Fix uninitialised variable usage

[Bastien Nocera]

Error: UNINIT (CWE-457): [#def41] [important]
bluez-5.75/src/shared/ecc.c:869:2: var_decl: Declaring variable "pk" without initializer.
bluez-5.75/src/shared/ecc.c:885:34: uninit_use_in_call: Using uninitialized element of array "pk.x" when calling "ecc_point_is_zero".
883|
884|		ecc_point_mult(&pk, &curve_g, priv, NULL, vli_num_bits(priv));
885|->	} while (ecc_point_is_zero(&pk));
886|
887|	ecc_native2bytes(priv, private_key);

Error: UNINIT (CWE-457): [#def42] [important]
bluez-5.75/src/shared/ecc.c:869:2: var_decl: Declaring variable "pk" without initializer.
bluez-5.75/src/shared/ecc.c:885:34: uninit_use_in_call: Using uninitialized element of array "pk.x" when calling "ecc_point_is_zero".
bluez-5.75/src/shared/ecc.c:885:34: uninit_use_in_call: Using uninitialized element of array "pk.y" when calling "ecc_point_is_zero".
883|
884|		ecc_point_mult(&pk, &curve_g, priv, NULL, vli_num_bits(priv));
885|->	} while (ecc_point_is_zero(&pk));
886|
887|	ecc_native2bytes(priv, private_key);

Error: UNINIT (CWE-457): [#def43] [important]
bluez-5.75/src/shared/ecc.c:869:2: var_decl: Declaring variable "pk" without initializer.
bluez-5.75/src/shared/ecc.c:889:2: uninit_use_in_call: Using uninitialized value "*pk.y" when calling "ecc_native2bytes".
887|	ecc_native2bytes(priv, private_key);
888|	ecc_native2bytes(pk.x, public_key);
889|->	ecc_native2bytes(pk.y, &public_key[32]);
890|
891|	return true;
---
 src/shared/ecc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/shared/ecc.c b/src/shared/ecc.c
index adaae2082e1f..02bccbd430f6 100644
--- a/src/shared/ecc.c
+++ b/src/shared/ecc.c
@@ -870,6 +870,8 @@ bool ecc_make_key(uint8_t public_key[64], uint8_t private_key[32])
 	uint64_t priv[NUM_ECC_DIGITS];
 	unsigned int tries = 0;
 
+	memset(&pk, 0, sizeof(pk));
+
 	do {
 		if (!get_random_number(priv) || (tries++ >= MAX_TRIES))
 			return false;
-- 
2.44.0

[BlueZ 03/15] shared/gatt-client: Fix uninitialised variable usage

[Bastien Nocera]

Error: UNINIT (CWE-457): [#def44] [important]
bluez-5.75/src/shared/gatt-client.c:1669:2: var_decl: Declaring variable "value" without initializer.
bluez-5.75/src/shared/gatt-client.c:1686:2: uninit_use_in_call: Using uninitialized value "value" when calling "bt_gatt_client_write_value".
1684|		}
1685|
1686|->		att_id = bt_gatt_client_write_value(notify_data->client,
1687|							notify_data->chrc->ccc_handle,
1688|							(void *)&value, sizeof(value),
---
 src/shared/gatt-client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index dcf6f0211a67..8e4ae7e5e230 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1666,7 +1666,7 @@ static bool notify_data_write_ccc(struct notify_data *notify_data, bool enable,
 					bt_gatt_client_callback_t callback)
 {
 	unsigned int att_id;
-	uint16_t value;
+	uint16_t value = 0x0000;
 	uint16_t properties = notify_data->chrc->properties;
 
 	assert(notify_data->chrc->ccc_handle);
-- 
2.44.0

[BlueZ 04/15] tools/mesh-cfgclient: Fix uninitialised variable usage

[Bastien Nocera]

Error: UNINIT (CWE-457): [#def64] [important]
bluez-5.75/tools/mesh-cfgclient.c:1992:2: var_decl: Declaring variable "result" without initializer.
bluez-5.75/tools/mesh-cfgclient.c:2041:3: uninit_use: Using uninitialized value "result". Field "result.last_seen" is uninitialized.
2039|							l_queue_length(devices) + 1);
2040|			dev = l_malloc(sizeof(struct unprov_device));
2041|->			*dev = result;
2042|
2043|		} else if (dev->rssi < result.rssi)

Error: UNINIT (CWE-457): [#def65] [important]
bluez-5.75/tools/mesh-cfgclient.c:1992:2: var_decl: Declaring variable "result" without initializer.
bluez-5.75/tools/mesh-cfgclient.c:2044:3: uninit_use: Using uninitialized value "result". Field "result.last_seen" is uninitialized.
2042|
2043|		} else if (dev->rssi < result.rssi)
2044|->			*dev = result;
2045|
2046|		dev->last_seen = time(NULL);
---
 tools/mesh-cfgclient.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/mesh-cfgclient.c b/tools/mesh-cfgclient.c
index 6d2d34409fe3..e39f145c6241 100644
--- a/tools/mesh-cfgclient.c
+++ b/tools/mesh-cfgclient.c
@@ -2021,6 +2021,7 @@ static struct l_dbus_message *scan_result_call(struct l_dbus *dbus,
 	result.server = server;
 	result.rssi = rssi;
 	result.id = 0;
+	result.last_seen = time(NULL);
 
 	if (n > 16 && n <= 18)
 		result.oob_info = l_get_be16(prov_data + 16);
@@ -2043,8 +2044,6 @@ static struct l_dbus_message *scan_result_call(struct l_dbus *dbus,
 	} else if (dev->rssi < result.rssi)
 		*dev = result;
 
-	dev->last_seen = time(NULL);
-
 	l_queue_insert(devices, dev, sort_rssi, NULL);
 
 done:
-- 
2.44.0

[BlueZ 05/15] test-runner: Remove unused envp

[Bastien Nocera]

Error: UNINIT (CWE-457): [#def70] [important]
bluez-5.75/tools/test-runner.c:644:2: var_decl: Declaring variable "envp" without initializer.
bluez-5.75/tools/test-runner.c:682:3: uninit_use_in_call: Using uninitialized value "*envp" when calling "execve".
680|
681|	if (pid == 0) {
682|->		execve(argv[0], argv, envp);
683|		exit(EXIT_SUCCESS);
684|	}

Error: UNINIT (CWE-457): [#def71] [important]
bluez-5.75/tools/test-runner.c:701:2: var_decl: Declaring variable "envp" without initializer.
bluez-5.75/tools/test-runner.c:739:3: uninit_use_in_call: Using uninitialized value "*envp" when calling "execve".
737|
738|	if (pid == 0) {
739|->		execve(argv[0], argv, envp);
740|		exit(EXIT_SUCCESS);
741|	}
---
 tools/test-runner.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/test-runner.c b/tools/test-runner.c
index 5bdcf42fcd7a..134e26f9c691 100644
--- a/tools/test-runner.c
+++ b/tools/test-runner.c
@@ -641,7 +641,7 @@ static const char *monitor_table[] = {
 static pid_t start_btmon(const char *home)
 {
 	const char *monitor = NULL;
-	char *argv[3], *envp[2];
+	char *argv[3];
 	pid_t pid;
 	int i;
 
@@ -679,7 +679,7 @@ static pid_t start_btmon(const char *home)
 	}
 
 	if (pid == 0) {
-		execve(argv[0], argv, envp);
+		execv(argv[0], argv);
 		exit(EXIT_SUCCESS);
 	}
 
-- 
2.44.0

[BlueZ 06/15] test-runner: Fix uninitialised variable usage

[Bastien Nocera]

Error: UNINIT (CWE-457): [#def72] [important]
bluez-5.75/tools/test-runner.c:856:2: var_decl: Declaring variable "argv" without initializer.
bluez-5.75/tools/test-runner.c:945:2: uninit_use: Using uninitialized value "argv[0]".
943|   	envp[pos] = NULL;
944|
945|-> 	printf("Running command %s\n", cmdname ? cmdname : argv[0]);
946|
947|   	pid = fork();
---
 tools/test-runner.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/test-runner.c b/tools/test-runner.c
index 134e26f9c691..ff5e19825801 100644
--- a/tools/test-runner.c
+++ b/tools/test-runner.c
@@ -912,6 +912,11 @@ static void run_command(char *cmdname, char *home)
 		audio_pid[0] = audio_pid[1] = -1;
 
 start_next:
+	if (!run_auto && !cmdname) {
+		fprintf(stderr, "Missing command argument\n");
+		return;
+	}
+
 	if (run_auto) {
 		if (chdir(home + 5) < 0) {
 			perror("Failed to change home test directory");
-- 
2.44.0

[BlueZ 07/15] test-runner: Fix uninitialised variable usage

[Bastien Nocera]

Error: UNINIT (CWE-457): [#def64] [important]
bluez-5.75/tools/test-runner.c:701:2: var_decl: Declaring variable "envp" without initializer.
bluez-5.75/tools/test-runner.c:739:3: uninit_use_in_call: Using uninitialized value "*envp" when calling "execve".
737|
738|	if (pid == 0) {
739|->		execve(argv[0], argv, envp);
740|		exit(EXIT_SUCCESS);
741|	}
---
 tools/test-runner.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/test-runner.c b/tools/test-runner.c
index ff5e19825801..908327255ad7 100644
--- a/tools/test-runner.c
+++ b/tools/test-runner.c
@@ -698,7 +698,7 @@ static const char *btvirt_table[] = {
 static pid_t start_btvirt(const char *home)
 {
 	const char *btvirt = NULL;
-	char *argv[3], *envp[2];
+	char *argv[3];
 	pid_t pid;
 	int i;
 
@@ -736,7 +736,7 @@ static pid_t start_btvirt(const char *home)
 	}
 
 	if (pid == 0) {
-		execve(argv[0], argv, envp);
+		execv(argv[0], argv);
 		exit(EXIT_SUCCESS);
 	}
 
-- 
2.44.0

[BlueZ 08/15] shared/bap: Fix possible use-after-free

[Bastien Nocera]

stream_set_state() might call bap_stream_detach() if the stream is in
the process of being detached, causing a use-after-free.

Return false from stream_set_state() if the stream is unsafe to
manipulate (ie. was in the process of being detached and freed).

Error: USE_AFTER_FREE (CWE-416): [#def37] [important]
bluez-5.75/src/shared/bap.c:2490:2: freed_arg: "stream_set_state" frees "stream".
bluez-5.75/src/shared/bap.c:2493:2: deref_after_free: Dereferencing freed pointer "stream".
2491|
2492|		/* Sink can autonomously for to Streaming state if io already exits */
2493|->		if (stream->io && stream->ep->dir == BT_BAP_SINK)
2494|			stream_set_state(stream, BT_BAP_STREAM_STATE_STREAMING);
2495|
---
 src/shared/bap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 1316d7c73d02..0026bc8dc989 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -1298,7 +1298,8 @@ static void bap_stream_state_changed(struct bt_bap_stream *stream)
 	}
 }
 
-static void stream_set_state(struct bt_bap_stream *stream, uint8_t state)
+/* Return false if the stream is being detached */
+static bool stream_set_state(struct bt_bap_stream *stream, uint8_t state)
 {
 	struct bt_bap *bap = stream->bap;
 
@@ -1308,13 +1309,14 @@ static void stream_set_state(struct bt_bap_stream *stream, uint8_t state)
 	bap = bt_bap_ref_safe(bap);
 	if (!bap) {
 		bap_stream_detach(stream);
-		return;
+		return false;
 	}
 
 	if (stream->ops && stream->ops->set_state)
 		stream->ops->set_state(stream, state);
 
 	bt_bap_unref(bap);
+	return true;
 }
 
 static void ep_config_cb(struct bt_bap_stream *stream, int err)
@@ -2487,7 +2489,8 @@ static uint8_t stream_enable(struct bt_bap_stream *stream, struct iovec *meta,
 	util_iov_free(stream->meta, 1);
 	stream->meta = util_iov_dup(meta, 1);
 
-	stream_set_state(stream, BT_BAP_STREAM_STATE_ENABLING);
+	if (!stream_set_state(stream, BT_BAP_STREAM_STATE_ENABLING))
+		return 1;
 
 	/* Sink can autonomously for to Streaming state if io already exits */
 	if (stream->io && stream->ep->dir == BT_BAP_SINK)
-- 
2.44.0

[BlueZ 09/15] isotest: Fix bad free

[Bastien Nocera]

Error: BAD_FREE (CWE-763): [#def58] [important]
bluez-5.75/tools/isotest.c:1461:5: address: Taking offset from "strchr(filename, 44)".
bluez-5.75/tools/isotest.c:1461:5: assign: Assigning: "filename" = "strchr(filename, 44) + 1".
bluez-5.75/tools/isotest.c:1536:2: incorrect_free: "free" frees incorrect pointer "filename".
1534|
1535|   done:
1536|->		free(filename);
1537|
1538|		syslog(LOG_INFO, "Exit");
---
 tools/isotest.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/isotest.c b/tools/isotest.c
index 58293133a304..fc1c26b23c3b 100644
--- a/tools/isotest.c
+++ b/tools/isotest.c
@@ -1457,8 +1457,11 @@ int main(int argc, char *argv[])
 		switch (mode) {
 		case SEND:
 			send_mode(filename, argv[optind + i], i, repeat);
-			if (filename && strchr(filename, ','))
-				filename = strchr(filename, ',') + 1;
+			if (filename && strchr(filename, ',')) {
+				char *tmp = filename;
+				filename = strdup(strchr(filename, ',') + 1);
+				free(tmp);
+			}
 			break;
 
 		case RECONNECT:
-- 
2.44.0

[BlueZ 10/15] test-runner: Fix fd leak on failure

[Bastien Nocera]

Error: RESOURCE_LEAK (CWE-772): [#def65] [important]
bluez-5.75/tools/test-runner.c:877:3: open_fn: Returning handle opened by "attach_proto".
bluez-5.75/tools/test-runner.c:877:3: var_assign: Assigning: "serial_fd" = handle returned from "attach_proto(node, 0U, basic_flags, extra_flags)".
bluez-5.75/tools/test-runner.c:955:3: leaked_handle: Handle variable "serial_fd" going out of scope leaks the handle.
953|	if (pid < 0) {
954|		perror("Failed to fork new process");
955|->		return;
956|	}
957|
---
 tools/test-runner.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/test-runner.c b/tools/test-runner.c
index 908327255ad7..de0f2260480c 100644
--- a/tools/test-runner.c
+++ b/tools/test-runner.c
@@ -952,6 +952,8 @@ start_next:
 	pid = fork();
 	if (pid < 0) {
 		perror("Failed to fork new process");
+		if (serial_fd >= 0)
+			close(serial_fd);
 		return;
 	}
 
-- 
2.44.0

[BlueZ 11/15] isotest: Fix string size expectations

[Bastien Nocera]

Verify that the peer is a valid bdaddr (and so has the correct length)
before using it.

Error: STRING_SIZE (CWE-120): [#def54] [important]
bluez-5.75/tools/isotest.c:1198:26: string_size_argv: "argv" contains strings with unknown size.
bluez-5.75/tools/isotest.c:1459:4: string_size: Passing string "argv[optind + i]" of unknown size to "send_mode", which expects a string of a particular size.

Error: STRING_SIZE (CWE-120): [#def55] [important]
bluez-5.75/tools/isotest.c:1198:26: string_size_argv: "argv" contains strings with unknown size.
bluez-5.75/tools/isotest.c:1476:4: var_assign_var: Assigning: "peer" = "argv[optind + i]". Both are now tainted.
bluez-5.75/tools/isotest.c:1484:5: string_size: Passing string "peer" of unknown size to "bcast_do_connect_mbis", which expects a string of a particular size.

Error: STRING_SIZE (CWE-120): [#def56] [important]
bluez-5.75/tools/isotest.c:1198:26: string_size_argv: "argv" contains strings with unknown size.
bluez-5.75/tools/isotest.c:1476:4: var_assign_var: Assigning: "peer" = "argv[optind + i]". Both are now tainted.
bluez-5.75/tools/isotest.c:1514:5: string_size: Passing string "argv[optind + i]" of unknown size to "do_connect", which expects a string of a particular size.
---
 tools/isotest.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/isotest.c b/tools/isotest.c
index fc1c26b23c3b..f98f25497b85 100644
--- a/tools/isotest.c
+++ b/tools/isotest.c
@@ -1456,7 +1456,12 @@ int main(int argc, char *argv[])
 
 		switch (mode) {
 		case SEND:
-			send_mode(filename, argv[optind + i], i, repeat);
+			peer = argv[optind + i];
+			if (bachk(peer) < 0) {
+				fprintf(stderr, "Invalid peer address '%s'\n", peer);
+				exit(1);
+			}
+			send_mode(filename, peer, i, repeat);
 			if (filename && strchr(filename, ',')) {
 				char *tmp = filename;
 				filename = strdup(strchr(filename, ',') + 1);
@@ -1474,6 +1479,10 @@ int main(int argc, char *argv[])
 
 		case CONNECT:
 			peer = argv[optind + i];
+			if (bachk(peer) < 0) {
+				fprintf(stderr, "Invalid peer address '%s'\n", peer);
+				exit(1);
+			}
 
 			mgmt_set_experimental();
 
@@ -1511,7 +1520,7 @@ int main(int argc, char *argv[])
 
 				free(sk_arr);
 			} else {
-				sk = do_connect(argv[optind + i]);
+				sk = do_connect(peer);
 				if (sk < 0)
 					exit(1);
 
-- 
2.44.0

[BlueZ 12/15] mgmt-tester: Fix non-nul-terminated string

[Bastien Nocera]

Error: STRING_NULL (CWE-170): [#def59] [important]
bluez-5.75/tools/mgmt-tester.c:12670:2: string_null_source: Function "vhci_read_devcd" does not terminate string "buf".
bluez-5.75/tools/mgmt-tester.c:12677:2: string_null: Passing unterminated string "buf" to "strtok_r", which expects a null-terminated string.
12675|
12676|		/* Verify if all devcoredump header fields are present */
12677|->	line = strtok_r(buf, delim, &saveptr);
12678|		while (strlen(test->expect_dump_data[i])) {
12679|			if (!line || strcmp(line, test->expect_dump_data[i])) {
---
 tools/mgmt-tester.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
index 8a4fbc2eb6a6..8076ec105ebb 100644
--- a/tools/mgmt-tester.c
+++ b/tools/mgmt-tester.c
@@ -12656,18 +12656,22 @@ static void verify_devcd(void *user_data)
 	struct test_data *data = tester_get_data();
 	const struct generic_data *test = data->test_data;
 	struct vhci *vhci = hciemu_get_vhci(data->hciemu);
-	char buf[MAX_COREDUMP_BUF_LEN] = {0};
+	char buf[MAX_COREDUMP_BUF_LEN + 1] = {0};
+	int read;
 	char delim[] = "\n";
 	char *line;
 	char *saveptr;
 	int i = 0;
 
 	/* Read the generated devcoredump file */
-	if (vhci_read_devcd(vhci, buf, sizeof(buf)) <= 0) {
+	read = vhci_read_devcd(vhci, buf, MAX_COREDUMP_BUF_LEN);
+	if (read <= 0) {
 		tester_warn("Unable to read devcoredump");
 		tester_test_failed();
 		return;
 	}
+	/* Make sure buf is nul-terminated */
+	buf[read + 1] = '\0';
 
 	/* Verify if all devcoredump header fields are present */
 	line = strtok_r(buf, delim, &saveptr);
-- 
2.44.0

[BlueZ 13/15] gdbus: Check sprintf retval

[Bastien Nocera]

Error: SNYK_CODE_WARNING (CWE-125): [#def63] [important]
bluez-5.75/gdbus/watch.c:131:11: error[cpp/NegativeIndex]: The value from snprintf, a standard library function that can return a negative value is used as an index. A negative array index can lead to reading or writing outside the bounds of the array. Ensure the value of the index used is within bounds before use.
129|	int offset;
130|
131|->	offset = snprintf(rule, size, "type='signal'");
132|	sender = data->name ? : data->owner;
133|
---
 gdbus/watch.c | 46 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index 25f367613a52..22f77ea72861 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -123,29 +123,51 @@ static struct filter_data *filter_data_find(DBusConnection *connection)
 	return NULL;
 }
 
-static void format_rule(struct filter_data *data, char *rule, size_t size)
+static gboolean format_rule(struct filter_data *data, char *rule, size_t size)
 {
 	const char *sender;
-	int offset;
+	int offset, ret;
 
 	offset = snprintf(rule, size, "type='signal'");
+	if (offset < 0)
+		return FALSE;
 	sender = data->name ? : data->owner;
 
-	if (sender)
-		offset += snprintf(rule + offset, size - offset,
+	if (sender) {
+		ret = snprintf(rule + offset, size - offset,
 				",sender='%s'", sender);
-	if (data->path)
-		offset += snprintf(rule + offset, size - offset,
+		if (ret < 0)
+			return FALSE;
+		offset += ret;
+	}
+	if (data->path) {
+		ret = snprintf(rule + offset, size - offset,
 				",path='%s'", data->path);
-	if (data->interface)
-		offset += snprintf(rule + offset, size - offset,
+		if (ret < 0)
+			return FALSE;
+		offset += ret;
+	}
+	if (data->interface) {
+		ret = snprintf(rule + offset, size - offset,
 				",interface='%s'", data->interface);
-	if (data->member)
-		offset += snprintf(rule + offset, size - offset,
+		if (ret < 0)
+			return FALSE;
+		offset += ret;
+	}
+	if (data->member) {
+		ret = snprintf(rule + offset, size - offset,
 				",member='%s'", data->member);
-	if (data->argument)
-		snprintf(rule + offset, size - offset,
+		if (ret < 0)
+			return FALSE;
+		offset += ret;
+	}
+	if (data->argument) {
+		ret = snprintf(rule + offset, size - offset,
 				",arg0='%s'", data->argument);
+		if (ret < 0)
+			return FALSE;
+	}
+	return TRUE;
 }
 
 static gboolean add_match(struct filter_data *data,
-- 
2.44.0

[BlueZ 14/15] shared/bap: Fix memory leak in error path

[Bastien Nocera]

Error: RESOURCE_LEAK (CWE-772): [#def38] [important]
bluez-5.75/src/shared/bap.c:6066:27: alloc_fn: Storage is returned from allocation function "util_malloc".
bluez-5.75/src/shared/bap.c:6066:27: var_assign: Assigning: "__p" = storage returned from "util_malloc(__n * __s)".
bluez-5.75/src/shared/bap.c:6066:27: noescape: Resource "__p" is not freed or pointed-to in "memset". [Note: The source code implementation of the function has been overridden by a builtin model.]
bluez-5.75/src/shared/bap.c:6066:27: leaked_storage: Variable "__p" going out of scope leaks the storage it points to.
bluez-5.75/src/shared/bap.c:6066:2: var_assign: Assigning: "base_iov" = "({...; __p;})".
bluez-5.75/src/shared/bap.c:6070:2: noescape: Resource "base_iov" is not freed or pointed-to in "util_iov_push_le24".
bluez-5.75/src/shared/bap.c:6071:3: leaked_storage: Variable "base_iov" going out of scope leaks the storage it points to.
6069|
6070|		if (!util_iov_push_le24(base_iov, base->pres_delay))
6071|->		return NULL;
6072|
6073|		if (!util_iov_push_u8(base_iov,

Error: RESOURCE_LEAK (CWE-772): [#def39] [important]
bluez-5.75/src/shared/bap.c:6066:27: alloc_fn: Storage is returned from allocation function "util_malloc".
bluez-5.75/src/shared/bap.c:6066:27: var_assign: Assigning: "__p" = storage returned from "util_malloc(__n * __s)".
bluez-5.75/src/shared/bap.c:6066:27: noescape: Resource "__p" is not freed or pointed-to in "memset". [Note: The source code implementation of the function has been overridden by a builtin model.]
bluez-5.75/src/shared/bap.c:6066:27: leaked_storage: Variable "__p" going out of scope leaks the storage it points to.
bluez-5.75/src/shared/bap.c:6066:2: var_assign: Assigning: "base_iov" = "({...; __p;})".
bluez-5.75/src/shared/bap.c:6070:2: noescape: Resource "base_iov" is not freed or pointed-to in "util_iov_push_le24".
bluez-5.75/src/shared/bap.c:6073:2: noescape: Resource "base_iov" is not freed or pointed-to in "util_iov_push_u8".
bluez-5.75/src/shared/bap.c:6075:3: leaked_storage: Variable "base_iov" going out of scope leaks the storage it points to.
6073|		if (!util_iov_push_u8(base_iov,
6074|				queue_length(base->subgroups)))
6075|->			return NULL;
6076|
6077|		queue_foreach(base->subgroups, generate_subgroup_base,
---
 src/shared/bap.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 0026bc8dc989..48b6d7f4ea85 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -6067,12 +6067,18 @@ static struct iovec *generate_base(struct bt_base *base)
 
 	base_iov->iov_base = util_malloc(BASE_MAX_LENGTH);
 
-	if (!util_iov_push_le24(base_iov, base->pres_delay))
+	if (!util_iov_push_le24(base_iov, base->pres_delay)) {
+		free(base_iov->iov_base);
+		free(base_iov);
 		return NULL;
+	}
 
 	if (!util_iov_push_u8(base_iov,
-			queue_length(base->subgroups)))
+			queue_length(base->subgroups))) {
+		free(base_iov->iov_base);
+		free(base_iov);
 		return NULL;
+	}
 
 	queue_foreach(base->subgroups, generate_subgroup_base,
 				base_iov);
-- 
2.44.0

[BlueZ 15/15] android/handsfree: Check sprintf retval

[Bastien Nocera]

Error: SNYK_CODE_WARNING (CWE-125): [#def62] [important]
bluez-5.75/android/handsfree.c:1247:15: error[cpp/NegativeIndex]: The value from sprintf, a standard library function that can return a negative value is used as an index. A negative array index can lead to reading or writing outside the bounds of the array. Ensure the value of the index used is within bounds before use.
1245|			buf = g_malloc(len);
1246|
1247|->			ptr = buf + sprintf(buf, "+CIND:");
1248|
1249|			for (i = 0; i < IND_COUNT; i++) {
---
 android/handsfree.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/android/handsfree.c b/android/handsfree.c
index 2365356c2cf7..7b803fae5263 100644
--- a/android/handsfree.c
+++ b/android/handsfree.c
@@ -1243,15 +1243,22 @@ static void at_cmd_cind(struct hfp_context *result, enum hfp_gw_cmd_type type,
 		}
 
 		buf = g_malloc(len);
-
-		ptr = buf + sprintf(buf, "+CIND:");
+		if (sprintf(buf, "+CIND:") != strlen("+CIND:")) {
+			g_free(buf);
+			break;
+		}
+		ptr = buf + strlen("+CIND:");
 
 		for (i = 0; i < IND_COUNT; i++) {
-			ptr += sprintf(ptr, "(\"%s\",(%d%c%d)),",
+			int printed;
+			printed = sprintf(ptr, "(\"%s\",(%d%c%d)),",
 					dev->inds[i].name,
 					dev->inds[i].min,
 					dev->inds[i].max == 1 ? ',' : '-',
 					dev->inds[i].max);
+			if (printed < 0)
+				goto fail;
+			ptr += printed;
 		}
 
 		ptr--;
@@ -1273,6 +1280,7 @@ static void at_cmd_cind(struct hfp_context *result, enum hfp_gw_cmd_type type,
 		break;
 	}
 
+fail:
 	hfp_gw_send_result(dev->gw, HFP_RESULT_ERROR);
 
 	if (dev->state != HAL_EV_HANDSFREE_CONN_STATE_SLC_CONNECTED)
-- 
2.44.0

Re: [BlueZ 00/15] Fix a number of static analysis issues #2

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Thu, 16 May 2024 11:03:04 +0200 you wrote:
> "main: Simplify variable assignment" makes a come back, moving out the
> single special-case out of the function.
> 
> For "shared/gatt-client: Fix uninitialised variable usage", please verify
> that this default value is correct.
> 
> Happy to iterate on any patches you feel are suboptimal. Note that the
> only patches that received any sort of real-world testing are the one
> mentioned above and the gdbus one.
> 
> [...]

Here is the summary with links:
  - [BlueZ,01/15] main: Simplify variable assignment
    (no matching commit)
  - [BlueZ,02/15] shared/ecc: Fix uninitialised variable usage
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=0a1159dc1055
  - [BlueZ,03/15] shared/gatt-client: Fix uninitialised variable usage
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=75eda690c4af
  - [BlueZ,04/15] tools/mesh-cfgclient: Fix uninitialised variable usage
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=c63b7b0d732e
  - [BlueZ,05/15] test-runner: Remove unused envp
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9f4b2d0287ef
  - [BlueZ,06/15] test-runner: Fix uninitialised variable usage
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=0640d99ebfae
  - [BlueZ,07/15] test-runner: Fix uninitialised variable usage
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9672cf410f8b
  - [BlueZ,08/15] shared/bap: Fix possible use-after-free
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=52336ad64548
  - [BlueZ,09/15] isotest: Fix bad free
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7a6385570494
  - [BlueZ,10/15] test-runner: Fix fd leak on failure
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=566af9c2f5ef
  - [BlueZ,11/15] isotest: Fix string size expectations
    (no matching commit)
  - [BlueZ,12/15] mgmt-tester: Fix non-nul-terminated string
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=49d06560692f
  - [BlueZ,13/15] gdbus: Check sprintf retval
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=20a0255b9e5f
  - [BlueZ,14/15] shared/bap: Fix memory leak in error path
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=377f2ec0721f
  - [BlueZ,15/15] android/handsfree: Check sprintf retval
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=c9fe888793e5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [BlueZ 00/15] Fix a number of static analysis issues #2

[Bastien Nocera]

On Thu, 2024-05-16 at 20:50 +0000, patchwork-bot+bluetooth@kernel.org
wrote:
> Hello:
> 
> This series was applied to bluetooth/bluez.git (master)
> by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
> 
> On Thu, 16 May 2024 11:03:04 +0200 you wrote:
> > "main: Simplify variable assignment" makes a come back, moving out
> > the
> > single special-case out of the function.
> > 
> > For "shared/gatt-client: Fix uninitialised variable usage", please
> > verify
> > that this default value is correct.
> > 
> > Happy to iterate on any patches you feel are suboptimal. Note that
> > the
> > only patches that received any sort of real-world testing are the
> > one
> > mentioned above and the gdbus one.
> > 
> > [...]
> 
> Here is the summary with links:
>   - [BlueZ,01/15] main: Simplify variable assignment
>     (no matching commit)

This one wasn't applied. I'll resubmit it separately with a better
commit message.

>   - [BlueZ,02/15] shared/ecc: Fix uninitialised variable usage
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=0a1159dc1055
>   - [BlueZ,03/15] shared/gatt-client: Fix uninitialised variable
> usage
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=75eda690c4af
>   - [BlueZ,04/15] tools/mesh-cfgclient: Fix uninitialised variable
> usage
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=c63b7b0d732e
>   - [BlueZ,05/15] test-runner: Remove unused envp
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9f4b2d0287ef
>   - [BlueZ,06/15] test-runner: Fix uninitialised variable usage
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=0640d99ebfae
>   - [BlueZ,07/15] test-runner: Fix uninitialised variable usage
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9672cf410f8b
>   - [BlueZ,08/15] shared/bap: Fix possible use-after-free
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=52336ad64548
>   - [BlueZ,09/15] isotest: Fix bad free
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7a6385570494
>   - [BlueZ,10/15] test-runner: Fix fd leak on failure
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=566af9c2f5ef
>   - [BlueZ,11/15] isotest: Fix string size expectations
>     (no matching commit)

Committed as:
https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=f05e448cf81b6ff0a8c7fc1e3accdb4f436a46e0

>   - [BlueZ,12/15] mgmt-tester: Fix non-nul-terminated string
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=49d06560692f
>   - [BlueZ,13/15] gdbus: Check sprintf retval
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=20a0255b9e5f
>   - [BlueZ,14/15] shared/bap: Fix memory leak in error path
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=377f2ec0721f
>   - [BlueZ,15/15] android/handsfree: Check sprintf retval
>    
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=c9fe888793e5
> 
> You are awesome, thank you!