Customer has this valgrind trace:
Direct leak of
2829120 byte(s) in 70728 object(s) allocated from:
0 in community_new ../bgpd/bgp_community.c:39
1 in community_uniq_sort ../bgpd/bgp_community.c:170
2 in route_set_community ../bgpd/bgp_routemap.c:2342
3 in route_map_apply_ext ../lib/routemap.c:2673
4 in subgroup_announce_check ../bgpd/bgp_route.c:2367
5 in subgroup_process_announce_selected ../bgpd/bgp_route.c:2914
6 in group_announce_route_walkcb ../bgpd/bgp_updgrp_adv.c:199
7 in hash_walk ../lib/hash.c:285
8 in update_group_af_walk ../bgpd/bgp_updgrp.c:2061
9 in group_announce_route ../bgpd/bgp_updgrp_adv.c:1059
10 in bgp_process_main_one ../bgpd/bgp_route.c:3221
11 in bgp_process_wq ../bgpd/bgp_route.c:3221
12 in work_queue_run ../lib/workqueue.c:282
The above leak detected by valgrind was from a screenshot so I copied it
by hand. Any mistakes in line numbers are purely from my transcription.
Additionally this is against a slightly modified 8.5.1 version of FRR.
Code inspection of 8.5.1 -vs- latest master shows the same problem
exists. Code should be able to be followed from there to here.
What is happening:
There is a route-map being applied that modifes the outgoing community
to a peer. This is saved in the attr copy created in
subgroup_process_announce_selected. This community pointer is not
interned. So the community->refcount is still 0. Normally when
a prefix is announced, the attr and the prefix are placed on a
adjency out structure where the attribute is interned. This will
cause the community to be saved in the community hash list as well.
In a non-normal operation when the decision to send is aborted after
the route-map application, the attribute is just dropped and the
pointer to the community is just dropped too, leading to situations
where the memory is leaked. The usage of bgp suppress-fib would
would be a case where the community is caused to be leaked.
Additionally the previous commit where an unsuppress-map is used
to modify the outgoing attribute but since unsuppress-map was
not considered part of outgoing policy the attribute would be dropped as
well. This pointer drop also extends to any dynamically allocated
memory saved by the attribute pointer that was not interned yet as well.
So let's modify the return case where the decision is made to
not send the prefix to the peer to always just flush the attribute
to ensure memory is not leaked.
Fixes: #15459
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
if (update_type == UPDATE_TYPE_ADVERTISE &&
subgroup_announce_check(dest, pi, subgrp, dest_p,
&attr, &advmap_attr)) {
- bgp_adj_out_set_subgroup(dest, subgrp, &attr,
- pi);
+ if (!bgp_adj_out_set_subgroup(dest, subgrp,
+ &attr, pi))
+ bgp_attr_flush(&attr);
} else {
/* If default originate is enabled for
* the peer, do not send explicit
{
const struct prefix *p;
struct peer *onlypeer;
- struct attr attr;
afi_t afi;
safi_t safi;
+ struct attr attr = { 0 }, *pattr = &attr;
struct bgp *bgp;
bool advertise;
advertise = bgp_check_advertise(bgp, dest);
if (selected) {
- if (subgroup_announce_check(dest, selected, subgrp, p, &attr,
+ if (subgroup_announce_check(dest, selected, subgrp, p, pattr,
NULL)) {
/* Route is selected, if the route is already installed
* in FIB, then it is advertised
*/
if (advertise) {
if (!bgp_check_withdrawal(bgp, dest)) {
- struct attr *adv_attr =
- bgp_attr_intern(&attr);
-
- bgp_adj_out_set_subgroup(dest, subgrp,
- adv_attr,
- selected);
- } else
+ if (!bgp_adj_out_set_subgroup(dest,
+ subgrp,
+ pattr,
+ selected))
+ bgp_attr_flush(pattr);
+ } else {
bgp_adj_out_unset_subgroup(
dest, subgrp, 1, addpath_tx_id);
- }
- } else
+ bgp_attr_flush(pattr);
+ }
+ } else
+ bgp_attr_flush(pattr);
+ } else {
bgp_adj_out_unset_subgroup(dest, subgrp, 1,
addpath_tx_id);
+ bgp_attr_flush(pattr);
+ }
}
/* If selected is NULL we must withdraw the path using addpath_tx_id */
extern void bgp_adj_out_remove_subgroup(struct bgp_dest *dest,
struct bgp_adj_out *adj,
struct update_subgroup *subgrp);
-extern void bgp_adj_out_set_subgroup(struct bgp_dest *dest,
+extern bool bgp_adj_out_set_subgroup(struct bgp_dest *dest,
struct update_subgroup *subgrp,
struct attr *attr,
struct bgp_path_info *path);
return next;
}
-void bgp_adj_out_set_subgroup(struct bgp_dest *dest,
+bool bgp_adj_out_set_subgroup(struct bgp_dest *dest,
struct update_subgroup *subgrp, struct attr *attr,
struct bgp_path_info *path)
{
bgp = SUBGRP_INST(subgrp);
if (DISABLE_BGP_ANNOUNCE)
- return;
+ return false;
/* Look for adjacency information. */
adj = adj_lookup(
bgp_addpath_id_for_peer(peer, afi, safi,
&path->tx_addpath));
if (!adj)
- return;
+ return false;
subgrp->pscount++;
}
* will never be able to coalesce the 3rd peer down
*/
subgrp->version = MAX(subgrp->version, dest->version);
- return;
+ return false;
}
if (adj->adv)
bgp_adv_fifo_add_tail(&subgrp->sync->update, adv);
subgrp->version = MAX(subgrp->version, dest->version);
+
+ return true;
}
/* The only time 'withdraw' will be false is if we are sending
void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
{
struct bgp *bgp;
- struct attr attr;
+ struct attr attr = { 0 };
struct attr *new_attr = &attr;
struct aspath *aspath;
struct prefix p;
if (dest) {
for (pi = bgp_dest_get_bgp_path_info(dest); pi;
pi = pi->next) {
- if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED))
- if (subgroup_announce_check(
- dest, pi, subgrp,
- bgp_dest_get_prefix(dest),
- &attr, NULL)) {
- struct attr *default_attr =
- bgp_attr_intern(&attr);
-
- bgp_adj_out_set_subgroup(
- dest, subgrp,
- default_attr, pi);
- }
+ if (!CHECK_FLAG(pi->flags, BGP_PATH_SELECTED))
+ continue;
+
+ if (subgroup_announce_check(dest, pi, subgrp,
+ bgp_dest_get_prefix(
+ dest),
+ &attr, NULL)) {
+ if (!bgp_adj_out_set_subgroup(dest,
+ subgrp,
+ &attr, pi))
+ bgp_attr_flush(&attr);
+ } else
+ bgp_attr_flush(&attr);
}
bgp_dest_unlock_node(dest);
}
projects / matthieu / frr.git / commitdiff