aboutsummaryrefslogtreecommitdiffstats
path: root/src/vty
diff options
context:
space:
mode:
authorNeels Hofmeyr <neels@hofmeyr.de>2018-07-09 23:22:21 +0200
committerNeels Hofmeyr <neels@hofmeyr.de>2018-07-11 15:47:08 +0200
commit5314c513f23688462d7f7937e5ae5e0d5cd4548e (patch)
tree9a2eea692f88cf5f87534a4cdc85014d244af4c2 /src/vty
parent4e0add239f6516c11631a7539653bac6b6a51970 (diff)
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
Diffstat (limited to 'src/vty')
-rw-r--r--src/vty/command.c63
1 files changed, 43 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 */