This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgNeels Hofmeyr has uploaded this change for review. ( https://gerrit.osmocom.org/9939
Change subject: vty: fix use-after-free and memleaks in is_cmd_ambiguous()
......................................................................
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
---
M src/vty/command.c
M tests/vty/vty_test.c
M tests/vty/vty_test.ok
3 files changed, 129 insertions(+), 18 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/39/9939/1
diff --git a/src/vty/command.c b/src/vty/command.c
index 51dece3..caa5ea1 100644
--- a/src/vty/command.c
+++ b/src/vty/command.c
@@ -1305,7 +1305,7 @@
/* helper to retrieve the 'real' argument string from an optional argument */
static char *
-cmd_deopt(const char *str)
+cmd_deopt_with_ctx(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 +1315,13 @@
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 char *
+cmd_deopt(const char *str)
+{
+ return cmd_deopt_with_ctx(tall_vty_cmd_ctx, str);
}
static enum match_type
@@ -1475,6 +1481,7 @@
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 +1489,16 @@
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 +1510,15 @@
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_with_ctx(cmd_deopt_ctx, str);
+ if (str == NULL)
continue;
+ }
switch (type) {
case exact_match:
@@ -1509,9 +1532,10 @@
{
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 +1545,10 @@
(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 +1562,10 @@
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 +1579,10 @@
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 +1595,15 @@
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 a3478e1..30efb9a 100644
--- a/tests/vty/vty_test.c
+++ b/tests/vty/vty_test.c
@@ -385,6 +385,42 @@
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 @@
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 @@
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 bd6c5d6..2f76ff9 100644
--- a/tests/vty/vty_test.ok
+++ b/tests/vty/vty_test.ok
@@ -286,4 +286,23 @@
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
--
To view, visit https://gerrit.osmocom.org/9939
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia71ba742108b5ff020997bfb612ad5eb30d04fcd
Gerrit-Change-Number: 9939
Gerrit-PatchSet: 1
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180709/204bc872/attachment.htm>