ospf6d: Fix memory leaks

Free route node upon asbr redistribute route cleanup from
external_id_table route tale.
Free route node when route_remove is called and
node->info is set to null.
Decrement route node lock in route_lookup api as it
is incremented as part of node_lookup api.
use local variable for nexthop vs. malloc in zebra parse
routine.

two of the memory leaks related to nexthops per route were not freed.
two of the memory leak detected per frr service restart

Signed-off-by: Chirag Shah <chirag@cumulusnetworks.com>

diff --git a/ospf6d/ospf6_asbr.c b/ospf6d/ospf6_asbr.c
index 7f8341d0e42c67ab91a8fad2eca5c28760fc4853..dd3630af160ce32278a95c2a0f82531313c08757 100644 (file)
--- a/ospf6d/ospf6_asbr.c
+++ b/ospf6d/ospf6_asbr.c
@@ -622,7 +622,8 @@ void ospf6_asbr_redistribute_remove(int type, ifindex_t ifindex,
        node = route_node_lookup(ospf6->external_id_table, &prefix_id);
        assert(node);
        node->info = NULL;
-       route_unlock_node(node);
+       route_unlock_node(node); /* to free the lookup lock */
+       route_unlock_node(node); /* to free the original lock */
 
        ospf6_route_remove(match, ospf6->external_table);
        XFREE(MTYPE_OSPF6_EXTERNAL_INFO, info);

diff --git a/ospf6d/ospf6_lsa.c b/ospf6d/ospf6_lsa.c
index 329060a16f7b2f685e919c7e0c02f5dd59802ba3..7993916807764cf7586bfbbeb004b3c2573a6595 100644 (file)
--- a/ospf6d/ospf6_lsa.c
+++ b/ospf6d/ospf6_lsa.c
@@ -509,7 +509,8 @@ struct ospf6_lsa *ospf6_lsa_create(struct ospf6_lsa_header *header)
 
        /* allocate memory for this LSA */
        new_header =
-               (struct ospf6_lsa_header *)XMALLOC(MTYPE_OSPF6_LSA, lsa_size);
+               (struct ospf6_lsa_header *)XMALLOC(MTYPE_OSPF6_LSA_HEADER,
+                                                  lsa_size);
 
        /* copy LSA from original header */
        memcpy(new_header, header, lsa_size);
@@ -537,7 +538,7 @@ struct ospf6_lsa *ospf6_lsa_create_headeronly(struct ospf6_lsa_header *header)
 
        /* allocate memory for this LSA */
        new_header = (struct ospf6_lsa_header *)XMALLOC(
-               MTYPE_OSPF6_LSA, sizeof(struct ospf6_lsa_header));
+               MTYPE_OSPF6_LSA_HEADER, sizeof(struct ospf6_lsa_header));
 
        /* copy LSA from original header */
        memcpy(new_header, header, sizeof(struct ospf6_lsa_header));
@@ -568,7 +569,7 @@ void ospf6_lsa_delete(struct ospf6_lsa *lsa)
        THREAD_OFF(lsa->refresh);
 
        /* do free */
-       XFREE(MTYPE_OSPF6_LSA, lsa->header);
+       XFREE(MTYPE_OSPF6_LSA_HEADER, lsa->header);
        XFREE(MTYPE_OSPF6_LSA, lsa);
 }
 

diff --git a/ospf6d/ospf6_lsdb.c b/ospf6d/ospf6_lsdb.c
index 7e08d58791e13ee57d7f8a31d58e2cfdec36d7f4..8764a549d2572f084762198d007bdcb240e71f4e 100644 (file)
--- a/ospf6d/ospf6_lsdb.c
+++ b/ospf6d/ospf6_lsdb.c
@@ -139,6 +139,8 @@ void ospf6_lsdb_add(struct ospf6_lsa *lsa, struct ospf6_lsdb *lsdb)
                                        (*lsdb->hook_add)(lsa);
                        }
                }
+               /* to free the lookup lock in node get*/
+               route_unlock_node(current);
                ospf6_lsa_unlock(old);
        }
 

