monitor: Fix potential memory leak

PATCH Applied Tedd Ho-Jeong An 3 participants 8 messages Nov 20, 2020 – Nov 24, 2020

Subsystems (2)

Bluetooth drivers Library code

lore

Share & tools

lore

Tedd Ho-Jeong An

author

2020-11-20 20:07 UTC #0 lore raw

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(-)

monitor/control.c +17 -3

index 962da4980..d1ba97d37 100644
@@ -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;
 }

monitor/hcidump.c +11 -3

index 690b9b913..fac9c8a08 100644
@@ -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;
 }

Signature · 1 line

2.25.4

Tedd Ho-Jeong An

author

2020-11-20 20:07 UTC #1 lore raw

PATCH 2/6 monitor: Fix the unchecked return value

This patch fixes the unchecked return value.

---
 monitor/a2dp.c  | 30 ++++++++++++++++++++----------
 monitor/l2cap.c |  3 +++
 2 files changed, 23 insertions(+), 10 deletions(-)

monitor/a2dp.c +20 ~~-10~~

index ac2e1a228..f6e99ab26 100644
@@ -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;

monitor/l2cap.c +3 -0

index ca1997a0c..f16f82532 100644
@@ -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;

Signature · 1 line

2.25.4

Tedd Ho-Jeong An

author

2020-11-20 20:07 UTC #2 lore raw

PATCH 3/6 btio: Fix the unchecked return value

In reply to Tedd Ho-Jeong An

This patch fixes the unchecked return value.

---
 btio/btio.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

btio/btio.c +10 -4

index c18b6a012..8230212b4 100644
@@ -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);

Signature · 1 line

2.25.4

Tedd Ho-Jeong An

author

2020-11-20 20:07 UTC #3 lore raw

PATCH 4/6 emulator: Fix the unchecked return value

In reply to 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(-)

emulator/phy.c +8 -2

index 4517ad107..2ae6ad3a2 100644
@@ -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;
 }

emulator/server.c +5 -1

index 3b07a7156..ceb417a40 100644
@@ -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;

Signature · 1 line

2.25.4

Tedd Ho-Jeong An

author

2020-11-20 20:07 UTC #4 lore raw

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

In reply to Tedd Ho-Jeong An

This patch fixes the unchecked return value.

---
 profiles/network/bnep.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

profiles/network/bnep.c +5 -1

index 4dde55786..7e777e29c 100644
@@ -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)

Signature · 1 line

2.25.4

Tedd Ho-Jeong An

author

2020-11-20 20:07 UTC #5 lore raw

PATCH 6/6 lib: Fix the unchecked return value

In reply to 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(-)

lib/hci.c +4 -2

index 4bd33f241..53af0a114 100644
@@ -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;
 }

lib/sdp.c +2 -1

index ebaed3e40..844ae0d25 100644
@@ -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) &&

Signature · 1 line

2.25.4

[email protected]

2020-11-20 20:27 UTC #6 lore raw

In reply to Tedd Ho-Jeong An

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

Luiz Augusto von Dentz

maintainer Applied

2020-11-24 21:22 UTC #7 lore raw

Hi Tedd,

Quoted 38 lines · 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

Quoted 8 lines

##############################
Test: MakeCheck - PASS
---
Regards,
Linux Bluetooth

Applied, thanks.

Signature · 1 line

Luiz Augusto von Dentz