diff options
author | Donatas Abraitis <donatas@opensourcerouting.org> | 2023-09-13 20:52:37 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-09-13 20:52:37 +0200 |
commit | 75dbd45c5570c088dc0e2cf83e1b22b0ec0a30b3 (patch) | |
tree | 55850222126c35b83b5f5242218e0d9b31984d1e /bgpd | |
parent | Merge pull request #14410 from opensourcerouting/fix/keep_su_remote_local (diff) | |
parent | bgpd: Ensure that leak_update does not free memory before it is being used (diff) | |
download | frr-75dbd45c5570c088dc0e2cf83e1b22b0ec0a30b3.tar.xz frr-75dbd45c5570c088dc0e2cf83e1b22b0ec0a30b3.zip |
Merge pull request #14383 from donaldsharp/bgp_coverity_cleanup_early_sept
Bgp coverity cleanup early sept
Diffstat (limited to 'bgpd')
-rw-r--r-- | bgpd/bgp_advertise.c | 12 | ||||
-rw-r--r-- | bgpd/bgp_advertise.h | 4 | ||||
-rw-r--r-- | bgpd/bgp_evpn.c | 56 | ||||
-rw-r--r-- | bgpd/bgp_evpn_mh.c | 9 | ||||
-rw-r--r-- | bgpd/bgp_label.c | 3 | ||||
-rw-r--r-- | bgpd/bgp_mplsvpn.c | 3 | ||||
-rw-r--r-- | bgpd/bgp_nexthop.c | 4 | ||||
-rw-r--r-- | bgpd/bgp_route.c | 80 | ||||
-rw-r--r-- | bgpd/bgp_route.h | 3 | ||||
-rw-r--r-- | bgpd/bgp_table.c | 5 | ||||
-rw-r--r-- | bgpd/bgp_table.h | 2 |
11 files changed, 122 insertions, 59 deletions
diff --git a/bgpd/bgp_advertise.c b/bgpd/bgp_advertise.c index da7b496d6..6f4916b3c 100644 --- a/bgpd/bgp_advertise.c +++ b/bgpd/bgp_advertise.c @@ -188,22 +188,22 @@ void bgp_adj_in_set(struct bgp_dest *dest, struct peer *peer, struct attr *attr, bgp_dest_lock_node(dest); } -void bgp_adj_in_remove(struct bgp_dest *dest, struct bgp_adj_in *bai) +void bgp_adj_in_remove(struct bgp_dest **dest, struct bgp_adj_in *bai) { bgp_attr_unintern(&bai->attr); - BGP_ADJ_IN_DEL(dest, bai); - bgp_dest_unlock_node(dest); + BGP_ADJ_IN_DEL(*dest, bai); + *dest = bgp_dest_unlock_node(*dest); peer_unlock(bai->peer); /* adj_in peer reference */ XFREE(MTYPE_BGP_ADJ_IN, bai); } -bool bgp_adj_in_unset(struct bgp_dest *dest, struct peer *peer, +bool bgp_adj_in_unset(struct bgp_dest **dest, struct peer *peer, uint32_t addpath_id) { struct bgp_adj_in *adj; struct bgp_adj_in *adj_next; - adj = dest->adj_in; + adj = (*dest)->adj_in; if (!adj) return false; @@ -215,6 +215,8 @@ bool bgp_adj_in_unset(struct bgp_dest *dest, struct peer *peer, bgp_adj_in_remove(dest, adj); adj = adj_next; + + assert(*dest); } return true; diff --git a/bgpd/bgp_advertise.h b/bgpd/bgp_advertise.h index a063208b6..94168e2fd 100644 --- a/bgpd/bgp_advertise.h +++ b/bgpd/bgp_advertise.h @@ -130,9 +130,9 @@ extern bool bgp_adj_out_lookup(struct peer *peer, struct bgp_dest *dest, uint32_t addpath_tx_id); extern void bgp_adj_in_set(struct bgp_dest *dest, struct peer *peer, struct attr *attr, uint32_t addpath_id); -extern bool bgp_adj_in_unset(struct bgp_dest *dest, struct peer *peer, +extern bool bgp_adj_in_unset(struct bgp_dest **dest, struct peer *peer, uint32_t addpath_id); -extern void bgp_adj_in_remove(struct bgp_dest *dest, struct bgp_adj_in *bai); +extern void bgp_adj_in_remove(struct bgp_dest **dest, struct bgp_adj_in *bai); extern unsigned int bgp_advertise_attr_hash_key(const void *p); extern bool bgp_advertise_attr_hash_cmp(const void *p1, const void *p2); diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index e5d33e6d5..aa23f0676 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2035,10 +2035,10 @@ static void evpn_zebra_reinstall_best_route(struct bgp *bgp, * additional handling to prevent bgp from injecting and holding on to a * non-best local path. */ -static void evpn_cleanup_local_non_best_route(struct bgp *bgp, - struct bgpevpn *vpn, - struct bgp_dest *dest, - struct bgp_path_info *local_pi) +static struct bgp_dest * +evpn_cleanup_local_non_best_route(struct bgp *bgp, struct bgpevpn *vpn, + struct bgp_dest *dest, + struct bgp_path_info *local_pi) { /* local path was not picked as the winner; kick it out */ if (bgp_debug_zebra(NULL)) @@ -2046,10 +2046,11 @@ static void evpn_cleanup_local_non_best_route(struct bgp *bgp, dest); evpn_delete_old_local_route(bgp, vpn, dest, local_pi, NULL); - bgp_path_info_reap(dest, local_pi); /* tell zebra to re-add the best remote path */ evpn_zebra_reinstall_best_route(bgp, vpn, dest); + + return bgp_path_info_reap(dest, local_pi); } static inline bool bgp_evpn_route_add_l3_ecomm_ok(struct bgpevpn *vpn, @@ -2186,7 +2187,8 @@ static int update_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, } else { if (!CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) { route_change = 0; - evpn_cleanup_local_non_best_route(bgp, vpn, dest, pi); + dest = evpn_cleanup_local_non_best_route(bgp, vpn, dest, + pi); } else { bool new_is_sync; @@ -2203,7 +2205,8 @@ static int update_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, } bgp_path_info_unlock(pi); - bgp_dest_unlock_node(dest); + if (dest) + bgp_dest_unlock_node(dest); /* If this is a new route or some attribute has changed, export the * route to the global table. The route will be advertised to peers @@ -2327,9 +2330,13 @@ static int delete_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, */ delete_evpn_route_entry(bgp, afi, safi, dest, &pi); if (pi) { - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + assert(dest); evpn_route_select_install(bgp, vpn, dest); } + + /* dest should still exist due to locking make coverity happy */ + assert(dest); bgp_dest_unlock_node(dest); return 0; @@ -2571,7 +2578,8 @@ static void delete_global_type2_routes(struct bgp *bgp, struct bgpevpn *vpn) } } -static void delete_vni_type2_route(struct bgp *bgp, struct bgp_dest *dest) +static struct bgp_dest *delete_vni_type2_route(struct bgp *bgp, + struct bgp_dest *dest) { struct bgp_path_info *pi; afi_t afi = AFI_L2VPN; @@ -2581,13 +2589,15 @@ static void delete_vni_type2_route(struct bgp *bgp, struct bgp_dest *dest) (const struct prefix_evpn *)bgp_dest_get_prefix(dest); if (evp->prefix.route_type != BGP_EVPN_MAC_IP_ROUTE) - return; + return dest; delete_evpn_route_entry(bgp, afi, safi, dest, &pi); /* Route entry in local table gets deleted immediately. */ if (pi) - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + + return dest; } static void delete_vni_type2_routes(struct bgp *bgp, struct bgpevpn *vpn) @@ -2598,12 +2608,16 @@ static void delete_vni_type2_routes(struct bgp *bgp, struct bgpevpn *vpn) * routes. */ for (dest = bgp_table_top(vpn->mac_table); dest; - dest = bgp_route_next(dest)) - delete_vni_type2_route(bgp, dest); + dest = bgp_route_next(dest)) { + dest = delete_vni_type2_route(bgp, dest); + assert(dest); + } for (dest = bgp_table_top(vpn->ip_table); dest; - dest = bgp_route_next(dest)) - delete_vni_type2_route(bgp, dest); + dest = bgp_route_next(dest)) { + dest = delete_vni_type2_route(bgp, dest); + assert(dest); + } } /* @@ -2635,7 +2649,9 @@ static void delete_all_vni_routes(struct bgp *bgp, struct bgpevpn *vpn) (pi != NULL) && (nextpi = pi->next, 1); pi = nextpi) { bgp_evpn_remote_ip_hash_del(vpn, pi); bgp_path_info_delete(dest, pi); - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + + assert(dest); } } @@ -2644,7 +2660,9 @@ static void delete_all_vni_routes(struct bgp *bgp, struct bgpevpn *vpn) for (pi = bgp_dest_get_bgp_path_info(dest); (pi != NULL) && (nextpi = pi->next, 1); pi = nextpi) { bgp_path_info_delete(dest, pi); - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + + assert(dest); } } } @@ -3011,14 +3029,14 @@ static int install_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, /* Process for route leaking. */ vpn_leak_from_vrf_update(bgp_get_default(), bgp_vrf, pi); - bgp_dest_unlock_node(dest); - if (bgp_debug_zebra(NULL)) zlog_debug("... %s pi dest %p (l %d) pi %p (l %d, f 0x%x)", new_pi ? "new" : "update", dest, bgp_dest_get_lock_count(dest), pi, pi->lock, pi->flags); + bgp_dest_unlock_node(dest); + return ret; } diff --git a/bgpd/bgp_evpn_mh.c b/bgpd/bgp_evpn_mh.c index 91db9a061..ffb1b17ec 100644 --- a/bgpd/bgp_evpn_mh.c +++ b/bgpd/bgp_evpn_mh.c @@ -323,7 +323,9 @@ static void bgp_evpn_es_route_del_all(struct bgp *bgp, struct bgp_evpn_es *es) for (pi = bgp_dest_get_bgp_path_info(dest); (pi != NULL) && (nextpi = pi->next, 1); pi = nextpi) { bgp_path_info_delete(dest, pi); - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + + assert(dest); } } } @@ -513,8 +515,11 @@ static int bgp_evpn_mh_route_delete(struct bgp *bgp, struct bgp_evpn_es *es, */ delete_evpn_route_entry(bgp, afi, safi, dest, &pi); if (pi) - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + + assert(dest); bgp_dest_unlock_node(dest); + return 0; } diff --git a/bgpd/bgp_label.c b/bgpd/bgp_label.c index 30090e059..b8ce1ae46 100644 --- a/bgpd/bgp_label.c +++ b/bgpd/bgp_label.c @@ -195,7 +195,8 @@ int bgp_reg_for_label_callback(mpls_label_t new_label, void *labelid, return -1; } - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + assert(dest); if (BGP_DEBUG(labelpool, LABELPOOL)) zlog_debug("%s: FEC %pRN label=%u, allocated=%d", __func__, diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index 3ecb72b4e..9b5d56863 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -1238,13 +1238,14 @@ leak_update(struct bgp *to_bgp, struct bgp_dest *bn, bgp_aggregate_increment(to_bgp, p, new, afi, safi); bgp_path_info_add(bn, new); - bgp_dest_unlock_node(bn); bgp_process(to_bgp, bn, afi, safi); if (debug) zlog_debug("%s: ->%s: %pBD: Added new route", __func__, to_bgp->name_pretty, bn); + bgp_dest_unlock_node(bn); + return new; } diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index d8a52c62b..7f1a6e73e 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -484,7 +484,9 @@ void bgp_connected_delete(struct bgp *bgp, struct connected *ifc) XFREE(MTYPE_BGP_CONN, bc); bgp_dest_set_bgp_connected_ref_info(dest, NULL); } - bgp_dest_unlock_node(dest); + + dest = bgp_dest_unlock_node(dest); + assert(dest); bgp_dest_unlock_node(dest); } diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index bfde4ad6a..227a68ee5 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -138,7 +138,9 @@ struct bgp_dest *bgp_afi_node_get(struct bgp_table *table, afi_t afi, bgp_dest_set_bgp_table_info( pdest, bgp_table_init(table->bgp, afi, safi)); else - bgp_dest_unlock_node(pdest); + pdest = bgp_dest_unlock_node(pdest); + + assert(pdest); table = bgp_dest_get_bgp_table_info(pdest); } @@ -438,7 +440,8 @@ void bgp_path_info_add_with_caller(const char *name, struct bgp_dest *dest, /* Do the actual removal of info from RIB, for use by bgp_process completion callback *only* */ -void bgp_path_info_reap(struct bgp_dest *dest, struct bgp_path_info *pi) +struct bgp_dest *bgp_path_info_reap(struct bgp_dest *dest, + struct bgp_path_info *pi) { if (pi->next) pi->next->prev = pi->prev; @@ -450,7 +453,8 @@ void bgp_path_info_reap(struct bgp_dest *dest, struct bgp_path_info *pi) bgp_path_info_mpath_dequeue(pi); bgp_path_info_unlock(pi); hook_call(bgp_snmp_update_stats, dest, pi, false); - bgp_dest_unlock_node(dest); + + return bgp_dest_unlock_node(dest); } void bgp_path_info_delete(struct bgp_dest *dest, struct bgp_path_info *pi) @@ -2812,9 +2816,11 @@ void bgp_best_selection(struct bgp *bgp, struct bgp_dest *dest, /* reap REMOVED routes, if needs be * selected route must stay for a while longer though */ - if (CHECK_FLAG(pi->flags, BGP_PATH_REMOVED) - && (pi != old_select)) - bgp_path_info_reap(dest, pi); + if (CHECK_FLAG(pi->flags, BGP_PATH_REMOVED) && + (pi != old_select)) { + dest = bgp_path_info_reap(dest, pi); + assert(dest); + } if (debug) zlog_debug( @@ -3500,11 +3506,12 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest, /* Clear any route change flags. */ bgp_zebra_clear_route_change_flags(dest); + UNSET_FLAG(dest->flags, BGP_NODE_PROCESS_SCHEDULED); + /* Reap old select bgp_path_info, if it has been removed */ if (old_select && CHECK_FLAG(old_select->flags, BGP_PATH_REMOVED)) bgp_path_info_reap(dest, old_select); - UNSET_FLAG(dest->flags, BGP_NODE_PROCESS_SCHEDULED); return; } @@ -5078,7 +5085,8 @@ void bgp_withdraw(struct peer *peer, const struct prefix *p, */ if (CHECK_FLAG(peer->af_flags[afi][safi], PEER_FLAG_SOFT_RECONFIG) && peer != bgp->peer_self) - if (!bgp_adj_in_unset(dest, peer, addpath_id)) { + if (!bgp_adj_in_unset(&dest, peer, addpath_id)) { + assert(dest); peer->stat_pfx_dup_withdraw++; if (bgp_debug_update(peer, p, NULL, 1)) { @@ -5095,6 +5103,7 @@ void bgp_withdraw(struct peer *peer, const struct prefix *p, } /* Lookup withdrawn route. */ + assert(dest); for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) if (pi->peer == peer && pi->type == type && pi->sub_type == sub_type @@ -5668,9 +5677,11 @@ static void bgp_clear_route_table(struct peer *peer, afi_t afi, safi_t safi, ain_next = ain->next; if (ain->peer == peer) - bgp_adj_in_remove(dest, ain); + bgp_adj_in_remove(&dest, ain); ain = ain_next; + + assert(dest); } for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = next) { @@ -5678,9 +5689,10 @@ static void bgp_clear_route_table(struct peer *peer, afi_t afi, safi_t safi, if (pi->peer != peer) continue; - if (force) - bgp_path_info_reap(dest, pi); - else { + if (force) { + dest = bgp_path_info_reap(dest, pi); + assert(dest); + } else { struct bgp_clear_node_queue *cnq; /* both unlocked in bgp_clear_node_queue_del */ @@ -5775,9 +5787,11 @@ void bgp_clear_adj_in(struct peer *peer, afi_t afi, safi_t safi) ain_next = ain->next; if (ain->peer == peer) - bgp_adj_in_remove(dest, ain); + bgp_adj_in_remove(&dest, ain); ain = ain_next; + + assert(dest); } } } @@ -5995,7 +6009,8 @@ static void bgp_cleanup_table(struct bgp *bgp, struct bgp_table *table, bgp_zebra_withdraw(p, pi, bgp, safi); } - bgp_path_info_reap(dest, pi); + dest = bgp_path_info_reap(dest, pi); + assert(dest); } } @@ -6024,7 +6039,9 @@ void bgp_cleanup_routes(struct bgp *bgp) bgp_cleanup_table(bgp, table, safi); bgp_table_finish(&table); bgp_dest_set_bgp_table_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + + assert(dest); } } safi = SAFI_ENCAP; @@ -6035,7 +6052,9 @@ void bgp_cleanup_routes(struct bgp *bgp) bgp_cleanup_table(bgp, table, safi); bgp_table_finish(&table); bgp_dest_set_bgp_table_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + + assert(dest); } } } @@ -6047,7 +6066,9 @@ void bgp_cleanup_routes(struct bgp *bgp) bgp_cleanup_table(bgp, table, SAFI_EVPN); bgp_table_finish(&table); bgp_dest_set_bgp_table_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + + assert(dest); } } } @@ -6696,7 +6717,8 @@ int bgp_static_set(struct vty *vty, bool negate, const char *ip_str, } bgp_dest_set_bgp_static_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + assert(dest); bgp_dest_unlock_node(dest); } else { dest = bgp_node_get(table, &p); @@ -6873,7 +6895,8 @@ void bgp_static_delete(struct bgp *bgp) bgp_static_free(bgp_static); bgp_dest_set_bgp_static_info(rm, NULL); - bgp_dest_unlock_node(rm); + rm = bgp_dest_unlock_node(rm); + assert(rm); } } else { bgp_static = bgp_dest_get_bgp_static_info(dest); @@ -6882,7 +6905,8 @@ void bgp_static_delete(struct bgp *bgp) afi, safi, NULL); bgp_static_free(bgp_static); bgp_dest_set_bgp_static_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + assert(dest); } } } @@ -8213,7 +8237,8 @@ static int bgp_aggregate_unset(struct vty *vty, const char *prefix_str, bgp_dest_set_bgp_aggregate_info(dest, NULL); bgp_free_aggregate_info(aggregate); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + assert(dest); bgp_dest_unlock_node(dest); return CMD_SUCCESS; @@ -8735,8 +8760,10 @@ void bgp_redistribute_withdraw(struct bgp *bgp, afi_t afi, int type, if (!CHECK_FLAG(bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS)) bgp_process(bgp, dest, afi, SAFI_UNICAST); - else - bgp_path_info_reap(dest, pi); + else { + dest = bgp_path_info_reap(dest, pi); + assert(dest); + } } } } @@ -12128,7 +12155,9 @@ static int bgp_show_route_in_table(struct vty *vty, struct bgp *bgp, rm_p); if (type5_pfxlen == match.prefixlen) { is_exact_pfxlen_match = true; - bgp_dest_unlock_node(rm); + rm = bgp_dest_unlock_node(rm); + + assert(rm); break; } } @@ -15031,7 +15060,8 @@ static int bgp_distance_unset(struct vty *vty, const char *distance_str, bgp_distance_free(bdistance); bgp_dest_set_bgp_path_info(dest, NULL); - bgp_dest_unlock_node(dest); + dest = bgp_dest_unlock_node(dest); + assert(dest); bgp_dest_unlock_node(dest); return CMD_SUCCESS; diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 109711314..7470954bf 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -727,7 +727,8 @@ extern struct bgp_path_info * bgp_get_imported_bpi_ultimate(struct bgp_path_info *info); extern void bgp_path_info_add(struct bgp_dest *dest, struct bgp_path_info *pi); extern void bgp_path_info_extra_free(struct bgp_path_info_extra **extra); -extern void bgp_path_info_reap(struct bgp_dest *dest, struct bgp_path_info *pi); +extern struct bgp_dest *bgp_path_info_reap(struct bgp_dest *dest, + struct bgp_path_info *pi); extern void bgp_path_info_delete(struct bgp_dest *dest, struct bgp_path_info *pi); extern struct bgp_path_info_extra * diff --git a/bgpd/bgp_table.c b/bgpd/bgp_table.c index 149a5b914..8465ada99 100644 --- a/bgpd/bgp_table.c +++ b/bgpd/bgp_table.c @@ -75,7 +75,7 @@ const char *bgp_dest_get_prefix_str(struct bgp_dest *dest) /* * bgp_dest_unlock_node */ -inline void bgp_dest_unlock_node(struct bgp_dest *dest) +inline struct bgp_dest *bgp_dest_unlock_node(struct bgp_dest *dest) { frrtrace(1, frr_bgp, bgp_dest_unlock, dest); bgp_delete_listnode(dest); @@ -89,9 +89,12 @@ inline void bgp_dest_unlock_node(struct bgp_dest *dest) rt->safi); } XFREE(MTYPE_BGP_NODE, dest); + dest = NULL; rn->info = NULL; } route_unlock_node(rn); + + return dest; } /* diff --git a/bgpd/bgp_table.h b/bgpd/bgp_table.h index 1f28624c1..5b4c3be21 100644 --- a/bgpd/bgp_table.h +++ b/bgpd/bgp_table.h @@ -112,7 +112,7 @@ extern struct bgp_table *bgp_table_init(struct bgp *bgp, afi_t, safi_t); extern void bgp_table_lock(struct bgp_table *); extern void bgp_table_unlock(struct bgp_table *); extern void bgp_table_finish(struct bgp_table **); -extern void bgp_dest_unlock_node(struct bgp_dest *dest); +extern struct bgp_dest *bgp_dest_unlock_node(struct bgp_dest *dest); extern struct bgp_dest *bgp_dest_lock_node(struct bgp_dest *dest); extern const char *bgp_dest_get_prefix_str(struct bgp_dest *dest); |