bgpd: Try fix extcommunity resource allocation probs, particularly with 'set extcom..'
* Extended communities has some kind of resource allocation problem which
causes a double-free if the 'set extcommunity ...' command is used.
Try fix by properly interning extcommunities.
Also, more generally, make unintern functions take a double pointer
so they can NULL out callers references - a usefully defensive programming
pattern for functions which make refs invalid.
Sadly, this patch doesn't fix the problem entirely - crashes still
occur on session clear.
* bgp_ecommunity.h: (ecommunity_{free,unintern}) take double pointer
args.
* bgp_community.h: (community_unintern) ditto
* bgp_attr.h: (bgp_attr_intern) ditto
* bgp_aspath.h: (bgp_aspath.h) ditto
* (general) update all callers of above
* bgp_routemap.c: (route_set_ecommunity_{rt,soo}) intern the new extcom added
to the attr, and unintern any old one.
(route_set_ecommunity_{rt,soo}_compile) intern the extcom to be used
for the route-map set.
(route_set_ecommunity_*_free) unintern to match, instead of free
(route_set_ecommunity_soo) Do as _rt does and don't just leak
any pre-existing community, add to it (is additive right though?)
diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index 7a0ab56..0c5355b 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -500,6 +500,7 @@
attre->ecommunity = ecommunity_intern (attre->ecommunity);
else
attre->ecommunity->refcnt++;
+
}
if (attre->cluster)
{
@@ -516,10 +517,10 @@
attre->transit->refcnt++;
}
}
-
+
find = (struct attr *) hash_get (attrhash, attr, bgp_attr_hash_alloc);
find->refcnt++;
-
+
return find;
}
@@ -561,7 +562,7 @@
new = bgp_attr_intern (&attr);
bgp_attr_extra_free (&attr);
- aspath_unintern (new->aspath);
+ aspath_unintern (&new->aspath);
return new;
}
@@ -613,13 +614,13 @@
new = bgp_attr_intern (&attr);
bgp_attr_extra_free (&attr);
- aspath_unintern (new->aspath);
+ aspath_unintern (&new->aspath);
return new;
}
/* Free bgp attribute and aspath. */
void
-bgp_attr_unintern (struct attr *attr)
+bgp_attr_unintern (struct attr **attr)
{
struct attr *ret;
struct aspath *aspath;
@@ -627,34 +628,35 @@
struct ecommunity *ecommunity = NULL;
struct cluster_list *cluster = NULL;
struct transit *transit = NULL;
-
+
/* Decrement attribute reference. */
- attr->refcnt--;
- aspath = attr->aspath;
- community = attr->community;
- if (attr->extra)
+ (*attr)->refcnt--;
+ aspath = (*attr)->aspath;
+ community = (*attr)->community;
+ if ((*attr)->extra)
{
- ecommunity = attr->extra->ecommunity;
- cluster = attr->extra->cluster;
- transit = attr->extra->transit;
+ ecommunity = (*attr)->extra->ecommunity;
+ cluster = (*attr)->extra->cluster;
+ transit = (*attr)->extra->transit;
}
-
+
/* If reference becomes zero then free attribute object. */
- if (attr->refcnt == 0)
+ if ((*attr)->refcnt == 0)
{
- ret = hash_release (attrhash, attr);
+ ret = hash_release (attrhash, *attr);
assert (ret != NULL);
- bgp_attr_extra_free (attr);
- XFREE (MTYPE_ATTR, attr);
+ bgp_attr_extra_free (*attr);
+ XFREE (MTYPE_ATTR, *attr);
+ *attr = NULL;
}
/* aspath refcount shoud be decrement. */
if (aspath)
- aspath_unintern (aspath);
+ aspath_unintern (&aspath);
if (community)
- community_unintern (community);
+ community_unintern (&community);
if (ecommunity)
- ecommunity_unintern (ecommunity);
+ ecommunity_unintern (&ecommunity);
if (cluster)
cluster_unintern (cluster);
if (transit)
@@ -671,8 +673,9 @@
if (attr->extra)
{
struct attr_extra *attre = attr->extra;
+
if (attre->ecommunity && ! attre->ecommunity->refcnt)
- ecommunity_free (attre->ecommunity);
+ ecommunity_free (&attre->ecommunity);
if (attre->cluster && ! attre->cluster->refcnt)
cluster_free (attre->cluster);
if (attre->transit && ! attre->transit->refcnt)
@@ -840,7 +843,7 @@
{
aspath = aspath_dup (attr->aspath);
aspath = aspath_add_seq (aspath, peer->change_local_as);
- aspath_unintern (attr->aspath);
+ aspath_unintern (&attr->aspath);
attr->aspath = aspath_intern (aspath);
}
@@ -1146,7 +1149,7 @@
if ( !ignore_as4_path && (attr->flag & ( ATTR_FLAG_BIT( BGP_ATTR_AS4_PATH))) )
{
newpath = aspath_reconcile_as4 (attr->aspath, as4_path);
- aspath_unintern (attr->aspath);
+ aspath_unintern (&attr->aspath);
attr->aspath = aspath_intern (newpath);
}
return 0;
@@ -1707,8 +1710,7 @@
*/
if ( as4_path )
{
- aspath_unintern( as4_path ); /* unintern - it is in the hash */
- as4_path = NULL;
+ aspath_unintern (&as4_path); /* unintern - it is in the hash */
/* The flag that we got this is still there, but that does not
* do any trouble
*/