| Message ID | 20210512133932.4e2b4bd0@ivy-bridge (mailing list archive) |
|---|---|
| State | New, archived |
| 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 >
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.
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(-)