From f6f434b2822c453f898552537180a812538bd19e Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Tue, 23 Nov 2010 21:28:03 +0000 Subject: 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?) --- bgpd/bgp_attr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'bgpd/bgp_attr.h') diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index af9dcf5e..3433a1a5 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -142,7 +142,7 @@ extern struct attr_extra *bgp_attr_extra_get (struct attr *); extern void bgp_attr_extra_free (struct attr *); extern void bgp_attr_dup (struct attr *, struct attr *); extern struct attr *bgp_attr_intern (struct attr *attr); -extern void bgp_attr_unintern (struct attr *); +extern void bgp_attr_unintern (struct attr **); extern void bgp_attr_flush (struct attr *); extern struct attr *bgp_attr_default_set (struct attr *attr, u_char); extern struct attr *bgp_attr_default_intern (u_char); -- cgit v1.2.1 From b881c7074bb698aeb1b099175b325734fc6e44d2 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Tue, 23 Nov 2010 16:35:42 +0000 Subject: bgpd: Implement revised error handling for partial optional/trans. attributes * BGP error handling generally boils down to "reset session". This was fine when all BGP speakers pretty much understood all BGP messages. However the increasing deployment of new attribute types has shown this approach to cause problems, in particular where a new attribute type is "tunneled" over some speakers which do not understand it, and then arrives at a speaker which does but considers it malformed (e.g. corruption along the way, or because of early implementation bugs/interop issues). To mitigate this drafts before the IDR (likely to be adopted) propose to treat errors in partial (i.e. not understood by neighbour), optional transitive attributes, when received from eBGP peers, as withdrawing only the NLRIs in the affected UPDATE, rather than causing the entire session to be reset. See: http://tools.ietf.org/html/draft-scudder-idr-optional-transitive * bgp_aspath.c: (assegments_parse) Replace the "NULL means valid, 0-length OR an error" return value with an error code - instead taking pointer to result structure as arg. (aspath_parse) adjust to suit previous change, but here NULL really does mean error in the external interface. * bgp_attr.h (bgp_attr_parse) use an explictly typed and enumerated value to indicate return result. (bgp_attr_unintern_sub) cleans up just the members of an attr, but not the attr itself, for benefit of those who use a stack-local attr. * bgp_attr.c: (bgp_attr_unintern_sub) split out from bgp_attr_unintern (bgp_attr_unintern) as previous. (bgp_attr_malformed) helper function to centralise decisions on how to handle errors in attributes. (bgp_attr_{aspathlimit,origin,etc..}) Use bgp_attr_malformed. (bgp_attr_aspathlimit) Subcode for error specifc to this attr should be BGP_NOTIFY_UPDATE_OPT_ATTR_ERR. (bgp_attr_as4_path) be more rigorous about checks, ala bgp_attr_as_path. (bgp_attr_parse) Adjust to deal with the additional error level that bgp_attr_ parsers can raise, and also similarly return appropriate error back up to (bgp_update_receive). Try to avoid leaking as4_path. * bgp_packet.c: (bgp_update_receive) Adjust to deal with BGP_ATTR_PARSE_WITHDRAW error level from bgp_attr_parse, which should lead to a withdraw, by making the attribute parameter in call to (bgp_nlri_parse) conditional on the error, so the update case morphs also into a withdraw. Use bgp_attr_unintern_sub from above, instead of doing this itself. Fix error case returns which were not calling bgp_attr_unintern_sub and probably leaking memory. * tests/aspath_test.c: Fix to work for null return with bad segments --- bgpd/bgp_attr.h | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'bgpd/bgp_attr.h') diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index 3433a1a5..c0112514 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -132,16 +132,24 @@ struct transit #define ATTR_FLAG_BIT(X) (1 << ((X) - 1)) +typedef enum { + BGP_ATTR_PARSE_PROCEED = 0, + BGP_ATTR_PARSE_ERROR = -1, + BGP_ATTR_PARSE_WITHDRAW = -2, +} bgp_attr_parse_ret_t; + /* Prototypes. */ extern void bgp_attr_init (void); extern void bgp_attr_finish (void); -extern int bgp_attr_parse (struct peer *, struct attr *, bgp_size_t, - struct bgp_nlri *, struct bgp_nlri *); +extern bgp_attr_parse_ret_t bgp_attr_parse (struct peer *, struct attr *, + bgp_size_t, struct bgp_nlri *, + struct bgp_nlri *); extern int bgp_attr_check (struct peer *, struct attr *); extern struct attr_extra *bgp_attr_extra_get (struct attr *); extern void bgp_attr_extra_free (struct attr *); extern void bgp_attr_dup (struct attr *, struct attr *); extern struct attr *bgp_attr_intern (struct attr *attr); +extern void bgp_attr_unintern_sub (struct attr *); extern void bgp_attr_unintern (struct attr **); extern void bgp_attr_flush (struct attr *); extern struct attr *bgp_attr_default_set (struct attr *attr, u_char); -- cgit v1.2.1 From 565b828dc00cafd477dd69ce15f0f551ece67710 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Mon, 10 Oct 2011 21:08:33 +0400 Subject: bgpd: add flag checks for MP_(UN)REACH_NLRI * bgp_attr.[ch] * bgp_mp_reach_parse(): add extra arguments and a uniform flag check block * bgp_mp_unreach_parse(): idem * bgp_attr_parse(): provide extra arguments * bgp_mp_attr_test.c * parse_test(): justify respective calls --- bgpd/bgp_attr.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'bgpd/bgp_attr.h') diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index c0112514..e6300740 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -180,8 +180,9 @@ extern void cluster_unintern (struct cluster_list *); void transit_unintern (struct transit *); /* Exported for unit-test purposes only */ -extern int bgp_mp_reach_parse (struct peer *, bgp_size_t, struct attr *, - struct bgp_nlri *); -extern int bgp_mp_unreach_parse (struct peer *, bgp_size_t, struct bgp_nlri *); +extern int bgp_mp_reach_parse (struct peer *, const bgp_size_t, struct attr *, + const u_char, u_char *, struct bgp_nlri *); +extern int bgp_mp_unreach_parse (struct peer *, const bgp_size_t, const u_char, + u_char *, struct bgp_nlri *); #endif /* _QUAGGA_BGP_ATTR_H */ -- cgit v1.2.1 From 835315bfb49bff2b2fb354f2075c6d6693c2a151 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Wed, 18 Jan 2012 12:28:30 +0000 Subject: bgpd: Move up flag-check calls, parcel up attr-parser args, and other cleanups * bgp_attr.h: (struct bgp_attr_parser_args) Attribute parsing context, containing common arguments. * bgp_attr.c: (general) Move the bgp_attr_flag_invalid flag-check calls up, out of each individual attr parser function, to be done once in attr_parse. Similarly move the calculation of the 'total' attribute length field up to attr_parse. Bundle together common arguments to attr-parsing functions and helpers into (struct bgp_attr_parser_args), so it can be passed by reference down the stack & also de-clutter the argument lists & make it easier to add/modify the context for attr-parsing - add local const aliases to avoid modifying body of code too much. This also should help avoid cut & paste errors, where calls to helpers with hard-coded attribute types are pasted to other functions but the code isn't changed. (bgp_attr_flags_diagnose) as above. (bgp_attr_flag_invalid) as above. (bgp_attr_{origin,aspath,as4_path,nexthop,med,local_pref,atomic}) as above. (bgp_attr_{aggregator,as4_aggregator,community,originator_id}) as above (bgp_attr_{cluster_list,ext_communities},bgp_mp_{un,}reach_parse) as above (bgp_attr_unknown) as above. (bgp_attr_malformed) as above. Also, startp and length have to be special-cased, because whether or not to send attribute data depends on the particular error - a separate length argument, distinct from args->length, indicates whether or not the attribute data should be sent in the NOTIFY. (bgp_attr_aspath_check) Call to bgp_attr_malformed is wrong here, there is no attribute parsing context - e.g. the 'flag' argument is unlikely to be right, remove it. Explicitly handle the error instead. (bgp_attr_munge_as4_attrs) Flag argument is pointless. As the comment notes, the check here is pointless as AS_PATH presence already checked elsewhere. (bgp_attr_parse) Do bgp_attr_flag_invalid call here. Use (struct bgp_attr_parser_args) for args to attr parser functions. Remove out-of-context 'flag' argument to as4 checking functions. --- bgpd/bgp_attr.h | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) (limited to 'bgpd/bgp_attr.h') diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index e6300740..df87c863 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -179,10 +179,19 @@ extern void cluster_unintern (struct cluster_list *); /* Transit attribute prototypes. */ void transit_unintern (struct transit *); -/* Exported for unit-test purposes only */ -extern int bgp_mp_reach_parse (struct peer *, const bgp_size_t, struct attr *, - const u_char, u_char *, struct bgp_nlri *); -extern int bgp_mp_unreach_parse (struct peer *, const bgp_size_t, const u_char, - u_char *, struct bgp_nlri *); +/* Below exported for unit-test purposes only */ +struct bgp_attr_parser_args { + struct peer *peer; + bgp_size_t length; /* attribute data length; */ + bgp_size_t total; /* total length, inc header */ + struct attr *attr; + u_int8_t type; + u_int8_t flags; + u_char *startp; +}; +extern int bgp_mp_reach_parse (struct bgp_attr_parser_args *args, + struct bgp_nlri *); +extern int bgp_mp_unreach_parse (struct bgp_attr_parser_args *args, + struct bgp_nlri *); #endif /* _QUAGGA_BGP_ATTR_H */ -- cgit v1.2.1