| Message ID | 20240702142436.833138-1-hadess@hadess.net |
|---|---|
| Headers | show |
| Series | Fix a number of static analysis issues #4 | expand |
Hi Bastien, On Tue, Jul 2, 2024 at 10:25 AM Bastien Nocera <hadess@hadess.net> 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 > >
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 > > > > > >
Hello: This series was applied to bluetooth/bluez.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Tue, 2 Jul 2024 16:23:32 +0200 you wrote: > "main: Simplify parse_config_string()" is a repeat. If you don't want > the patch, please let me know and I'll carry it downstream. > > More fixes incoming, please review carefully, thanks! > > Re-sent with the correct prefix > > [...] Here is the summary with links: - [BlueZ,resend,1/9] main: Simplify parse_config_string() (no matching commit) - [BlueZ,resend,2/9] avdtp: Fix manipulating struct as an array https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7c896d7b73cb - [BlueZ,resend,3/9] mesh: Avoid accessing array out-of-bounds https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=3f1b3c624a96 - [BlueZ,resend,4/9] obexd: Fix possible memleak https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=99750d2acd9d - [BlueZ,resend,5/9] obexd: Fix memory leak in entry struct https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=4b3fe69df7c7 - [BlueZ,resend,6/9] obexd: Fix leak in backup_object struct https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=5475aba84edc - [BlueZ,resend,7/9] health/mcap: Fix memory leak in mcl struct https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=d79e429a9fc3 - [BlueZ,resend,8/9] sdp: Fix memory leak in sdp_data_alloc*() https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=5dcc52a486f2 - [BlueZ,resend,9/9] sdp: Check memory allocation in sdp_copy_seq() https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=1707a8362230 You are awesome, thank you!