From f94a7703c0725766f7dd91931718d3869ed28116 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 6 Aug 2021 16:00:32 -0400 Subject: [PATCH 1/2] zebra: Prevent memory leak if route is rejected early When receiving a route via zapi, if the route is rejected there exists a code path where we would not free the corresponding re created. Signed-off-by: Donald Sharp --- zebra/zapi_msg.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 27fb5d7c22..08ad390d32 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -2110,6 +2110,13 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) ret = rib_add_multipath_nhe(afi, api.safi, &api.prefix, src_p, re, &nhe); + /* + * rib_add_multipath_nhe only fails in a couple spots + * and in those spots we have not freed memory + */ + if (ret == -1) + XFREE(MTYPE_RE, re); + /* At this point, these allocations are not needed: 're' has been * retained or freed, and if 're' still exists, it is using * a reference to a shared group object. From 38c764dde42b605e12a35d3bcb5b93cf72272d30 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 9 Aug 2021 08:01:06 -0400 Subject: [PATCH 2/2] zebra: Properly note add/update for rib_add_multipath_nhe When calling rib_add_multipath_nhe ensure that we have well aligned return codes that mean something so that interersted parties can properly handle the situation. Signed-off-by: Donald Sharp --- zebra/rib.h | 5 +++++ zebra/zapi_msg.c | 12 +++++++----- zebra/zebra_rib.c | 9 +++++++-- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/zebra/rib.h b/zebra/rib.h index 31d9dfd265..a663575e10 100644 --- a/zebra/rib.h +++ b/zebra/rib.h @@ -403,6 +403,11 @@ extern int rib_add(afi_t afi, safi_t safi, vrf_id_t vrf_id, int type, extern int rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p, struct prefix_ipv6 *src_p, struct route_entry *re, struct nexthop_group *ng); +/* + * -1 -> some sort of error + * 0 -> an add + * 1 -> an update + */ extern int rib_add_multipath_nhe(afi_t afi, safi_t safi, struct prefix *p, struct prefix_ipv6 *src_p, struct route_entry *re, diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 08ad390d32..f297bafaf0 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -2114,8 +2114,10 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) * rib_add_multipath_nhe only fails in a couple spots * and in those spots we have not freed memory */ - if (ret == -1) + if (ret == -1) { + client->error_cnt++; XFREE(MTYPE_RE, re); + } /* At this point, these allocations are not needed: 're' has been * retained or freed, and if 're' still exists, it is using @@ -2128,15 +2130,15 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) /* Stats */ switch (api.prefix.family) { case AF_INET: - if (ret > 0) + if (ret == 0) client->v4_route_add_cnt++; - else if (ret < 0) + else if (ret == 1) client->v4_route_upd8_cnt++; break; case AF_INET6: - if (ret > 0) + if (ret == 0) client->v6_route_add_cnt++; - else if (ret < 0) + else if (ret == 1) client->v6_route_upd8_cnt++; break; } diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index c51dd759a6..2ed025857d 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -3453,6 +3453,10 @@ void rib_lookup_and_pushup(struct prefix_ipv4 *p, vrf_id_t vrf_id) * allocate: if they allocate a nexthop_group or backup nexthop info, they * must free those objects. If this returns < 0, an error has occurred and the * route_entry 're' has not been captured; the caller should free that also. + * + * -1 -> error + * 0 -> Add + * 1 -> update */ int rib_add_multipath_nhe(afi_t afi, safi_t safi, struct prefix *p, struct prefix_ipv6 *src_p, struct route_entry *re, @@ -3567,11 +3571,12 @@ int rib_add_multipath_nhe(afi_t afi, safi_t safi, struct prefix *p, SET_FLAG(re->status, ROUTE_ENTRY_CHANGED); rib_addnode(rn, re, 1); - ret = 1; /* Free implicit route.*/ - if (same) + if (same) { + ret = 1; rib_delnode(rn, same); + } /* See if we can remove some RE entries that are queued for * removal, but won't be considered in rib processing.