[BlueZ,resend,1/9] main: Simplify parse_config_string()

Commit Message

Bastien Nocera July 2, 2024, 2:23 p.m. UTC

The memory management done by parse_config_string() was quite
complicated, as it expected to be able to free the value in the return
variable if it was already allocated.

That particular behaviour was only used for a single variable which was
set to its default value during startup and might be overwritten after
this function call.

Use an intermediate variable to check whether we need to free
btd_opts.name and simplify parse_config_string().

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

Comments

Luiz Augusto von Dentz July 2, 2024, 3:08 p.m. UTC | #1

Hi Bastien,

On Tue, Jul 2, 2024 at 10:25 AM Bastien Nocera <[e-mail]> wrote:
>
> The memory management done by parse_config_string() was quite
> complicated, as it expected to be able to free the value in the return
> variable if it was already allocated.
>
> That particular behaviour was only used for a single variable which was
> set to its default value during startup and might be overwritten after
> this function call.
>
> Use an intermediate variable to check whether we need to free
> btd_opts.name and simplify parse_config_string().
>
> 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 62453bffaf57..9db8d7000490 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);

Nah, let's just use if (!val) return false as we normally do.

> +
> +       *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;
>  }
> @@ -1004,7 +1000,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;
> +       }

Yeah, I skip this on purpose, I don't quite like the idea that we have
to do this for 1 specific entry, perhaps the better option is to
incorporate the default values into the table table so we avoid having
this problem of having to free like this.

