vty: fix use-after-free and memleaks in is_cmd_ambiguous()

vty_test: add test against ambiguous cmd causing use-after-free and memory
leaks. Add this test along with the fix, because the new test triggers the
memory use-after-free and leaks, causing build failures.

Add cmd_deopt_with_ctx() to allow passing a specific talloc ctx.

is_cmd_ambiguous(): keep all cmd_deopt() allocations until the function exits.
Add a comment explaining why. Before this, if a command matched an optional
"[arg]" with square brackets, we would keep it in local var 'matched', but we
would free the string it points to at the end of that loop iteration; upon
encountering another match, we would attempt to strcmp against the freed
'matched'. Instead of adding hard-to-read and -verify free/alloc dances to keep
the 'matched' accurately freed/non-freed/..., just keep all cmd_deopt() string
allocated until done.

Needless to say that this should have been implemented on a lower level upon
inventing optional args, but at least this is fixing a program crash.

Related: OS#33903390
Change-Id: Ia71ba742108b5ff020997bfb612ad5eb30d04fcd

3 files changed, 124 insertions, 20 deletions

diff --git a/src/vty/command.c b/src/vty/command.c
index 51dece3a..43f974ce 100644
--- a/src/vty/command.c
+++ b/src/vty/command.c
@@ -1304,8 +1304,7 @@ static int cmd_range_match(const char *range, const char *str)
 }
 
 /* helper to retrieve the 'real' argument string from an optional argument */
-static char *
-cmd_deopt(const char *str)
+static char *cmd_deopt(void *ctx, const char *str)
 {
 	/* we've got "[blah]". We want to strip off the []s and redo the
 	 * match check for "blah"
@@ -1315,7 +1314,7 @@ cmd_deopt(const char *str)
 	if (len < 3)
 		return NULL;
 
-	return talloc_strndup(tall_vty_cmd_ctx, str + 1, len - 2);
+	return talloc_strndup(ctx, str + 1, len - 2);
 }
 
 static enum match_type
@@ -1326,7 +1325,7 @@ cmd_match(const char *str, const char *command,
 	if (recur && CMD_OPTION(str))
 	{
 		enum match_type ret;
-		char *tmp = cmd_deopt(str);
+		char *tmp = cmd_deopt(tall_vty_cmd_ctx, str);
 
 		/* this would be a bug in a command, however handle it gracefully
 		 * as it we only discover it if a user tries to run it
@@ -1475,6 +1474,7 @@ cmd_filter(char *command, vector v, unsigned int index, enum match_type level)
 static int
 is_cmd_ambiguous(char *command, vector v, int index, enum match_type type)
 {
+	int ret = 0;
 	unsigned int i;
 	unsigned int j;
 	struct cmd_element *cmd_element;
@@ -1482,6 +1482,16 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type)
 	vector descvec;
 	struct desc *desc;
 
+	/* In this loop, when a match is found, 'matched' points to it. If on a later iteration, an
+	 * identical match is found, the command is ambiguous. The trickiness is that a string may be
+	 * enclosed in '[str]' square brackets, which get removed by a talloc_strndup(), via cmd_deopt().
+	 * Such a string is usually needed for one loop iteration, except when 'matched' points to it. In
+	 * that case, the string must remain allocated until this function exits or another match comes
+	 * around. This is sufficiently confusing to justify a separate talloc tree to store all of the
+	 * odd allocations, and to free them all at the end. We are not expecting too many optional args
+	 * or ambiguities to cause a noticeable memory footprint from keeping all allocations. */
+	void *cmd_deopt_ctx = NULL;
+
 	for (i = 0; i < vector_active(v); i++)
 		if ((cmd_element = vector_slot(v, i)) != NULL) {
 			int match = 0;
@@ -1493,9 +1503,15 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type)
 					enum match_type ret;
 					const char *str = desc->cmd;
 
-					if (CMD_OPTION(str))
-						if ((str = cmd_deopt(str)) == NULL)
+					if (CMD_OPTION(str)) {
+						if (!cmd_deopt_ctx)
+							cmd_deopt_ctx =
+								talloc_named_const(tall_vty_cmd_ctx, 0,
+										   __func__);
+						str = cmd_deopt(cmd_deopt_ctx, str);
+						if (str == NULL)
 							continue;
+					}
 
 					switch (type) {
 					case exact_match:
@@ -1509,9 +1525,10 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type)
 						{
 							if (matched
 							    && strcmp(matched,
-								      str) != 0)
-								return 1;	/* There is ambiguous match. */
-							else
+								      str) != 0) {
+								ret = 1; /* There is ambiguous match. */
+								goto free_and_return;
+							} else
 								matched = str;
 							match++;
 						}
