lib: fix route map crash on prefix list removal

Changes:
- Refactor list entry deletion to use a function that properly notifies
  route map on deletion (fixes a heap-use-after-free).
- Prefix list entry wild card sets `le` to maximum IP mask value and
  `any` is a boolean.
- Fix prefix list trie removal order (in `prefix_list_entry_update_start`).
- Let only the `any` callback change the value of field `any`.

Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>

diff --git a/lib/filter_nb.c b/lib/filter_nb.c
index 54811ac24576a63c54dda81163021bed4b645b18..9df37d7cccc21ec720fa7127dfbbdcba6807e83a 100644 (file)
--- a/lib/filter_nb.c
+++ b/lib/filter_nb.c
@@ -98,6 +98,19 @@ prefix_list_length_validate(const struct lyd_node *dnode)
        return NB_ERR_VALIDATION;
 }
 
+/**
+ * Sets prefix list entry to blank value.
+ *
+ * \param[out] ple prefix list entry to modify.
+ */
+static void prefix_list_entry_set_empty(struct prefix_list_entry *ple)
+{
+       ple->any = false;
+       memset(&ple->prefix, 0, sizeof(ple->prefix));
+       ple->ge = 0;
+       ple->le = 0;
+}
+
 /*
  * XPath: /frr-filter:lib/access-list-legacy
  */
@@ -836,8 +849,8 @@ static int lib_prefix_list_entry_create(struct nb_cb_create_args *args)
        pl = nb_running_get_entry(args->dnode, NULL, true);
        ple = prefix_list_entry_new();
        ple->pl = pl;
-       ple->any = 1;
        ple->seq = yang_dnode_get_uint32(args->dnode, "./sequence");
+       prefix_list_entry_set_empty(ple);
        nb_running_set_entry(args->dnode, ple);
 
        return NB_OK;
@@ -852,7 +865,7 @@ static int lib_prefix_list_entry_destroy(struct nb_cb_destroy_args *args)
 
        ple = nb_running_unset_entry(args->dnode);
        if (ple->installed)
-               prefix_list_entry_delete(ple->pl, ple, 0);
+               prefix_list_entry_delete2(ple);
        else
                prefix_list_entry_free(ple);
 
@@ -904,7 +917,6 @@ lib_prefix_list_entry_ipv4_prefix_modify(struct nb_cb_modify_args *args)
        prefix_list_entry_update_start(ple);
 
        yang_dnode_get_prefix(&ple->prefix, args->dnode, NULL);
-       ple->any = 0;
 
        /* Finish prefix entry update procedure. */
        prefix_list_entry_update_finish(ple);
@@ -926,7 +938,6 @@ lib_prefix_list_entry_ipv4_prefix_destroy(struct nb_cb_destroy_args *args)
        prefix_list_entry_update_start(ple);
 
        memset(&ple->prefix, 0, sizeof(ple->prefix));
-       ple->any = 1;
 
        /* Finish prefix entry update procedure. */
        prefix_list_entry_update_finish(ple);
