2024-05-16 09:04:16

by Bastien Nocera

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

"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



2024-05-16 09:04:21

by Bastien Nocera

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

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


2024-05-16 09:04:23

by Bastien Nocera

[permalink] [raw]
Subject: [BlueZ 09/15] isotest: Fix bad free

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


2024-05-16 09:04:29

by Bastien Nocera

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

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


2024-05-16 09:04:31

by Bastien Nocera

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

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


2024-05-16 09:04:34

by Bastien Nocera

[permalink] [raw]
Subject: [BlueZ 15/15] android/handsfree: Check sprintf retval

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


2024-05-16 09:04:38

by Bastien Nocera

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

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


2024-05-16 09:04:41

by Bastien Nocera

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

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


2024-05-16 09:07:21

by Bastien Nocera

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

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


2024-05-16 09:07:40

by Bastien Nocera

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

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


2024-05-16 09:07:53

by Bastien Nocera

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

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


2024-05-16 09:08:19

by Bastien Nocera

[permalink] [raw]
Subject: [BlueZ 13/15] gdbus: Check sprintf retval

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


2024-05-16 20:50:42

by patchwork-bot+bluetooth

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

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[email protected]>:

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



2024-05-27 08:23:19

by Bastien Nocera

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

On Thu, 2024-05-16 at 20:50 +0000, [email protected]
wrote:
> Hello:
>
> This series was applied to bluetooth/bluez.git (master)
> by Luiz Augusto von Dentz <[email protected]>:
>
> 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!