@@ -1521,9 +1538,10 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type)
 						    (str, command)) {
 							if (matched
 							    && strcmp(matched,
-								      str) != 0)
-								return 1;
-							else
+								      str) != 0) {
+								ret = 1;
+								goto free_and_return;
+							} else
 								matched = str;
 							match++;
 						}
@@ -1537,8 +1555,10 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type)
 						if ((ret =
 						     cmd_ipv6_prefix_match
 						     (command)) != no_match) {
-							if (ret == partly_match)
-								return 2;	/* There is incomplete match. */
+							if (ret == partly_match) {
+								ret = 2;	/* There is incomplete match. */
+								goto free_and_return;
+							}
 
 							match++;
 						}
@@ -1552,8 +1572,10 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type)
 						if ((ret =
 						     cmd_ipv4_prefix_match
 						     (command)) != no_match) {
-							if (ret == partly_match)
-								return 2;	/* There is incomplete match. */
+							if (ret == partly_match) {
+								ret = 2;	/* There is incomplete match. */
+								goto free_and_return;
+							}
 
 							match++;
 						}
@@ -1566,14 +1588,15 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type)
 					default:
 						break;
 					}
-
-					if (CMD_OPTION(desc->cmd))
-						talloc_free((void*)str);
 				}
 			if (!match)
 				vector_slot(v, i) = NULL;
 		}
-	return 0;
+
+free_and_return:
+	if (cmd_deopt_ctx)
+		talloc_free(cmd_deopt_ctx);
+	return ret;
 }
 
 /* If src matches dst return dst string, otherwise return NULL */
diff --git a/tests/vty/vty_test.c b/tests/vty/vty_test.c
index a3478e1d..30efb9af 100644
--- a/tests/vty/vty_test.c
+++ b/tests/vty/vty_test.c
@@ -385,6 +385,42 @@ DEFUN(cfg_level3_child, cfg_level3_child_cmd,
 	return CMD_SUCCESS;
 }
 
