Merge pull request #5637 from sworleys/NHG-Zapi-Fuzz

Zebra nhg/nexthop fixes

zebra/zapi_msg.c

@@ -1475,6 +1475,8 @@ static void zread_route_add(ZAPI_HANDLER_ARGS)
 		api_nh = &api.nexthops[i];
 		ifindex_t ifindex = 0;
 
+		nexthop = NULL;
+
 		if (IS_ZEBRA_DEBUG_RECV)
 			zlog_debug("nh type %d", api_nh->type);
 

zebra/zebra_nhg.c

@@ -99,31 +99,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 *
+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);
+	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 (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,
+/* 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)
+	/* On success, NULL will be returned from the
-		nhg_connected_tree_add(head, new);
+	 * 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
@@ -481,6 +510,8 @@ static void handle_recursive_depend(struct nhg_connected_tree_head *nhg_depends,
 	resolved_ng.nexthop = nh;
 
 	depend = zebra_nhg_rib_find(0, &resolved_ng, afi);
+
+	if (depend)
 		depends_add(nhg_depends, depend);
 }
 
@@ -1104,7 +1135,13 @@ done:
 static void depends_add(struct nhg_connected_tree_head *head,
 			struct nhg_hash_entry *depend)
 {
-	nhg_connected_tree_add_nhe(head, 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);
 }
 

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,
+
+/* 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);
-extern void nhg_connected_tree_add_nhe(struct nhg_connected_tree_head *head,
+/* 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__ */