lib, vtysh: Fix memory leaks, change cmd_element to const

Fix a few memory issues:

* Not freeing tab-completions upon input match failure
* Invalid write when null-terminating tab-completions
* Not freeing argv[] itself in additinon to elements
* Use XFREE() instead of free() as appropriate
* Not freeing final token of an [option] during parsing

Make a few minor changes to CLI internals:

* Improve documentation on matching & completion functions
* Only make one copy of cmd_token's when building argv,
  instead of three
* Don't make a copy of the matching cmd_element

Make one major(ish) change to CLI internals:

* Change all pointers to struct cmd_element to const

Code outside of the core CLI units should never have an
occasion to modify the internal state of the command system.
Doing so could easily amount to having a CLI interface that
changes during runtime, and could conceivably lead to security
issues. Explicitly disallowing this removes any chance of
confusion.

Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>

5 files changed, 73 insertions, 49 deletions

diff --git a/lib/command.c b/lib/command.c
index 9eb43db94..593822a66 100644
--- a/lib/command.c
+++ b/lib/command.c
@@ -262,7 +262,10 @@ cmd_make_strvec (const char *string)
 
   // if the entire string was whitespace or a comment, return
   if (*copy == '\0' || *copy == '!' || *copy == '#')
+  {
+    XFREE (MTYPE_TMP, copystart);
     return NULL;
+  }
 
   vector strvec = vector_init (VECTOR_MIN_SIZE);
   const char *delim = " \n\r\t", *tok = NULL;
@@ -546,7 +549,7 @@ completions_to_vec (struct list *completions)
     }
 
     if (!exists)
-      vector_set (comps, copy_cmd_token (token));
+      vector_set (comps, token);
   }
 
   // sort completions
@@ -560,7 +563,7 @@ completions_to_vec (struct list *completions)
   {
     vector_set_index (comps, vector_active (comps), NULL);
     memmove (comps->index + 1, comps->index, (comps->alloced - 1) * sizeof (void *));
-    vector_set_index (comps, 0, copy_cmd_token (cr));
+    vector_set_index (comps, 0, cr);
   }
 
   return comps;
@@ -584,13 +587,7 @@ cmd_complete_command_real (vector vline, struct vty *vty, int *status)
 
   if (MATCHER_ERROR(rv))
   {
-    switch (rv)
-    {
-      case MATCHER_AMBIGUOUS:
-        *status = CMD_ERR_AMBIGUOUS;
-      default:
-        *status = CMD_ERR_NO_MATCH;
-    }
+    *status = CMD_ERR_NO_MATCH;
     return NULL;
   }
 
@@ -687,20 +684,17 @@ cmd_complete_command (vector vline, struct vty *vty, int *status)
       struct cmd_token *token = vector_slot (initial_comps, i);
       if (token->type == WORD_TKN)
         vector_set (comps, token);
-      else
-        del_cmd_token (token);
     }
     vector_free (initial_comps);
 
     // copy completions text into an array of char*
-    ret = XMALLOC (MTYPE_TMP, vector_active (comps) * sizeof (char *) + 1);
+    ret = XMALLOC (MTYPE_TMP, (vector_active (comps)+1) * sizeof (char *));
     unsigned int i;
     for (i = 0; i < vector_active (comps); i++)
       {
         struct cmd_token *token = vector_slot (comps, i);
         ret[i] = XSTRDUP (MTYPE_TMP, token->text);
         vector_unset (comps, i);
-        del_cmd_token (token);
       }
     // set the last element to NULL, because this array is used in
     // a Readline completion_generator function which expects NULL
