2020-11-20 20:09:11

by Tedd Ho-Jeong An

[permalink] [raw]
Subject: [PATCH 1/6] monitor: Fix potential memory leak

If the mainloop_add_fd() returns with failure, the destroy callback is
never called so any reosurces need to be released never freed/closed.

This potential leakage is checked with valgrind after failing the
mainloop_add_fd() function manually.

==258684== 1,500 bytes in 1 blocks are definitely lost in loss record 3 of 3
==258684== at 0x483BB1A: calloc (vg_replace_malloc.c:760)
==258684== by 0x123F1A: open_channel (control.c:1058)
==258684== by 0x125B09: control_tracing (control.c:1540)
==258684== by 0x122764: main (main.c:255)
==258684==
==258684== LEAK SUMMARY:
==258684== definitely lost: 1,500 bytes in 1 blocks
==258684== indirectly lost: 0 bytes in 0 blocks
==258684== possibly lost: 0 bytes in 0 blocks
==258684== still reachable: 48 bytes in 2 blocks
==258684== suppressed: 0 bytes in 0 blocks

This patch frees/closes the resources if the function returns with
failure.
---
monitor/control.c | 20 +++++++++++++++++---
monitor/hcidump.c | 14 +++++++++++---
2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/monitor/control.c b/monitor/control.c
index 962da4980..d1ba97d37 100644
--- a/monitor/control.c
+++ b/monitor/control.c
@@ -1071,7 +1071,12 @@ static int open_channel(uint16_t channel)
if (filter_index != HCI_DEV_NONE)
attach_index_filter(data->fd, filter_index);

- mainloop_add_fd(data->fd, EPOLLIN, data_callback, data, free_data);
+ if (mainloop_add_fd(data->fd, EPOLLIN, data_callback,
+ data, free_data) < 0) {
+ close(data->fd);
+ free(data);
+ return -1;
+ };

return 0;
}
@@ -1148,7 +1153,11 @@ static void server_accept_callback(int fd, uint32_t events, void *user_data)
data->channel = HCI_CHANNEL_MONITOR;
data->fd = nfd;

- mainloop_add_fd(data->fd, EPOLLIN, client_callback, data, free_data);
+ if (mainloop_add_fd(data->fd, EPOLLIN, client_callback,
+ data, free_data) < 0) {
+ close(data->fd);
+ free(data);
+ }
}

static int server_fd = -1;
@@ -1399,7 +1408,12 @@ int control_tty(const char *path, unsigned int speed)
data->channel = HCI_CHANNEL_MONITOR;
data->fd = fd;

- mainloop_add_fd(data->fd, EPOLLIN, tty_callback, data, free_data);
+ if (mainloop_add_fd(data->fd, EPOLLIN, tty_callback,
+ data, free_data) < 0) {
+ close(data->fd);
+ free(data);
+ return -1;
+ }

return 0;
}
diff --git a/monitor/hcidump.c b/monitor/hcidump.c
index 690b9b913..fac9c8a08 100644
--- a/monitor/hcidump.c
+++ b/monitor/hcidump.c
@@ -184,7 +184,11 @@ static void open_device(uint16_t index)
return;
}

- mainloop_add_fd(data->fd, EPOLLIN, device_callback, data, free_data);
+ if (mainloop_add_fd(data->fd, EPOLLIN, device_callback,
+ data, free_data) < 0) {
+ close(data->fd);
+ free(data);
+ }
}

static void device_info(int fd, uint16_t index, uint8_t *type, uint8_t *bus,
@@ -393,8 +397,12 @@ int hcidump_tracing(void)
return -1;
}

- mainloop_add_fd(data->fd, EPOLLIN, stack_internal_callback,
- data, free_data);
+ if (mainloop_add_fd(data->fd, EPOLLIN, stack_internal_callback,
+ data, free_data) < 0) {
+ close(data->fd);
+ free(data);
+ return -1;
+ }

return 0;
}
--
2.25.4


2020-11-20 20:11:08

by Tedd Ho-Jeong An

[permalink] [raw]
Subject: [PATCH 5/6] profile/bnep: Fix the unchecked return value

