From 5bf15faa19288144c214ca48ef26e01512037ef6 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 6 Jan 2020 12:58:41 -0500 Subject: [PATCH] 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 --- zebra/zebra_nhg.c | 55 ++++++++++++++++++++++++++++++++------- zebra/zebra_nhg_private.h | 21 ++++++++++++--- 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 7430307320..0925705a25 100644 --- a/zebra/zebra_nhg.c +++ b/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 *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 @@ -1104,8 +1133,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 79107b047c..92f438fcec 100644 --- 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__ */ -- 2.39.5