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

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

[Tedd Ho-Jeong An]

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

[PATCH 2/6] monitor: Fix the unchecked return value

[Tedd Ho-Jeong An]

This patch fixes the unchecked return value.
---
 monitor/a2dp.c  | 30 ++++++++++++++++++++----------
 monitor/l2cap.c |  3 +++
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/monitor/a2dp.c b/monitor/a2dp.c
index ac2e1a228..f6e99ab26 100644
--- a/monitor/a2dp.c
+++ b/monitor/a2dp.c
@@ -364,7 +364,8 @@ static bool codec_mpeg12_cap(uint8_t losc, struct l2cap_frame *frame)
 	if (losc != 4)
 		return false;
 
-	l2cap_frame_get_be16(frame, &cap);
+	if (!l2cap_frame_get_be16(frame, &cap))
+		return false;
 
 	layer = (cap >> 8) & 0xe0;
 	crc = cap & 0x1000;
@@ -372,7 +373,8 @@ static bool codec_mpeg12_cap(uint8_t losc, struct l2cap_frame *frame)
 	mpf = cap & 0x0040;
 	freq = cap & 0x003f;
 
-	l2cap_frame_get_be16(frame, &cap);
+	if (!l2cap_frame_get_be16(frame, &cap))
+		return false;
 
 	vbr = cap & 0x8000;
 	bitrate = cap & 0x7fff;
@@ -414,7 +416,8 @@ static bool codec_mpeg12_cfg(uint8_t losc, struct l2cap_frame *frame)
 	if (losc != 4)
 		return false;
 
-	l2cap_frame_get_be16(frame, &cap);
+	if (!l2cap_frame_get_be16(frame, &cap))
+		return false;
 
 	layer = (cap >> 8) & 0xe0;
 	crc = cap & 0x1000;
@@ -422,7 +425,8 @@ static bool codec_mpeg12_cfg(uint8_t losc, struct l2cap_frame *frame)
 	mpf = cap & 0x0040;
 	freq = cap & 0x003f;
 
-	l2cap_frame_get_be16(frame, &cap);
+	if (!l2cap_frame_get_be16(frame, &cap))
+		return false;
 
 	vbr = cap & 0x8000;
 	bitrate = cap & 0x7fff;
@@ -466,19 +470,22 @@ static bool codec_aac_cap(uint8_t losc, struct l2cap_frame *frame)
 	if (losc != 6)
 		return false;
 
-	l2cap_frame_get_be16(frame, &cap);
+	if (!l2cap_frame_get_be16(frame, &cap))
+		return false;
 
 	type = cap >> 8;
 	freq = cap << 8;
 
-	l2cap_frame_get_be16(frame, &cap);
+	if (!l2cap_frame_get_be16(frame, &cap))
+		return false;
 
 	freq |= (cap >> 8) & 0xf0;
 	chan = (cap >> 8) & 0x0c;
 	bitrate = (cap << 16) & 0x7f0000;
 	vbr = cap & 0x0080;
 
-	l2cap_frame_get_be16(frame, &cap);
+	if (!l2cap_frame_get_be16(frame, &cap))
+		return false;
 
 	bitrate |= cap;
 
@@ -509,19 +516,22 @@ static bool codec_aac_cfg(uint8_t losc, struct l2cap_frame *frame)
 	if (losc != 6)
 		return false;
 
-	l2cap_frame_get_be16(frame, &cap);
+	if (!l2cap_frame_get_be16(frame, &cap))
+		return false;
 
 	type = cap >> 8;
 	freq = cap << 8;
 
-	l2cap_frame_get_be16(frame, &cap);
+	if (!l2cap_frame_get_be16(frame, &cap))
+		return false;
 
 	freq |= (cap >> 8) & 0xf0;
 	chan = (cap >> 8) & 0x0c;
 	bitrate = (cap << 16) & 0x7f0000;
 	vbr = cap & 0x0080;
 
-	l2cap_frame_get_be16(frame, &cap);
+	if (!l2cap_frame_get_be16(frame, &cap))
+		return false;
 
 	bitrate |= cap;
 
