From d107621dbcd2d0061bb2159ad0a7bdea6516ce3e Mon Sep 17 00:00:00 2001 From: Chirag Shah Date: Thu, 13 Jul 2017 11:39:22 -0700 Subject: [PATCH] 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 --- ospf6d/ospf6_asbr.c | 3 ++- ospf6d/ospf6_lsa.c | 7 +++--- ospf6d/ospf6_lsdb.c | 2 ++ ospf6d/ospf6_memory.c | 1 + ospf6d/ospf6_memory.h | 1 + ospf6d/ospf6_route.c | 53 ++++++++++++++++++++++++------------------- ospf6d/ospf6_route.h | 1 - ospf6d/ospf6_spf.c | 1 + ospf6d/ospf6_zebra.c | 19 ++++++---------- 9 files changed, 48 insertions(+), 40 deletions(-) diff --git a/ospf6d/ospf6_asbr.c b/ospf6d/ospf6_asbr.c index 7f8341d0e4..dd3630af16 100644 --- 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 329060a16f..7993916807 100644 --- 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 7e08d58791..8764a549d2 100644 --- 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 133dc2cb3c..56c232d6da 100644 --- 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 324065c3a1..fe72ee3669 100644 --- 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 5e8fd0e15d..a4e7af1d8b 100644 --- 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 69d275f8b1..166074fb70 100644 --- 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 86f893bc61..6d589aff8f 100644 --- 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 d33f41730e..e7481ba723 100644 --- 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; } -- 2.39.5