Hello,
This is the first batch of bug fixes for issues found by Red Hat's
internal static analysis tools.
The best course of action would probably be to merge the one that
are ready for upstream inclusion after review, and mark the others as
needing work, so I can include a new version of the patch in following
batches.
Cheers
Bastien Nocera (14):
adapter: Use false instead of 0 for bool
attrib/gatt: Guard against possible integer overflow
client/gatt: Don't pass negative fd on error
client/gatt: Check write_value() retval
client/main: Fix array access
client/main: Fix mismatched free
monitor/att: Fix memory leak
bap: Fix memory leaks
media: Fix memory leak
main: Fix memory leaks
isotest: Consider "0" fd to be valid
isotest: Fix error check after opening file
client/player: Fix copy/paste error
shared/vcp: Fix copy/paste error
attrib/gatt.c | 8 ++++---
client/gatt.c | 21 +++++++++++++++----
client/main.c | 7 ++++++-
client/player.c | 2 +-
monitor/att.c | 19 +++++++++++++++++
profiles/audio/bap.c | 47 +++++++++++++++++++++++++++++-------------
profiles/audio/media.c | 1 +
src/adapter.c | 2 +-
src/main.c | 4 ++++
src/shared/vcp.c | 2 +-
tools/isotest.c | 4 ++--
11 files changed, 90 insertions(+), 27 deletions(-)
--
2.44.0
Error: RESOURCE_LEAK (CWE-772): [#def79] [important]
bluez-5.75/tools/isotest.c:923:4: open_fn: Returning handle opened by "open_file".
bluez-5.75/tools/isotest.c:923:4: var_assign: Assigning: "fd" = handle returned from "open_file(altername)".
bluez-5.75/tools/isotest.c:925:3: off_by_one: Testing whether handle "fd" is strictly greater than zero is suspicious. "fd" leaks when it is zero.
bluez-5.75/tools/isotest.c:925:3: remediation: Did you intend to include equality with zero?
bluez-5.75/tools/isotest.c:926:4: overwrite_var: Overwriting handle "fd" in "fd = open_file(filename)" leaks the handle.
924|
925| if (fd <= 0)
926|-> fd = open_file(filename);
927| }
928|
---
tools/isotest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/isotest.c b/tools/isotest.c
index 7e875fa58b15..810d15d2df2a 100644
--- a/tools/isotest.c
+++ b/tools/isotest.c
@@ -922,7 +922,7 @@ static void send_mode(char *filename, char *peer, int i, bool repeat)
if (!err)
fd = open_file(altername);
- if (fd <= 0)
+ if (fd < 0)
fd = open_file(filename);
}
--
2.44.0
Consider "0" to be a valid fd.
---
tools/isotest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/isotest.c b/tools/isotest.c
index 810d15d2df2a..ddace0da3044 100644
--- a/tools/isotest.c
+++ b/tools/isotest.c
@@ -720,7 +720,7 @@ static int open_file(const char *filename)
syslog(LOG_INFO, "Opening %s ...", filename);
fd = open(filename, O_RDONLY);
- if (fd <= 0) {
+ if (fd < 0) {
syslog(LOG_ERR, "Can't open file %s: %s\n",
filename, strerror(errno));
}
--
2.44.0
Error: COPY_PASTE_ERROR (CWE-398): [#def95] [important]
bluez-5.75/client/player.c:1846:6: original: "qos->sync_cte_type" looks like the original copy.
bluez-5.75/client/player.c:1852:6: copy_paste_error: "sync_cte_type" in "qos->sync_cte_type" looks like a copy-paste error.
bluez-5.75/client/player.c:1852:6: remediation: Should it say "mse" instead?
1850| }
1851|
1852|-> if (qos->sync_cte_type) {
1853| bt_shell_printf("MSE %u\n", qos->mse);
1854| g_dbus_dict_append_entry(iter, "MSE", DBUS_TYPE_BYTE,
---
client/player.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/client/player.c b/client/player.c
index 6b70e9ed3f9d..7f67425aaf8f 100644
--- a/client/player.c
+++ b/client/player.c
@@ -1849,7 +1849,7 @@ static void append_bcast_qos(DBusMessageIter *iter, struct endpoint_config *cfg)
&qos->sync_cte_type);
}
- if (qos->sync_cte_type) {
+ if (qos->mse) {
bt_shell_printf("MSE %u\n", qos->mse);
g_dbus_dict_append_entry(iter, "MSE", DBUS_TYPE_BYTE,
&qos->mse);
--
2.44.0
Error: COPY_PASTE_ERROR (CWE-398): [#def97] [important]
bluez-5.75/src/shared/vcp.c:2610:16: original: "aics->gain_stting_prop" looks like the original copy.
bluez-5.75/src/shared/vcp.c:2625:16: copy_paste_error: "gain_stting_prop" in "aics->gain_stting_prop" looks like a copy-paste error.
bluez-5.75/src/shared/vcp.c:2625:16: remediation: Should it say "aud_ip_type" instead?
2623|
2624| aics = vcp_get_aics(vcp);
2625|-> if (!aics || aics->gain_stting_prop)
2626| return;
2627|
---
src/shared/vcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/shared/vcp.c b/src/shared/vcp.c
index 7ba54e64adc0..b7e17e448b74 100644
--- a/src/shared/vcp.c
+++ b/src/shared/vcp.c
@@ -2622,7 +2622,7 @@ static void foreach_aics_char(struct gatt_db_attribute *attr, void *user_data)
value_handle);
aics = vcp_get_aics(vcp);
- if (!aics || aics->gain_stting_prop)
+ if (!aics || aics->aud_ip_type)
return;
aics->aud_ip_type = attr;
--
2.44.0
Error: RESOURCE_LEAK (CWE-772): [#def47] [important]
bluez-5.75/profiles/audio/media.c:1278:2: alloc_arg: "asprintf" allocates memory that is stored into "name". [Note: The source code implementation of the function has been overridden by a builtin model.]
bluez-5.75/profiles/audio/media.c:1291:2: noescape: Resource "name" is not freed or pointed-to in "bt_bap_add_vendor_pac".
bluez-5.75/profiles/audio/media.c:1297:3: leaked_storage: Variable "name" going out of scope leaks the storage it points to.
1295| error("Unable to create PAC");
1296| free(metadata);
1297|-> return false;
1298| }
1299|
---
profiles/audio/media.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 07147a25d532..4bbd584deaba 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -1293,6 +1293,7 @@ static bool endpoint_init_pac(struct media_endpoint *endpoint, uint8_t type,
&data, metadata);
if (!endpoint->pac) {
error("Unable to create PAC");
+ free(name);
free(metadata);
return false;
}
--
2.44.0
Error: RESOURCE_LEAK (CWE-772): [#def51] [important]
bluez-5.75/src/main.c:451:2: alloc_arg: "parse_config_string" allocates memory that is stored into "str".
bluez-5.75/src/main.c:454:2: identity_transfer: Passing "str" as argument 1 to function "strtol", which sets "endptr" to that argument.
bluez-5.75/src/main.c:456:3: noescape: Assuming resource "str" is not freed or pointed-to as ellipsis argument to "btd_error".
bluez-5.75/src/main.c:457:3: leaked_storage: Variable "endptr" going out of scope leaks the storage it points to.
bluez-5.75/src/main.c:457:3: leaked_storage: Variable "str" going out of scope leaks the storage it points to.
455| if (!endptr || *endptr != '\0') {
456| error("%s.%s = %s is not integer", group, key, str);
457|-> return false;
458| }
459|
Error: RESOURCE_LEAK (CWE-772): [#def52] [important]
bluez-5.75/src/main.c:451:2: alloc_arg: "parse_config_string" allocates memory that is stored into "str".
bluez-5.75/src/main.c:454:2: identity_transfer: Passing "str" as argument 1 to function "strtol", which sets "endptr" to that argument.
bluez-5.75/src/main.c:463:3: leaked_storage: Variable "endptr" going out of scope leaks the storage it points to.
bluez-5.75/src/main.c:463:3: leaked_storage: Variable "str" going out of scope leaks the storage it points to.
461| warn("%s.%s = %zu is out of range (< %zu)", group, key, tmp,
462| min);
463|-> return false;
464| }
465|
Error: RESOURCE_LEAK (CWE-772): [#def53] [important]
bluez-5.75/src/main.c:451:2: alloc_arg: "parse_config_string" allocates memory that is stored into "str".
bluez-5.75/src/main.c:454:2: identity_transfer: Passing "str" as argument 1 to function "strtol", which sets "endptr" to that argument.
bluez-5.75/src/main.c:475:2: leaked_storage: Variable "endptr" going out of scope leaks the storage it points to.
bluez-5.75/src/main.c:475:2: leaked_storage: Variable "str" going out of scope leaks the storage it points to.
473| *val = tmp;
474|
475|-> return true;
476| }
477|
---
src/main.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/main.c b/src/main.c
index 23af6781d931..ac840d684f6d 100644
--- a/src/main.c
+++ b/src/main.c
@@ -454,21 +454,25 @@ static bool parse_config_int(GKeyFile *config, const char *group,
tmp = strtol(str, &endptr, 0);
if (!endptr || *endptr != '\0') {
error("%s.%s = %s is not integer", group, key, str);
+ g_free(str);
return false;
}
if (tmp < min) {
+ g_free(str);
warn("%s.%s = %zu is out of range (< %zu)", group, key, tmp,
min);
return false;
}
if (tmp > max) {
+ g_free(str);
warn("%s.%s = %zu is out of range (> %zu)", group, key, tmp,
max);
return false;
}
+ g_free(str);
if (val)
*val = tmp;
--
2.44.0