| Message ID | 20210512133932.4e2b4bd0@ivy-bridge |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] Fix various memory leaks | expand |
Hi Steve, On Wed, May 12, 2021 at 10:58 AM Steve Grubb <sgrubb@redhat.com> wrote: > > Hello, > > I was checking the code via static analysis and found a number of > leaks throughout the code. This patch should address most of what > was discovered. Could you please split these changes into mesh, obex, etc, this should make it easier to bisect if we found there is some problem introduced by these changes. > Signed-off-by: Steve Grubb <sgrubb@redhat.com> > --- > mesh/rpl.c | 4 +++- > obexd/plugins/filesystem.c | 2 +- > obexd/plugins/ftp.c | 8 ++++++-- > obexd/plugins/messages-dummy.c | 1 + > plugins/hostname.c | 3 +-- > profiles/audio/avrcp.c | 4 +++- > src/main.c | 1 + > src/shared/shell.c | 1 + > tools/mesh-cfgclient.c | 2 +- > tools/mesh-gatt/gatt.c | 1 + > tools/mesh-gatt/node.c | 12 +++++++++--- > 11 files changed, 28 insertions(+), 11 deletions(-) > > diff --git a/mesh/rpl.c b/mesh/rpl.c > index ac0f6b6f2..c53c6fbfd 100644 > --- a/mesh/rpl.c > +++ b/mesh/rpl.c > @@ -143,8 +143,10 @@ static void get_entries(const char *iv_path, struct l_queue *rpl_list) > return; > > iv_txt = basename(iv_path); > - if (sscanf(iv_txt, "%08x", &iv_index) != 1) > + if (sscanf(iv_txt, "%08x", &iv_index) != 1) { > + closedir(dir); > return; > + } > > memset(seq_txt, 0, sizeof(seq_txt)); > > diff --git a/obexd/plugins/filesystem.c b/obexd/plugins/filesystem.c > index 09bff8ad0..44e3cf3d2 100644 > --- a/obexd/plugins/filesystem.c > +++ b/obexd/plugins/filesystem.c > @@ -415,7 +415,7 @@ static void *capability_open(const char *name, int oflag, mode_t mode, > goto fail; > } > > - object->buffer = g_string_new(buf); > + object->buffer = buf; > > if (size) > *size = object->buffer->len; > diff --git a/obexd/plugins/ftp.c b/obexd/plugins/ftp.c > index 259bfcae2..4b04bab06 100644 > --- a/obexd/plugins/ftp.c > +++ b/obexd/plugins/ftp.c > @@ -386,8 +386,10 @@ static int ftp_copy(struct ftp_session *ftp, const char *name, > ret = verify_path(destdir); > g_free(destdir); > > - if (ret < 0) > + if (ret < 0) { > + g_free(destination); > return ret; > + } > > source = g_build_filename(ftp->folder, name, NULL); > > @@ -424,8 +426,10 @@ static int ftp_move(struct ftp_session *ftp, const char *name, > ret = verify_path(destdir); > g_free(destdir); > > - if (ret < 0) > + if (ret < 0) { > + g_free(destination); > return ret; > + } > > source = g_build_filename(ftp->folder, name, NULL); > > diff --git a/obexd/plugins/messages-dummy.c b/obexd/plugins/messages-dummy.c > index 34199fa05..e37b52df6 100644 > --- a/obexd/plugins/messages-dummy.c > +++ b/obexd/plugins/messages-dummy.c > @@ -488,6 +488,7 @@ int messages_get_messages_listing(void *session, const char *name, > int err = -errno; > DBG("fopen(): %d, %s", -err, strerror(-err)); > g_free(path); > + g_free(mld); > return -EBADR; > } > } > diff --git a/plugins/hostname.c b/plugins/hostname.c > index f7ab9e8bc..1a9513adb 100644 > --- a/plugins/hostname.c > +++ b/plugins/hostname.c > @@ -213,11 +213,10 @@ static void read_dmi_fallback(void) > return; > > type = atoi(contents); > + g_free(contents); > if (type < 0 || type > 0x1D) > return; > > - g_free(contents); > - > /* from systemd hostname chassis list */ > switch (type) { > case 0x3: > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c > index c6a342ee3..58d30b24d 100644 > --- a/profiles/audio/avrcp.c > +++ b/profiles/audio/avrcp.c > @@ -3508,8 +3508,10 @@ static struct avrcp_player *create_ct_player(struct avrcp *session, > path = device_get_path(session->dev); > > mp = media_player_controller_create(path, id); > - if (mp == NULL) > + if (mp == NULL) { > + g_free(player); > return NULL; > + } > > media_player_set_callbacks(mp, &ct_cbs, player); > player->user_data = mp; > diff --git a/src/main.c b/src/main.c > index c32bda7d4..94141b1e4 100644 > --- a/src/main.c > +++ b/src/main.c > @@ -795,6 +795,7 @@ static void parse_config(GKeyFile *config) > > parse_br_config(config); > parse_le_config(config); > + g_free(str); > } > > static void init_defaults(void) > diff --git a/src/shared/shell.c b/src/shared/shell.c > index c0de1640d..f05fb2820 100644 > --- a/src/shared/shell.c > +++ b/src/shared/shell.c > @@ -611,6 +611,7 @@ void bt_shell_prompt_input(const char *label, const char *msg, > prompt->user_data = user_data; > > queue_push_tail(data.prompts, prompt); > + g_free(str); > > return; > } > diff --git a/tools/mesh-cfgclient.c b/tools/mesh-cfgclient.c > index 1eeed2a1a..49069674f 100644 > --- a/tools/mesh-cfgclient.c > +++ b/tools/mesh-cfgclient.c > @@ -914,7 +914,7 @@ static void cmd_import_node(int argc, char *argv[]) > > /* Number of elements */ > if (sscanf(argv[4], "%u", &req->arg3) != 1) > - return; > + goto fail; > > /* DevKey */ > req->data2 = l_util_from_hexstring(argv[5], &sz); > diff --git a/tools/mesh-gatt/gatt.c b/tools/mesh-gatt/gatt.c > index b99234f91..c8a8123fb 100644 > --- a/tools/mesh-gatt/gatt.c > +++ b/tools/mesh-gatt/gatt.c > @@ -525,6 +525,7 @@ bool mesh_gatt_notify(GDBusProxy *proxy, bool enable, GDBusReturnFunction cb, > notify_io_destroy(); > if (cb) > cb(NULL, user_data); > + g_free(data); > return true; > } else { > method = "StopNotify"; > diff --git a/tools/mesh-gatt/node.c b/tools/mesh-gatt/node.c > index 6afda3387..356e1cd1a 100644 > --- a/tools/mesh-gatt/node.c > +++ b/tools/mesh-gatt/node.c > @@ -396,8 +396,10 @@ bool node_parse_composition(struct mesh_node *node, uint8_t *data, uint16_t len) > uint16_t vendor_id; > struct mesh_element *ele; > ele = g_malloc0(sizeof(struct mesh_element)); > - if (!ele) > + if (!ele) { > + g_free(comp); > return false; > + } > > ele->index = i; > ele->loc = get_le16(data); > @@ -412,8 +414,10 @@ bool node_parse_composition(struct mesh_node *node, uint8_t *data, uint16_t len) > mod_id = get_le16(data); > /* initialize uppper 16 bits to 0xffff for SIG models */ > mod_id |= 0xffff0000; > - if (!node_set_model(node, ele->index, mod_id)) > + if (!node_set_model(node, ele->index, mod_id)) { > + g_free(comp); > return false; > + } > data += 2; > len -= 2; > } > @@ -421,8 +425,10 @@ bool node_parse_composition(struct mesh_node *node, uint8_t *data, uint16_t len) > mod_id = get_le16(data + 2); > vendor_id = get_le16(data); > mod_id |= (vendor_id << 16); > - if (!node_set_model(node, ele->index, mod_id)) > + if (!node_set_model(node, ele->index, mod_id)) { > + g_free(comp); > return false; > + } > data += 4; > len -= 4; > } > -- > 2.31.1 > -- Luiz Augusto von Dentz
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=481423
---Test result---
Test Summary:
CheckPatch PASS 0.45 seconds
GitLint PASS 0.12 seconds
Prep - Setup ELL PASS 38.66 seconds
Build - Prep PASS 0.10 seconds
Build - Configure PASS 6.91 seconds
Build - Make FAIL 9.37 seconds
Make Check FAIL 0.42 seconds
Make Distcheck FAIL 70.14 seconds
Build w/ext ELL - Configure PASS 6.92 seconds
Build w/ext ELL - Make FAIL 9.23 seconds
Details
##############################
Test: CheckPatch - PASS
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
##############################
Test: GitLint - PASS
Desc: Run gitlint with rule in .gitlint
##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL
##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build
##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree
##############################
Test: Build - Make - FAIL
Desc: Build the BlueZ source tree
Output:
src/shared/shell.c: In function ‘bt_shell_prompt_input’:
src/shared/shell.c:614:3: error: implicit declaration of function ‘g_free’; did you mean ‘rl_free’? [-Werror=implicit-function-declaration]
614 | g_free(str);
| ^~~~~~
| rl_free
cc1: all warnings being treated as errors
make[1]: *** [Makefile:6947: src/shared/shell.lo] Error 1
make: *** [Makefile:4128: all] Error 2
##############################
Test: Make Check - FAIL
Desc: Run 'make check'
Output:
src/shared/shell.c: In function ‘bt_shell_prompt_input’:
src/shared/shell.c:614:3: error: implicit declaration of function ‘g_free’; did you mean ‘rl_free’? [-Werror=implicit-function-declaration]
614 | g_free(str);
| ^~~~~~
| rl_free
cc1: all warnings being treated as errors
make[1]: *** [Makefile:6947: src/shared/shell.lo] Error 1
make: *** [Makefile:10398: check] Error 2
##############################
Test: Make Distcheck - FAIL
Desc: Run distcheck to check the distribution
Output:
../../src/shared/shell.c: In function ‘bt_shell_prompt_input’:
../../src/shared/shell.c:614:3: warning: implicit declaration of function ‘g_free’; did you mean ‘rl_free’? [-Wimplicit-function-declaration]
614 | g_free(str);
| ^~~~~~
| rl_free
/usr/bin/ld: src/.libs/libshared-ell.a(shell.o): in function `bt_shell_prompt_input':
/github/workspace/src/bluez-5.58/_build/sub/../../src/shared/shell.c:614: undefined reference to `g_free'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:5956: tools/mesh-cfgclient] Error 1
make[1]: *** [Makefile:4128: all] Error 2
make: *** [Makefile:10319: distcheck] Error 1
##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration
##############################
Test: Build w/ext ELL - Make - FAIL
Desc: Build BlueZ source with '--enable-external-ell' configuration
Output:
src/shared/shell.c: In function ‘bt_shell_prompt_input’:
src/shared/shell.c:614:3: error: implicit declaration of function ‘g_free’; did you mean ‘rl_free’? [-Werror=implicit-function-declaration]
614 | g_free(str);
| ^~~~~~
| rl_free
cc1: all warnings being treated as errors
make[1]: *** [Makefile:6947: src/shared/shell.lo] Error 1
make: *** [Makefile:4128: all] Error 2
---
Regards,
Linux Bluetooth
On Wed, 2021-05-12 at 13:39 -0400, Steve Grubb wrote: > diff --git a/obexd/plugins/filesystem.c b/obexd/plugins/filesystem.c > index 09bff8ad0..44e3cf3d2 100644 > --- a/obexd/plugins/filesystem.c > +++ b/obexd/plugins/filesystem.c > @@ -415,7 +415,7 @@ static void *capability_open(const char *name, > int oflag, mode_t mode, > goto fail; > } > > - object->buffer = g_string_new(buf); > + object->buffer = buf; > > if (size) > *size = object->buffer->len; Pretty certain this is wrong. It might be best to split the patch up into its individual parts so it's easier to merge the non-broken ones.
On Wednesday, May 12, 2021 6:35:54 PM EDT Bastien Nocera wrote: > On Wed, 2021-05-12 at 13:39 -0400, Steve Grubb wrote: > > diff --git a/obexd/plugins/filesystem.c b/obexd/plugins/filesystem.c > > index 09bff8ad0..44e3cf3d2 100644 > > --- a/obexd/plugins/filesystem.c > > +++ b/obexd/plugins/filesystem.c > > @@ -415,7 +415,7 @@ static void *capability_open(const char *name, > > int oflag, mode_t mode, > > goto fail; > > } > > > > - object->buffer = g_string_new(buf); > > + object->buffer = buf; > > > > if (size) > > *size = object->buffer->len; > > Pretty certain this is wrong. Yeah, now that you mention it...that is a GString object. I guess we g_free(buf) right after the g_string_new(). Should I resend just that one patch or do I need to regenerate all 7 emails? -Steve
Hi Steve, On Wed, May 12, 2021 at 7:12 PM Steve Grubb <sgrubb@redhat.com> wrote: > > On Wednesday, May 12, 2021 6:35:54 PM EDT Bastien Nocera wrote: > > On Wed, 2021-05-12 at 13:39 -0400, Steve Grubb wrote: > > > diff --git a/obexd/plugins/filesystem.c b/obexd/plugins/filesystem.c > > > index 09bff8ad0..44e3cf3d2 100644 > > > --- a/obexd/plugins/filesystem.c > > > +++ b/obexd/plugins/filesystem.c > > > @@ -415,7 +415,7 @@ static void *capability_open(const char *name, > > > int oflag, mode_t mode, > > > goto fail; > > > } > > > > > > - object->buffer = g_string_new(buf); > > > + object->buffer = buf; > > > > > > if (size) > > > *size = object->buffer->len; > > > > Pretty certain this is wrong. > > Yeah, now that you mention it...that is a GString object. I guess we > g_free(buf) right after the g_string_new(). Should I resend just that one > patch or do I need to regenerate all 7 emails? Please resend as v7, also while at it remove the signed-off-by as we don't use that in userspace. -- Luiz Augusto von Dentz
diff --git a/mesh/rpl.c b/mesh/rpl.c index ac0f6b6f2..c53c6fbfd 100644 --- a/mesh/rpl.c +++ b/mesh/rpl.c @@ -143,8 +143,10 @@ static void get_entries(const char *iv_path, struct l_queue *rpl_list) return; iv_txt = basename(iv_path); - if (sscanf(iv_txt, "%08x", &iv_index) != 1) + if (sscanf(iv_txt, "%08x", &iv_index) != 1) { + closedir(dir); return; + } memset(seq_txt, 0, sizeof(seq_txt)); diff --git a/obexd/plugins/filesystem.c b/obexd/plugins/filesystem.c index 09bff8ad0..44e3cf3d2 100644 --- a/obexd/plugins/filesystem.c +++ b/obexd/plugins/filesystem.c @@ -415,7 +415,7 @@ static void *capability_open(const char *name, int oflag, mode_t mode, goto fail; } - object->buffer = g_string_new(buf); + object->buffer = buf; if (size) *size = object->buffer->len; diff --git a/obexd/plugins/ftp.c b/obexd/plugins/ftp.c index 259bfcae2..4b04bab06 100644 --- a/obexd/plugins/ftp.c +++ b/obexd/plugins/ftp.c @@ -386,8 +386,10 @@ static int ftp_copy(struct ftp_session *ftp, const char *name, ret = verify_path(destdir); g_free(destdir); - if (ret < 0) + if (ret < 0) { + g_free(destination); return ret; + } source = g_build_filename(ftp->folder, name, NULL); @@ -424,8 +426,10 @@ static int ftp_move(struct ftp_session *ftp, const char *name, ret = verify_path(destdir); g_free(destdir); - if (ret < 0) + if (ret < 0) { + g_free(destination); return ret; + } source = g_build_filename(ftp->folder, name, NULL); diff --git a/obexd/plugins/messages-dummy.c b/obexd/plugins/messages-dummy.c index 34199fa05..e37b52df6 100644 --- a/obexd/plugins/messages-dummy.c +++ b/obexd/plugins/messages-dummy.c @@ -488,6 +488,7 @@ int messages_get_messages_listing(void *session, const char *name, int err = -errno; DBG("fopen(): %d, %s", -err, strerror(-err)); g_free(path); + g_free(mld); return -EBADR; } } diff --git a/plugins/hostname.c b/plugins/hostname.c index f7ab9e8bc..1a9513adb 100644 --- a/plugins/hostname.c +++ b/plugins/hostname.c @@ -213,11 +213,10 @@ static void read_dmi_fallback(void) return; type = atoi(contents); + g_free(contents); if (type < 0 || type > 0x1D) return; - g_free(contents); - /* from systemd hostname chassis list */ switch (type) { case 0x3: diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c index c6a342ee3..58d30b24d 100644 --- a/profiles/audio/avrcp.c +++ b/profiles/audio/avrcp.c @@ -3508,8 +3508,10 @@ static struct avrcp_player *create_ct_player(struct avrcp *session, path = device_get_path(session->dev); mp = media_player_controller_create(path, id); - if (mp == NULL) + if (mp == NULL) { + g_free(player); return NULL; + } media_player_set_callbacks(mp, &ct_cbs, player); player->user_data = mp; diff --git a/src/main.c b/src/main.c index c32bda7d4..94141b1e4 100644 --- a/src/main.c +++ b/src/main.c @@ -795,6 +795,7 @@ static void parse_config(GKeyFile *config) parse_br_config(config); parse_le_config(config); + g_free(str); } static void init_defaults(void) diff --git a/src/shared/shell.c b/src/shared/shell.c index c0de1640d..f05fb2820 100644 --- a/src/shared/shell.c +++ b/src/shared/shell.c @@ -611,6 +611,7 @@ void bt_shell_prompt_input(const char *label, const char *msg, prompt->user_data = user_data; queue_push_tail(data.prompts, prompt); + g_free(str); return; } diff --git a/tools/mesh-cfgclient.c b/tools/mesh-cfgclient.c index 1eeed2a1a..49069674f 100644 --- a/tools/mesh-cfgclient.c +++ b/tools/mesh-cfgclient.c @@ -914,7 +914,7 @@ static void cmd_import_node(int argc, char *argv[]) /* Number of elements */ if (sscanf(argv[4], "%u", &req->arg3) != 1) - return; + goto fail; /* DevKey */ req->data2 = l_util_from_hexstring(argv[5], &sz); diff --git a/tools/mesh-gatt/gatt.c b/tools/mesh-gatt/gatt.c index b99234f91..c8a8123fb 100644 --- a/tools/mesh-gatt/gatt.c +++ b/tools/mesh-gatt/gatt.c @@ -525,6 +525,7 @@ bool mesh_gatt_notify(GDBusProxy *proxy, bool enable, GDBusReturnFunction cb, notify_io_destroy(); if (cb) cb(NULL, user_data); + g_free(data); return true; } else { method = "StopNotify"; diff --git a/tools/mesh-gatt/node.c b/tools/mesh-gatt/node.c index 6afda3387..356e1cd1a 100644 --- a/tools/mesh-gatt/node.c +++ b/tools/mesh-gatt/node.c @@ -396,8 +396,10 @@ bool node_parse_composition(struct mesh_node *node, uint8_t *data, uint16_t len) uint16_t vendor_id; struct mesh_element *ele; ele = g_malloc0(sizeof(struct mesh_element)); - if (!ele) + if (!ele) { + g_free(comp); return false; + } ele->index = i; ele->loc = get_le16(data); @@ -412,8 +414,10 @@ bool node_parse_composition(struct mesh_node *node, uint8_t *data, uint16_t len) mod_id = get_le16(data); /* initialize uppper 16 bits to 0xffff for SIG models */ mod_id |= 0xffff0000; - if (!node_set_model(node, ele->index, mod_id)) + if (!node_set_model(node, ele->index, mod_id)) { + g_free(comp); return false; + } data += 2; len -= 2; } @@ -421,8 +425,10 @@ bool node_parse_composition(struct mesh_node *node, uint8_t *data, uint16_t len) mod_id = get_le16(data + 2); vendor_id = get_le16(data); mod_id |= (vendor_id << 16); - if (!node_set_model(node, ele->index, mod_id)) + if (!node_set_model(node, ele->index, mod_id)) { + g_free(comp); return false; + } data += 4; len -= 4; }
Hello, I was checking the code via static analysis and found a number of leaks throughout the code. This patch should address most of what was discovered. Signed-off-by: Steve Grubb <sgrubb@redhat.com> --- mesh/rpl.c | 4 +++- obexd/plugins/filesystem.c | 2 +- obexd/plugins/ftp.c | 8 ++++++-- obexd/plugins/messages-dummy.c | 1 + plugins/hostname.c | 3 +-- profiles/audio/avrcp.c | 4 +++- src/main.c | 1 + src/shared/shell.c | 1 + tools/mesh-cfgclient.c | 2 +- tools/mesh-gatt/gatt.c | 1 + tools/mesh-gatt/node.c | 12 +++++++++--- 11 files changed, 28 insertions(+), 11 deletions(-)