+DEFUN(cfg_ambiguous_nr_1, cfg_ambiguous_nr_1_cmd,
+	"ambiguous_nr [<0-23>]",
+	"testing is_cmd_ambiguous()\n"
+	"optional number arg\n")
+{
+	printf("Called: 'ambiguous_nr [<0-23>]' (argc=%d)\n", argc);
+	return CMD_SUCCESS;
+}
+
+DEFUN(cfg_ambiguous_nr_2, cfg_ambiguous_nr_2_cmd,
+	"ambiguous_nr <0-23> keyword",
+	"testing is_cmd_ambiguous()\n"
+	"optional number arg\n")
+{
+	printf("Called: 'ambiguous_nr <0-23> keyword'\n");
+	return CMD_SUCCESS;
+}
+
+DEFUN(cfg_ambiguous_str_1, cfg_ambiguous_str_1_cmd,
+	"ambiguous_str [ARG]",
+	"testing is_cmd_ambiguous()\n"
+	"optional string arg\n")
+{
+	printf("Called: 'ambiguous_str [ARG]' (argc=%d)\n", argc);
+	return CMD_SUCCESS;
+}
+
+DEFUN(cfg_ambiguous_str_2, cfg_ambiguous_str_2_cmd,
+	"ambiguous_str ARG keyword",
+	"testing is_cmd_ambiguous()\n"
+	"optional string arg\n")
+{
+	printf("Called: 'ambiguous_str ARG keyword'\n");
+	return CMD_SUCCESS;
+}
+
 void test_vty_add_cmds()
 {
 	install_element(CONFIG_NODE, &cfg_level1_cmd);
@@ -398,6 +434,30 @@ void test_vty_add_cmds()
 
 	install_node(&level3_node, NULL);
 	install_element(LEVEL3_NODE, &cfg_level3_child_cmd);
+
+	install_element_ve(&cfg_ambiguous_nr_1_cmd);
+	install_element_ve(&cfg_ambiguous_nr_2_cmd);
+	install_element_ve(&cfg_ambiguous_str_1_cmd);
+	install_element_ve(&cfg_ambiguous_str_2_cmd);
+}
+
+void test_is_cmd_ambiguous()
+{
+	struct vty *vty;
+	struct vty_test test;
+
+	printf("Going to test is_cmd_ambiguous()\n");
+	vty = create_test_vty(&test);
+
+	OSMO_ASSERT(do_vty_command(vty, "ambiguous_nr") == CMD_SUCCESS);
+	OSMO_ASSERT(do_vty_command(vty, "ambiguous_nr 23") == CMD_SUCCESS);
+	OSMO_ASSERT(do_vty_command(vty, "ambiguous_nr 23 keyword") == CMD_SUCCESS);
+
+	OSMO_ASSERT(do_vty_command(vty, "ambiguous_str") == CMD_SUCCESS);
+	OSMO_ASSERT(do_vty_command(vty, "ambiguous_str arg") == CMD_SUCCESS);
+	OSMO_ASSERT(do_vty_command(vty, "ambiguous_str arg keyword") == CMD_SUCCESS);
+
+	destroy_test_vty(&test, vty);
 }
 
 static int go_parent_cb(struct vty *vty)
@@ -465,6 +525,8 @@ int main(int argc, char **argv)
 	test_exit_by_indent("ok_indented_root.cfg", 0);
 	test_exit_by_indent("ok_empty_parent.cfg", 0);
 
+	test_is_cmd_ambiguous();
+
 	/* Leak check */
 	OSMO_ASSERT(talloc_total_blocks(stats_ctx) == 1);
 
diff --git a/tests/vty/vty_test.ok b/tests/vty/vty_test.ok
index bd6c5d66..2f76ff91 100644
--- a/tests/vty/vty_test.ok
+++ b/tests/vty/vty_test.ok
@@ -286,4 +286,23 @@ called level2 node k
 called level3 node k
 called level1 child cmd k
 got rc=0
+Going to test is_cmd_ambiguous()
+Going to execute 'ambiguous_nr'
+Called: 'ambiguous_nr [<0-23>]' (argc=0)
+Returned: 0, Current node: 1 '%s> '
+Going to execute 'ambiguous_nr 23'
+Called: 'ambiguous_nr [<0-23>]' (argc=1)
+Returned: 0, Current node: 1 '%s> '
+Going to execute 'ambiguous_nr 23 keyword'
+Called: 'ambiguous_nr <0-23> keyword'
+Returned: 0, Current node: 1 '%s> '
+Going to execute 'ambiguous_str'
+Called: 'ambiguous_str [ARG]' (argc=0)
+Returned: 0, Current node: 1 '%s> '
+Going to execute 'ambiguous_str arg'
+Called: 'ambiguous_str [ARG]' (argc=1)
+Returned: 0, Current node: 1 '%s> '
+Going to execute 'ambiguous_str arg keyword'
+Called: 'ambiguous_str ARG keyword'
+Returned: 0, Current node: 1 '%s> '
 All tests passed