This patch fixes the unchecked return value.
---
profiles/network/bnep.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c
index 4dde55786..7e777e29c 100644
--- a/profiles/network/bnep.c
+++ b/profiles/network/bnep.c
@@ -255,7 +255,11 @@ static gboolean bnep_setup_cb(GIOChannel *chan, GIOCondition cond,

memset(&timeo, 0, sizeof(timeo));
timeo.tv_sec = 0;
- setsockopt(sk, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo));
+ if (setsockopt(sk, SOL_SOCKET, SO_RCVTIMEO, &timeo,
+ sizeof(timeo)) < 0) {
+ error("bnep: Set setsockopt failed: %s", strerror(errno));
+ goto failed;
+ };

sk = g_io_channel_unix_get_fd(session->io);
if (bnep_connadd(sk, session->src, session->iface) < 0)
--
2.25.4

2020-11-20 20:11:08

by Tedd Ho-Jeong An

[permalink] [raw]
Subject: [PATCH 3/6] btio: Fix the unchecked return value

This patch fixes the unchecked return value.
---
btio/btio.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/btio/btio.c b/btio/btio.c
index c18b6a012..8230212b4 100644
--- a/btio/btio.c
+++ b/btio/btio.c
@@ -1701,8 +1701,11 @@ GIOChannel *bt_io_connect(BtIOConnect connect, gpointer user_data,

/* Use DEFER_SETUP when connecting using Ext-Flowctl */
if (opts.mode == BT_IO_MODE_EXT_FLOWCTL && opts.defer) {
- setsockopt(sock, SOL_BLUETOOTH, BT_DEFER_SETUP, &opts.defer,
- sizeof(opts.defer));
+ if (setsockopt(sock, SOL_BLUETOOTH, BT_DEFER_SETUP,
+ &opts.defer, sizeof(opts.defer)) < 0) {
+ ERROR_FAILED(gerr, "setsockopt(BT_DEFER_SETUP)", errno);
+ return NULL;
+ }
}

switch (opts.type) {
@@ -1761,8 +1764,11 @@ GIOChannel *bt_io_listen(BtIOConnect connect, BtIOConfirm confirm,
sock = g_io_channel_unix_get_fd(io);

if (confirm)
- setsockopt(sock, SOL_BLUETOOTH, BT_DEFER_SETUP, &opts.defer,
- sizeof(opts.defer));
+ if (setsockopt(sock, SOL_BLUETOOTH, BT_DEFER_SETUP,
+ &opts.defer, sizeof(opts.defer)) < 0) {
+ ERROR_FAILED(err, "setsockopt(BT_DEFER_SETUP)", errno);
+ return NULL;
+ }

if (listen(sock, 5) < 0) {
ERROR_FAILED(err, "listen", errno);
--
2.25.4

2020-11-20 20:29:11

by bluez.test.bot

[permalink] [raw]
Subject: RE: [1/6] monitor: Fix potential memory leak

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=388665

---Test result---

##############################
Test: CheckPatch - FAIL
Output:
monitor: Fix potential memory leak
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#12:
==258684== 1,500 bytes in 1 blocks are definitely lost in loss record 3 of 3

- total: 0 errors, 1 warnings, 64 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.

"[PATCH] monitor: Fix potential memory leak" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED 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: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth

2020-11-24 23:58:31

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [1/6] monitor: Fix potential memory leak

Hi Tedd,

On Fri, Nov 20, 2020 at 12:31 PM <[email protected]> wrote:
>
> 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=388665
>
> ---Test result---
>
> ##############################
> Test: CheckPatch - FAIL
> Output:
> monitor: Fix potential memory leak
> WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #12:
> ==258684== 1,500 bytes in 1 blocks are definitely lost in loss record 3 of 3
>
> - total: 0 errors, 1 warnings, 64 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.
>
> "[PATCH] monitor: Fix potential memory leak" has style problems, please review.
>
> NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED 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: CheckGitLint - PASS
>
> ##############################
> Test: CheckBuild - PASS
>
> ##############################
> Test: MakeCheck - PASS
>
>
>
> ---
> Regards,
> Linux Bluetooth

Applied, thanks.

--
Luiz Augusto von Dentz