diff --git a/ospf6d/ospf6_memory.c b/ospf6d/ospf6_memory.c
index 133dc2cb3cbebe9e273d6e640bc5387308b1a833..56c232d6da98bbb90c07f2cef085e5fec8e56ca6 100644 (file)
--- a/ospf6d/ospf6_memory.c
+++ b/ospf6d/ospf6_memory.c
@@ -34,6 +34,7 @@ DEFINE_MTYPE(OSPF6D, OSPF6_ROUTE, "OSPF6 route")
 DEFINE_MTYPE(OSPF6D, OSPF6_PREFIX, "OSPF6 prefix")
 DEFINE_MTYPE(OSPF6D, OSPF6_MESSAGE, "OSPF6 message")
 DEFINE_MTYPE(OSPF6D, OSPF6_LSA, "OSPF6 LSA")
+DEFINE_MTYPE(OSPF6D, OSPF6_LSA_HEADER, "OSPF6 LSA header")
 DEFINE_MTYPE(OSPF6D, OSPF6_LSA_SUMMARY, "OSPF6 LSA summary")
 DEFINE_MTYPE(OSPF6D, OSPF6_LSDB, "OSPF6 LSA database")
 DEFINE_MTYPE(OSPF6D, OSPF6_VERTEX, "OSPF6 vertex")

diff --git a/ospf6d/ospf6_memory.h b/ospf6d/ospf6_memory.h
index 324065c3a10e50d50de5208c299a0449ecc99c86..fe72ee3669763e89cdd4fe490bdb446dc90864e8 100644 (file)
--- a/ospf6d/ospf6_memory.h
+++ b/ospf6d/ospf6_memory.h
@@ -33,6 +33,7 @@ DECLARE_MTYPE(OSPF6_ROUTE)
 DECLARE_MTYPE(OSPF6_PREFIX)
 DECLARE_MTYPE(OSPF6_MESSAGE)
 DECLARE_MTYPE(OSPF6_LSA)
+DECLARE_MTYPE(OSPF6_LSA_HEADER)
 DECLARE_MTYPE(OSPF6_LSA_SUMMARY)
 DECLARE_MTYPE(OSPF6_LSDB)
 DECLARE_MTYPE(OSPF6_VERTEX)

diff --git a/ospf6d/ospf6_route.c b/ospf6d/ospf6_route.c
index 5e8fd0e15d7bf5f47902b43f45f0131fc47c9578..a4e7af1d8bad0f4d7475dded605727cd29bcc98f 100644 (file)
--- a/ospf6d/ospf6_route.c
+++ b/ospf6d/ospf6_route.c
@@ -178,17 +178,6 @@ void ospf6_nexthop_delete(struct ospf6_nexthop *nh)
                XFREE(MTYPE_OSPF6_NEXTHOP, nh);
 }
 
-void ospf6_free_nexthops(struct list *nh_list)
-{
-       struct ospf6_nexthop *nh;
-       struct listnode *node, *nnode;
-
-       if (nh_list) {
-               for (ALL_LIST_ELEMENTS(nh_list, node, nnode, nh))
-                       ospf6_nexthop_delete(nh);
-       }
-}
-
 void ospf6_clear_nexthops(struct list *nh_list)
 {
        struct listnode *node;
@@ -340,19 +329,29 @@ int ospf6_route_get_first_nh_index(struct ospf6_route *route)
        return (-1);
 }
 
+static int ospf6_nexthop_cmp(struct ospf6_nexthop *a, struct ospf6_nexthop *b)
+{
+       if ((a)->ifindex == (b)->ifindex &&
+           IN6_ARE_ADDR_EQUAL(&(a)->address, &(b)->address))
+               return 1;
+       return 0;
+}
+
 struct ospf6_route *ospf6_route_create(void)
 {
        struct ospf6_route *route;
        route = XCALLOC(MTYPE_OSPF6_ROUTE, sizeof(struct ospf6_route));
        route->nh_list = list_new();
+       route->nh_list->cmp = (int (*)(void *, void *))ospf6_nexthop_cmp;
+       route->nh_list->del = (void (*) (void *))ospf6_nexthop_delete;
        return route;
 }
 
 void ospf6_route_delete(struct ospf6_route *route)
 {
        if (route) {
-               ospf6_free_nexthops(route->nh_list);
-               list_free(route->nh_list);
+               if (route->nh_list)
+                       list_free(route->nh_list);
                XFREE(MTYPE_OSPF6_ROUTE, route);
        }
 }
@@ -439,6 +438,7 @@ struct ospf6_route *ospf6_route_lookup(struct prefix *prefix,
                return NULL;
 
        route = (struct ospf6_route *)node->info;
+       route_unlock_node(node); /* to free the lookup lock */
        return route;
 }
 
