abf: add API parameter n_paths range checks

Also check for non-zero rpath length in CLI cmd.
While there, no need to use "else" after a return.
Also while there, notice and fix numerous input_line
buffer leaks and fix them.

Type: fix
Fixes: 669d07dc016757b856e1014a415996cf9f0ebc58

Signed-off-by: Jon Loeliger <jdl@netgate.com>
Change-Id: I18ea44b7b82e8938c3e793e7c2a04dfe157076d8

diff --git a/src/plugins/abf/abf.api b/src/plugins/abf/abf.api
index 1cd3da7..a748de4 100644 (file)
--- a/src/plugins/abf/abf.api
+++ b/src/plugins/abf/abf.api
@@ -51,7 +51,7 @@ define abf_plugin_get_version_reply
 /** \brief A description of an ABF policy
     @param policy_id User chosen Identifier for the policy
     @param acl_index The ACL that the policy will match against
-    @param n_paths Number of paths
+    @param n_paths Number of paths, 1..255
     @param paths The set of forwarding paths that are being added or removed.
  */
 typedef abf_policy

diff --git a/src/plugins/abf/abf_api.c b/src/plugins/abf/abf_api.c
index 66121ac..222e1f4 100644 (file)
--- a/src/plugins/abf/abf_api.c
+++ b/src/plugins/abf/abf_api.c
@@ -69,6 +69,12 @@ vl_api_abf_policy_add_del_t_handler (vl_api_abf_policy_add_del_t * mp)
   int rv = 0;
   u8 pi;
 
+  if (mp->policy.n_paths == 0)
+    {
+      rv = VNET_API_ERROR_INVALID_VALUE;
+      goto done;
+    }
+
   vec_validate (paths, mp->policy.n_paths - 1);
 
   for (pi = 0; pi < mp->policy.n_paths; pi++)

diff --git a/src/plugins/abf/abf_policy.c b/src/plugins/abf/abf_policy.c
index 1921df1..8f0fc11 100644 (file)
--- a/src/plugins/abf/abf_policy.c
+++ b/src/plugins/abf/abf_policy.c
@@ -192,50 +192,45 @@ abf_policy_delete (u32 policy_id, const fib_route_path_t * rpaths)
        */
       return (VNET_API_ERROR_INVALID_VALUE);
     }
-  else
-    {
-      /*
-       * update an existing policy.
-       * - add the path to the path-list and swap our ancestry
-       * - backwalk to poke all attachments to update
-       */
-      fib_node_index_t old_pl;
 
-      ap = abf_policy_get (api);
-      old_pl = ap->ap_pl;
+  /*
+   * update an existing policy.
+   * - add the path to the path-list and swap our ancestry
+   * - backwalk to poke all attachments to update
+   */
+  fib_node_index_t old_pl;
 
-      fib_path_list_lock (old_pl);
-      ap->ap_pl =
-       fib_path_list_copy_and_path_remove (ap->ap_pl,
-                                           (FIB_PATH_LIST_FLAG_SHARED |
-                                            FIB_PATH_LIST_FLAG_NO_URPF),
-                                           rpaths);
+  ap = abf_policy_get (api);
+  old_pl = ap->ap_pl;
 
-      fib_path_list_child_remove (old_pl, ap->ap_sibling);
-      ap->ap_sibling = ~0;
+  fib_path_list_lock (old_pl);
+  ap->ap_pl = fib_path_list_copy_and_path_remove (
+    ap->ap_pl, (FIB_PATH_LIST_FLAG_SHARED | FIB_PATH_LIST_FLAG_NO_URPF),
+    rpaths);
 
-      if (FIB_NODE_INDEX_INVALID == ap->ap_pl)
-       {
-         /*
-          * no more paths on this policy. It's toast
-          * remove the CLI/API's lock
-          */
-         fib_node_unlock (&ap->ap_node);
-       }
-      else
-       {
-         ap->ap_sibling = fib_path_list_child_add (ap->ap_pl,
-                                                   abf_policy_fib_node_type,
-                                                   api);
+  fib_path_list_child_remove (old_pl, ap->ap_sibling);
+  ap->ap_sibling = ~0;
 
-         fib_node_back_walk_ctx_t ctx = {
-           .fnbw_reason = FIB_NODE_BW_REASON_FLAG_EVALUATE,
-         };
+  if (FIB_NODE_INDEX_INVALID == ap->ap_pl)
+    {
+      /*
+       * no more paths on this policy. It's toast
+       * remove the CLI/API's lock
+       */
+      fib_node_unlock (&ap->ap_node);
+    }
+  else
+    {
+      ap->ap_sibling =
+       fib_path_list_child_add (ap->ap_pl, abf_policy_fib_node_type, api);
 
-         fib_walk_sync (abf_policy_fib_node_type, api, &ctx);
-       }
-      fib_path_list_unlock (old_pl);
+      fib_node_back_walk_ctx_t ctx = {
+       .fnbw_reason = FIB_NODE_BW_REASON_FLAG_EVALUATE,
+      };
+
+      fib_walk_sync (abf_policy_fib_node_type, api, &ctx);
     }
+  fib_path_list_unlock (old_pl);
 
   return (0);
 }
@@ -272,14 +267,25 @@ abf_policy_cmd (vlib_main_t * vm,
                         unformat_fib_route_path, &rpath, &payload_proto))
        vec_add1 (rpaths, rpath);
       else
-       return (clib_error_return (0, "unknown input '%U'",
-                                  format_unformat_error, line_input));
+       {
+         clib_error_t *err;
+         err = clib_error_return (0, "unknown input '%U'",
+                                  format_unformat_error, line_input);
+         unformat_free (line_input);
+         return err;
+       }
     }
 
   if (INDEX_INVALID == policy_id)
     {
       vlib_cli_output (vm, "Specify a Policy ID");
-      return 0;
+      goto out;
+    }
+
+  if (vec_len (rpaths) == 0)
+    {
+      vlib_cli_output (vm, "Hop path must not be empty");
+      goto out;
     }
 
   if (!is_del)
@@ -287,7 +293,7 @@ abf_policy_cmd (vlib_main_t * vm,
       if (INDEX_INVALID == acl_index)
        {
          vlib_cli_output (vm, "ACL index must be set");
-         return 0;
+         goto out;
        }
 
       rv = abf_policy_update (policy_id, acl_index, rpaths);
@@ -296,7 +302,7 @@ abf_policy_cmd (vlib_main_t * vm,
        {
          vlib_cli_output (vm,
                           "ACL index must match existing ACL index in policy");
-         return 0;
+         goto out;
        }
     }
   else
@@ -304,6 +310,7 @@ abf_policy_cmd (vlib_main_t * vm,
       abf_policy_delete (policy_id, rpaths);
     }
 
+out:
   unformat_free (line_input);
   return (NULL);
 }