zebra: don't created connected if duplicate depend

Since we are using a UNIQUE RB tree, we need to handle the
case of adding in a duplicate entry into it.

The list API code returns NULL when a successfull add
occurs, so lets pull that handling further up into
the connected handlers. Then, free the allocated
connected struct if it is a duplicate.

This is a pretty unlikely situation to happen.

Also, pull up the RB handling of _del RB API as well.

This was found with the zapi fuzzing code.

```
==1052840==
==1052840== 200 bytes in 5 blocks are definitely lost in loss record 545 of 663
==1052840==    at 0x483BB1A: calloc (vg_replace_malloc.c:762)
==1052840==    by 0x48E1008: qcalloc (memory.c:110)
==1052840==    by 0x44D357: nhg_connected_new (zebra_nhg.c:73)
==1052840==    by 0x44D300: nhg_connected_tree_add_nhe (zebra_nhg.c:123)
==1052840==    by 0x44FBDC: depends_add (zebra_nhg.c:1077)
==1052840==    by 0x44FD62: depends_find_add (zebra_nhg.c:1090)
==1052840==    by 0x44E46D: zebra_nhg_find (zebra_nhg.c:567)
==1052840==    by 0x44E1FE: zebra_nhg_rib_find (zebra_nhg.c:1126)
==1052840==    by 0x45AD3D: rib_add_multipath (zebra_rib.c:2616)
==1052840==    by 0x4977DC: zread_route_add (zapi_msg.c:1596)
==1052840==    by 0x49ABB9: zserv_handle_commands (zapi_msg.c:2636)
==1052840==    by 0x428B11: main (main.c:309)
```

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
(cherry picked from commit 5bf15faa19288144c214ca48ef26e01512037ef6)

diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c
index 72c22a1a29bebc34edd743e0776bd0013863f188..7a5b639206bb171ca21e8918306e44479c73cdab 100644 (file)
--- a/zebra/zebra_nhg.c
+++ b/zebra/zebra_nhg.c
@@ -102,31 +102,60 @@ nhg_connected_tree_root(struct nhg_connected_tree_head *head)
        return nhg_connected_tree_first(head);
 }
 
-void nhg_connected_tree_del_nhe(struct nhg_connected_tree_head *head,
-                               struct nhg_hash_entry *depend)
+struct nhg_hash_entry *
+nhg_connected_tree_del_nhe(struct nhg_connected_tree_head *head,
+                          struct nhg_hash_entry *depend)
 {
        struct nhg_connected lookup = {};
        struct nhg_connected *remove = NULL;
+       struct nhg_hash_entry *removed_nhe;
 
        lookup.nhe = depend;
 
        /* Lookup to find the element, then remove it */
        remove = nhg_connected_tree_find(head, &lookup);
-       remove = nhg_connected_tree_del(head, remove);
-
        if (remove)
+               /* Re-returning here just in case this API changes..
+                * the _del list api's are a bit undefined at the moment.
+                *
+                * So hopefully returning here will make it fail if the api
+                * changes to something different than currently expected.
+                */
+               remove = nhg_connected_tree_del(head, remove);
+
+       /* If the entry was sucessfully removed, free the 'connected` struct */
+       if (remove) {
+               removed_nhe = remove->nhe;
                nhg_connected_free(remove);
+               return removed_nhe;
+       }
+
+       return NULL;
 }
 
-void nhg_connected_tree_add_nhe(struct nhg_connected_tree_head *head,
-                               struct nhg_hash_entry *depend)
+/* Assuming UNIQUE RB tree. If this changes, assumptions here about
+ * insertion need to change.
+ */
+struct nhg_hash_entry *
+nhg_connected_tree_add_nhe(struct nhg_connected_tree_head *head,
+                          struct nhg_hash_entry *depend)
 {
        struct nhg_connected *new = NULL;
 
        new = nhg_connected_new(depend);
 
-       if (new)
-               nhg_connected_tree_add(head, new);
+       /* On success, NULL will be returned from the
+        * RB code.
+        */
+       if (new && (nhg_connected_tree_add(head, new) == NULL))
+               return NULL;
+
+       /* If it wasn't successful, it must be a duplicate. We enforce the
+        * unique property for the `nhg_connected` tree.
+        */
+       nhg_connected_free(new);
+
+       return depend;
 }
 
 static void
@@ -1118,8 +1147,14 @@ done:
 static void depends_add(struct nhg_connected_tree_head *head,
                        struct nhg_hash_entry *depend)
 {
-       nhg_connected_tree_add_nhe(head, depend);
-       zebra_nhg_increment_ref(depend);
+       /* If NULL is returned, it was successfully added and
+        * needs to have its refcnt incremented.
+        *
+        * Else the NHE is already present in the tree and doesn't
+        * need to increment the refcnt.
+        */
+       if (nhg_connected_tree_add_nhe(head, depend) == NULL)
+               zebra_nhg_increment_ref(depend);
 }
 
 static struct nhg_hash_entry *

diff --git a/zebra/zebra_nhg_private.h b/zebra/zebra_nhg_private.h
index 79107b047c95c1485d81f596d1d847f0e5e72614..92f438fcec75c217b43f70fa0f73b72cf940d8c9 100644 (file)
--- a/zebra/zebra_nhg_private.h
+++ b/zebra/zebra_nhg_private.h
@@ -52,9 +52,22 @@ extern bool
 nhg_connected_tree_is_empty(const struct nhg_connected_tree_head *head);
 extern struct nhg_connected *
 nhg_connected_tree_root(struct nhg_connected_tree_head *head);
-extern void nhg_connected_tree_del_nhe(struct nhg_connected_tree_head *head,
-                                      struct nhg_hash_entry *nhe);
-extern void nhg_connected_tree_add_nhe(struct nhg_connected_tree_head *head,
-                                      struct nhg_hash_entry *nhe);
+
+/* I realize _add/_del returns are backwords.
+ *
+ * Currently the list APIs are not standardized for what happens in
+ * the _del() function when the item isn't present.
+ *
+ * We are choosing to return NULL if not found in the _del case for now.
+ */
+
+/* Delete NHE from the tree. On success, return the NHE, otherwise NULL. */
+extern struct nhg_hash_entry *
+nhg_connected_tree_del_nhe(struct nhg_connected_tree_head *head,
+                          struct nhg_hash_entry *nhe);
+/* ADD NHE to the tree. On success, return NULL, otherwise return the NHE. */
+extern struct nhg_hash_entry *
+nhg_connected_tree_add_nhe(struct nhg_connected_tree_head *head,
+                          struct nhg_hash_entry *nhe);
 
 #endif /* __ZEBRA_NHG_PRIVATE_H__ */