@@ -583,6 +583,8 @@ struct ospf6_route *ospf6_route_add(struct ospf6_route *route,
                        SET_FLAG(old->flag, OSPF6_ROUTE_ADD);
                        ospf6_route_table_assert(table);
 
+                       /* to free the lookup lock */
+                       route_unlock_node(node);
                        return old;
                }
 
@@ -628,9 +630,10 @@ struct ospf6_route *ospf6_route_add(struct ospf6_route *route,
        if (prev || next) {
                if (IS_OSPF6_DEBUG_ROUTE(MEMORY))
                        zlog_debug(
-                               "%s %p: route add %p: another path: prev %p, next %p",
+                               "%s %p: route add %p: another path: prev %p, next %p node lock %u",
                                ospf6_route_table_name(table), (void *)table,
-                               (void *)route, (void *)prev, (void *)next);
+                               (void *)route, (void *)prev, (void *)next,
+                               node->lock);
                else if (IS_OSPF6_DEBUG_ROUTE(TABLE))
                        zlog_debug("%s: route add: another path found",
                                   ospf6_route_table_name(table));
@@ -755,9 +758,9 @@ void ospf6_route_remove(struct ospf6_route *route,
                prefix2str(&route->prefix, buf, sizeof(buf));
 
        if (IS_OSPF6_DEBUG_ROUTE(MEMORY))
-               zlog_debug("%s %p: route remove %p: %s",
+               zlog_debug("%s %p: route remove %p: %s rnode lock %u",
                           ospf6_route_table_name(table), (void *)table,
-                          (void *)route, buf);
+                          (void *)route, buf, route->rnode->lock);
        else if (IS_OSPF6_DEBUG_ROUTE(TABLE))
                zlog_debug("%s: route remove: %s",
                           ospf6_route_table_name(table), buf);
@@ -768,11 +771,9 @@ void ospf6_route_remove(struct ospf6_route *route,
        /* find the route to remove, making sure that the route pointer
           is from the route table. */
        current = node->info;
-       while (current && ospf6_route_is_same(current, route)) {
-               if (current == route)
-                       break;
+       while (current && current != route)
                current = current->next;
-       }
+
        assert(current == route);
 
        /* adjust doubly linked list */
@@ -785,10 +786,14 @@ void ospf6_route_remove(struct ospf6_route *route,
                if (route->next && route->next->rnode == node) {
                        node->info = route->next;
                        SET_FLAG(route->next->flag, OSPF6_ROUTE_BEST);
-               } else
-                       node->info = NULL; /* should unlock route_node here ? */
+               } else {
+                       node->info = NULL;
+                       route->rnode = NULL;
+                       route_unlock_node(node); /* to free the original lock */
+               }
        }
 
+       route_unlock_node(node); /* to free the lookup lock */
        table->count--;
        ospf6_route_table_assert(table);
 
@@ -935,6 +940,7 @@ struct ospf6_route_table *ospf6_route_table_create(int s, int t)
 void ospf6_route_table_delete(struct ospf6_route_table *table)
 {
        ospf6_route_remove_all(table);
+       bf_free(table->idspace);
        route_table_finish(table->table);
        XFREE(MTYPE_OSPF6_ROUTE, table);
 }
@@ -1062,6 +1068,7 @@ void ospf6_route_show_detail(struct vty *vty, struct ospf6_route *route)
        vty_out(vty, "Metric: %d (%d)\n", route->path.cost,
                route->path.u.cost_e2);
 
+       vty_out(vty, "Nexthop count: %u\n", route->nh_list->count);
        /* Nexthops */
        vty_out(vty, "Nexthop:\n");
        for (ALL_LIST_ELEMENTS_RO(route->nh_list, node, nh)) {

diff --git a/ospf6d/ospf6_route.h b/ospf6d/ospf6_route.h
index 69d275f8b119d315da30edbbe61d425d638275a0..166074fb705c5c2d239dac829eb5a07cf8f6d2d8 100644 (file)
--- a/ospf6d/ospf6_route.h
+++ b/ospf6d/ospf6_route.h
@@ -256,7 +256,6 @@ extern void ospf6_linkstate_prefix2str(struct prefix *prefix, char *buf,
 
 extern struct ospf6_nexthop *ospf6_nexthop_create(void);
 extern void ospf6_nexthop_delete(struct ospf6_nexthop *nh);
-extern void ospf6_free_nexthops(struct list *nh_list);
 extern void ospf6_clear_nexthops(struct list *nh_list);
 extern int ospf6_num_nexthops(struct list *nh_list);
 extern void ospf6_copy_nexthops(struct list *dst, struct list *src);

diff --git a/ospf6d/ospf6_spf.c b/ospf6d/ospf6_spf.c
index 86f893bc61b925506a26ad0aab187a31b297b92a..6d589aff8f9b960f869442acdfbcc0eb1c4653d1 100644 (file)
--- a/ospf6d/ospf6_spf.c
+++ b/ospf6d/ospf6_spf.c
@@ -141,6 +141,7 @@ static struct ospf6_vertex *ospf6_vertex_create(struct ospf6_lsa *lsa)
        v->options[2] = *(u_char *)(OSPF6_LSA_HEADER_END(lsa->header) + 3);
 
        v->nh_list = list_new();
+       v->nh_list->del = (void (*) (void *))ospf6_nexthop_delete;
 
        v->parent = NULL;
        v->child_list = list_new();

diff --git a/ospf6d/ospf6_zebra.c b/ospf6d/ospf6_zebra.c
index d33f41730e2130b9c1f0403f9f5e4b2c3476bde5..e7481ba7237c223906d935c0f19c108482dafa17 100644 (file)
--- a/ospf6d/ospf6_zebra.c
+++ b/ospf6d/ospf6_zebra.c
@@ -214,14 +214,14 @@ static int ospf6_zebra_read_ipv6(int command, struct zclient *zclient,
        struct zapi_ipv6 api;
        unsigned long ifindex;
        struct prefix p, src_p;
-       struct in6_addr *nexthop;
+       struct in6_addr nexthop;
 
        if (ospf6 == NULL)
                return 0;
 
        s = zclient->ibuf;
        ifindex = 0;
-       nexthop = NULL;
+       memset(&nexthop, 0, sizeof(struct in6_addr));
        memset(&api, 0, sizeof(api));
 
        /* Type, flags, message. */
@@ -250,10 +250,7 @@ static int ospf6_zebra_read_ipv6(int command, struct zclient *zclient,
        /* Nexthop, ifindex, distance, metric. */
        if (CHECK_FLAG(api.message, ZAPI_MESSAGE_NEXTHOP)) {
                api.nexthop_num = stream_getc(s);
-               nexthop = (struct in6_addr *)malloc(api.nexthop_num
-                                                   * sizeof(struct in6_addr));
-               stream_get(nexthop, s,
-                          api.nexthop_num * sizeof(struct in6_addr));
+               stream_get(&nexthop, s, IPV6_MAX_BYTELEN);
        }
        if (CHECK_FLAG(api.message, ZAPI_MESSAGE_IFINDEX)) {
                api.ifindex_num = stream_getc(s);
@@ -276,9 +273,9 @@ static int ospf6_zebra_read_ipv6(int command, struct zclient *zclient,
        if (IS_OSPF6_DEBUG_ZEBRA(RECV)) {
                char prefixstr[PREFIX2STR_BUFFER], nexthopstr[128];
                prefix2str((struct prefix *)&p, prefixstr, sizeof(prefixstr));
-               if (nexthop)
-                       inet_ntop(AF_INET6, nexthop, nexthopstr,
-                                 sizeof(nexthopstr));
+               if (api.nexthop_num)
+                       inet_ntop(AF_INET6, &nexthop, nexthopstr,
+                                  sizeof(nexthopstr));
                else
                        snprintf(nexthopstr, sizeof(nexthopstr), "::");
 
@@ -292,12 +289,10 @@ static int ospf6_zebra_read_ipv6(int command, struct zclient *zclient,
 
        if (command == ZEBRA_REDISTRIBUTE_IPV6_ADD)
                ospf6_asbr_redistribute_add(api.type, ifindex, &p,
-                                           api.nexthop_num, nexthop, api.tag);
+                                           api.nexthop_num, &nexthop, api.tag);
        else
                ospf6_asbr_redistribute_remove(api.type, ifindex, &p);
 
-       if (CHECK_FLAG(api.message, ZAPI_MESSAGE_NEXTHOP))
-               free(nexthop);
 
        return 0;
 }