From bede77445018ed62042f677adc82654ab32ba4c9 Mon Sep 17 00:00:00 2001 From: Lou Berger Date: Sat, 17 Dec 2016 17:58:33 -0500 Subject: [PATCH] bgp: Use intern/unintern for encap to fix valgrind identified memory leak Signed-off-by: Lou Berger --- bgpd/bgp_attr.c | 223 ++++++++++++++++++++++++++++-------- bgpd/bgp_attr.h | 2 + bgpd/bgp_route.c | 1 + bgpd/rfapi/rfapi_import.c | 4 + bgpd/rfapi/vnc_export_bgp.c | 1 - 5 files changed, 183 insertions(+), 48 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 827e4610f4..2115fb5efc 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -220,6 +220,11 @@ cluster_finish (void) cluster_hash = NULL; } +static struct hash *encap_hash = NULL; +#if ENABLE_BGP_VNC +static struct hash *vnc_hash = NULL; +#endif + struct bgp_attr_encap_subtlv * encap_tlv_dup(struct bgp_attr_encap_subtlv *orig) { @@ -287,14 +292,10 @@ encap_same(struct bgp_attr_encap_subtlv *h1, struct bgp_attr_encap_subtlv *h2) struct bgp_attr_encap_subtlv *p; struct bgp_attr_encap_subtlv *q; - if (!h1 && !h2) - return 1; - if (h1 && !h2) - return 0; - if (!h1 && h2) - return 0; if (h1 == h2) return 1; + if (h1 == NULL || h2 == NULL) + return 0; for (p = h1; p; p = p->next) { for (q = h2; q; q = q->next) { @@ -325,6 +326,96 @@ encap_same(struct bgp_attr_encap_subtlv *h1, struct bgp_attr_encap_subtlv *h2) return 1; } +static void * +encap_hash_alloc (void *p) +{ + /* Encap structure is already allocated. */ + return p; +} + +typedef enum +{ + ENCAP_SUBTLV_TYPE, +#if ENABLE_BGP_VNC + VNC_SUBTLV_TYPE +#endif +} encap_subtlv_type; + +static struct bgp_attr_encap_subtlv * +encap_intern (struct bgp_attr_encap_subtlv *encap, encap_subtlv_type type) +{ + struct bgp_attr_encap_subtlv *find; + struct hash *hash = encap_hash; +#if ENABLE_BGP_VNC + if (type == VNC_SUBTLV_TYPE) + hash = vnc_hash; +#endif + + find = hash_get (hash, encap, encap_hash_alloc); + if (find != encap) + encap_free (encap); + find->refcnt++; + + return find; +} + +static void +encap_unintern (struct bgp_attr_encap_subtlv **encapp, encap_subtlv_type type) +{ + struct bgp_attr_encap_subtlv *encap = *encapp; + if (encap->refcnt) + encap->refcnt--; + + if (encap->refcnt == 0) + { + struct hash *hash = encap_hash; +#if ENABLE_BGP_VNC + if (type == VNC_SUBTLV_TYPE) + hash = vnc_hash; +#endif + hash_release (hash, encap); + encap_free (encap); + *encapp = NULL; + } +} + +static unsigned int +encap_hash_key_make (void *p) +{ + const struct bgp_attr_encap_subtlv * encap = p; + + return jhash(encap->value, encap->length, 0); +} + +static int +encap_hash_cmp (const void *p1, const void *p2) +{ + return encap_same((struct bgp_attr_encap_subtlv *)p1, + (struct bgp_attr_encap_subtlv *)p2); +} + +static void +encap_init (void) +{ + encap_hash = hash_create (encap_hash_key_make, encap_hash_cmp); +#if ENABLE_BGP_VNC + vnc_hash = hash_create (encap_hash_key_make, encap_hash_cmp); +#endif +} + +static void +encap_finish (void) +{ + hash_clean (encap_hash, (void (*)(void *))encap_free); + hash_free (encap_hash); + encap_hash = NULL; +#if ENABLE_BGP_VNC + hash_clean (vnc_hash, (void (*)(void *))encap_free); + hash_free (vnc_hash); + vnc_hash = NULL; +#endif +} + /* Unknown transit attribute. */ static struct hash *transit_hash; @@ -433,16 +524,6 @@ bgp_attr_extra_free (struct attr *attr) { if (attr->extra) { - if (attr->extra->encap_subtlvs) { - encap_free(attr->extra->encap_subtlvs); - attr->extra->encap_subtlvs = NULL; - } -#if ENABLE_BGP_VNC - if (attr->extra->vnc_subtlvs) { - encap_free(attr->extra->vnc_subtlvs); - attr->extra->vnc_subtlvs = NULL; - } -#endif XFREE (MTYPE_ATTR_EXTRA, attr->extra); attr->extra = NULL; } @@ -480,28 +561,12 @@ bgp_attr_dup (struct attr *new, struct attr *orig) memset(new->extra, 0, sizeof(struct attr_extra)); if (orig->extra) { *new->extra = *orig->extra; - if (orig->extra->encap_subtlvs) { - new->extra->encap_subtlvs = encap_tlv_dup(orig->extra->encap_subtlvs); - } -#if ENABLE_BGP_VNC - if (orig->extra->vnc_subtlvs) { - new->extra->vnc_subtlvs = encap_tlv_dup(orig->extra->vnc_subtlvs); - } -#endif } } else if (orig->extra) { new->extra = bgp_attr_extra_new(); *new->extra = *orig->extra; - if (orig->extra->encap_subtlvs) { - new->extra->encap_subtlvs = encap_tlv_dup(orig->extra->encap_subtlvs); - } -#if ENABLE_BGP_VNC - if (orig->extra->vnc_subtlvs) { - new->extra->vnc_subtlvs = encap_tlv_dup(orig->extra->vnc_subtlvs); - } -#endif } } @@ -522,6 +587,12 @@ bgp_attr_deep_dup (struct attr *new, struct attr *orig) new->extra->cluster = cluster_dup(orig->extra->cluster); if (orig->extra->transit) new->extra->transit = transit_dup(orig->extra->transit); + if (orig->extra->encap_subtlvs) + new->extra->encap_subtlvs = encap_tlv_dup(orig->extra->encap_subtlvs); +#if ENABLE_BGP_VNC + if (orig->extra->vnc_subtlvs) + new->extra->vnc_subtlvs = encap_tlv_dup(orig->extra->vnc_subtlvs); +#endif } } @@ -542,6 +613,12 @@ bgp_attr_deep_free (struct attr *attr) cluster_free(attr->extra->cluster); if (attr->extra->transit) transit_free(attr->extra->transit); + if (attr->extra->encap_subtlvs) + encap_free(attr->extra->encap_subtlvs); +#if ENABLE_BGP_VNC + if (attr->extra->vnc_subtlvs) + encap_free(attr->extra->vnc_subtlvs); +#endif } } @@ -598,7 +675,12 @@ attrhash_key_make (void *p) MIX(cluster_hash_key_make (extra->cluster)); if (extra->transit) MIX(transit_hash_key_make (extra->transit)); - + if (extra->encap_subtlvs) + MIX(encap_hash_key_make (extra->encap_subtlvs)); +#if ENABLE_BGP_VNC + if (extra->vnc_subtlvs) + MIX(encap_hash_key_make (extra->vnc_subtlvs)); +#endif #ifdef HAVE_IPV6 MIX(extra->mp_nexthop_len); key = jhash(extra->mp_nexthop_global.s6_addr, IPV6_MAX_BYTELEN, key); @@ -711,13 +793,12 @@ bgp_attr_hash_alloc (void *p) { attr->extra = bgp_attr_extra_new (); *attr->extra = *val->extra; - - if (attr->extra->encap_subtlvs) { - attr->extra->encap_subtlvs = encap_tlv_dup(attr->extra->encap_subtlvs); + if (val->extra->encap_subtlvs) { + val->extra->encap_subtlvs = NULL; } #if ENABLE_BGP_VNC - if (attr->extra->vnc_subtlvs) { - attr->extra->vnc_subtlvs = encap_tlv_dup(attr->extra->vnc_subtlvs); + if (val->extra->vnc_subtlvs) { + val->extra->vnc_subtlvs = NULL; } #endif } @@ -772,11 +853,27 @@ bgp_attr_intern (struct attr *attr) else attre->transit->refcnt++; } + if (attre->encap_subtlvs) + { + if (! attre->encap_subtlvs->refcnt) + attre->encap_subtlvs = encap_intern (attre->encap_subtlvs, ENCAP_SUBTLV_TYPE); + else + attre->encap_subtlvs->refcnt++; + } +#if ENABLE_BGP_VNC + if (attre->vnc_subtlvs) + { + if (! attre->vnc_subtlvs->refcnt) + attre->vnc_subtlvs = encap_intern (attre->vnc_subtlvs, VNC_SUBTLV_TYPE); + else + attre->vnc_subtlvs->refcnt++; + } +#endif } find = (struct attr *) hash_get (attrhash, attr, bgp_attr_hash_alloc); find->refcnt++; - + return find; } @@ -810,6 +907,14 @@ bgp_attr_refcount (struct attr *attr) if (attre->transit) attre->transit->refcnt++; + + if (attre->encap_subtlvs) + attre->encap_subtlvs->refcnt++; + +#if ENABLE_BGP_VNC + if (attre->vnc_subtlvs) + attre->vnc_subtlvs->refcnt++; +#endif } attr->refcnt++; return attr; @@ -935,6 +1040,14 @@ bgp_attr_unintern_sub (struct attr *attr) if (attr->extra->transit) transit_unintern (attr->extra->transit); + + if (attr->extra->encap_subtlvs) + encap_unintern (&attr->extra->encap_subtlvs, ENCAP_SUBTLV_TYPE); + +#if ENABLE_BGP_VNC + if (attr->extra->vnc_subtlvs) + encap_unintern (&attr->extra->vnc_subtlvs, VNC_SUBTLV_TYPE); +#endif } } @@ -1000,11 +1113,17 @@ bgp_attr_flush (struct attr *attr) transit_free (attre->transit); attre->transit = NULL; } - encap_free(attre->encap_subtlvs); - attre->encap_subtlvs = NULL; + if (attre->encap_subtlvs && ! attre->encap_subtlvs->refcnt) + { + encap_free(attre->encap_subtlvs); + attre->encap_subtlvs = NULL; + } #if ENABLE_BGP_VNC - encap_free(attre->vnc_subtlvs); - attre->vnc_subtlvs = NULL; + if (attre->vnc_subtlvs && ! attre->vnc_subtlvs->refcnt) + { + encap_free(attre->vnc_subtlvs); + attre->vnc_subtlvs = NULL; + } #endif } } @@ -2492,10 +2611,18 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, if (ret != BGP_ATTR_PARSE_PROCEED) return ret; } - - /* Finally intern unknown attribute. */ - if (attr->extra && attr->extra->transit) - attr->extra->transit = transit_intern (attr->extra->transit); + if (attr->extra) + { + /* Finally intern unknown attribute. */ + if (attr->extra->transit) + attr->extra->transit = transit_intern (attr->extra->transit); + if (attr->extra->encap_subtlvs) + attr->extra->encap_subtlvs = encap_intern (attr->extra->encap_subtlvs, ENCAP_SUBTLV_TYPE); +#if ENABLE_BGP_VNC + if (attr->extra->vnc_subtlvs) + attr->extra->vnc_subtlvs = encap_intern (attr->extra->vnc_subtlvs, VNC_SUBTLV_TYPE); +#endif + } return BGP_ATTR_PARSE_PROCEED; } @@ -3183,6 +3310,7 @@ bgp_attr_init (void) ecommunity_init (); cluster_init (); transit_init (); + encap_init (); } void @@ -3194,6 +3322,7 @@ bgp_attr_finish (void) ecommunity_finish (); cluster_finish (); transit_finish (); + encap_finish (); } /* Make attribute packet. */ diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index d4f45ba60a..6e639078d6 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -58,6 +58,8 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA struct bgp_attr_encap_subtlv { struct bgp_attr_encap_subtlv *next; /* for chaining */ + /* Reference count of this attribute. */ + unsigned long refcnt; uint16_t type; uint16_t length; uint8_t value[1]; /* will be extended */ diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index e4e321ad05..c717a930b1 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -1800,6 +1800,7 @@ subgroup_process_announce_selected (struct update_subgroup *subgrp, PEER_STATUS_ORF_WAIT_REFRESH)) return 0; + memset(&extra, 0, sizeof(struct attr_extra)); /* It's initialized in bgp_announce_check() */ attr.extra = &extra; diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 4c3274c2cf..25c05e65f8 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -2151,6 +2151,7 @@ rfapiBgpInfoAttachSorted ( info_new->next = next; if (next) next->prev = info_new; + bgp_attr_intern (info_new->attr); } static void @@ -2159,6 +2160,7 @@ rfapiBgpInfoDetach (struct route_node *rn, struct bgp_info *bi) /* * Remove the route (doubly-linked) */ + // bgp_attr_unintern (&bi->attr); if (bi->next) bi->next->prev = bi->prev; if (bi->prev) @@ -2509,6 +2511,7 @@ rfapiMonitorEncapAdd ( __func__, import_table, vpn_bi, afi, rn, m); RFAPI_CHECK_REFCOUNT (rn, SAFI_ENCAP, 0); + bgp_attr_intern (vpn_bi->attr); } static void @@ -3011,6 +3014,7 @@ rfapiBiStartWithdrawTimer ( wcb->node = rn; wcb->info = bi; wcb->import_table = import_table; + bgp_attr_intern (bi->attr); if (VNC_DEBUG(VERBOSE)) { diff --git a/bgpd/rfapi/vnc_export_bgp.c b/bgpd/rfapi/vnc_export_bgp.c index bcfa145c67..f20e9ed674 100644 --- a/bgpd/rfapi/vnc_export_bgp.c +++ b/bgpd/rfapi/vnc_export_bgp.c @@ -76,7 +76,6 @@ encap_attr_export_ce ( memset (new, 0, sizeof (struct attr)); bgp_attr_dup (new, orig); bgp_attr_extra_get (new); - bgp_attr_flush_encap (new); /* * Set nexthop -- 2.39.5