>         parse_config_hex(config, "General", "Class", &btd_opts.class);
>         parse_config_u32(config, "General", "DiscoverableTimeout",
>                                                 &btd_opts.discovto,
> --
> 2.45.2
>
>

[e-mail] July 2, 2024, 7:43 p.m. UTC | #2

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=867580

---Test result---

Test Summary:
CheckPatch                    FAIL      3.64 seconds
GitLint                       FAIL      2.63 seconds
BuildEll                      PASS      24.86 seconds
BluezMake                     PASS      1747.83 seconds
MakeCheck                     PASS      13.68 seconds
MakeDistcheck                 PASS      181.65 seconds
CheckValgrind                 PASS      257.62 seconds
CheckSmatch                   PASS      365.38 seconds
bluezmakeextell               PASS      121.48 seconds
IncrementalBuild              PASS      14618.82 seconds
ScanBuild                     WARNING   1034.43 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,resend,2/9] avdtp: Fix manipulating struct as an array
WARNING:LINE_SPACING: Missing a blank line after declarations
#134: FILE: profiles/audio/avdtp.c:1684:
+		struct seid seid = start->seids[i];
+		if (seid.seid == id) {

WARNING:LINE_SPACING: Missing a blank line after declarations
#150: FILE: profiles/audio/avdtp.c:1699:
+		struct seid seid = suspend->seids[i];
+		if (seid.seid == id) {

WARNING:LINE_SPACING: Missing a blank line after declarations
#194: FILE: profiles/audio/avdtp.c:1916:
+		struct seid seid = req->seids[i];
+		failed_seid = seid.seid;

/github/workspace/src/src/13719809.patch total: 0 errors, 3 warnings, 105 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13719809.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,resend,1/9] main: Simplify parse_config_string()

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
15: B1 Line exceeds max length (108>80): "bluez-5.75/src/main.c:425:2: alloc_fn: Storage is returned from allocation function "g_key_file_get_string"."
16: B1 Line exceeds max length (132>80): "bluez-5.75/src/main.c:425:2: var_assign: Assigning: "tmp" = storage returned from "g_key_file_get_string(config, group, key, &err)"."
17: B1 Line exceeds max length (126>80): "bluez-5.75/src/main.c:433:2: noescape: Assuming resource "tmp" is not freed or pointed-to as ellipsis argument to "btd_debug"."
18: B1 Line exceeds max length (110>80): "bluez-5.75/src/main.c:440:2: leaked_storage: Variable "tmp" going out of scope leaks the storage it points to."
19: B3 Line contains hard tab characters (\t): "438|	}"
21: B3 Line contains hard tab characters (\t): "440|->	return true;"
[BlueZ,resend,2/9] avdtp: Fix manipulating struct as an array

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
7: B1 Line exceeds max length (122>80): "bluez-5.76/profiles/audio/avdtp.c:1675:2: address_of: Taking address with "&start->first_seid" yields a singleton pointer."
8: B1 Line exceeds max length (91>80): "bluez-5.76/profiles/audio/avdtp.c:1675:2: assign: Assigning: "seid" = "&start->first_seid"."
9: B1 Line exceeds max length (142>80): "bluez-5.76/profiles/audio/avdtp.c:1679:25: ptr_arith: Using "seid" as an array.  This might corrupt or misinterpret adjacent memory locations."
17: B1 Line exceeds max length (124>80): "bluez-5.76/profiles/audio/avdtp.c:1690:2: address_of: Taking address with "&suspend->first_seid" yields a singleton pointer."
18: B1 Line exceeds max length (93>80): "bluez-5.76/profiles/audio/avdtp.c:1690:2: assign: Assigning: "seid" = "&suspend->first_seid"."
19: B1 Line exceeds max length (142>80): "bluez-5.76/profiles/audio/avdtp.c:1694:25: ptr_arith: Using "seid" as an array.  This might corrupt or misinterpret adjacent memory locations."
20: B3 Line contains hard tab characters (\t): "1692|		int i;"
22: B3 Line contains hard tab characters (\t): "1694|->		for (i = 0; i < count; i++, seid++) {"
23: B3 Line contains hard tab characters (\t): "1695|			if (seid->seid == id) {"
24: B3 Line contains hard tab characters (\t): "1696|				req->collided = TRUE;"
27: B1 Line exceeds max length (120>80): "bluez-5.76/profiles/audio/avdtp.c:1799:2: address_of: Taking address with "&req->first_seid" yields a singleton pointer."
28: B1 Line exceeds max length (89>80): "bluez-5.76/profiles/audio/avdtp.c:1799:2: assign: Assigning: "seid" = "&req->first_seid"."
29: B1 Line exceeds max length (142>80): "bluez-5.76/profiles/audio/avdtp.c:1801:30: ptr_arith: Using "seid" as an array.  This might corrupt or misinterpret adjacent memory locations."
30: B3 Line contains hard tab characters (\t): "1799|		seid = &req->first_seid;"
32: B3 Line contains hard tab characters (\t): "1801|->		for (i = 0; i < seid_count; i++, seid++) {"
33: B3 Line contains hard tab characters (\t): "1802|			failed_seid = seid->seid;"
37: B1 Line exceeds max length (120>80): "bluez-5.76/profiles/audio/avdtp.c:1912:2: address_of: Taking address with "&req->first_seid" yields a singleton pointer."
38: B1 Line exceeds max length (89>80): "bluez-5.76/profiles/audio/avdtp.c:1912:2: assign: Assigning: "seid" = "&req->first_seid"."
39: B1 Line exceeds max length (142>80): "bluez-5.76/profiles/audio/avdtp.c:1914:30: ptr_arith: Using "seid" as an array.  This might corrupt or misinterpret adjacent memory locations."
40: B3 Line contains hard tab characters (\t): "1912|		seid = &req->first_seid;"
42: B3 Line contains hard tab characters (\t): "1914|->	for (i = 0; i < seid_count; i++, seid++) {"
43: B3 Line contains hard tab characters (\t): "1915|			failed_seid = seid->seid;"
[BlueZ,resend,3/9] mesh: Avoid accessing array out-of-bounds

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
8: B1 Line exceeds max length (133>80): "bluez-5.76/mesh/prov-initiator.c:676:2: cond_at_least: Checking "type >= 10UL" implies that "type" is at least 10 on the true branch."
9: B1 Line exceeds max length (195>80): "bluez-5.76/mesh/prov-initiator.c:678:3: overrun-local: Overrunning array "expected_pdu_size" of 10 2-byte elements at element index 10 (byte offset 21) using index "type" (which evaluates to 10)."
10: B3 Line contains hard tab characters (\t): "676|	if (type >= L_ARRAY_SIZE(expected_pdu_size) ||"
11: B3 Line contains hard tab characters (\t): "677|					len != expected_pdu_size[type]) {"
12: B3 Line contains hard tab characters (\t): "678|->		l_error("Expected PDU size %d, Got %d (type: %2.2x)","
13: B3 Line contains hard tab characters (\t): "679|			expected_pdu_size[type], len, type);"
14: B3 Line contains hard tab characters (\t): "680|		fail_code[1] = PROV_ERR_INVALID_FORMAT;"
[BlueZ,resend,4/9] obexd: Fix possible memleak

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
7: B1 Line exceeds max length (122>80): "bluez-5.76/obexd/plugins/messages-dummy.c:362:4: alloc_fn: Storage is returned from allocation function "g_strdup_inline"."
8: B1 Line exceeds max length (141>80): "bluez-5.76/obexd/plugins/messages-dummy.c:362:4: var_assign: Assigning: "entry->handle" = storage returned from "g_strdup_inline(values[i])"."
9: B1 Line exceeds max length (189>80): "bluez-5.76/obexd/plugins/messages-dummy.c:362:4: overwrite_var: Overwriting "entry->handle" in "entry->handle = g_strdup_inline(values[i])" leaks the storage that "entry->handle" points to."
10: B3 Line contains hard tab characters (\t): "360|	for (i = 0 ; names[i]; ++i) {"
11: B3 Line contains hard tab characters (\t): "361|		if (g_strcmp0(names[i], "handle") == 0) {"
12: B3 Line contains hard tab characters (\t): "362|->			entry->handle = g_strdup(values[i]);"
13: B3 Line contains hard tab characters (\t): "363|			mld->size++;"
14: B3 Line contains hard tab characters (\t): "364|			continue;"
17: B1 Line exceeds max length (122>80): "bluez-5.76/obexd/plugins/messages-dummy.c:367:4: alloc_fn: Storage is returned from allocation function "g_strdup_inline"."
18: B1 Line exceeds max length (150>80): "bluez-5.76/obexd/plugins/messages-dummy.c:367:4: var_assign: Assigning: "entry->attachment_size" = storage returned from "g_strdup_inline(values[i])"."
19: B1 Line exceeds max length (216>80): "bluez-5.76/obexd/plugins/messages-dummy.c:367:4: overwrite_var: Overwriting "entry->attachment_size" in "entry->attachment_size = g_strdup_inline(values[i])" leaks the storage that "entry->attachment_size" points to."
20: B3 Line contains hard tab characters (\t): "365|		}"
21: B3 Line contains hard tab characters (\t): "366|		if (g_strcmp0(names[i], "attachment_size") == 0) {"
22: B3 Line contains hard tab characters (\t): "367|->			entry->attachment_size = g_strdup(values[i]);"
23: B3 Line contains hard tab characters (\t): "368|			continue;"
24: B3 Line contains hard tab characters (\t): "369|		}"
27: B1 Line exceeds max length (122>80): "bluez-5.76/obexd/plugins/messages-dummy.c:371:4: alloc_fn: Storage is returned from allocation function "g_strdup_inline"."
28: B1 Line exceeds max length (143>80): "bluez-5.76/obexd/plugins/messages-dummy.c:371:4: var_assign: Assigning: "entry->datetime" = storage returned from "g_strdup_inline(values[i])"."
29: B1 Line exceeds max length (195>80): "bluez-5.76/obexd/plugins/messages-dummy.c:371:4: overwrite_var: Overwriting "entry->datetime" in "entry->datetime = g_strdup_inline(values[i])" leaks the storage that "entry->datetime" points to."
30: B3 Line contains hard tab characters (\t): "369|		}"
31: B3 Line contains hard tab characters (\t): "370|		if (g_strcmp0(names[i], "datetime") == 0) {"
32: B3 Line contains hard tab characters (\t): "371|->			entry->datetime = g_strdup(values[i]);"
33: B3 Line contains hard tab characters (\t): "372|			continue;"
34: B3 Line contains hard tab characters (\t): "373|		}"
37: B1 Line exceeds max length (122>80): "bluez-5.76/obexd/plugins/messages-dummy.c:375:4: alloc_fn: Storage is returned from allocation function "g_strdup_inline"."
38: B1 Line exceeds max length (142>80): "bluez-5.76/obexd/plugins/messages-dummy.c:375:4: var_assign: Assigning: "entry->subject" = storage returned from "g_strdup_inline(values[i])"."
39: B1 Line exceeds max length (192>80): "bluez-5.76/obexd/plugins/messages-dummy.c:375:4: overwrite_var: Overwriting "entry->subject" in "entry->subject = g_strdup_inline(values[i])" leaks the storage that "entry->subject" points to."
40: B3 Line contains hard tab characters (\t): "373|		}"
41: B3 Line contains hard tab characters (\t): "374|		if (g_strcmp0(names[i], "subject") == 0) {"
42: B3 Line contains hard tab characters (\t): "375|->			entry->subject = g_strdup(values[i]);"
43: B3 Line contains hard tab characters (\t): "376|			continue;"
44: B3 Line contains hard tab characters (\t): "377|		}"
47: B1 Line exceeds max length (122>80): "bluez-5.76/obexd/plugins/messages-dummy.c:379:4: alloc_fn: Storage is returned from allocation function "g_strdup_inline"."
48: B1 Line exceeds max length (155>80): "bluez-5.76/obexd/plugins/messages-dummy.c:379:4: var_assign: Assigning: "entry->recipient_addressing" = storage returned from "g_strdup_inline(values[i])"."
49: B1 Line exceeds max length (231>80): "bluez-5.76/obexd/plugins/messages-dummy.c:379:4: overwrite_var: Overwriting "entry->recipient_addressing" in "entry->recipient_addressing = g_strdup_inline(values[i])" leaks the storage that "entry->recipient_addressing" points to."
50: B3 Line contains hard tab characters (\t): "377|		}"
51: B3 Line contains hard tab characters (\t): "378|		if (g_strcmp0(names[i], "recipient_addressing") == 0) {"
52: B3 Line contains hard tab characters (\t): "379|->			entry->recipient_addressing = g_strdup(values[i]);"
53: B3 Line contains hard tab characters (\t): "380|			continue;"
54: B3 Line contains hard tab characters (\t): "381|		}"
57: B1 Line exceeds max length (122>80): "bluez-5.76/obexd/plugins/messages-dummy.c:383:4: alloc_fn: Storage is returned from allocation function "g_strdup_inline"."
58: B1 Line exceeds max length (152>80): "bluez-5.76/obexd/plugins/messages-dummy.c:383:4: var_assign: Assigning: "entry->sender_addressing" = storage returned from "g_strdup_inline(values[i])"."
59: B1 Line exceeds max length (222>80): "bluez-5.76/obexd/plugins/messages-dummy.c:383:4: overwrite_var: Overwriting "entry->sender_addressing" in "entry->sender_addressing = g_strdup_inline(values[i])" leaks the storage that "entry->sender_addressing" points to."
60: B3 Line contains hard tab characters (\t): "381|		}"
61: B3 Line contains hard tab characters (\t): "382|		if (g_strcmp0(names[i], "sender_addressing") == 0) {"
62: B3 Line contains hard tab characters (\t): "383|->			entry->sender_addressing = g_strdup(values[i]);"
63: B3 Line contains hard tab characters (\t): "384|			continue;"
64: B3 Line contains hard tab characters (\t): "385|		}"
67: B1 Line exceeds max length (122>80): "bluez-5.76/obexd/plugins/messages-dummy.c:387:4: alloc_fn: Storage is returned from allocation function "g_strdup_inline"."
68: B1 Line exceeds max length (139>80): "bluez-5.76/obexd/plugins/messages-dummy.c:387:4: var_assign: Assigning: "entry->type" = storage returned from "g_strdup_inline(values[i])"."
69: B1 Line exceeds max length (183>80): "bluez-5.76/obexd/plugins/messages-dummy.c:387:4: overwrite_var: Overwriting "entry->type" in "entry->type = g_strdup_inline(values[i])" leaks the storage that "entry->type" points to."
70: B3 Line contains hard tab characters (\t): "385|		}"
71: B3 Line contains hard tab characters (\t): "386|		if (g_strcmp0(names[i], "type") == 0) {"
72: B3 Line contains hard tab characters (\t): "387|->			entry->type = g_strdup(values[i]);"
73: B3 Line contains hard tab characters (\t): "388|			continue;"
74: B3 Line contains hard tab characters (\t): "389|		}"
77: B1 Line exceeds max length (122>80): "bluez-5.76/obexd/plugins/messages-dummy.c:391:4: alloc_fn: Storage is returned from allocation function "g_strdup_inline"."
78: B1 Line exceeds max length (151>80): "bluez-5.76/obexd/plugins/messages-dummy.c:391:4: var_assign: Assigning: "entry->reception_status" = storage returned from "g_strdup_inline(values[i])"."
79: B1 Line exceeds max length (219>80): "bluez-5.76/obexd/plugins/messages-dummy.c:391:4: overwrite_var: Overwriting "entry->reception_status" in "entry->reception_status = g_strdup_inline(values[i])" leaks the storage that "entry->reception_status" points to."
80: B3 Line contains hard tab characters (\t): "389|		}"
81: B3 Line contains hard tab characters (\t): "390|		if (g_strcmp0(names[i], "reception_status") == 0)"
82: B3 Line contains hard tab characters (\t): "391|->			entry->reception_status = g_strdup(values[i]);"
83: B3 Line contains hard tab characters (\t): "392|	}"
[BlueZ,resend,5/9] obexd: Fix memory leak in entry struct

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
6: B1 Line exceeds max length (122>80): "bluez-5.76/obexd/plugins/messages-dummy.c:379:4: alloc_fn: Storage is returned from allocation function "g_strdup_inline"."
7: B1 Line exceeds max length (155>80): "bluez-5.76/obexd/plugins/messages-dummy.c:379:4: var_assign: Assigning: "entry->recipient_addressing" = storage returned from "g_strdup_inline(values[i])"."
8: B1 Line exceeds max length (194>80): "bluez-5.76/obexd/plugins/messages-dummy.c:404:2: leaked_storage: Freeing "entry" without freeing its pointer field "recipient_addressing" leaks the storage that "recipient_addressing" points to."
9: B3 Line contains hard tab characters (\t): "402|	g_free(entry->attachment_size);"
10: B3 Line contains hard tab characters (\t): "403|	g_free(entry->handle);"
11: B3 Line contains hard tab characters (\t): "404|->	g_free(entry);"
[BlueZ,resend,6/9] obexd: Fix leak in backup_object struct

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
4: B1 Line exceeds max length (119>80): "bluez-5.76/obexd/plugins/pcsuite.c:370:2: alloc_fn: Storage is returned from allocation function "g_path_get_basename"."
5: B1 Line exceeds max length (128>80): "bluez-5.76/obexd/plugins/pcsuite.c:370:2: var_assign: Assigning: "obj->cmd" = storage returned from "g_path_get_basename(name)"."
6: B1 Line exceeds max length (151>80): "bluez-5.76/obexd/plugins/pcsuite.c:379:3: leaked_storage: Freeing "obj" without freeing its pointer field "cmd" leaks the storage that "cmd" points to."
8: B3 Line contains hard tab characters (\t): "378|	if (send_backup_dbus_message("open", obj, size) == FALSE) {"
9: B3 Line contains hard tab characters (\t): "379|->		g_free(obj);"
10: B3 Line contains hard tab characters (\t): "380|		obj = NULL;"
11: B3 Line contains hard tab characters (\t): "381|	}"
[BlueZ,resend,7/9] health/mcap: Fix memory leak in mcl struct

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
4: B1 Line exceeds max length (117>80): "bluez-5.76/profiles/health/mcap.c:2052:3: alloc_arg: "set_default_cb" allocates memory that is stored into "mcl->cb"."
5: B1 Line exceeds max length (149>80): "bluez-5.76/profiles/health/mcap.c:2055:4: leaked_storage: Freeing "mcl" without freeing its pointer field "cb" leaks the storage that "cb" points to."
6: B3 Line contains hard tab characters (\t): "2053|			if (util_getrandom(&val, sizeof(val), 0) < 0) {"
7: B3 Line contains hard tab characters (\t): "2054|				mcap_instance_unref(mcl->mi);"
8: B3 Line contains hard tab characters (\t): "2055|->				g_free(mcl);"
9: B3 Line contains hard tab characters (\t): "2056|				goto drop;"
10: B3 Line contains hard tab characters (\t): "2057|			}"
[BlueZ,resend,8/9] sdp: Fix memory leak in sdp_data_alloc*()

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
7: B1 Line exceeds max length (100>80): "bluez-5.76/lib/sdp.c:542:4: alloc_fn: Storage is returned from allocation function "sdp_data_alloc"."
8: B1 Line exceeds max length (115>80): "bluez-5.76/lib/sdp.c:542:4: var_assign: Assigning: "data" = storage returned from "sdp_data_alloc(dtd, values[i])"."
13: B1 Line exceeds max length (109>80): "bluez-5.76/lib/sdp.c:545:4: leaked_storage: Variable "seq" going out of scope leaks the storage it points to."
15: B3 Line contains hard tab characters (\t): "544|		if (!data)"
16: B3 Line contains hard tab characters (\t): "545|->			return NULL;"
18: B3 Line contains hard tab characters (\t): "547|		if (curr)"
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
lib/sdp.c:509:16: warning: Dereference of undefined pointer value
                int8_t dtd = *(uint8_t *) dtds[i];
                             ^~~~~~~~~~~~~~~~~~~~
lib/sdp.c:539:17: warning: Dereference of undefined pointer value
                uint8_t dtd = *(uint8_t *) dtds[i];
                              ^~~~~~~~~~~~~~~~~~~~
lib/sdp.c:586:12: warning: Access to field 'attrId' results in a dereference of a null pointer (loaded from variable 'd')
        d->attrId = attr;
        ~         ^
lib/sdp.c:967:10: warning: Access to field 'dtd' results in a dereference of a null pointer (loaded from variable 'd')
        switch (d->dtd) {
                ^~~~~~
lib/sdp.c:1881:26: warning: Potential leak of memory pointed to by 'ap'
        for (; pdlist; pdlist = pdlist->next) {
                                ^~~~~~
lib/sdp.c:1895:6: warning: Potential leak of memory pointed to by 'pds'
                ap = sdp_list_append(ap, pds);
                ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
lib/sdp.c:1940:10: warning: Potential leak of memory pointed to by 'u'
                        *seqp = sdp_list_append(*seqp, u);
                        ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/sdp.c:2045:4: warning: Potential leak of memory pointed to by 'lang'
                        sdp_list_free(*langSeq, free);
                        ^~~~~~~~~~~~~
lib/sdp.c:2134:9: warning: Potential leak of memory pointed to by 'profDesc'
        return 0;
               ^
lib/sdp.c:3266:8: warning: Potential leak of memory pointed to by 'pSvcRec'
                pSeq = sdp_list_append(pSeq, pSvcRec);
                ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/sdp.c:3267:9: warning: Potential leak of memory pointed to by 'pSeq'
                pdata += sizeof(uint32_t);
                ~~~~~~^~~~~~~~~~~~~~~~~~~
lib/sdp.c:4603:13: warning: Potential leak of memory pointed to by 'rec_list'
                        } while (scanned < attr_list_len && pdata_len > 0);
                                 ^~~~~~~
lib/sdp.c:4899:40: warning: Potential leak of memory pointed to by 'tseq'
        for (d = sdpdata->val.dataseq; d; d = d->next) {
                                              ^
lib/sdp.c:4935:8: warning: Potential leak of memory pointed to by 'subseq'
                tseq = sdp_list_append(tseq, subseq);
                ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
14 warnings generated.
profiles/audio/avdtp.c:899:25: warning: Use of memory after it is freed
                session->prio_queue = g_slist_remove(session->prio_queue, req);
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
profiles/audio/avdtp.c:906:24: warning: Use of memory after it is freed
                session->req_queue = g_slist_remove(session->req_queue, req);
                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.



---
Regards,
Linux Bluetooth

Bastien Nocera July 3, 2024, 9:01 a.m. UTC | #3

On Tue, 2024-07-02 at 11:08 -0400, Luiz Augusto von Dentz wrote:
> > 
<snip>
> > -       tmp = g_key_file_get_string(config, group, key, &err);
> > +       g_return_val_if_fail(val, false);
> 
> Nah, let's just use if (!val) return false as we normally do.

g_return_val_if_fail() will print a warning when passed NULL as a
holder for the value, which I think is correct given that it's a
programmer error to pass NULL.

I changed this to open code a warning instead.

> > 
> >  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;
> > +       }
> 
> Yeah, I skip this on purpose, I don't quite like the idea that we
> have
> to do this for 1 specific entry, perhaps the better option is to
> incorporate the default values into the table table so we avoid
> having
> this problem of having to free like this.

It's the only option that has an already set default value that
requires allocating. Look at btd_opts in src/btd.h, and you'll see its
the only string and the only allocated memory in the struct.

This code isn't so different from parse_privacy(),
parse_multi_profile(), or many of the other calls to
parse_config_string().

I don't understand what you mean by incorporating "the default values
into the table table". If you want to have the default values in the
arrays pointed to by valid_groups[], then we would still need a special
case because this entry is the only one that requires memory
allocation.

I can send out this patch with the g_return_if_fail() removal and no
other changes if you'll take it. Otherwise it seems like too big a
change for a static analysis fix to be making, especially when that
change will probably not simplify the code we want to simplify.

> 
> >         parse_config_hex(config, "General", "Class",
> > &btd_opts.class);
> >         parse_config_u32(config, "General", "DiscoverableTimeout",
> >                                                 &btd_opts.discovto,
> > --
> > 2.45.2
> > 
> > 
> 
>

Patch

diff --git a/src/main.c b/src/main.c
index 62453bffaf57..9db8d7000490 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;
 }
@@ -1004,7 +1000,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,