<p>Neels Hofmeyr has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/9939">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">vty: fix use-after-free and memleaks in is_cmd_ambiguous()<br><br>vty_test: add test against ambiguous cmd causing use-after-free and memory<br>leaks. Add this test along with the fix, because the new test triggers the<br>memory use-after-free and leaks, causing build failures.<br><br>Add cmd_deopt_with_ctx() to allow passing a specific talloc ctx.<br><br>is_cmd_ambiguous(): keep all cmd_deopt() allocations until the function exits.<br>Add a comment explaining why. Before this, if a command matched an optional<br>"[arg]" with square brackets, we would keep it in local var 'matched', but we<br>would free the string it points to at the end of that loop iteration; upon<br>encountering another match, we would attempt to strcmp against the freed<br>'matched'. Instead of adding hard-to-read and -verify free/alloc dances to keep<br>the 'matched' accurately freed/non-freed/..., just keep all cmd_deopt() string<br>allocated until done.<br><br>Needless to say that this should have been implemented on a lower level upon<br>inventing optional args, but at least this is fixing a program crash.<br><br>Related: OS#33903390<br>Change-Id: Ia71ba742108b5ff020997bfb612ad5eb30d04fcd<br>---<br>M src/vty/command.c<br>M tests/vty/vty_test.c<br>M tests/vty/vty_test.ok<br>3 files changed, 129 insertions(+), 18 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/39/9939/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/vty/command.c b/src/vty/command.c</span><br><span>index 51dece3..caa5ea1 100644</span><br><span>--- a/src/vty/command.c</span><br><span>+++ b/src/vty/command.c</span><br><span>@@ -1305,7 +1305,7 @@</span><br><span> </span><br><span> /* helper to retrieve the 'real' argument string from an optional argument */</span><br><span> static char *</span><br><span style="color: hsl(0, 100%, 40%);">-cmd_deopt(const char *str)</span><br><span style="color: hsl(120, 100%, 40%);">+cmd_deopt_with_ctx(void *ctx, const char *str)</span><br><span> {</span><br><span> /* we've got "[blah]". We want to strip off the []s and redo the</span><br><span> * match check for "blah"</span><br><span>@@ -1315,7 +1315,13 @@</span><br><span> if (len < 3)</span><br><span> return NULL;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- return talloc_strndup(tall_vty_cmd_ctx, str + 1, len - 2);</span><br><span style="color: hsl(120, 100%, 40%);">+ return talloc_strndup(ctx, str + 1, len - 2);</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+static char *</span><br><span style="color: hsl(120, 100%, 40%);">+cmd_deopt(const char *str)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ return cmd_deopt_with_ctx(tall_vty_cmd_ctx, str);</span><br><span> }</span><br><span> </span><br><span> static enum match_type</span><br><span>@@ -1475,6 +1481,7 @@</span><br><span> static int</span><br><span> is_cmd_ambiguous(char *command, vector v, int index, enum match_type type)</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ int ret = 0;</span><br><span> unsigned int i;</span><br><span> unsigned int j;</span><br><span> struct cmd_element *cmd_element;</span><br><span>@@ -1482,6 +1489,16 @@</span><br><span> vector descvec;</span><br><span> struct desc *desc;</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ /* In this loop, when a match is found, 'matched' points to it. If on a later iteration, an</span><br><span style="color: hsl(120, 100%, 40%);">+ * identical match is found, the command is ambiguous. The trickiness is that a string may be</span><br><span style="color: hsl(120, 100%, 40%);">+ * enclosed in '[str]' square brackets, which get removed by a talloc_strndup(), via cmd_deopt().</span><br><span style="color: hsl(120, 100%, 40%);">+ * Such a string is usually needed for one loop iteration, except when 'matched' points to it. In</span><br><span style="color: hsl(120, 100%, 40%);">+ * that case, the string must remain allocated until this function exits or another match comes</span><br><span style="color: hsl(120, 100%, 40%);">+ * around. This is sufficiently confusing to justify a separate talloc tree to store all of the</span><br><span style="color: hsl(120, 100%, 40%);">+ * odd allocations, and to free them all at the end. We are not expecting too many optional args</span><br><span style="color: hsl(120, 100%, 40%);">+ * or ambiguities to cause a noticeable memory footprint from keeping all allocations. */</span><br><span style="color: hsl(120, 100%, 40%);">+ void *cmd_deopt_ctx = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> for (i = 0; i < vector_active(v); i++)</span><br><span> if ((cmd_element = vector_slot(v, i)) != NULL) {</span><br><span> int match = 0;</span><br><span>@@ -1493,9 +1510,15 @@</span><br><span> enum match_type ret;</span><br><span> const char *str = desc->cmd;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (CMD_OPTION(str))</span><br><span style="color: hsl(0, 100%, 40%);">- if ((str = cmd_deopt(str)) == NULL)</span><br><span style="color: hsl(120, 100%, 40%);">+ if (CMD_OPTION(str)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!cmd_deopt_ctx)</span><br><span style="color: hsl(120, 100%, 40%);">+ cmd_deopt_ctx =</span><br><span style="color: hsl(120, 100%, 40%);">+ talloc_named_const(tall_vty_cmd_ctx, 0,</span><br><span style="color: hsl(120, 100%, 40%);">+ __func__);</span><br><span style="color: hsl(120, 100%, 40%);">+ str = cmd_deopt_with_ctx(cmd_deopt_ctx, str);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (str == NULL)</span><br><span> continue;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> </span><br><span> switch (type) {</span><br><span> case exact_match:</span><br><span>@@ -1509,9 +1532,10 @@</span><br><span> {</span><br><span> if (matched</span><br><span> && strcmp(matched,</span><br><span style="color: hsl(0, 100%, 40%);">- str) != 0)</span><br><span style="color: hsl(0, 100%, 40%);">- return 1; /* There is ambiguous match. */</span><br><span style="color: hsl(0, 100%, 40%);">- else</span><br><span style="color: hsl(120, 100%, 40%);">+ str) != 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ret = 1; /* There is ambiguous match. */</span><br><span style="color: hsl(120, 100%, 40%);">+ goto free_and_return;</span><br><span style="color: hsl(120, 100%, 40%);">+ } else</span><br><span> matched = str;</span><br><span> match++;</span><br><span> }</span><br><span>@@ -1521,9 +1545,10 @@</span><br><span> (str, command)) {</span><br><span> if (matched</span><br><span> && strcmp(matched,</span><br><span style="color: hsl(0, 100%, 40%);">- str) != 0)</span><br><span style="color: hsl(0, 100%, 40%);">- return 1;</span><br><span style="color: hsl(0, 100%, 40%);">- else</span><br><span style="color: hsl(120, 100%, 40%);">+ str) != 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ret = 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ goto free_and_return;</span><br><span style="color: hsl(120, 100%, 40%);">+ } else</span><br><span> matched = str;</span><br><span> match++;</span><br><span> }</span><br><span>@@ -1537,8 +1562,10 @@</span><br><span> if ((ret =</span><br><span> cmd_ipv6_prefix_match</span><br><span> (command)) != no_match) {</span><br><span style="color: hsl(0, 100%, 40%);">- if (ret == partly_match)</span><br><span style="color: hsl(0, 100%, 40%);">- return 2; /* There is incomplete match. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ret == partly_match) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ret = 2; /* There is incomplete match. */</span><br><span style="color: hsl(120, 100%, 40%);">+ goto free_and_return;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> </span><br><span> match++;</span><br><span> }</span><br><span>@@ -1552,8 +1579,10 @@</span><br><span> if ((ret =</span><br><span> cmd_ipv4_prefix_match</span><br><span> (command)) != no_match) {</span><br><span style="color: hsl(0, 100%, 40%);">- if (ret == partly_match)</span><br><span style="color: hsl(0, 100%, 40%);">- return 2; /* There is incomplete match. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ret == partly_match) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ret = 2; /* There is incomplete match. */</span><br><span style="color: hsl(120, 100%, 40%);">+ goto free_and_return;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> </span><br><span> match++;</span><br><span> }</span><br><span>@@ -1566,14 +1595,15 @@</span><br><span> default:</span><br><span> break;</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- if (CMD_OPTION(desc->cmd))</span><br><span style="color: hsl(0, 100%, 40%);">- talloc_free((void*)str);</span><br><span> }</span><br><span> if (!match)</span><br><span> vector_slot(v, i) = NULL;</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+free_and_return:</span><br><span style="color: hsl(120, 100%, 40%);">+ if (cmd_deopt_ctx)</span><br><span style="color: hsl(120, 100%, 40%);">+ talloc_free(cmd_deopt_ctx);</span><br><span style="color: hsl(120, 100%, 40%);">+ return ret;</span><br><span> }</span><br><span> </span><br><span> /* If src matches dst return dst string, otherwise return NULL */</span><br><span>diff --git a/tests/vty/vty_test.c b/tests/vty/vty_test.c</span><br><span>index a3478e1..30efb9a 100644</span><br><span>--- a/tests/vty/vty_test.c</span><br><span>+++ b/tests/vty/vty_test.c</span><br><span>@@ -385,6 +385,42 @@</span><br><span> return CMD_SUCCESS;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+DEFUN(cfg_ambiguous_nr_1, cfg_ambiguous_nr_1_cmd,</span><br><span style="color: hsl(120, 100%, 40%);">+ "ambiguous_nr [<0-23>]",</span><br><span style="color: hsl(120, 100%, 40%);">+ "testing is_cmd_ambiguous()\n"</span><br><span style="color: hsl(120, 100%, 40%);">+ "optional number arg\n")</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Called: 'ambiguous_nr [<0-23>]' (argc=%d)\n", argc);</span><br><span style="color: hsl(120, 100%, 40%);">+ return CMD_SUCCESS;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+DEFUN(cfg_ambiguous_nr_2, cfg_ambiguous_nr_2_cmd,</span><br><span style="color: hsl(120, 100%, 40%);">+ "ambiguous_nr <0-23> keyword",</span><br><span style="color: hsl(120, 100%, 40%);">+ "testing is_cmd_ambiguous()\n"</span><br><span style="color: hsl(120, 100%, 40%);">+ "optional number arg\n")</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Called: 'ambiguous_nr <0-23> keyword'\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ return CMD_SUCCESS;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+DEFUN(cfg_ambiguous_str_1, cfg_ambiguous_str_1_cmd,</span><br><span style="color: hsl(120, 100%, 40%);">+ "ambiguous_str [ARG]",</span><br><span style="color: hsl(120, 100%, 40%);">+ "testing is_cmd_ambiguous()\n"</span><br><span style="color: hsl(120, 100%, 40%);">+ "optional string arg\n")</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Called: 'ambiguous_str [ARG]' (argc=%d)\n", argc);</span><br><span style="color: hsl(120, 100%, 40%);">+ return CMD_SUCCESS;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+DEFUN(cfg_ambiguous_str_2, cfg_ambiguous_str_2_cmd,</span><br><span style="color: hsl(120, 100%, 40%);">+ "ambiguous_str ARG keyword",</span><br><span style="color: hsl(120, 100%, 40%);">+ "testing is_cmd_ambiguous()\n"</span><br><span style="color: hsl(120, 100%, 40%);">+ "optional string arg\n")</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Called: 'ambiguous_str ARG keyword'\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ return CMD_SUCCESS;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> void test_vty_add_cmds()</span><br><span> {</span><br><span> install_element(CONFIG_NODE, &cfg_level1_cmd);</span><br><span>@@ -398,6 +434,30 @@</span><br><span> </span><br><span> install_node(&level3_node, NULL);</span><br><span> install_element(LEVEL3_NODE, &cfg_level3_child_cmd);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ install_element_ve(&cfg_ambiguous_nr_1_cmd);</span><br><span style="color: hsl(120, 100%, 40%);">+ install_element_ve(&cfg_ambiguous_nr_2_cmd);</span><br><span style="color: hsl(120, 100%, 40%);">+ install_element_ve(&cfg_ambiguous_str_1_cmd);</span><br><span style="color: hsl(120, 100%, 40%);">+ install_element_ve(&cfg_ambiguous_str_2_cmd);</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+void test_is_cmd_ambiguous()</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct vty *vty;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct vty_test test;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Going to test is_cmd_ambiguous()\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ vty = create_test_vty(&test);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(do_vty_command(vty, "ambiguous_nr") == CMD_SUCCESS);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(do_vty_command(vty, "ambiguous_nr 23") == CMD_SUCCESS);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(do_vty_command(vty, "ambiguous_nr 23 keyword") == CMD_SUCCESS);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(do_vty_command(vty, "ambiguous_str") == CMD_SUCCESS);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(do_vty_command(vty, "ambiguous_str arg") == CMD_SUCCESS);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(do_vty_command(vty, "ambiguous_str arg keyword") == CMD_SUCCESS);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ destroy_test_vty(&test, vty);</span><br><span> }</span><br><span> </span><br><span> static int go_parent_cb(struct vty *vty)</span><br><span>@@ -465,6 +525,8 @@</span><br><span> test_exit_by_indent("ok_indented_root.cfg", 0);</span><br><span> test_exit_by_indent("ok_empty_parent.cfg", 0);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ test_is_cmd_ambiguous();</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /* Leak check */</span><br><span> OSMO_ASSERT(talloc_total_blocks(stats_ctx) == 1);</span><br><span> </span><br><span>diff --git a/tests/vty/vty_test.ok b/tests/vty/vty_test.ok</span><br><span>index bd6c5d6..2f76ff9 100644</span><br><span>--- a/tests/vty/vty_test.ok</span><br><span>+++ b/tests/vty/vty_test.ok</span><br><span>@@ -286,4 +286,23 @@</span><br><span> called level3 node k</span><br><span> called level1 child cmd k</span><br><span> got rc=0</span><br><span style="color: hsl(120, 100%, 40%);">+Going to test is_cmd_ambiguous()</span><br><span style="color: hsl(120, 100%, 40%);">+Going to execute 'ambiguous_nr'</span><br><span style="color: hsl(120, 100%, 40%);">+Called: 'ambiguous_nr [<0-23>]' (argc=0)</span><br><span style="color: hsl(120, 100%, 40%);">+Returned: 0, Current node: 1 '%s> '</span><br><span style="color: hsl(120, 100%, 40%);">+Going to execute 'ambiguous_nr 23'</span><br><span style="color: hsl(120, 100%, 40%);">+Called: 'ambiguous_nr [<0-23>]' (argc=1)</span><br><span style="color: hsl(120, 100%, 40%);">+Returned: 0, Current node: 1 '%s> '</span><br><span style="color: hsl(120, 100%, 40%);">+Going to execute 'ambiguous_nr 23 keyword'</span><br><span style="color: hsl(120, 100%, 40%);">+Called: 'ambiguous_nr <0-23> keyword'</span><br><span style="color: hsl(120, 100%, 40%);">+Returned: 0, Current node: 1 '%s> '</span><br><span style="color: hsl(120, 100%, 40%);">+Going to execute 'ambiguous_str'</span><br><span style="color: hsl(120, 100%, 40%);">+Called: 'ambiguous_str [ARG]' (argc=0)</span><br><span style="color: hsl(120, 100%, 40%);">+Returned: 0, Current node: 1 '%s> '</span><br><span style="color: hsl(120, 100%, 40%);">+Going to execute 'ambiguous_str arg'</span><br><span style="color: hsl(120, 100%, 40%);">+Called: 'ambiguous_str [ARG]' (argc=1)</span><br><span style="color: hsl(120, 100%, 40%);">+Returned: 0, Current node: 1 '%s> '</span><br><span style="color: hsl(120, 100%, 40%);">+Going to execute 'ambiguous_str arg keyword'</span><br><span style="color: hsl(120, 100%, 40%);">+Called: 'ambiguous_str ARG keyword'</span><br><span style="color: hsl(120, 100%, 40%);">+Returned: 0, Current node: 1 '%s> '</span><br><span> All tests passed</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/9939">change 9939</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/9939"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: Ia71ba742108b5ff020997bfb612ad5eb30d04fcd </div>
<div style="display:none"> Gerrit-Change-Number: 9939 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>