@@ -778,11 +772,11 @@ static int
 cmd_execute_command_real (vector vline,
                           enum filter_type filter,
                           struct vty *vty,
-                          struct cmd_element **cmd)
+                          const struct cmd_element **cmd)
 {
   struct list *argv_list;
   enum matcher_rv status;
-  struct cmd_element *matched_element = NULL;
+  const struct cmd_element *matched_element = NULL;
 
   struct graph *cmdgraph = cmd_node_graph (cmdvec, vty->node);
   status = command_match (cmdgraph, vline, &argv_list, &matched_element);
@@ -792,6 +786,7 @@ cmd_execute_command_real (vector vline,
 
   // if matcher error, return corresponding CMD_ERR
   if (MATCHER_ERROR(status))
+  {
     switch (status)
     {
       case MATCHER_INCOMPLETE:
@@ -801,6 +796,7 @@ cmd_execute_command_real (vector vline,
       default:
         return CMD_ERR_NO_MATCH;
     }
+  }
 
   // build argv array from argv list
   struct cmd_token **argv = XMALLOC (MTYPE_TMP, argv_list->count * sizeof (struct cmd_token *));
@@ -820,6 +816,7 @@ cmd_execute_command_real (vector vline,
 
   // delete list and cmd_token's in it
   list_delete (argv_list);
+  XFREE (MTYPE_TMP, argv);
 
   return ret;
 }
@@ -840,14 +837,16 @@ cmd_execute_command_real (vector vline,
  *         as to why no command could be executed.
  */
 int
-cmd_execute_command (vector vline, struct vty *vty, struct cmd_element **cmd,
-                     int vtysh) {
+cmd_execute_command (vector vline, struct vty *vty,
+                     const struct cmd_element **cmd,
+                     int vtysh)
+{
   int ret, saved_ret = 0;
   enum node_type onode, try_node;
 
   onode = try_node = vty->node;
 
-  if ( cmd_try_do_shortcut(vty->node, vector_slot(vline, 0) ) )
+  if (cmd_try_do_shortcut(vty->node, vector_slot(vline, 0)))
     {
       vector shifted_vline;
       unsigned int index;
@@ -867,7 +866,6 @@ cmd_execute_command (vector vline, struct vty *vty, struct cmd_element **cmd,
       return ret;
   }
 
-
   saved_ret = ret = cmd_execute_command_real (vline, FILTER_RELAXED, vty, cmd);
 
   if (vtysh)
@@ -907,7 +905,7 @@ cmd_execute_command (vector vline, struct vty *vty, struct cmd_element **cmd,
  */
 int
 cmd_execute_command_strict (vector vline, struct vty *vty,
-                            struct cmd_element **cmd)
+                            const struct cmd_element **cmd)
 {
   return cmd_execute_command_real(vline, FILTER_STRICT, vty, cmd);
 }
@@ -925,7 +923,7 @@ cmd_execute_command_strict (vector vline, struct vty *vty,
  *         as to why no command could be executed.
  */
 int
-command_config_read_one_line (struct vty *vty, struct cmd_element **cmd, int use_daemon)
+command_config_read_one_line (struct vty *vty, const struct cmd_element **cmd, int use_daemon)
 {
   vector vline;
   int saved_node;
@@ -2404,13 +2402,13 @@ del_cmd_token (struct cmd_token *token)
   if (!token) return;
 
   if (token->text)
-    free (token->text);
+    XFREE (MTYPE_CMD_TOKENS, token->text);
   if (token->desc)
-    free (token->desc);
+    XFREE (MTYPE_CMD_TOKENS, token->desc);
   if (token->arg)
-    free (token->arg);
+    XFREE (MTYPE_CMD_TOKENS, token->arg);
 
-  free (token);
+  XFREE (MTYPE_CMD_TOKENS, token);
 }
 
 struct cmd_token *
@@ -2436,7 +2434,7 @@ del_cmd_element(struct cmd_element *cmd)
 }
 
 struct cmd_element *
-copy_cmd_element(struct cmd_element *cmd)
+copy_cmd_element(const struct cmd_element *cmd)
 {
   struct cmd_element *el = XMALLOC(MTYPE_CMD_TOKENS, sizeof (struct cmd_element));
   el->string = cmd->string ? XSTRDUP(MTYPE_CMD_TOKENS, cmd->string) : NULL;
diff --git a/lib/command.h b/lib/command.h
index 51f7f7808..3bcd66468 100644
--- a/lib/command.h
+++ b/lib/command.h
@@ -210,7 +210,7 @@ struct cmd_element
   u_char attr;                  /* Command attributes */
 
   /* handler function for command */
-  int (*func) (struct cmd_element *, struct vty *, int, struct cmd_token *[]);
+  int (*func) (const struct cmd_element *, struct vty *, int, struct cmd_token *[]);
 };
 
 /* Return value of the commands. */
@@ -245,11 +245,11 @@ struct cmd_element
   };
 
 #define DEFUN_CMD_FUNC_DECL(funcname) \
-  static int funcname (struct cmd_element *, struct vty *, int, struct cmd_token *[]);
+  static int funcname (const struct cmd_element *, struct vty *, int, struct cmd_token *[]);
 
 #define DEFUN_CMD_FUNC_TEXT(funcname) \
   static int funcname \
-    (struct cmd_element *self __attribute__ ((unused)), \
+    (const struct cmd_element *self __attribute__ ((unused)), \
      struct vty *vty __attribute__ ((unused)), \
      int argc __attribute__ ((unused)), \
      struct cmd_token *argv[] __attribute__ ((unused)) )
@@ -410,11 +410,11 @@ extern char *cmd_concat_strvec (vector);
 extern vector cmd_describe_command (vector, struct vty *, int *status);
 extern char **cmd_complete_command (vector, struct vty *, int *status);
 extern const char *cmd_prompt (enum node_type);
-extern int command_config_read_one_line (struct vty *vty, struct cmd_element **, int use_config_node);
+extern int command_config_read_one_line (struct vty *vty, const struct cmd_element **, int use_config_node);
 extern int config_from_file (struct vty *, FILE *, unsigned int *line_num);
 extern enum node_type node_parent (enum node_type);
-extern int cmd_execute_command (vector, struct vty *, struct cmd_element **, int);
-extern int cmd_execute_command_strict (vector, struct vty *, struct cmd_element **);
+extern int cmd_execute_command (vector, struct vty *, const struct cmd_element **, int);
+extern int cmd_execute_command_strict (vector, struct vty *, const struct cmd_element **);
 extern void cmd_init (int);
 extern void cmd_terminate (void);
 
@@ -422,7 +422,7 @@ extern void cmd_terminate (void);
 void
 del_cmd_element(struct cmd_element *);
 struct cmd_element *
-copy_cmd_element(struct cmd_element *cmd);
+copy_cmd_element(const struct cmd_element *cmd);
 
 /* memory management for cmd_token */
 struct cmd_token *
diff --git a/lib/command_match.c b/lib/command_match.c
index ce03563ac..82090be73 100644
--- a/lib/command_match.c
+++ b/lib/command_match.c
@@ -87,7 +87,7 @@ enum matcher_rv
 command_match (struct graph *cmdgraph,
                vector vline,
                struct list **argv,
-               struct cmd_element **el)
+               const struct cmd_element **el)
 {
   matcher_rv = MATCHER_NO_MATCH;
 
@@ -171,13 +171,16 @@ command_match (struct graph *cmdgraph,
  * In the event that two children are found to match with the same precedence,
  * then the input is ambiguous for the passed cmd_element and NULL is returned.
  *
- * The ultimate return value is an ordered linked list of nodes that comprise
- * the best match for the command, each with their `arg` fields pointing to the
- * matching token string.
- *
  * @param[in] start the start node.
  * @param[in] vline the vectorized input line.
  * @param[in] n the index of the first input token.
+ * @return A linked list of n elements. The first n-1 elements are pointers to
+ * struct cmd_token and represent the sequence of tokens matched by the input.
+ * The ->arg field of each token points to a copy of the input matched on it.
+ * The final nth element is a pointer to struct cmd_element, which is the
+ * command that was matched.
+ *
+ * If no match was found, the return value is NULL.
  */
 static struct list *
 command_match_r (struct graph_node *start, vector vline, unsigned int n)
@@ -246,7 +249,7 @@ command_match_r (struct graph_node *start, vector vline, unsigned int n)
               // deleting this list the last node must be
               // manually deleted
               struct cmd_element *el = leaf->data;
-              listnode_add (currbest, copy_cmd_element (el));
+              listnode_add (currbest, el);
               currbest->del = (void (*)(void *)) &del_cmd_token;
               break;
             }
@@ -385,10 +388,14 @@ command_complete (struct graph *graph,
      MATCHER_OK :
      MATCHER_NO_MATCH;
 
-  // extract cmd_token into list
-  *completions = list_new ();
-  for (ALL_LIST_ELEMENTS_RO (next,node,gn))
-    listnode_add (*completions, gn->data);
+  *completions = NULL;
+  if (!MATCHER_ERROR(matcher_rv))
+  {
+    // extract cmd_token into list
+    *completions = list_new ();
+    for (ALL_LIST_ELEMENTS_RO (next,node,gn))
+      listnode_add (*completions, gn->data);
+  }
 
   list_delete (current);
   list_delete (next);
@@ -560,6 +567,8 @@ disambiguate (struct list *first,
  * but arglists have cmd_element as the data for the tail, this function
  * manually deletes the tail before deleting the rest of the list as usual.
  *
+ * The cmd_element at the end is *not* a copy. It is the one and only.
+ *
  * @param list the arglist to delete
  */
 static void
@@ -567,7 +576,6 @@ del_arglist (struct list *list)
 {
   // manually delete last node
   struct listnode *tail = listtail (list);
-  del_cmd_element (tail->data);
   tail->data = NULL;
   list_delete_node (list, tail);
 
diff --git a/lib/command_match.h b/lib/command_match.h
index ac4e70c31..9e18b8d90 100644
--- a/lib/command_match.h
+++ b/lib/command_match.h
@@ -70,23 +70,40 @@ enum match_type
  *
  * @param[in] cmdgraph command graph to match against
  * @param[in] vline vectorized input string
- * @param[out] argv pointer to argument list if successful match
- * @param[out] element pointer to matched cmd_element if successful match
+ * @param[out] argv pointer to argument list if successful match, NULL
+ * otherwise. The elements of this list are pointers to struct cmd_token
+ * and represent the sequence of tokens matched by the inpu. The ->arg
+ * field of each token points to a copy of the input matched on it. These
+ * may be safely deleted or modified.
+ * @param[out] element pointer to matched cmd_element if successful match,
+ * or NULL when MATCHER_ERROR(rv) is true. The cmd_element may *not* be
+ * safely deleted or modified; it is the instance initialized on startup.
  * @return matcher status
  */
 enum matcher_rv
 command_match (struct graph *cmdgraph,
                vector vline,
                struct list **argv,
-               struct cmd_element **element);
+               const struct cmd_element **element);
 
 /**
  * Compiles possible completions for a given line of user input.
  *
  * @param[in] start the start node of the DFA to match against
  * @param[in] vline vectorized input string
- * @param[in] completions pointer to list of cmd_token representing
- *            acceptable next inputs
+ * @param[out] completions pointer to list of cmd_token representing
+ * acceptable next inputs, or NULL when MATCHER_ERROR(rv) is true.
+ * The elements of this list are pointers to struct cmd_token and take on a
+ * variety of forms depending on the passed vline. If the last element in vline
+ * is NULL, all previous elements are considered to be complete words (the case
+ * when a space is the last token of the line) and completions are generated
+ * based on what could follow that input. If the last element in vline is not
+ * NULL and each sequential element matches the corresponding tokens of one or
+ * more commands exactly (e.g. 'encapv4' and not 'en') the same result is
+ * generated. If the last element is not NULL and the best possible match is a
+ * partial match, then the result generated will be all possible continuations
+ * of that element (e.g. 'encapv4', 'encapv6', etc for input 'en').
+ * @return matcher status
  */
 enum matcher_rv
 command_complete (struct graph *cmdgraph,
diff --git a/lib/command_parse.y b/lib/command_parse.y
index 3ef0b42eb..6348643b8 100644
--- a/lib/command_parse.y
+++ b/lib/command_parse.y
@@ -383,6 +383,7 @@ option_token_seq:
   $$->start = $1->start;
   $$->end   = $2->end;
   free ($1);
+  free ($2);
 }
 ;