From 547357c4a54815e949bdf7db103c08825aa73907 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Tue, 26 Nov 2019 14:42:26 -0500 Subject: [PATCH 1/3] bgpd: ensure transit ptr is nulled on free Signed-off-by: Quentin Young --- bgpd/bgp_attr.c | 15 ++++++++------- bgpd/bgp_attr.h | 3 --- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index bfa578085d..513971bf8d 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -421,14 +421,15 @@ static struct transit *transit_intern(struct transit *transit) return find; } -void transit_unintern(struct transit *transit) +static void transit_unintern(struct transit **transit) { - if (transit->refcnt) - transit->refcnt--; + if ((*transit)->refcnt) + (*transit)->refcnt--; - if (transit->refcnt == 0) { - hash_release(transit_hash, transit); - transit_free(transit); + if ((*transit)->refcnt == 0) { + hash_release(transit_hash, *transit); + transit_free(*transit); + *transit = NULL; } } @@ -860,7 +861,7 @@ void bgp_attr_unintern_sub(struct attr *attr) UNSET_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_CLUSTER_LIST)); if (attr->transit) - transit_unintern(attr->transit); + transit_unintern(&attr->transit); if (attr->encap_subtlvs) encap_unintern(&attr->encap_subtlvs, ENCAP_SUBTLV_TYPE); diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index 375a2272e1..1785f4f68d 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -305,9 +305,6 @@ extern unsigned long int attr_unknown_count(void); extern int cluster_loop_check(struct cluster_list *, struct in_addr); extern void cluster_unintern(struct cluster_list *); -/* Transit attribute prototypes. */ -void transit_unintern(struct transit *); - /* Below exported for unit-test purposes only */ struct bgp_attr_parser_args { struct peer *peer; From b6a171c7c010c3a5809ae620ff823ef3a439efb0 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Tue, 26 Nov 2019 14:42:40 -0500 Subject: [PATCH 2/3] bgpd: clean up attribute parsing state before ret Early exits without appropriate cleanup were causing obscure double frees and other issues later on in the attribute parsing code. If we return anything except a hard attribute parse error, we have cleanup and refcounts to manage. Signed-off-by: Quentin Young --- bgpd/bgp_attr.c | 103 +++++++++++++++++++++++++----------------------- 1 file changed, 53 insertions(+), 50 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 513971bf8d..5233211b47 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -2493,7 +2493,8 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, bgp_notify_send(peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return BGP_ATTR_PARSE_ERROR; + ret = BGP_ATTR_PARSE_ERROR; + goto done; } /* Fetch attribute flag and type. */ @@ -2516,7 +2517,8 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, bgp_notify_send(peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return BGP_ATTR_PARSE_ERROR; + ret = BGP_ATTR_PARSE_ERROR; + goto done; } /* Check extended attribue length bit. */ @@ -2537,7 +2539,8 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, bgp_notify_send(peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_MAL_ATTR); - return BGP_ATTR_PARSE_ERROR; + ret = BGP_ATTR_PARSE_ERROR; + goto done; } /* Set type to bitmap to check duplicate attribute. `type' is @@ -2594,7 +2597,8 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, ndata, ndl + lfl + 1); - return BGP_ATTR_PARSE_ERROR; + ret = BGP_ATTR_PARSE_ERROR; + goto done; } struct bgp_attr_parser_args attr_args = { @@ -2619,7 +2623,7 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, attr_args.total); if (ret == BGP_ATTR_PARSE_PROCEED) continue; - return ret; + goto done; } /* OK check attribute and store it's value. */ @@ -2697,32 +2701,25 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, bgp_notify_send(peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_MAL_ATTR); ret = BGP_ATTR_PARSE_ERROR; + goto done; } if (ret == BGP_ATTR_PARSE_EOR) { - if (as4_path) - aspath_unintern(&as4_path); - return ret; + goto done; } - /* If hard error occurred immediately return to the caller. */ if (ret == BGP_ATTR_PARSE_ERROR) { flog_warn(EC_BGP_ATTRIBUTE_PARSE_ERROR, "%s: Attribute %s, parse error", peer->host, lookup_msg(attr_str, type, NULL)); - if (as4_path) - aspath_unintern(&as4_path); - return ret; + goto done; } if (ret == BGP_ATTR_PARSE_WITHDRAW) { - flog_warn( EC_BGP_ATTRIBUTE_PARSE_WITHDRAW, "%s: Attribute %s, parse error - treating as withdrawal", peer->host, lookup_msg(attr_str, type, NULL)); - if (as4_path) - aspath_unintern(&as4_path); - return ret; + goto done; } /* Check the fetched length. */ @@ -2732,9 +2729,8 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, peer->host, lookup_msg(attr_str, type, NULL)); bgp_notify_send(peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - if (as4_path) - aspath_unintern(&as4_path); - return BGP_ATTR_PARSE_ERROR; + ret = BGP_ATTR_PARSE_ERROR; + goto done; } } @@ -2745,9 +2741,9 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, lookup_msg(attr_str, type, NULL)); bgp_notify_send(peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - if (as4_path) - aspath_unintern(&as4_path); - return BGP_ATTR_PARSE_ERROR; + + ret = BGP_ATTR_PARSE_ERROR; + goto done; } /* @@ -2766,16 +2762,14 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)) && !CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_MP_REACH_NLRI))) { if (bgp_attr_nexthop_valid(peer, attr) < 0) { - return BGP_ATTR_PARSE_ERROR; + ret = BGP_ATTR_PARSE_ERROR; + goto done; } } /* Check all mandatory well-known attributes are present */ - if ((ret = bgp_attr_check(peer, attr)) < 0) { - if (as4_path) - aspath_unintern(&as4_path); - return ret; - } + if ((ret = bgp_attr_check(peer, attr)) < 0) + goto done; /* * At this place we can see whether we got AS4_PATH and/or @@ -2798,22 +2792,10 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, &as4_aggregator_addr)) { bgp_notify_send(peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_MAL_ATTR); - if (as4_path) - aspath_unintern(&as4_path); - return BGP_ATTR_PARSE_ERROR; + ret = BGP_ATTR_PARSE_ERROR; + goto done; } - /* At this stage, we have done all fiddling with as4, and the - * resulting info is in attr->aggregator resp. attr->aspath - * so we can chuck as4_aggregator and as4_path alltogether in - * order to save memory - */ - if (as4_path) { - 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 - */ - } /* * The "rest" of the code does nothing with as4_aggregator. * there is no memory attached specifically which is not part @@ -2827,21 +2809,42 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, if (attr->flag & (ATTR_FLAG_BIT(BGP_ATTR_AS_PATH))) { ret = bgp_attr_aspath_check(peer, attr); if (ret != BGP_ATTR_PARSE_PROCEED) - return ret; + goto done; } - /* Finally intern unknown attribute. */ - if (attr->transit) - attr->transit = transit_intern(attr->transit); - if (attr->encap_subtlvs) - attr->encap_subtlvs = - encap_intern(attr->encap_subtlvs, ENCAP_SUBTLV_TYPE); #if ENABLE_BGP_VNC if (attr->vnc_subtlvs) attr->vnc_subtlvs = encap_intern(attr->vnc_subtlvs, VNC_SUBTLV_TYPE); #endif - return BGP_ATTR_PARSE_PROCEED; + ret = BGP_ATTR_PARSE_PROCEED; + +done: + /* + * At this stage, we have done all fiddling with as4, and the + * resulting info is in attr->aggregator resp. attr->aspath so + * we can chuck as4_aggregator and as4_path alltogether in order + * to save memory + */ + if (as4_path) { + /* + * unintern - it is in the hash + * The flag that we got this is still there, but that + * does not do any trouble + */ + aspath_unintern(&as4_path); + } + + if (ret != BGP_ATTR_PARSE_ERROR) { + /* Finally intern unknown attribute. */ + if (attr->transit) + attr->transit = transit_intern(attr->transit); + if (attr->encap_subtlvs) + attr->encap_subtlvs = encap_intern(attr->encap_subtlvs, + ENCAP_SUBTLV_TYPE); + } + + return ret; } /* From 5e0e9c09f6c609d8d1548054c04360ba3a390acf Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Tue, 3 Dec 2019 15:48:27 -0500 Subject: [PATCH 3/3] bgpd: more attribute parsing cleanup & paranoia * Move VNC interning to the appropriate spot * Use existing bgp_attr_flush_encap to free encap sets * Assert that refcounts are correct before exiting to keep the demons contained in their fiery prison Signed-off-by: Quentin Young --- bgpd/bgp_attr.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 5233211b47..88663bf30d 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -2796,12 +2796,6 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, goto done; } - /* - * The "rest" of the code does nothing with as4_aggregator. - * there is no memory attached specifically which is not part - * of the attr. - * so ignoring just means do nothing. - */ /* * Finally do the checks on the aspath we did not do yet * because we waited for a potentially synthesized aspath. @@ -2811,15 +2805,10 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr, if (ret != BGP_ATTR_PARSE_PROCEED) goto done; } -#if ENABLE_BGP_VNC - if (attr->vnc_subtlvs) - attr->vnc_subtlvs = - encap_intern(attr->vnc_subtlvs, VNC_SUBTLV_TYPE); -#endif ret = BGP_ATTR_PARSE_PROCEED; - done: + /* * At this stage, we have done all fiddling with as4, and the * resulting info is in attr->aggregator resp. attr->aspath so @@ -2842,7 +2831,29 @@ done: if (attr->encap_subtlvs) attr->encap_subtlvs = encap_intern(attr->encap_subtlvs, ENCAP_SUBTLV_TYPE); - } +#if ENABLE_BGP_VNC + if (attr->vnc_subtlvs) + attr->vnc_subtlvs = encap_intern(attr->vnc_subtlvs, + VNC_SUBTLV_TYPE); +#endif + } else { + if (attr->transit) { + transit_free(attr->transit); + attr->transit = NULL; + } + + bgp_attr_flush_encap(attr); + }; + + /* Sanity checks */ + if (attr->transit) + assert(attr->transit->refcnt > 0); + if (attr->encap_subtlvs) + assert(attr->encap_subtlvs->refcnt > 0); +#if ENABLE_BGP_VNC + if (attr->vnc_subtlvs) + assert(attr->vnc_subtlvs->refcnt > 0); +#endif return ret; }