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?)

1 files changed, 19 insertions, 8 deletions

diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c
index 81ff48db..451458b5 100644
--- a/bgpd/bgp_routemap.c
+++ b/bgpd/bgp_routemap.c
@@ -1475,10 +1475,10 @@ route_set_ecommunity_rt (void *rule, struct prefix *prefix,
       else
 	new_ecom = ecommunity_dup (ecom);
 
-      bgp_info->attr->extra->ecommunity = new_ecom;
+      bgp_info->attr->extra->ecommunity = ecommunity_intern (new_ecom);
 
       if (old_ecom)
-	ecommunity_free (old_ecom);
+	ecommunity_unintern (&old_ecom);
 
       bgp_info->attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_EXT_COMMUNITIES);
     }
@@ -1494,7 +1494,7 @@ route_set_ecommunity_rt_compile (const char *arg)
   ecom = ecommunity_str2com (arg, ECOMMUNITY_ROUTE_TARGET, 0);
   if (! ecom)
     return NULL;
-  return ecom;
+  return ecommunity_intern (ecom);
 }
 
 /* Free function for set community. */
@@ -1502,7 +1502,7 @@ static void
 route_set_ecommunity_rt_free (void *rule)
 {
   struct ecommunity *ecom = rule;
-  ecommunity_free (ecom);
+  ecommunity_unintern (&ecom);
 }
 
 /* Set community rule structure. */
@@ -1521,7 +1521,7 @@ static route_map_result_t
 route_set_ecommunity_soo (void *rule, struct prefix *prefix, 
 			 route_map_object_t type, void *object)
 {
-  struct ecommunity *ecom;
+  struct ecommunity *ecom, *old_ecom, *new_ecom;
   struct bgp_info *bgp_info;
 
   if (type == RMAP_BGP)
@@ -1532,8 +1532,19 @@ route_set_ecommunity_soo (void *rule, struct prefix *prefix,
       if (! ecom)
 	return RMAP_OKAY;
     
+      old_ecom = (bgp_attr_extra_get (bgp_info->attr))->ecommunity;
+      
+      if (old_ecom)
+	new_ecom = ecommunity_merge (ecommunity_dup (old_ecom), ecom);
+      else
+	new_ecom = ecommunity_dup (ecom);
+
+      bgp_info->attr->extra->ecommunity = ecommunity_intern (new_ecom);
+
+      if (old_ecom)
+	ecommunity_unintern (&old_ecom);
+
       bgp_info->attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_EXT_COMMUNITIES);
-      (bgp_attr_extra_get (bgp_info->attr))->ecommunity = ecommunity_dup (ecom);
     }
   return RMAP_OKAY;
 }
@@ -1548,7 +1559,7 @@ route_set_ecommunity_soo_compile (const char *arg)
   if (! ecom)
     return NULL;
   
-  return ecom;
+  return ecommunity_intern (ecom);
 }
 
 /* Free function for set community. */
@@ -1556,7 +1567,7 @@ static void
 route_set_ecommunity_soo_free (void *rule)
 {
   struct ecommunity *ecom = rule;
-  ecommunity_free (ecom);
+  ecommunity_unintern (&ecom);
 }
 
 /* Set community rule structure. */