@@ -1087,6 +1098,7 @@ static int lib_prefix_list_entry_ipv6_prefix_length_lesser_or_equal_destroy(
 static int lib_prefix_list_entry_any_create(struct nb_cb_create_args *args)
 {
        struct prefix_list_entry *ple;
+       int type;
 
        if (args->event != NB_EV_APPLY)
                return NB_OK;
@@ -1096,8 +1108,24 @@ static int lib_prefix_list_entry_any_create(struct nb_cb_create_args *args)
        /* Start prefix entry update procedure. */
        prefix_list_entry_update_start(ple);
 
+       ple->any = true;
+
+       /* Fill prefix struct from scratch. */
        memset(&ple->prefix, 0, sizeof(ple->prefix));
-       ple->any = 1;
+
+       type = yang_dnode_get_enum(args->dnode, "../../type");
+       switch (type) {
+       case 0: /* ipv4 */
+               ple->prefix.family = AF_INET;
+               ple->ge = 0;
+               ple->le = IPV4_MAX_BITLEN;
+               break;
+       case 1: /* ipv6 */
+               ple->prefix.family = AF_INET6;
+               ple->ge = 0;
+               ple->le = IPV6_MAX_BITLEN;
+               break;
+       }
 
        /* Finish prefix entry update procedure. */
        prefix_list_entry_update_finish(ple);
@@ -1117,8 +1145,7 @@ static int lib_prefix_list_entry_any_destroy(struct nb_cb_destroy_args *args)
        /* Start prefix entry update procedure. */
        prefix_list_entry_update_start(ple);
 
-       memset(&ple->prefix, 0, sizeof(ple->prefix));
-       ple->any = 1;
+       prefix_list_entry_set_empty(ple);
 
        /* Finish prefix entry update procedure. */
        prefix_list_entry_update_finish(ple);

diff --git a/lib/plist.c b/lib/plist.c
index 958d98dc966f85a04a5142dc9c6c6b41f5beefd1..981e86e2acb787209579a28d36f95ff1e98bda01 100644 (file)
--- a/lib/plist.c
+++ b/lib/plist.c
@@ -661,6 +661,8 @@ void prefix_list_entry_update_start(struct prefix_list_entry *ple)
        if (!ple->installed)
                return;
 
+       prefix_list_trie_del(pl, ple);
+
        /* List manipulation: shameless copy from `prefix_list_entry_delete`. */
        if (ple->prev)
                ple->prev->next = ple->next;
@@ -671,11 +673,17 @@ void prefix_list_entry_update_start(struct prefix_list_entry *ple)
        else
                pl->tail = ple->prev;
 
-       prefix_list_trie_del(pl, ple);
        route_map_notify_pentry_dependencies(pl->name, ple,
                                             RMAP_EVENT_PLIST_DELETED);
        pl->count--;
 
+       route_map_notify_dependencies(pl->name, RMAP_EVENT_PLIST_DELETED);
+       if (pl->master->delete_hook)
+               (*pl->master->delete_hook)(pl);
+
+       if (pl->head || pl->tail || pl->desc)
+               pl->master->recent = pl;
+
        ple->installed = false;
 }
 
@@ -695,6 +703,14 @@ void prefix_list_entry_update_finish(struct prefix_list_entry *ple)
        if (ple->installed)
                return;
 
+       /*
+        * Check if the entry is installable:
+        * We can only install entry if at least the prefix is provided (IPv4
+        * or IPv6).
+        */
+       if (ple->prefix.family != AF_INET && ple->prefix.family != AF_INET6)
+               return;
+
        /* List manipulation: shameless copy from `prefix_list_entry_add`. */
        if (pl->tail && ple->seq > pl->tail->seq)
                point = NULL;
@@ -742,6 +758,21 @@ void prefix_list_entry_update_finish(struct prefix_list_entry *ple)
        ple->installed = true;
 }
 
+/**
+ * Same as `prefix_list_entry_delete` but without `free()`ing the list if its
+ * empty.
+ *
+ * \param[in] ple prefix list entry.
+ */
+void prefix_list_entry_delete2(struct prefix_list_entry *ple)
+{
+       /* Does the boiler plate list removal and entry removal notification. */
+       prefix_list_entry_update_start(ple);
+
+       /* Effective `free()` memory. */
+       prefix_list_entry_free(ple);
+}
+
 /* Return string of prefix_list_type. */
 static const char *prefix_list_type_str(struct prefix_list_entry *pentry)
 {

diff --git a/lib/plist_int.h b/lib/plist_int.h
index cef78a3ef7a089764776279b603b87b4a7a07efa..5e0beabbc690b39929664eb06d90cabcdbcadd7a 100644 (file)
--- a/lib/plist_int.h
+++ b/lib/plist_int.h
@@ -78,6 +78,7 @@ struct prefix_list_entry {
 };
 
 extern void prefix_list_entry_free(struct prefix_list_entry *pentry);
+extern void prefix_list_entry_delete2(struct prefix_list_entry *ple);
 extern void prefix_list_entry_update_start(struct prefix_list_entry *ple);
 extern void prefix_list_entry_update_finish(struct prefix_list_entry *ple);