[PATCH 1/2] tools/btmgmt: Fix crash in completion in interactive mode

[PATCH 1/2] tools/btmgmt: Fix crash in completion in interactive mode

[Szymon Janc]

Use separate indexes while iterating over all_cmd and interactive_cmd.
Fix following crash:

[mgmt]# ==2224== Invalid read of size 1
==2224==    at 0x4A092F2: strlen (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==2224==    by 0x323C8860AD: strdup (in /usr/lib64/libc-2.18.so)
==2224==    by 0x323EC1D550: rl_completion_matches (in /usr/lib64/libreadline.so.6.2)
==2224==    by 0x402BBC: cmd_completion (btmgmt.c:3427)
==2224==    by 0x323EC1D608: ??? (in /usr/lib64/libreadline.so.6.2)
==2224==    by 0x323EC1D783: rl_complete_internal (in /usr/lib64/libreadline.so.6.2)
==2224==    by 0x323EC156DD: _rl_dispatch_subseq (in /usr/lib64/libreadline.so.6.2)
==2224==    by 0x323EC159FF: readline_internal_char (in /usr/lib64/libreadline.so.6.2)
==2224==    by 0x323EC2AB6C: rl_callback_read_char (in /usr/lib64/libreadline.so.6.2)
==2224==    by 0x4032E8: prompt_read (btmgmt.c:3551)
==2224==    by 0x419048: io_callback (io-mainloop.c:123)
==2224==    by 0x419842: mainloop_run (mainloop.c:157)
==2224==  Address 0x68 is not stack'd, malloc'd or (recently) free'd
---
 tools/btmgmt.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/btmgmt.c b/tools/btmgmt.c
index e262350..0686ed6 100644
--- a/tools/btmgmt.c
+++ b/tools/btmgmt.c
@@ -3375,23 +3375,24 @@ static struct cmd_info interactive_cmd[] = {
 
 static char *cmd_generator(const char *text, int state)
 {
-	static int index, len;
+	static int i, j, len;
 	const char *cmd;
 
 	if (!state) {
-		index = 0;
+		i = 0;
+		j = 0;
 		len = strlen(text);
 	}
 
-	while ((cmd = all_cmd[index].cmd)) {
-		index++;
+	while ((cmd = all_cmd[i].cmd)) {
+		i++;
 
 		if (!strncmp(cmd, text, len))
 			return strdup(cmd);
 	}
 
-	while ((cmd = interactive_cmd[index].cmd)) {
-		index++;
+	while ((cmd = interactive_cmd[j].cmd)) {
+		j++;
 
 		if (!strncmp(cmd, text, len))
 			return strdup(cmd);
-- 
1.9.3

[PATCH 2/2] tools/btmgmt: Handle commands tables in consistent way

[Szymon Janc]

In some places commands tables were accessed by fixed element count
and in some places it was assumed that last element is zeroed.

This could lead to accessing invalid memory since tables were
not terminated with zeroed element (I couldn't crash the tool but
this is most likely only due to static memory being zeroed).

Be consistent and terminate arrays with zeroed element and use this
for iterating.
---
 tools/btmgmt.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/tools/btmgmt.c b/tools/btmgmt.c
index 0686ed6..c8fc9f6 100644
--- a/tools/btmgmt.c
+++ b/tools/btmgmt.c
@@ -3289,6 +3289,7 @@ static struct cmd_info all_cmd[] = {
 	{ "add-device", cmd_add_device, "Add Device"			},
 	{ "del-device", cmd_del_device, "Remove Device"			},
 	{ "clr-devices",cmd_clr_devices,"Clear Devices"			},
+	{ }
 };
 
 static void cmd_quit(struct mgmt *mgmt, uint16_t index,
@@ -3371,6 +3372,7 @@ static struct cmd_info interactive_cmd[] = {
 	{ "quit",	cmd_quit,	"Exit program"			},
 	{ "exit",	cmd_quit,	"Exit program"			},
 	{ "help",	NULL,		"List supported commands"	},
+	{ }
 };
 
 static char *cmd_generator(const char *text, int state)
@@ -3432,12 +3434,11 @@ static char **cmd_completion(const char *text, int start, int end)
 	return matches;
 }
 
-static struct cmd_info *find_cmd(const char *cmd, struct cmd_info table[],
-							size_t cmd_count)
+static struct cmd_info *find_cmd(const char *cmd, struct cmd_info table[])
 {
-	size_t i;
+	int i;
 
-	for (i = 0; i < cmd_count; i++) {
+	for (i = 0; table[i].cmd; i++) {
 		if (!strcmp(table[i].cmd, cmd))
 			return &table[i];
 	}
@@ -3478,9 +3479,9 @@ static void rl_handler(char *input)
 	argv = w.we_wordv;
 	argc = w.we_wordc;
 
-	c = find_cmd(cmd, all_cmd, NELEM(all_cmd));
+	c = find_cmd(cmd, all_cmd);
 	if (!c && interactive)
-		c = find_cmd(cmd, interactive_cmd, NELEM(interactive_cmd));
+		c = find_cmd(cmd, interactive_cmd);
 
 	if (c && c->func) {
 		c->func(mgmt, mgmt_index, argc, argv);
@@ -3494,7 +3495,7 @@ static void rl_handler(char *input)
 
 	print("Available commands:");
 
-	for (i = 0; i < NELEM(all_cmd); i++) {
+	for (i = 0; all_cmd[i].cmd; i++) {
 		c = &all_cmd[i];
 		if (c->doc)
 			print("  %s %-*s %s", c->cmd,
@@ -3504,7 +3505,7 @@ static void rl_handler(char *input)
 	if (!interactive)
 		goto free_we;
 
-	for (i = 0; i < NELEM(interactive_cmd); i++) {
+	for (i = 0; interactive_cmd[i].cmd; i++) {
 		c = &interactive_cmd[i];
 		if (c->doc)
 			print("  %s %-*s %s", c->cmd,
@@ -3603,7 +3604,7 @@ int main(int argc, char *argv[])
 	if (argc > 0) {
 		struct cmd_info *c;
 
-		c = find_cmd(argv[0], all_cmd, NELEM(all_cmd));
+		c = find_cmd(argv[0], all_cmd);
 		if (!c) {
 			fprintf(stderr, "Unknown command: %s\n", argv[0]);
 			mgmt_unref(mgmt);
-- 
1.9.3

Re: [PATCH 1/2] tools/btmgmt: Fix crash in completion in interactive mode

[Johan Hedberg]

Hi Szymon,

On Thu, Feb 12, 2015, Szymon Janc wrote:
> Use separate indexes while iterating over all_cmd and interactive_cmd.
> Fix following crash:
> 
> [mgmt]# ==2224== Invalid read of size 1
> ==2224==    at 0x4A092F2: strlen (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==2224==    by 0x323C8860AD: strdup (in /usr/lib64/libc-2.18.so)
> ==2224==    by 0x323EC1D550: rl_completion_matches (in /usr/lib64/libreadline.so.6.2)
> ==2224==    by 0x402BBC: cmd_completion (btmgmt.c:3427)
> ==2224==    by 0x323EC1D608: ??? (in /usr/lib64/libreadline.so.6.2)
> ==2224==    by 0x323EC1D783: rl_complete_internal (in /usr/lib64/libreadline.so.6.2)
> ==2224==    by 0x323EC156DD: _rl_dispatch_subseq (in /usr/lib64/libreadline.so.6.2)
> ==2224==    by 0x323EC159FF: readline_internal_char (in /usr/lib64/libreadline.so.6.2)
> ==2224==    by 0x323EC2AB6C: rl_callback_read_char (in /usr/lib64/libreadline.so.6.2)
> ==2224==    by 0x4032E8: prompt_read (btmgmt.c:3551)
> ==2224==    by 0x419048: io_callback (io-mainloop.c:123)
> ==2224==    by 0x419842: mainloop_run (mainloop.c:157)
> ==2224==  Address 0x68 is not stack'd, malloc'd or (recently) free'd
> ---
>  tools/btmgmt.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

This patch has been applied. Thanks.

For your second patch I went actually in the other directions and used
NELEM() everywhere. I prefer that since it's a stronger guarantee of the
table length than having to remember to put an empty element at the end
of it.

Johan

Re: [PATCH 1/2] tools/btmgmt: Fix crash in completion in interactive mode

[Szymon Janc]

Hi Johan,

On Thursday 12 February 2015 22:02:10 Johan Hedberg wrote:
> Hi Szymon,
> 
> On Thu, Feb 12, 2015, Szymon Janc wrote:
> > Use separate indexes while iterating over all_cmd and interactive_cmd.
> > Fix following crash:
> > 
> > [mgmt]# ==2224== Invalid read of size 1
> > ==2224==    at 0x4A092F2: strlen (in
> > /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==2224==    by
> > 0x323C8860AD: strdup (in /usr/lib64/libc-2.18.so)
> > ==2224==    by 0x323EC1D550: rl_completion_matches (in
> > /usr/lib64/libreadline.so.6.2) ==2224==    by 0x402BBC: cmd_completion
> > (btmgmt.c:3427)
> > ==2224==    by 0x323EC1D608: ??? (in /usr/lib64/libreadline.so.6.2)
> > ==2224==    by 0x323EC1D783: rl_complete_internal (in
> > /usr/lib64/libreadline.so.6.2) ==2224==    by 0x323EC156DD:
> > _rl_dispatch_subseq (in /usr/lib64/libreadline.so.6.2) ==2224==    by
> > 0x323EC159FF: readline_internal_char (in /usr/lib64/libreadline.so.6.2)
> > ==2224==    by 0x323EC2AB6C: rl_callback_read_char (in
> > /usr/lib64/libreadline.so.6.2) ==2224==    by 0x4032E8: prompt_read
> > (btmgmt.c:3551)
> > ==2224==    by 0x419048: io_callback (io-mainloop.c:123)
> > ==2224==    by 0x419842: mainloop_run (mainloop.c:157)
> > ==2224==  Address 0x68 is not stack'd, malloc'd or (recently) free'd
> > ---
> > 
> >  tools/btmgmt.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> This patch has been applied. Thanks.
> 
> For your second patch I went actually in the other directions and used
> NELEM() everywhere. I prefer that since it's a stronger guarantee of the
> table length than having to remember to put an empty element at the end
> of it.
> 
> Johan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

I've decided to go with it since I used it in RFC serie where command table is 
passed to common interactive code. But I guess it shouldn't be a problem to 
pass length along with it.


-- 
Szymon K. Janc
szymon.janc@gmail.com