diff --git a/monitor/l2cap.c b/monitor/l2cap.c
index ca1997a0c..f16f82532 100644
--- a/monitor/l2cap.c
+++ b/monitor/l2cap.c
@@ -3243,6 +3243,9 @@ void l2cap_frame(uint16_t index, bool in, uint16_t handle, uint16_t cid,
 		case L2CAP_MODE_LE_FLOWCTL:
 		case L2CAP_MODE_ECRED:
 			chan = get_chan(&frame);
+			if (!chan)
+				return;
+
 			if (!chan->sdu) {
 				if (!l2cap_frame_get_le16(&frame, &chan->sdu))
 					return;
-- 
2.25.4

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

[Tedd Ho-Jeong An]

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

[PATCH 4/6] emulator: Fix the unchecked return value

[Tedd Ho-Jeong An]

This patch fixes the unchecked return value.
---
 emulator/phy.c    | 10 ++++++++--
 emulator/server.c |  6 +++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/emulator/phy.c b/emulator/phy.c
index 4517ad107..2ae6ad3a2 100644
--- a/emulator/phy.c
+++ b/emulator/phy.c
@@ -115,7 +115,10 @@ static int create_rx_socket(void)
 	if (fd < 0)
 		return -1;
 
-	setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
+	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0) {
+		close(fd);
+		return -1;
+	}
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sin_family = AF_INET;
@@ -138,7 +141,10 @@ static int create_tx_socket(void)
 	if (fd < 0)
 		return -1;
 
-	setsockopt(fd, SOL_SOCKET, SO_BROADCAST, &opt, sizeof(opt));
+	if (setsockopt(fd, SOL_SOCKET, SO_BROADCAST, &opt, sizeof(opt)) < 0) {
+		close(fd);
+		return -1;
+	}
 
 	return fd;
 }
diff --git a/emulator/server.c b/emulator/server.c
index 3b07a7156..ceb417a40 100644
--- a/emulator/server.c
+++ b/emulator/server.c
@@ -322,7 +322,11 @@ static int open_tcp(void)
 		return -1;
 	}
 
-	setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
+	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0) {
+		perror("Failed to set socket option");
+		close(fd);
+		return -1;
+	}
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sin_family = AF_INET;
-- 
2.25.4

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

[Tedd Ho-Jeong An]

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

[PATCH 6/6] lib: Fix the unchecked return value

[Tedd Ho-Jeong An]

This patch fixes the unchecked return value.
---
 lib/hci.c | 6 ++++--
 lib/sdp.c | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/hci.c b/lib/hci.c
index 4bd33f241..53af0a114 100644
--- a/lib/hci.c
+++ b/lib/hci.c
@@ -1246,12 +1246,14 @@ int hci_send_req(int dd, struct hci_request *r, int to)
 
 failed:
 	err = errno;
-	setsockopt(dd, SOL_HCI, HCI_FILTER, &of, sizeof(of));
+	if (setsockopt(dd, SOL_HCI, HCI_FILTER, &of, sizeof(of)) < 0)
+		err = errno;
 	errno = err;
 	return -1;
 
 done:
-	setsockopt(dd, SOL_HCI, HCI_FILTER, &of, sizeof(of));
+	if (setsockopt(dd, SOL_HCI, HCI_FILTER, &of, sizeof(of)) < 0)
+		return -1;
 	return 0;
 }
 
diff --git a/lib/sdp.c b/lib/sdp.c
index ebaed3e40..844ae0d25 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -4705,7 +4705,8 @@ static int sdp_connect_l2cap(const bdaddr_t *src,
 
 	if (flags & SDP_WAIT_ON_CLOSE) {
 		struct linger l = { .l_onoff = 1, .l_linger = 1 };
-		setsockopt(sk, SOL_SOCKET, SO_LINGER, &l, sizeof(l));
+		if (setsockopt(sk, SOL_SOCKET, SO_LINGER, &l, sizeof(l)) < 0)
+			return -1;
 	}
 
 	if ((flags & SDP_LARGE_MTU) &&
-- 
2.25.4

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

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

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

[Luiz Augusto von Dentz]

Hi Tedd,

On Fri, Nov 20, 2020 at 12:31 PM <bluez.test.bot@gmail.com> 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