From ab005298526f4b14126cae1a6461ad3d700af29c Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Sat, 27 Nov 2010 22:48:34 +0000 Subject: bgpd: Rollback some of the changes made for invalid AS_PATH segment fix Some of the changes made in commit cddb8112b80fa9867156c637d63e6e79eeac67bb don't work particularly well for other changes that need to be made to address BGP attribute error handling problems. In particular, returning a pointer from complex attribute data parsing functions will not suffice to express the require range of return status conditions. * bgp_aspath.c: (assegments_parse) Rollback to a more minimal set of changes to fix the original problem. (aspath_parse) Slightly needless pushing around of code, and taking 2 parameters to say whether ot use 2 or 4 byte encoding seems unnecessary. * bgp_attr.c: (bgp_attr_as{,4}path) Rollback, in preparation for BGP attribute error handling update. --- bgpd/bgp_aspath.c | 131 +++++++++++++++++++++++++----------------------------- bgpd/bgp_aspath.h | 2 +- bgpd/bgp_attr.c | 100 ++++++++++++++++++----------------------- 3 files changed, 105 insertions(+), 128 deletions(-) diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index 5a73eeff..89a27531 100644 --- a/bgpd/bgp_aspath.c +++ b/bgpd/bgp_aspath.c @@ -671,78 +671,77 @@ aspath_hash_alloc (void *arg) return aspath; } -/* parse as-segment byte stream in struct assegment - * - * Returns NULL if the AS_PATH or AS4_PATH is not valid. - */ +/* parse as-segment byte stream in struct assegment */ static struct assegment * -assegments_parse (struct stream *s, size_t length, int use32bit, int as4_path) +assegments_parse (struct stream *s, size_t length, int use32bit) { struct assegment_header segh; struct assegment *seg, *prev = NULL, *head = NULL; + size_t bytes = 0; - assert (length > 0); /* does not expect empty AS_PATH or AS4_PATH */ + /* empty aspath (ie iBGP or somesuch) */ + if (length == 0) + return NULL; if (BGP_DEBUG (as4, AS4_SEGMENT)) zlog_debug ("[AS4SEG] Parse aspath segment: got total byte length %lu", (unsigned long) length); - - /* double check that length does not exceed stream */ - if (STREAM_READABLE(s) < length) + /* basic checks */ + if ((STREAM_READABLE(s) < length) + || (STREAM_READABLE(s) < AS_HEADER_SIZE) + || (length % AS16_VALUE_SIZE )) return NULL; - /* deal with each segment in turn */ - while (length > 0) + while (bytes < length) { int i; size_t seg_size; - /* softly softly, get the header first on its own */ - if (length >= AS_HEADER_SIZE) + if ((length - bytes) <= AS_HEADER_SIZE) { + if (head) + assegment_free_all (head); + return NULL; + } + + /* softly softly, get the header first on its own */ segh.type = stream_getc (s); segh.length = stream_getc (s); seg_size = ASSEGMENT_SIZE(segh.length, use32bit); - /* includes the header bytes */ if (BGP_DEBUG (as4, AS4_SEGMENT)) zlog_debug ("[AS4SEG] Parse aspath segment: got type %d, length %d", segh.type, segh.length); - switch (segh.type) - { - case AS_SEQUENCE: - case AS_SET: - break ; - - case AS_CONFED_SEQUENCE: - case AS_CONFED_SET: - if (!as4_path) - break ; - /* RFC4893 3: "invalid for the AS4_PATH attribute" */ - /* fall through */ - - default: /* reject unknown or invalid AS_PATH segment types */ - seg_size = 0 ; - } ; - } - else - seg_size = 0 ; - - /* Stop now if segment is not valid (discarding anything collected to date) - * - * RFC4271 4.3, Path Attributes, b) AS_PATH: - * - * "path segment value field contains one or more AS numbers" + /* check it.. */ + if ( ((bytes + seg_size) > length) + /* 1771bis 4.3b: seg length contains one or more */ + || (segh.length == 0) + /* Paranoia in case someone changes type of segment length. + * Shift both values by 0x10 to make the comparison operate + * on more, than 8 bits (otherwise it's a warning, bug #564). */ - if ((seg_size == 0) || (seg_size > length) || (segh.length == 0)) + || ((sizeof segh.length > 1) + && (0x10 + segh.length > 0x10 + AS_SEGMENT_MAX))) { + if (head) assegment_free_all (head); return NULL; } - length -= seg_size ; + switch (segh.type) + { + case AS_SEQUENCE: + case AS_SET: + case AS_CONFED_SEQUENCE: + case AS_CONFED_SET: + break; + default: + if (head) + assegment_free_all (head); + return NULL; + } /* now its safe to trust lengths */ seg = assegment_new (segh.type, segh.length); @@ -755,9 +754,11 @@ assegments_parse (struct stream *s, size_t length, int use32bit, int as4_path) for (i = 0; i < segh.length; i++) seg->as[i] = (use32bit) ? stream_getl (s) : stream_getw (s); + bytes += seg_size; + if (BGP_DEBUG (as4, AS4_SEGMENT)) - zlog_debug ("[AS4SEG] Parse aspath segment: length left: %lu", - (unsigned long) length); + zlog_debug ("[AS4SEG] Parse aspath segment: Bytes now: %lu", + (unsigned long) bytes); prev = seg; } @@ -765,42 +766,30 @@ assegments_parse (struct stream *s, size_t length, int use32bit, int as4_path) return assegment_normalise (head); } -/* AS path parse function -- parses AS_PATH and AS4_PATH attributes - * - * Requires: s -- stream, currently positioned before first segment - * of AS_PATH or AS4_PATH (ie after attribute header) - * length -- length of the value of the AS_PATH or AS4_PATH - * use32bit -- true <=> 4Byte ASN, otherwise 2Byte ASN - * as4_path -- true <=> AS4_PATH, otherwise AS_PATH - * - * Returns: if valid: address of struct aspath in the hash of known aspaths, - * with reference count incremented. - * else: NULL - * - * NB: empty AS path (length == 0) is valid. The returned struct aspath will - * have segments == NULL and str == zero length string (unique). - */ +/* AS path parse function. pnt is a pointer to byte stream and length + is length of byte stream. If there is same AS path in the the AS + path hash then return it else make new AS path structure. */ struct aspath * -aspath_parse (struct stream *s, size_t length, int use32bit, int as4_path) +aspath_parse (struct stream *s, size_t length, int use32bit) { struct aspath as; struct aspath *find; - /* Parse each segment and construct normalised list of struct assegment */ - memset (&as, 0, sizeof (struct aspath)); - if (length != 0) - { - as.segments = assegments_parse (s, length, use32bit, as4_path); + /* If length is odd it's malformed AS path. */ + /* Nit-picking: if (use32bit == 0) it is malformed if odd, + * otherwise its malformed when length is larger than 2 and (length-2) + * is not dividable by 4. + * But... this time we're lazy + */ + if (length % AS16_VALUE_SIZE ) + return NULL; - if (as.segments == NULL) - return NULL ; /* Invalid AS_PATH or AS4_PATH */ - } ; + memset (&as, 0, sizeof (struct aspath)); + as.segments = assegments_parse (s, length, use32bit); /* If already same aspath exist then return it. */ find = hash_get (ashash, &as, aspath_hash_alloc); - assert(find) ; /* valid aspath, so must find or create */ - /* aspath_hash_alloc dupes segments too. that probably could be * optimised out. */ @@ -808,6 +797,8 @@ aspath_parse (struct stream *s, size_t length, int use32bit, int as4_path) if (as.str) XFREE (MTYPE_AS_STR, as.str); + if (! find) + return NULL; find->refcnt++; return find; @@ -1631,7 +1622,7 @@ aspath_segment_add (struct aspath *as, int type) struct aspath * aspath_empty (void) { - return aspath_parse (NULL, 0, 1, 0); /* 32Bit ;-) not AS4_PATH */ + return aspath_parse (NULL, 0, 1); /* 32Bit ;-) */ } struct aspath * diff --git a/bgpd/bgp_aspath.h b/bgpd/bgp_aspath.h index d63b914c..b8a5dfab 100644 --- a/bgpd/bgp_aspath.h +++ b/bgpd/bgp_aspath.h @@ -65,7 +65,7 @@ struct aspath /* Prototypes. */ extern void aspath_init (void); extern void aspath_finish (void); -extern struct aspath *aspath_parse (struct stream *, size_t, int, int); +extern struct aspath *aspath_parse (struct stream *, size_t, int); extern struct aspath *aspath_dup (struct aspath *); extern struct aspath *aspath_aggregate (struct aspath *, struct aspath *); extern struct aspath *aspath_prepend (struct aspath *, struct aspath *); diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index e2b6054b..7a0ab560 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -746,78 +746,51 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, return 0; } -/* Parse AS path information. This function is wrapper of aspath_parse. - * - * Parses AS_PATH or AS4_PATH. - * - * Returns: if valid: address of struct aspath in the hash of known aspaths, - * with reference count incremented. - * else: NULL - * - * NB: empty AS path (length == 0) is valid. The returned struct aspath will - * have segments == NULL and str == zero length string (unique). - */ -static struct aspath * + +/* Parse AS path information. This function is wrapper of + aspath_parse. */ +static int bgp_attr_aspath (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp, int as4_path) + struct attr *attr, u_char flag, u_char *startp) { - u_char require ; - struct aspath *asp ; + bgp_size_t total; - /* Check the attribute flags */ - require = as4_path ? BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS - : BGP_ATTR_FLAG_TRANS ; + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - if ((flag & (BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS)) != require) + /* Flag check. */ + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) + || ! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) { - const char* path_type ; - bgp_size_t total; - - path_type = as4_path ? "AS4_PATH" : "AS_PATH" ; - - if (!CHECK_FLAG(flag, BGP_ATTR_FLAG_TRANS)) zlog (peer->log, LOG_ERR, - "%s attribute flag isn't transitive %d", path_type, flag) ; - - if ((flag & BGP_ATTR_FLAG_OPTIONAL) != (require & BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, - "%s attribute flag must %sbe optional %d", path_type, - (flag & BGP_ATTR_FLAG_OPTIONAL) ? "not " : "", flag) ; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - + "As-Path attribute flag isn't transitive %d", flag); bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + return -1; + } - return NULL ; - } ; - - /* Parse the AS_PATH/AS4_PATH body. - * - * For AS_PATH peer with AS4 => 4Byte ASN otherwise 2Byte ASN - * AS4_PATH 4Byte ASN + /* + * peer with AS4 => will get 4Byte ASnums + * otherwise, will get 16 Bit */ - asp = aspath_parse (peer->ibuf, length, - as4_path || CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV), as4_path) ; + attr->aspath = aspath_parse (peer->ibuf, length, + CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)); - if (asp != NULL) - { - attr->flag |= ATTR_FLAG_BIT (as4_path ? BGP_ATTR_AS4_PATH - : BGP_ATTR_AS_PATH) ; - } - else + /* In case of IBGP, length will be zero. */ + if (! attr->aspath) { zlog (peer->log, LOG_ERR, "Malformed AS path length is %d", length); - - /* TODO: should BGP_NOTIFY_UPDATE_MAL_AS_PATH be sent for AS4_PATH ?? */ bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_MAL_AS_PATH); - } ; + return -1; + } + + /* Set aspath attribute flag. */ + attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS_PATH); - return asp ; + return 0; } static int bgp_attr_aspath_check( struct peer *peer, @@ -875,6 +848,21 @@ static int bgp_attr_aspath_check( struct peer *peer, } +/* Parse AS4 path information. This function is another wrapper of + aspath_parse. */ +static int +bgp_attr_as4_path (struct peer *peer, bgp_size_t length, + struct attr *attr, struct aspath **as4_path) +{ + *as4_path = aspath_parse (peer->ibuf, length, 1); + + /* Set aspath attribute flag. */ + if (as4_path) + attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH); + + return 0; +} + /* Nexthop attribute. */ static int bgp_attr_nexthop (struct peer *peer, bgp_size_t length, @@ -1613,12 +1601,10 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_origin (peer, length, attr, flag, startp); break; case BGP_ATTR_AS_PATH: - attr->aspath = bgp_attr_aspath (peer, length, attr, flag, startp, 0); - ret = attr->aspath ? 0 : -1 ; + ret = bgp_attr_aspath (peer, length, attr, flag, startp); break; case BGP_ATTR_AS4_PATH: - as4_path = bgp_attr_aspath (peer, length, attr, flag, startp, 1); - ret = as4_path ? 0 : -1 ; + ret = bgp_attr_as4_path (peer, length, attr, &as4_path ); break; case BGP_ATTR_NEXT_HOP: ret = bgp_attr_nexthop (peer, length, attr, flag, startp); -- cgit v1.2.1 From 50ef565e4e689ba653b9709be4d28a01f6cca885 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Sat, 27 Nov 2010 23:14:02 +0000 Subject: tests: Extend aspath_test.c with cases for invalid segments & attributes * aspath_test.c: Add more test cases. In particular ones to cover the last invalid-segment problem. Also add ability to specify aspath attribute headers and test them somewhat. NB: It's obvious this test has not been run for a year by anyone, despite 2 non-trivial commits to bgpd aspath code. --- tests/aspath_test.c | 291 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 281 insertions(+), 10 deletions(-) diff --git a/tests/aspath_test.c b/tests/aspath_test.c index 9e51e8dd..ade60c67 100644 --- a/tests/aspath_test.c +++ b/tests/aspath_test.c @@ -6,6 +6,7 @@ #include "bgpd/bgpd.h" #include "bgpd/bgp_aspath.h" +#include "bgpd/bgp_attr.h" #define VT100_RESET "\x1b[0m" #define VT100_RED "\x1b[31m" @@ -420,7 +421,187 @@ static struct test_segment { { "", "", 0, 0, 0, 0, 0, 0 }, }, - { NULL, NULL, {0}, 0, { NULL, 0, 0 } } + { /* 27 */ + "invalid segment type", + "type=8(4096 3456)", + { 0x8,0x2, 0x10,0x00, 0x0d,0x80 }, + 14 + , + { "", "", + 0, 0, 0, 0, 0, 0 }, + }, { NULL, NULL, {0}, 0, { NULL, 0, 0 } } +}; + +/* */ +static struct aspath_tests { + const char *desc; + const struct test_segment *segment; + const char *shouldbe; /* String it should evaluate to */ + const enum as4 { AS4_DATA, AS2_DATA } + as4; /* whether data should be as4 or not (ie as2) */ + const int result; /* expected result for bgp_attr_parse */ + const int cap; /* capabilities to set for peer */ + const char attrheader [1024]; + size_t len; +} aspath_tests [] = +{ + /* 0 */ + { + "basic test", + &test_segments[0], + "8466 3 52737 4096", + AS2_DATA, 0, + 0, + { BGP_ATTR_FLAG_TRANS, + BGP_ATTR_AS_PATH, + 10, + }, + 3, + }, + /* 1 */ + { + "length too short", + &test_segments[0], + "8466 3 52737 4096", + AS2_DATA, -1, + 0, + { BGP_ATTR_FLAG_TRANS, + BGP_ATTR_AS_PATH, + 8, + }, + 3, + }, + /* 2 */ + { + "length too long", + &test_segments[0], + "8466 3 52737 4096", + AS2_DATA, -1, + 0, + { BGP_ATTR_FLAG_TRANS, + BGP_ATTR_AS_PATH, + 12, + }, + 3, + }, + /* 3 */ + { + "incorrect flag", + &test_segments[0], + "8466 3 52737 4096", + AS2_DATA, -1, + 0, + { BGP_ATTR_FLAG_TRANS|BGP_ATTR_FLAG_OPTIONAL, + BGP_ATTR_AS_PATH, + 10, + }, + 3, + }, + /* 4 */ + { + "as4_path, with as2 format data", + &test_segments[0], + "8466 3 52737 4096", + AS2_DATA, -1, + 0, + { BGP_ATTR_FLAG_TRANS|BGP_ATTR_FLAG_OPTIONAL, + BGP_ATTR_AS4_PATH, + 10, + }, + 3, + }, + /* 5 */ + { + "as4, with incorrect attr length", + &test_segments[0], + "8466 3 52737 4096", + AS4_DATA, -1, + PEER_CAP_AS4_RCV, + { BGP_ATTR_FLAG_TRANS|BGP_ATTR_FLAG_OPTIONAL, + BGP_ATTR_AS4_PATH, + 10, + }, + 3, + }, + /* 6 */ + { + "basic 4-byte as-path", + &test_segments[0], + "8466 3 52737 4096", + AS4_DATA, 0, + PEER_CAP_AS4_RCV|PEER_CAP_AS4_ADV, + { BGP_ATTR_FLAG_TRANS, + BGP_ATTR_AS_PATH, + 18, + }, + 3, + }, + /* 7 */ + { + "4b AS_PATH: too short", + &test_segments[0], + "8466 3 52737 4096", + AS4_DATA, -1, + PEER_CAP_AS4_RCV|PEER_CAP_AS4_ADV, + { BGP_ATTR_FLAG_TRANS, + BGP_ATTR_AS_PATH, + 16, + }, + 3, + }, + /* 8 */ + { + "4b AS_PATH: too long", + &test_segments[0], + "8466 3 52737 4096", + AS4_DATA, -1, + PEER_CAP_AS4_RCV|PEER_CAP_AS4_ADV, + { BGP_ATTR_FLAG_TRANS, + BGP_ATTR_AS_PATH, + 20, + }, + 3, + }, + /* 9 */ + { + "4b AS_PATH: too long2", + &test_segments[0], + "8466 3 52737 4096", + AS4_DATA, -1, + PEER_CAP_AS4_RCV|PEER_CAP_AS4_ADV, + { BGP_ATTR_FLAG_TRANS, + BGP_ATTR_AS_PATH, + 22, + }, + 3, + }, + /* 10 */ + { + "4b AS_PATH: bad flags", + &test_segments[0], + "8466 3 52737 4096", + AS4_DATA, -1, + PEER_CAP_AS4_RCV|PEER_CAP_AS4_ADV, + { BGP_ATTR_FLAG_TRANS|BGP_ATTR_FLAG_OPTIONAL, + BGP_ATTR_AS_PATH, + 18, + }, + 3, + }, + /* 11 */ + { + "4b AS_PATH: confed", + &test_segments[6], + "8466 3 52737 4096", + AS4_DATA, -1, + PEER_CAP_AS4_ADV, + { BGP_ATTR_FLAG_TRANS|BGP_ATTR_FLAG_OPTIONAL, + BGP_ATTR_AS4_PATH, + 14, + }, + 3, + }, + { NULL, NULL, NULL, 0, 0, 0, { 0 }, 0 }, }; /* prepending tests */ @@ -430,21 +611,25 @@ static struct tests { struct test_spec sp; } prepend_tests[] = { + /* 0 */ { &test_segments[0], &test_segments[1], { "8466 3 52737 4096 8722 4", "8466 3 52737 4096 8722 4", 6, 0, NOT_ALL_PRIVATE, 4096, 1, 8466 }, }, + /* 1 */ { &test_segments[1], &test_segments[3], { "8722 4 8482 51457 {5204}", "8722 4 8482 51457 {5204}", 5, 0, NOT_ALL_PRIVATE, 5204, 1, 8722 } }, + /* 2 */ { &test_segments[3], &test_segments[4], { "8482 51457 {5204} 8467 59649 {4196,48658} {17322,30745}", "8482 51457 {5204} 8467 59649 {4196,48658} {17322,30745}", 7, 0, NOT_ALL_PRIVATE, 5204, 1, 8482 }, }, + /* 3 */ { &test_segments[4], &test_segments[5], { "8467 59649 {4196,48658} {17322,30745} 6435 59408 21665" " {2457,4369,61697} 1842 41590 51793", @@ -452,11 +637,13 @@ static struct tests { " {2457,4369,61697} 1842 41590 51793", 11, 0, NOT_ALL_PRIVATE, 61697, 1, 8467 } }, + /* 4 */ { &test_segments[5], &test_segments[6], - { "6435 59408 21665 {2457,4369,61697} 1842 41590 51793 (123 456 789)", - "6435 59408 21665 {2457,4369,61697} 1842 41590 51793 (123 456 789)", - 7, 3, NOT_ALL_PRIVATE, 123, 1, 6435 }, + { "6435 59408 21665 {2457,4369,61697} 1842 41590 51793", + "6435 59408 21665 {2457,4369,61697} 1842 41590 51793", + 7, 0, NOT_ALL_PRIVATE, 1842, 1, 6435 }, }, + /* 5 */ { &test_segments[6], &test_segments[7], { "(123 456 789) (123 456 789) (111 222)", "", @@ -649,7 +836,7 @@ make_aspath (const u_char *data, size_t len, int use32bit) s = stream_new (len); stream_put (s, data, len); } - as = aspath_parse (s, len, use32bit, 0); + as = aspath_parse (s, len, use32bit); if (s) stream_free (s); @@ -993,30 +1180,106 @@ cmp_test () aspath_unintern (asp2); } } - + +static int +handle_attr_test (struct aspath_tests *t) +{ + struct bgp bgp = { 0 }; + struct peer peer = { 0 }; + struct attr attr = { 0 }; + int ret; + int initfail = failed; + struct aspath *asp; + size_t datalen; + + asp = make_aspath (t->segment->asdata, t->segment->len, 0); + + peer.ibuf = stream_new (BGP_MAX_PACKET_SIZE); + peer.obuf = stream_fifo_new (); + peer.bgp = &bgp; + peer.host = (char *)"none"; + peer.fd = -1; + peer.cap = t->cap; + + stream_write (peer.ibuf, t->attrheader, t->len); + datalen = aspath_put (peer.ibuf, asp, t->as4 == AS4_DATA); + + ret = bgp_attr_parse (&peer, &attr, t->len + datalen, NULL, NULL); + + if (ret != t->result) + { + printf ("bgp_attr_parse returned %d, expected %d\n", ret, t->result); + printf ("datalen %d\n", datalen); + failed++; + } + if (ret != 0) + goto out; + + if (attr.aspath == NULL) + { + printf ("aspath is NULL!\n"); + failed++; + } + if (attr.aspath && strcmp (attr.aspath->str, t->shouldbe)) + { + printf ("attr str and 'shouldbe' mismatched!\n" + "attr str: %s\n" + "shouldbe: %s\n", + attr.aspath->str, t->shouldbe); + failed++; + } + +out: + if (attr.aspath) + aspath_unintern (attr.aspath); + if (asp) + aspath_unintern (asp); + return failed - initfail; +} + +static void +attr_test (struct aspath_tests *t) +{ + printf ("%s\n", t->desc); + printf ("%s\n\n", handle_attr_test (t) ? FAILED : OK); +} + int main (void) { int i = 0; - aspath_init(); + bgp_master_init (); + master = bm->master; + bgp_attr_init (); + while (test_segments[i].name) { + printf ("test %u\n", i); parse_test (&test_segments[i]); empty_prepend_test (&test_segments[i++]); } i = 0; while (prepend_tests[i].test1) - prepend_test (&prepend_tests[i++]); + { + printf ("prepend test %u\n", i); + prepend_test (&prepend_tests[i++]); + } i = 0; while (aggregate_tests[i].test1) - aggregate_test (&aggregate_tests[i++]); + { + printf ("aggregate test %u\n", i); + aggregate_test (&aggregate_tests[i++]); + } i = 0; while (reconcile_tests[i].test1) - as4_reconcile_test (&reconcile_tests[i++]); + { + printf ("reconcile test %u\n", i); + as4_reconcile_test (&reconcile_tests[i++]); + } i = 0; @@ -1026,6 +1289,14 @@ main (void) empty_get_test(); + i = 0; + + while (aspath_tests[i].desc) + { + printf ("aspath_attr test %d\n", i); + attr_test (&aspath_tests[i++]); + } + printf ("failures: %d\n", failed); printf ("aspath count: %ld\n", aspath_count()); -- cgit v1.2.1 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_advertise.c | 10 ++++----- bgpd/bgp_aspath.c | 16 ++++++++------ bgpd/bgp_aspath.h | 2 +- bgpd/bgp_attr.c | 54 ++++++++++++++++++++++++---------------------- bgpd/bgp_attr.h | 2 +- bgpd/bgp_clist.c | 4 ++-- bgpd/bgp_community.c | 13 +++++------ bgpd/bgp_community.h | 2 +- bgpd/bgp_ecommunity.c | 33 ++++++++++++++-------------- bgpd/bgp_ecommunity.h | 4 ++-- bgpd/bgp_packet.c | 8 +++---- bgpd/bgp_route.c | 60 +++++++++++++++++++++++++-------------------------- bgpd/bgp_routemap.c | 27 ++++++++++++++++------- 13 files changed, 126 insertions(+), 109 deletions(-) diff --git a/bgpd/bgp_advertise.c b/bgpd/bgp_advertise.c index 87eb7ac7..666218fa 100644 --- a/bgpd/bgp_advertise.c +++ b/bgpd/bgp_advertise.c @@ -140,13 +140,13 @@ bgp_advertise_unintern (struct hash *hash, struct bgp_advertise_attr *baa) baa->refcnt--; if (baa->refcnt && baa->attr) - bgp_attr_unintern (baa->attr); + bgp_attr_unintern (&baa->attr); else { if (baa->attr) { hash_release (hash, baa); - bgp_attr_unintern (baa->attr); + bgp_attr_unintern (&baa->attr); } baa_free (baa); } @@ -319,7 +319,7 @@ bgp_adj_out_remove (struct bgp_node *rn, struct bgp_adj_out *adj, struct peer *peer, afi_t afi, safi_t safi) { if (adj->attr) - bgp_attr_unintern (adj->attr); + bgp_attr_unintern (&adj->attr); if (adj->adv) bgp_advertise_clean (peer, adj, afi, safi); @@ -339,7 +339,7 @@ bgp_adj_in_set (struct bgp_node *rn, struct peer *peer, struct attr *attr) { if (adj->attr != attr) { - bgp_attr_unintern (adj->attr); + bgp_attr_unintern (&adj->attr); adj->attr = bgp_attr_intern (attr); } return; @@ -355,7 +355,7 @@ bgp_adj_in_set (struct bgp_node *rn, struct peer *peer, struct attr *attr) void bgp_adj_in_remove (struct bgp_node *rn, struct bgp_adj_in *bai) { - bgp_attr_unintern (bai->attr); + bgp_attr_unintern (&bai->attr); BGP_ADJ_IN_DEL (rn, bai); peer_unlock (bai->peer); /* adj_in peer reference */ XFREE (MTYPE_BGP_ADJ_IN, bai); diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index 89a27531..176a960e 100644 --- a/bgpd/bgp_aspath.c +++ b/bgpd/bgp_aspath.c @@ -340,19 +340,21 @@ aspath_free (struct aspath *aspath) /* Unintern aspath from AS path bucket. */ void -aspath_unintern (struct aspath *aspath) +aspath_unintern (struct aspath **aspath) { struct aspath *ret; + struct aspath *asp = *aspath; + + if (asp->refcnt) + asp->refcnt--; - if (aspath->refcnt) - aspath->refcnt--; - - if (aspath->refcnt == 0) + if (asp->refcnt == 0) { /* This aspath must exist in aspath hash table. */ - ret = hash_release (ashash, aspath); + ret = hash_release (ashash, asp); assert (ret != NULL); - aspath_free (aspath); + aspath_free (asp); + *aspath = NULL; } } diff --git a/bgpd/bgp_aspath.h b/bgpd/bgp_aspath.h index b8a5dfab..d55f9ce6 100644 --- a/bgpd/bgp_aspath.h +++ b/bgpd/bgp_aspath.h @@ -80,7 +80,7 @@ extern struct aspath *aspath_empty_get (void); extern struct aspath *aspath_str2aspath (const char *); extern void aspath_free (struct aspath *); extern struct aspath *aspath_intern (struct aspath *); -extern void aspath_unintern (struct aspath *); +extern void aspath_unintern (struct aspath **); extern const char *aspath_print (struct aspath *); extern void aspath_print_vty (struct vty *, const char *, struct aspath *, const char *); extern void aspath_print_all_vty (struct vty *); diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 7a0ab560..0c5355b4 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -500,6 +500,7 @@ bgp_attr_intern (struct attr *attr) attre->ecommunity = ecommunity_intern (attre->ecommunity); else attre->ecommunity->refcnt++; + } if (attre->cluster) { @@ -516,10 +517,10 @@ bgp_attr_intern (struct attr *attr) attre->transit->refcnt++; } } - + find = (struct attr *) hash_get (attrhash, attr, bgp_attr_hash_alloc); find->refcnt++; - + return find; } @@ -561,7 +562,7 @@ bgp_attr_default_intern (u_char origin) new = bgp_attr_intern (&attr); bgp_attr_extra_free (&attr); - aspath_unintern (new->aspath); + aspath_unintern (&new->aspath); return new; } @@ -613,13 +614,13 @@ bgp_attr_aggregate_intern (struct bgp *bgp, u_char origin, 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 @@ bgp_attr_unintern (struct attr *attr) 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 @@ bgp_attr_flush (struct attr *attr) 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 @@ static int bgp_attr_aspath_check( struct peer *peer, { 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 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, 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 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, */ 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 */ 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); diff --git a/bgpd/bgp_clist.c b/bgpd/bgp_clist.c index d6601674..6c9976e3 100644 --- a/bgpd/bgp_clist.c +++ b/bgpd/bgp_clist.c @@ -70,7 +70,7 @@ community_entry_free (struct community_entry *entry) if (entry->config) XFREE (MTYPE_ECOMMUNITY_STR, entry->config); if (entry->u.ecom) - ecommunity_free (entry->u.ecom); + ecommunity_free (&entry->u.ecom); break; case COMMUNITY_LIST_EXPANDED: case EXTCOMMUNITY_LIST_EXPANDED: @@ -806,7 +806,7 @@ extcommunity_list_unset (struct community_list_handler *ch, entry = community_list_entry_lookup (list, str, direct); if (ecom) - ecommunity_free (ecom); + ecommunity_free (&ecom); if (regex) bgp_regex_free (regex); diff --git a/bgpd/bgp_community.c b/bgpd/bgp_community.c index ae1d7a15..2ba45f6e 100644 --- a/bgpd/bgp_community.c +++ b/bgpd/bgp_community.c @@ -321,21 +321,22 @@ community_intern (struct community *com) /* Free community attribute. */ void -community_unintern (struct community *com) +community_unintern (struct community **com) { struct community *ret; - if (com->refcnt) - com->refcnt--; + if ((*com)->refcnt) + (*com)->refcnt--; /* Pull off from hash. */ - if (com->refcnt == 0) + if ((*com)->refcnt == 0) { /* Community value com must exist in hash. */ - ret = (struct community *) hash_release (comhash, com); + ret = (struct community *) hash_release (comhash, *com); assert (ret != NULL); - community_free (com); + community_free (*com); + *com = NULL; } } diff --git a/bgpd/bgp_community.h b/bgpd/bgp_community.h index bc1e56ef..9e483770 100644 --- a/bgpd/bgp_community.h +++ b/bgpd/bgp_community.h @@ -57,7 +57,7 @@ extern void community_free (struct community *); extern struct community *community_uniq_sort (struct community *); extern struct community *community_parse (u_int32_t *, u_short); extern struct community *community_intern (struct community *); -extern void community_unintern (struct community *); +extern void community_unintern (struct community **); extern char *community_str (struct community *); extern unsigned int community_hash_make (struct community *); extern struct community *community_str2com (const char *); diff --git a/bgpd/bgp_ecommunity.c b/bgpd/bgp_ecommunity.c index 8d5fa741..8d91c746 100644 --- a/bgpd/bgp_ecommunity.c +++ b/bgpd/bgp_ecommunity.c @@ -42,13 +42,14 @@ ecommunity_new (void) /* Allocate ecommunities. */ void -ecommunity_free (struct ecommunity *ecom) +ecommunity_free (struct ecommunity **ecom) { - if (ecom->val) - XFREE (MTYPE_ECOMMUNITY_VAL, ecom->val); - if (ecom->str) - XFREE (MTYPE_ECOMMUNITY_STR, ecom->str); - XFREE (MTYPE_ECOMMUNITY, ecom); + if ((*ecom)->val) + XFREE (MTYPE_ECOMMUNITY_VAL, (*ecom)->val); + if ((*ecom)->str) + XFREE (MTYPE_ECOMMUNITY_STR, (*ecom)->str); + XFREE (MTYPE_ECOMMUNITY, *ecom); + ecom = NULL; } /* Add a new Extended Communities value to Extended Communities @@ -197,7 +198,7 @@ ecommunity_intern (struct ecommunity *ecom) find = (struct ecommunity *) hash_get (ecomhash, ecom, hash_alloc_intern); if (find != ecom) - ecommunity_free (ecom); + ecommunity_free (&ecom); find->refcnt++; @@ -209,18 +210,18 @@ ecommunity_intern (struct ecommunity *ecom) /* Unintern Extended Communities Attribute. */ void -ecommunity_unintern (struct ecommunity *ecom) +ecommunity_unintern (struct ecommunity **ecom) { struct ecommunity *ret; - if (ecom->refcnt) - ecom->refcnt--; - + if ((*ecom)->refcnt) + (*ecom)->refcnt--; + /* Pull off from hash. */ - if (ecom->refcnt == 0) + if ((*ecom)->refcnt == 0) { /* Extended community must be in the hash. */ - ret = (struct ecommunity *) hash_release (ecomhash, ecom); + ret = (struct ecommunity *) hash_release (ecomhash, *ecom); assert (ret != NULL); ecommunity_free (ecom); @@ -516,7 +517,7 @@ ecommunity_str2com (const char *str, int type, int keyword_included) if (! keyword_included || keyword) { if (ecom) - ecommunity_free (ecom); + ecommunity_free (&ecom); return NULL; } keyword = 1; @@ -536,7 +537,7 @@ ecommunity_str2com (const char *str, int type, int keyword_included) if (! keyword) { if (ecom) - ecommunity_free (ecom); + ecommunity_free (&ecom); return NULL; } keyword = 0; @@ -549,7 +550,7 @@ ecommunity_str2com (const char *str, int type, int keyword_included) case ecommunity_token_unknown: default: if (ecom) - ecommunity_free (ecom); + ecommunity_free (&ecom); return NULL; } } diff --git a/bgpd/bgp_ecommunity.h b/bgpd/bgp_ecommunity.h index 942fdc73..2f59dc40 100644 --- a/bgpd/bgp_ecommunity.h +++ b/bgpd/bgp_ecommunity.h @@ -67,13 +67,13 @@ struct ecommunity_val extern void ecommunity_init (void); extern void ecommunity_finish (void); -extern void ecommunity_free (struct ecommunity *); +extern void ecommunity_free (struct ecommunity **); extern struct ecommunity *ecommunity_parse (u_int8_t *, u_short); extern struct ecommunity *ecommunity_dup (struct ecommunity *); extern struct ecommunity *ecommunity_merge (struct ecommunity *, struct ecommunity *); extern struct ecommunity *ecommunity_intern (struct ecommunity *); extern int ecommunity_cmp (const void *, const void *); -extern void ecommunity_unintern (struct ecommunity *); +extern void ecommunity_unintern (struct ecommunity **); extern unsigned int ecommunity_hash_make (void *); extern struct ecommunity *ecommunity_str2com (const char *, int, int); extern char *ecommunity_ecom2str (struct ecommunity *, int); diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 9102add7..dee0b51f 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -206,7 +206,7 @@ bgp_update_packet (struct peer *peer, afi_t afi, safi_t safi) /* Synchnorize attribute. */ if (adj->attr) - bgp_attr_unintern (adj->attr); + bgp_attr_unintern (&adj->attr); else peer->scount[afi][safi]++; @@ -1785,13 +1785,13 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) /* Everything is done. We unintern temporary structures which interned in bgp_attr_parse(). */ if (attr.aspath) - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); if (attr.community) - community_unintern (attr.community); + community_unintern (&attr.community); if (attr.extra) { if (attr.extra->ecommunity) - ecommunity_unintern (attr.extra->ecommunity); + ecommunity_unintern (&attr.extra->ecommunity); if (attr.extra->cluster) cluster_unintern (attr.extra->cluster); if (attr.extra->transit) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 13ff63ca..87cb677c 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -137,7 +137,7 @@ static void bgp_info_free (struct bgp_info *binfo) { if (binfo->attr) - bgp_attr_unintern (binfo->attr); + bgp_attr_unintern (&binfo->attr); bgp_info_extra_free (&binfo->extra); @@ -1802,14 +1802,14 @@ bgp_update_rsclient (struct peer *rsclient, afi_t afi, safi_t safi, /* Apply import policy. */ if (bgp_import_modifier (rsclient, peer, p, &new_attr, afi, safi) == RMAP_DENY) { - bgp_attr_unintern (attr_new2); + bgp_attr_unintern (&attr_new2); reason = "import-policy;"; goto filtered; } attr_new = bgp_attr_intern (&new_attr); - bgp_attr_unintern (attr_new2); + bgp_attr_unintern (&attr_new2); /* IPv4 unicast next hop check. */ if (afi == AFI_IP && safi == SAFI_UNICAST) @@ -1818,7 +1818,7 @@ bgp_update_rsclient (struct peer *rsclient, afi_t afi, safi_t safi, if (new_attr.nexthop.s_addr == 0 || ntohl (new_attr.nexthop.s_addr) >= 0xe0000000) { - bgp_attr_unintern (attr_new); + bgp_attr_unintern (&attr_new); reason = "martian next-hop;"; goto filtered; @@ -1848,7 +1848,7 @@ bgp_update_rsclient (struct peer *rsclient, afi_t afi, safi_t safi, p->prefixlen, rsclient->host); bgp_unlock_node (rn); - bgp_attr_unintern (attr_new); + bgp_attr_unintern (&attr_new); return; } @@ -1868,7 +1868,7 @@ bgp_update_rsclient (struct peer *rsclient, afi_t afi, safi_t safi, bgp_info_set_flag (rn, ri, BGP_INFO_ATTR_CHANGED); /* Update to new attribute. */ - bgp_attr_unintern (ri->attr); + bgp_attr_unintern (&ri->attr); ri->attr = attr_new; /* Update MPLS tag. */ @@ -2128,7 +2128,7 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr, } bgp_unlock_node (rn); - bgp_attr_unintern (attr_new); + bgp_attr_unintern (&attr_new); bgp_attr_extra_free (&new_attr); return 0; @@ -2175,7 +2175,7 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr, } /* Update to new attribute. */ - bgp_attr_unintern (ri->attr); + bgp_attr_unintern (&ri->attr); ri->attr = attr_new; /* Update MPLS tag. */ @@ -2467,7 +2467,7 @@ bgp_default_originate (struct peer *peer, afi_t afi, safi_t safi, int withdraw) } bgp_attr_extra_free (&attr); - aspath_unintern (aspath); + aspath_unintern (&aspath); } static void @@ -3215,7 +3215,7 @@ bgp_static_update_rsclient (struct peer *rsclient, struct prefix *p, bgp_attr_flush (&attr_tmp); /* Unintern original. */ - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); bgp_static_withdraw_rsclient (bgp, rsclient, p, afi, safi); bgp_attr_extra_free (&attr); @@ -3242,8 +3242,8 @@ bgp_static_update_rsclient (struct peer *rsclient, struct prefix *p, bgp->peer_self->rmap_type = 0; - bgp_attr_unintern (attr_new); - aspath_unintern (attr.aspath); + bgp_attr_unintern (&attr_new); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); bgp_static_withdraw_rsclient (bgp, rsclient, p, afi, safi); @@ -3253,7 +3253,7 @@ bgp_static_update_rsclient (struct peer *rsclient, struct prefix *p, bgp->peer_self->rmap_type = 0; - bgp_attr_unintern (attr_new); + bgp_attr_unintern (&attr_new); attr_new = bgp_attr_intern (&new_attr); bgp_attr_extra_free (&new_attr); @@ -3268,8 +3268,8 @@ bgp_static_update_rsclient (struct peer *rsclient, struct prefix *p, !CHECK_FLAG(ri->flags, BGP_INFO_REMOVED)) { bgp_unlock_node (rn); - bgp_attr_unintern (attr_new); - aspath_unintern (attr.aspath); + bgp_attr_unintern (&attr_new); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); return; } @@ -3281,14 +3281,14 @@ bgp_static_update_rsclient (struct peer *rsclient, struct prefix *p, /* Rewrite BGP route information. */ if (CHECK_FLAG(ri->flags, BGP_INFO_REMOVED)) bgp_info_restore(rn, ri); - bgp_attr_unintern (ri->attr); + bgp_attr_unintern (&ri->attr); ri->attr = attr_new; ri->uptime = bgp_clock (); /* Process change. */ bgp_process (bgp, rn, afi, safi); bgp_unlock_node (rn); - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); return; } @@ -3313,7 +3313,7 @@ bgp_static_update_rsclient (struct peer *rsclient, struct prefix *p, bgp_process (bgp, rn, afi, safi); /* Unintern original. */ - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); } @@ -3363,7 +3363,7 @@ bgp_static_update_main (struct bgp *bgp, struct prefix *p, bgp_attr_flush (&attr_tmp); /* Unintern original. */ - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); bgp_static_withdraw (bgp, p, afi, safi); return; @@ -3384,8 +3384,8 @@ bgp_static_update_main (struct bgp *bgp, struct prefix *p, !CHECK_FLAG(ri->flags, BGP_INFO_REMOVED)) { bgp_unlock_node (rn); - bgp_attr_unintern (attr_new); - aspath_unintern (attr.aspath); + bgp_attr_unintern (&attr_new); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); return; } @@ -3399,7 +3399,7 @@ bgp_static_update_main (struct bgp *bgp, struct prefix *p, bgp_info_restore(rn, ri); else bgp_aggregate_decrement (bgp, p, ri, afi, safi); - bgp_attr_unintern (ri->attr); + bgp_attr_unintern (&ri->attr); ri->attr = attr_new; ri->uptime = bgp_clock (); @@ -3407,7 +3407,7 @@ bgp_static_update_main (struct bgp *bgp, struct prefix *p, bgp_aggregate_increment (bgp, p, ri, afi, safi); bgp_process (bgp, rn, afi, safi); bgp_unlock_node (rn); - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); return; } @@ -3435,7 +3435,7 @@ bgp_static_update_main (struct bgp *bgp, struct prefix *p, bgp_process (bgp, rn, afi, safi); /* Unintern original. */ - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); } @@ -5297,7 +5297,7 @@ bgp_redistribute_add (struct prefix *p, struct in_addr *nexthop, bgp_attr_extra_free (&attr_new); /* Unintern original. */ - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); bgp_redistribute_delete (p, type); return; @@ -5320,8 +5320,8 @@ bgp_redistribute_add (struct prefix *p, struct in_addr *nexthop, if (attrhash_cmp (bi->attr, new_attr) && !CHECK_FLAG(bi->flags, BGP_INFO_REMOVED)) { - bgp_attr_unintern (new_attr); - aspath_unintern (attr.aspath); + bgp_attr_unintern (&new_attr); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); bgp_unlock_node (bn); return; @@ -5336,7 +5336,7 @@ bgp_redistribute_add (struct prefix *p, struct in_addr *nexthop, bgp_info_restore(bn, bi); else bgp_aggregate_decrement (bgp, p, bi, afi, SAFI_UNICAST); - bgp_attr_unintern (bi->attr); + bgp_attr_unintern (&bi->attr); bi->attr = new_attr; bi->uptime = bgp_clock (); @@ -5344,7 +5344,7 @@ bgp_redistribute_add (struct prefix *p, struct in_addr *nexthop, bgp_aggregate_increment (bgp, p, bi, afi, SAFI_UNICAST); bgp_process (bgp, bn, afi, SAFI_UNICAST); bgp_unlock_node (bn); - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); return; } @@ -5366,7 +5366,7 @@ bgp_redistribute_add (struct prefix *p, struct in_addr *nexthop, } /* Unintern original. */ - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); bgp_attr_extra_free (&attr); } 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. */ -- cgit v1.2.1 From c112af27ed8f158ecece0d73ce2016c166076c00 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Sat, 27 Nov 2010 11:49:09 +0000 Subject: tools/multiple-bgpd.sh: set some community attributes to help test them --- tools/multiple-bgpd.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tools/multiple-bgpd.sh b/tools/multiple-bgpd.sh index 028ad696..d6a38ed4 100644 --- a/tools/multiple-bgpd.sh +++ b/tools/multiple-bgpd.sh @@ -25,13 +25,14 @@ for H in `seq 1 ${NUM}` ; do NEXTAS=$((${ASBASE} + $NEXT)) PREVADDR="${PREFIX}${PREV}" PREVAS=$((${ASBASE} + $PREV)) + ASN=$((64560+${H})) # Edit config to suit. cat > "$CONF" <<- EOF password whatever service advanced-vty ! - router bgp $((64560+${H})) + router bgp ${ASN} bgp router-id ${ADDR} network 10.${H}.1.0/24 pathlimit 1 network 10.${H}.2.0/24 pathlimit 2 @@ -40,6 +41,7 @@ for H in `seq 1 ${NUM}` ; do neighbor default update-source ${ADDR} neighbor default capability orf prefix-list both neighbor default soft-reconfiguration inbound + neighbor default route-map test out neighbor ${NEXTADDR} remote-as ${NEXTAS} neighbor ${NEXTADDR} peer-group default neighbor ${PREVADDR} remote-as ${PREVAS} @@ -53,10 +55,15 @@ for H in `seq 1 ${NUM}` ; do neighbor default activate neighbor default capability orf prefix-list both neighbor default default-originate + neighbor default route-map test out neighbor ${NEXTADDR} peer-group default neighbor ${PREVADDR} peer-group default exit-address-family ! + route-map test permit 10 + set extcommunity rt ${ASN}:1 + set extcommunity soo ${ASN}:2 + set community ${ASN}:1 line vty ! end -- 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_aspath.c | 26 +-- bgpd/bgp_attr.c | 497 ++++++++++++++++++++++++++++++++-------------------- bgpd/bgp_attr.h | 12 +- bgpd/bgp_packet.c | 76 +++++--- tests/aspath_test.c | 21 ++- 5 files changed, 393 insertions(+), 239 deletions(-) diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index 176a960e..ba4f5b48 100644 --- a/bgpd/bgp_aspath.c +++ b/bgpd/bgp_aspath.c @@ -674,8 +674,9 @@ aspath_hash_alloc (void *arg) } /* parse as-segment byte stream in struct assegment */ -static struct assegment * -assegments_parse (struct stream *s, size_t length, int use32bit) +static int +assegments_parse (struct stream *s, size_t length, + struct assegment **result, int use32bit) { struct assegment_header segh; struct assegment *seg, *prev = NULL, *head = NULL; @@ -683,7 +684,7 @@ assegments_parse (struct stream *s, size_t length, int use32bit) /* empty aspath (ie iBGP or somesuch) */ if (length == 0) - return NULL; + return 0; if (BGP_DEBUG (as4, AS4_SEGMENT)) zlog_debug ("[AS4SEG] Parse aspath segment: got total byte length %lu", @@ -692,7 +693,7 @@ assegments_parse (struct stream *s, size_t length, int use32bit) if ((STREAM_READABLE(s) < length) || (STREAM_READABLE(s) < AS_HEADER_SIZE) || (length % AS16_VALUE_SIZE )) - return NULL; + return -1; while (bytes < length) { @@ -703,7 +704,7 @@ assegments_parse (struct stream *s, size_t length, int use32bit) { if (head) assegment_free_all (head); - return NULL; + return -1; } /* softly softly, get the header first on its own */ @@ -729,7 +730,7 @@ assegments_parse (struct stream *s, size_t length, int use32bit) { if (head) assegment_free_all (head); - return NULL; + return -1; } switch (segh.type) @@ -742,7 +743,7 @@ assegments_parse (struct stream *s, size_t length, int use32bit) default: if (head) assegment_free_all (head); - return NULL; + return -1; } /* now its safe to trust lengths */ @@ -765,12 +766,16 @@ assegments_parse (struct stream *s, size_t length, int use32bit) prev = seg; } - return assegment_normalise (head); + *result = assegment_normalise (head); + return 0; } /* AS path parse function. pnt is a pointer to byte stream and length is length of byte stream. If there is same AS path in the the AS - path hash then return it else make new AS path structure. */ + path hash then return it else make new AS path structure. + + On error NULL is returned. + */ struct aspath * aspath_parse (struct stream *s, size_t length, int use32bit) { @@ -787,7 +792,8 @@ aspath_parse (struct stream *s, size_t length, int use32bit) return NULL; memset (&as, 0, sizeof (struct aspath)); - as.segments = assegments_parse (s, length, use32bit); + if (assegments_parse (s, length, &as.segments, use32bit) < 0) + return NULL; /* If already same aspath exist then return it. */ find = hash_get (ashash, &as, aspath_hash_alloc); diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 0c5355b4..6eab5ae8 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -618,26 +618,50 @@ bgp_attr_aggregate_intern (struct bgp *bgp, u_char origin, return new; } +/* Unintern just the sub-components of the attr, but not the attr */ +void +bgp_attr_unintern_sub (struct attr *attr) +{ + /* aspath refcount shoud be decrement. */ + if (attr->aspath) + aspath_unintern (&attr->aspath); + UNSET_FLAG(attr->flag, BGP_ATTR_AS_PATH); + + if (attr->community) + community_unintern (&attr->community); + UNSET_FLAG(attr->flag, BGP_ATTR_COMMUNITIES); + + if (attr->extra) + { + if (attr->extra->ecommunity) + ecommunity_unintern (&attr->extra->ecommunity); + UNSET_FLAG(attr->flag, BGP_ATTR_EXT_COMMUNITIES); + + if (attr->extra->cluster) + cluster_unintern (attr->extra->cluster); + UNSET_FLAG(attr->flag, BGP_ATTR_CLUSTER_LIST); + + if (attr->extra->transit) + transit_unintern (attr->extra->transit); + } +} + /* Free bgp attribute and aspath. */ void bgp_attr_unintern (struct attr **attr) { struct attr *ret; - struct aspath *aspath; - struct community *community; - struct ecommunity *ecommunity = NULL; - struct cluster_list *cluster = NULL; - struct transit *transit = NULL; + struct attr tmp; /* Decrement attribute reference. */ (*attr)->refcnt--; - aspath = (*attr)->aspath; - community = (*attr)->community; + + tmp = *(*attr); + if ((*attr)->extra) { - ecommunity = (*attr)->extra->ecommunity; - cluster = (*attr)->extra->cluster; - transit = (*attr)->extra->transit; + tmp.extra = bgp_attr_extra_new (); + memcpy (tmp.extra, (*attr)->extra, sizeof (struct attr_extra)); } /* If reference becomes zero then free attribute object. */ @@ -650,17 +674,7 @@ bgp_attr_unintern (struct attr **attr) *attr = NULL; } - /* aspath refcount shoud be decrement. */ - if (aspath) - aspath_unintern (&aspath); - if (community) - community_unintern (&community); - if (ecommunity) - ecommunity_unintern (&ecommunity); - if (cluster) - cluster_unintern (cluster); - if (transit) - transit_unintern (transit); + bgp_attr_unintern_sub (&tmp); } void @@ -683,8 +697,69 @@ bgp_attr_flush (struct attr *attr) } } +/* Implement draft-scudder-idr-optional-transitive behaviour and + * avoid resetting sessions for malformed attributes which are + * are partial/optional and hence where the error likely was not + * introduced by the sending neighbour. + */ +static bgp_attr_parse_ret_t +bgp_attr_malformed (struct peer *peer, u_char type, u_char flag, + u_char subcode, u_char *startp, bgp_size_t length) +{ + /* Only relax error handling for eBGP peers */ + if (peer_sort (peer) != BGP_PEER_EBGP) + { + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, subcode, + startp, length); + return BGP_ATTR_PARSE_ERROR; + + } + + switch (type) { + /* where an optional attribute is inconsequential, e.g. it does not affect + * route selection, and can be safely ignored then any such attributes + * which are malformed should just be ignored and the route processed as + * normal. + */ + case BGP_ATTR_AS4_AGGREGATOR: + case BGP_ATTR_AGGREGATOR: + case BGP_ATTR_ATOMIC_AGGREGATE: + return BGP_ATTR_PARSE_PROCEED; + + /* Core attributes, particularly ones which may influence route + * selection should always cause session resets + */ + case BGP_ATTR_ORIGIN: + case BGP_ATTR_AS_PATH: + case BGP_ATTR_NEXT_HOP: + case BGP_ATTR_MULTI_EXIT_DISC: + case BGP_ATTR_LOCAL_PREF: + case BGP_ATTR_COMMUNITIES: + case BGP_ATTR_ORIGINATOR_ID: + case BGP_ATTR_CLUSTER_LIST: + case BGP_ATTR_MP_REACH_NLRI: + case BGP_ATTR_MP_UNREACH_NLRI: + case BGP_ATTR_EXT_COMMUNITIES: + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, subcode, + startp, length); + return BGP_ATTR_PARSE_ERROR; + } + + /* Partial optional attributes that are malformed should not cause + * the whole session to be reset. Instead treat it as a withdrawal + * of the routes, if possible. + */ + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS) + && CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) + && CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + return BGP_ATTR_PARSE_WITHDRAW; + + /* default to reset */ + return BGP_ATTR_PARSE_ERROR; +} + /* Get origin attribute of the update message. */ -static int +static bgp_attr_parse_ret_t bgp_attr_origin (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag, u_char *startp) { @@ -702,11 +777,9 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Origin attribute flag isn't transitive %d", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* If any recognized attribute has Attribute Length that conflicts @@ -718,10 +791,9 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Origin attribute length is not one %d", length); - bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + startp, total); } /* Fetch origin attribute. */ @@ -736,12 +808,9 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Origin attribute value is invalid %d", attr->origin); - - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_INVAL_ORIGIN, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + BGP_NOTIFY_UPDATE_INVAL_ORIGIN, + startp, total); } /* Set oring attribute flag. */ @@ -766,11 +835,9 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "As-Path attribute flag isn't transitive %d", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* @@ -783,21 +850,22 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, /* In case of IBGP, length will be zero. */ if (! attr->aspath) { - zlog (peer->log, LOG_ERR, "Malformed AS path length is %d", length); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MAL_AS_PATH); - return -1; + zlog (peer->log, LOG_ERR, + "Malformed AS path from %s, length is %d", + peer->host, length); + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_MAL_AS_PATH, + NULL, 0); } /* Set aspath attribute flag. */ attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS_PATH); - return 0; + return BGP_ATTR_PARSE_PROCEED; } -static int bgp_attr_aspath_check( struct peer *peer, - struct attr *attr) +static bgp_attr_parse_ret_t +bgp_attr_aspath_check (struct peer *peer, struct attr *attr, u_char flag) { /* These checks were part of bgp_attr_aspath, but with * as4 we should to check aspath things when @@ -816,10 +884,9 @@ static int bgp_attr_aspath_check( struct peer *peer, (peer_sort (peer) == BGP_PEER_EBGP && aspath_confed_check (attr->aspath))) { zlog (peer->log, LOG_ERR, "Malformed AS path from %s", peer->host); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MAL_AS_PATH); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_MAL_AS_PATH, + NULL, 0); } /* First AS check for EBGP. */ @@ -830,10 +897,9 @@ static int bgp_attr_aspath_check( struct peer *peer, { zlog (peer->log, LOG_ERR, "%s incorrect first AS (must be %u)", peer->host, peer->as); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MAL_AS_PATH); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_MAL_AS_PATH, + NULL, 0); } } @@ -847,27 +913,53 @@ static int bgp_attr_aspath_check( struct peer *peer, attr->aspath = aspath_intern (aspath); } - return 0; - + return BGP_ATTR_PARSE_PROCEED; } /* Parse AS4 path information. This function is another wrapper of aspath_parse. */ static int -bgp_attr_as4_path (struct peer *peer, bgp_size_t length, - struct attr *attr, struct aspath **as4_path) +bgp_attr_as4_path (struct peer *peer, bgp_size_t length, + struct attr *attr, u_char flag, u_char *startp, + struct aspath **as4_path) { + bgp_size_t total; + + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + + /* Flag check. */ + if (!CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) + || !CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + { + zlog (peer->log, LOG_ERR, + "As4-Path attribute flag isn't optional/transitive %d", flag); + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + } + *as4_path = aspath_parse (peer->ibuf, length, 1); + /* In case of IBGP, length will be zero. */ + if (!*as4_path) + { + zlog (peer->log, LOG_ERR, + "Malformed AS4 path from %s, length is %d", + peer->host, length); + return bgp_attr_malformed (peer, BGP_ATTR_AS4_PATH, flag, + BGP_NOTIFY_UPDATE_MAL_AS_PATH, + NULL, 0); + } + /* Set aspath attribute flag. */ if (as4_path) attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Nexthop attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_nexthop (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag, u_char *startp) { @@ -881,11 +973,9 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Origin attribute flag isn't transitive %d", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* Check nexthop attribute length. */ @@ -894,21 +984,19 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, zlog (peer->log, LOG_ERR, "Nexthop attribute length isn't four [%d]", length); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + startp, total); } attr->nexthop.s_addr = stream_get_ipv4 (peer->ibuf); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* MED atrribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_med (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag, u_char *startp) { @@ -921,23 +1009,21 @@ bgp_attr_med (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "MED attribute length isn't four [%d]", length); - - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); - return -1; + + return bgp_attr_malformed (peer, BGP_ATTR_MULTI_EXIT_DISC, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + startp, total); } attr->med = stream_getl (peer->ibuf); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_MULTI_EXIT_DISC); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Local preference attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_local_pref (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag) { @@ -947,7 +1033,7 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, if (peer_sort (peer) == BGP_PEER_EBGP) { stream_forward_getp (peer->ibuf, length); - return 0; + return BGP_ATTR_PARSE_PROCEED; } if (length == 4) @@ -958,7 +1044,7 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, /* Set atomic aggregate flag. */ attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_LOCAL_PREF); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Atomic aggregate. */ @@ -970,16 +1056,15 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Bad atomic aggregate length %d", length); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + NULL, 0); } /* Set atomic aggregate flag. */ attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_ATOMIC_AGGREGATE); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Aggregator attribute */ @@ -991,17 +1076,16 @@ bgp_attr_aggregator (struct peer *peer, bgp_size_t length, struct attr_extra *attre = bgp_attr_extra_get (attr); /* peer with AS4 will send 4 Byte AS, peer without will send 2 Byte */ - if ( CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV ) ) + if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)) wantedlen = 8; if (length != wantedlen) { zlog (peer->log, LOG_ERR, "Aggregator length is not %d [%d]", wantedlen, length); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + NULL, 0); } if ( CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV ) ) @@ -1013,36 +1097,35 @@ bgp_attr_aggregator (struct peer *peer, bgp_size_t length, /* Set atomic aggregate flag. */ attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AGGREGATOR); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* New Aggregator attribute */ -static int +static bgp_attr_parse_ret_t bgp_attr_as4_aggregator (struct peer *peer, bgp_size_t length, - struct attr *attr, as_t *as4_aggregator_as, + struct attr *attr, u_char flag, + as_t *as4_aggregator_as, struct in_addr *as4_aggregator_addr) { if (length != 8) { zlog (peer->log, LOG_ERR, "New Aggregator length is not 8 [%d]", length); - - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AS4_AGGREGATOR, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + NULL, 0); } *as4_aggregator_as = stream_getl (peer->ibuf); as4_aggregator_addr->s_addr = stream_get_ipv4 (peer->ibuf); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS4_AGGREGATOR); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Munge Aggregator and New-Aggregator, AS_PATH and NEW_AS_PATH. */ -static int -bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, +static bgp_attr_parse_ret_t +bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, u_char flag, struct aspath *as4_path, as_t as4_aggregator, struct in_addr *as4_aggregator_addr) { @@ -1050,7 +1133,7 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, struct aspath *newpath; struct attr_extra *attre = attr->extra; - if ( CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV) ) + if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)) { /* peer can do AS4, so we ignore AS4_PATH and AS4_AGGREGATOR * if given. @@ -1068,11 +1151,11 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, peer->host, "AS4 capable peer, yet it sent"); } - return 0; + return BGP_ATTR_PARSE_PROCEED; } - if (attr->flag & ( ATTR_FLAG_BIT( BGP_ATTR_AS4_PATH)) - && !(attr->flag & ( ATTR_FLAG_BIT( BGP_ATTR_AS_PATH)))) + if (attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH)) + && !(attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS_PATH)))) { /* Hu? This is not supposed to happen at all! * got as4_path and no aspath, @@ -1084,10 +1167,9 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, zlog (peer->log, LOG_ERR, "%s BGP not AS4 capable peer sent AS4_PATH but" " no AS_PATH, cant do anything here", peer->host); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MAL_ATTR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_MAL_ATTR, + NULL, 0); } /* We have a asn16 peer. First, look for AS4_AGGREGATOR @@ -1095,7 +1177,7 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, */ if (attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS4_AGGREGATOR) ) ) { - if ( attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AGGREGATOR) ) ) + if (attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AGGREGATOR) ) ) { assert (attre); @@ -1111,7 +1193,7 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, * Aggregating node and the AS_PATH is to be * constructed "as in all other cases" */ - if ( attre->aggregator_as != BGP_AS_TRANS ) + if (attre->aggregator_as != BGP_AS_TRANS) { /* ignore */ if ( BGP_DEBUG(as4, AS4)) @@ -1146,24 +1228,27 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, } /* need to reconcile NEW_AS_PATH and AS_PATH */ - if ( !ignore_as4_path && (attr->flag & ( ATTR_FLAG_BIT( BGP_ATTR_AS4_PATH))) ) + 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); attr->aspath = aspath_intern (newpath); } - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Community attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_community (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag) + struct attr *attr, u_char flag, u_char *startp) { + bgp_size_t total + = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + if (length == 0) { attr->community = NULL; - return 0; + return BGP_ATTR_PARSE_PROCEED; } attr->community = @@ -1173,15 +1258,17 @@ bgp_attr_community (struct peer *peer, bgp_size_t length, stream_forward_getp (peer->ibuf, length); if (!attr->community) - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_COMMUNITIES, flag, + BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, + startp, total); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_COMMUNITIES); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Originator ID attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_originator_id (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag) { @@ -1189,10 +1276,9 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Bad originator ID length %d", length); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ORIGINATOR_ID, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + NULL, 0); } (bgp_attr_extra_get (attr))->originator_id.s_addr @@ -1200,11 +1286,11 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_ORIGINATOR_ID); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Cluster list attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag) { @@ -1213,20 +1299,20 @@ bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Bad cluster list length %d", length); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_CLUSTER_LIST, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + NULL, 0); } (bgp_attr_extra_get (attr))->cluster = cluster_parse ((struct in_addr *)stream_pnt (peer->ibuf), length); - - stream_forward_getp (peer->ibuf, length);; + + /* XXX: Fix cluster_parse to use stream API and then remove this */ + stream_forward_getp (peer->ibuf, length); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_CLUSTER_LIST); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Multiprotocol reachability information parse. */ @@ -1253,7 +1339,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, { zlog_info ("%s: %s sent invalid length, %lu", __func__, peer->host, (unsigned long)length); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* Load AFI, SAFI. */ @@ -1267,7 +1353,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, { zlog_info ("%s: %s, MP nexthop length, %u, goes past end of attribute", __func__, peer->host, attre->mp_nexthop_len); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* Nexthop length check. */ @@ -1315,14 +1401,14 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, default: zlog_info ("%s: (%s) Wrong multiprotocol next hop length: %d", __func__, peer->host, attre->mp_nexthop_len); - return -1; + return BGP_ATTR_PARSE_ERROR; } if (!LEN_LEFT) { zlog_info ("%s: (%s) Failed to read SNPA and NLRI(s)", __func__, peer->host); - return -1; + return BGP_ATTR_PARSE_ERROR; } { @@ -1338,7 +1424,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, { zlog_info ("%s: (%s) Failed to read NLRI", __func__, peer->host); - return -1; + return BGP_ATTR_PARSE_ERROR; } if (safi != BGP_SAFI_VPNV4) @@ -1348,7 +1434,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, { zlog_info ("%s: (%s) NLRI doesn't pass sanity check", __func__, peer->host); - return -1; + return BGP_ATTR_PARSE_ERROR; } } @@ -1359,7 +1445,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, stream_forward_getp (s, nlri_len); - return 0; + return BGP_ATTR_PARSE_PROCEED; #undef LEN_LEFT } @@ -1378,7 +1464,7 @@ bgp_mp_unreach_parse (struct peer *peer, bgp_size_t length, #define BGP_MP_UNREACH_MIN_SIZE 3 if ((length > STREAM_READABLE(s)) || (length < BGP_MP_UNREACH_MIN_SIZE)) - return -1; + return BGP_ATTR_PARSE_ERROR; afi = stream_getw (s); safi = stream_getc (s); @@ -1389,7 +1475,7 @@ bgp_mp_unreach_parse (struct peer *peer, bgp_size_t length, { ret = bgp_nlri_sanity_check (peer, afi, stream_pnt (s), withdraw_len); if (ret < 0) - return -1; + return BGP_ATTR_PARSE_ERROR; } mp_withdraw->afi = afi; @@ -1399,20 +1485,23 @@ bgp_mp_unreach_parse (struct peer *peer, bgp_size_t length, stream_forward_getp (s, withdraw_len); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Extended Community attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_ext_communities (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag) + struct attr *attr, u_char flag, u_char *startp) { + bgp_size_t total + = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + if (length == 0) { if (attr->extra) attr->extra->ecommunity = NULL; /* Empty extcomm doesn't seem to be invalid per se */ - return 0; + return BGP_ATTR_PARSE_PROCEED; } (bgp_attr_extra_get (attr))->ecommunity = @@ -1421,15 +1510,17 @@ bgp_attr_ext_communities (struct peer *peer, bgp_size_t length, stream_forward_getp (peer->ibuf, length); if (!attr->extra->ecommunity) - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_EXT_COMMUNITIES, + flag, BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, + startp, total); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_EXT_COMMUNITIES); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* BGP unknown attribute treatment. */ -static int +static bgp_attr_parse_ret_t bgp_attr_unknown (struct peer *peer, struct attr *attr, u_char flag, u_char type, bgp_size_t length, u_char *startp) { @@ -1455,20 +1546,17 @@ bgp_attr_unknown (struct peer *peer, struct attr *attr, u_char flag, then the Error Subcode is set to Unrecognized Well-known Attribute. The Data field contains the unrecognized attribute (type, length and value). */ - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + if (!CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) { - /* Adjust startp to do not include flag value. */ - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_UNREC_ATTR, - startp, total); - return -1; + return bgp_attr_malformed (peer, type, flag, + BGP_NOTIFY_UPDATE_UNREC_ATTR, + startp, total); } /* Unrecognized non-transitive optional attributes must be quietly ignored and not passed along to other BGP peers. */ if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - return 0; + return BGP_ATTR_PARSE_PROCEED; /* If a path with recognized transitive optional attribute is accepted and passed along to other BGP peers and the Partial bit @@ -1491,17 +1579,17 @@ bgp_attr_unknown (struct peer *peer, struct attr *attr, u_char flag, memcpy (transit->val + transit->length, startp, total); transit->length += total; - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Read attribute of update packet. This function is called from bgp_update() in bgpd.c. */ -int +bgp_attr_parse_ret_t bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, struct bgp_nlri *mp_update, struct bgp_nlri *mp_withdraw) { int ret; - u_char flag; + u_char flag = 0; u_char type = 0; bgp_size_t length; u_char *startp, *endp; @@ -1518,7 +1606,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, /* End pointer of BGP attribute. */ endp = BGP_INPUT_PNT (peer) + size; - + /* Get attributes to the end of attribute length. */ while (BGP_INPUT_PNT (peer) < endp) { @@ -1534,7 +1622,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* Fetch attribute flag and type. */ @@ -1554,7 +1642,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* Check extended attribue length bit. */ @@ -1576,7 +1664,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_MAL_ATTR); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* Set type to bitmap to check duplicate attribute. `type' is @@ -1594,7 +1682,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* OK check attribute and store it's value. */ @@ -1607,7 +1695,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_aspath (peer, length, attr, flag, startp); break; case BGP_ATTR_AS4_PATH: - ret = bgp_attr_as4_path (peer, length, attr, &as4_path ); + ret = bgp_attr_as4_path (peer, length, attr, flag, startp, &as4_path); break; case BGP_ATTR_NEXT_HOP: ret = bgp_attr_nexthop (peer, length, attr, flag, startp); @@ -1625,10 +1713,12 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_aggregator (peer, length, attr, flag); break; case BGP_ATTR_AS4_AGGREGATOR: - ret = bgp_attr_as4_aggregator (peer, length, attr, &as4_aggregator, &as4_aggregator_addr); + ret = bgp_attr_as4_aggregator (peer, length, attr, flag, + &as4_aggregator, + &as4_aggregator_addr); break; case BGP_ATTR_COMMUNITIES: - ret = bgp_attr_community (peer, length, attr, flag); + ret = bgp_attr_community (peer, length, attr, flag, startp); break; case BGP_ATTR_ORIGINATOR_ID: ret = bgp_attr_originator_id (peer, length, attr, flag); @@ -1643,26 +1733,39 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_mp_unreach_parse (peer, length, mp_withdraw); break; case BGP_ATTR_EXT_COMMUNITIES: - ret = bgp_attr_ext_communities (peer, length, attr, flag); + ret = bgp_attr_ext_communities (peer, length, attr, flag, startp); break; default: ret = bgp_attr_unknown (peer, attr, flag, type, length, startp); break; } - - /* If error occured immediately return to the caller. */ - if (ret < 0) + + /* If hard error occured immediately return to the caller. */ + if (ret == BGP_ATTR_PARSE_ERROR) { zlog (peer->log, LOG_WARNING, "%s: Attribute %s, parse error", peer->host, LOOKUP (attr_str, type)); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MAL_ATTR); - return ret; + bgp_notify_send (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_MAL_ATTR); + if (as4_path) + aspath_unintern (&as4_path); + return ret; } - + if (ret == BGP_ATTR_PARSE_WITHDRAW) + { + + zlog (peer->log, LOG_WARNING, + "%s: Attribute %s, parse error - treating as withdrawal", + peer->host, + LOOKUP (attr_str, type)); + if (as4_path) + aspath_unintern (&as4_path); + return ret; + } + /* Check the fetched length. */ if (BGP_INPUT_PNT (peer) != attr_endp) { @@ -1672,7 +1775,9 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + if (as4_path) + aspath_unintern (&as4_path); + return BGP_ATTR_PARSE_ERROR; } } @@ -1685,7 +1790,9 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + if (as4_path) + aspath_unintern (&as4_path); + return BGP_ATTR_PARSE_ERROR; } /* @@ -1699,16 +1806,20 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, * all attributes first, including these 32bit ones, and now, * afterwards, we look what and if something is to be done for as4. */ - if (bgp_attr_munge_as4_attrs (peer, attr, as4_path, + if (bgp_attr_munge_as4_attrs (peer, attr, flag, as4_path, as4_aggregator, &as4_aggregator_addr)) - return -1; + { + if (as4_path) + aspath_unintern (&as4_path); + return BGP_ATTR_PARSE_ERROR; + } /* 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 ) + 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 @@ -1725,10 +1836,10 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, * Finally do the checks on the aspath we did not do yet * because we waited for a potentially synthesized aspath. */ - if ( attr->flag & ( ATTR_FLAG_BIT( BGP_ATTR_AS_PATH))) + if (attr->flag & (ATTR_FLAG_BIT(BGP_ATTR_AS_PATH))) { - ret = bgp_attr_aspath_check( peer, attr ); - if ( ret < 0 ) + ret = bgp_attr_aspath_check (peer, attr, flag); + if (ret != BGP_ATTR_PARSE_PROCEED) return ret; } @@ -1736,7 +1847,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, if (attr->extra && attr->extra->transit) attr->extra->transit = transit_intern (attr->extra->transit); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Well-known attribute check. */ @@ -1767,9 +1878,9 @@ bgp_attr_check (struct peer *peer, struct attr *attr) BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_MISS_ATTR, &type, 1); - return -1; + return BGP_ATTR_PARSE_ERROR; } - return 0; + return BGP_ATTR_PARSE_PROCEED; } int stream_put_prefix (struct stream *, struct prefix *); 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); diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index dee0b51f..8de78c75 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1589,26 +1589,47 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) BGP_NOTIFY_UPDATE_MAL_ATTR); return -1; } + + /* Certain attribute parsing errors should not be considered bad enough + * to reset the session for, most particularly any partial/optional + * attributes that have 'tunneled' over speakers that don't understand + * them. Instead we withdraw only the prefix concerned. + * + * Complicates the flow a little though.. + */ + bgp_attr_parse_ret_t attr_parse_ret = BGP_ATTR_PARSE_PROCEED; + /* This define morphs the update case into a withdraw when lower levels + * have signalled an error condition where this is best. + */ +#define NLRI_ATTR_ARG (attr_parse_ret != BGP_ATTR_PARSE_WITHDRAW ? &attr : NULL) /* Parse attribute when it exists. */ if (attribute_len) { - ret = bgp_attr_parse (peer, &attr, attribute_len, + attr_parse_ret = bgp_attr_parse (peer, &attr, attribute_len, &mp_update, &mp_withdraw); - if (ret < 0) + if (attr_parse_ret == BGP_ATTR_PARSE_ERROR) return -1; } - + /* Logging the attribute. */ - if (BGP_DEBUG (update, UPDATE_IN)) + if (attr_parse_ret == BGP_ATTR_PARSE_WITHDRAW + || BGP_DEBUG (update, UPDATE_IN)) { ret= bgp_dump_attr (peer, &attr, attrstr, BUFSIZ); + int lvl = (attr_parse_ret == BGP_ATTR_PARSE_WITHDRAW) + ? LOG_ERR : LOG_DEBUG; + + if (attr_parse_ret == BGP_ATTR_PARSE_WITHDRAW) + zlog (peer->log, LOG_ERR, + "%s rcvd UPDATE with errors in attr(s)!! Withdrawing route.", + peer->host); if (ret) - zlog (peer->log, LOG_DEBUG, "%s rcvd UPDATE w/ attr: %s", + zlog (peer->log, lvl, "%s rcvd UPDATE w/ attr: %s", peer->host, attrstr); } - + /* Network Layer Reachability Information. */ update_len = end - stream_pnt (s); @@ -1617,7 +1638,12 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) /* Check NLRI packet format and prefix length. */ ret = bgp_nlri_sanity_check (peer, AFI_IP, stream_pnt (s), update_len); if (ret < 0) - return -1; + { + bgp_attr_unintern_sub (&attr); + if (attr.extra) + bgp_attr_extra_free (&attr); + return -1; + } /* Set NLRI portion to structure. */ update.afi = AFI_IP; @@ -1640,15 +1666,20 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) update. */ ret = bgp_attr_check (peer, &attr); if (ret < 0) - return -1; + { + bgp_attr_unintern_sub (&attr); + if (attr.extra) + bgp_attr_extra_free (&attr); + return -1; + } - bgp_nlri_parse (peer, &attr, &update); + bgp_nlri_parse (peer, NLRI_ATTR_ARG, &update); } if (mp_update.length && mp_update.afi == AFI_IP && mp_update.safi == SAFI_UNICAST) - bgp_nlri_parse (peer, &attr, &mp_update); + bgp_nlri_parse (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP @@ -1675,7 +1706,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (mp_update.length && mp_update.afi == AFI_IP && mp_update.safi == SAFI_MULTICAST) - bgp_nlri_parse (peer, &attr, &mp_update); + bgp_nlri_parse (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP @@ -1705,7 +1736,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (mp_update.length && mp_update.afi == AFI_IP6 && mp_update.safi == SAFI_UNICAST) - bgp_nlri_parse (peer, &attr, &mp_update); + bgp_nlri_parse (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP6 @@ -1734,7 +1765,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (mp_update.length && mp_update.afi == AFI_IP6 && mp_update.safi == SAFI_MULTICAST) - bgp_nlri_parse (peer, &attr, &mp_update); + bgp_nlri_parse (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP6 @@ -1762,7 +1793,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (mp_update.length && mp_update.afi == AFI_IP && mp_update.safi == BGP_SAFI_VPNV4) - bgp_nlri_parse_vpnv4 (peer, &attr, &mp_update); + bgp_nlri_parse_vpnv4 (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP @@ -1784,21 +1815,10 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) /* Everything is done. We unintern temporary structures which interned in bgp_attr_parse(). */ - if (attr.aspath) - aspath_unintern (&attr.aspath); - if (attr.community) - community_unintern (&attr.community); + bgp_attr_unintern_sub (&attr); if (attr.extra) - { - if (attr.extra->ecommunity) - ecommunity_unintern (&attr.extra->ecommunity); - if (attr.extra->cluster) - cluster_unintern (attr.extra->cluster); - if (attr.extra->transit) - transit_unintern (attr.extra->transit); - bgp_attr_extra_free (&attr); - } - + bgp_attr_extra_free (&attr); + /* If peering is stopped due to some reason, do not generate BGP event. */ if (peer->status != Established) diff --git a/tests/aspath_test.c b/tests/aspath_test.c index ade60c67..4a2ce9aa 100644 --- a/tests/aspath_test.c +++ b/tests/aspath_test.c @@ -408,7 +408,7 @@ static struct test_segment { "#ASNs = 0, data = seq(8466 3 52737 4096 3456)", { 0x2,0x0, 0x21,0x12, 0x00,0x03, 0xce,0x01, 0x10,0x00, 0x0d,0x80 }, 12, - { "", "", + { NULL, NULL, 0, 0, 0, 0, 0, 0 }, }, { /* 26 */ @@ -418,7 +418,7 @@ static struct test_segment { 0x2,0x2, 0x10,0x00, 0x0d,0x80 }, 14 , - { "", "", + { NULL, NULL, 0, 0, 0, 0, 0, 0 }, }, { /* 27 */ @@ -427,7 +427,7 @@ static struct test_segment { { 0x8,0x2, 0x10,0x00, 0x0d,0x80 }, 14 , - { "", "", + { NULL, NULL, 0, 0, 0, 0, 0, 0 }, }, { NULL, NULL, {0}, 0, { NULL, 0, 0 } } }; @@ -869,6 +869,12 @@ validate (struct aspath *as, const struct test_spec *sp) static struct stream *s; struct aspath *asinout, *asconfeddel, *asstr, *as4; + if (as == NULL && sp->shouldbe == NULL) + { + printf ("Correctly failed to parse\n"); + return fails; + } + out = aspath_snmp_pathseg (as, &bytes); asinout = make_aspath (out, bytes, 0); @@ -1013,7 +1019,7 @@ parse_test (struct test_segment *t) printf ("%s: %s\n", t->name, t->desc); asp = make_aspath (t->asdata, t->len, 0); - + printf ("aspath: %s\nvalidating...:\n", aspath_print (asp)); if (!validate (asp, &t->sp)) @@ -1022,7 +1028,9 @@ parse_test (struct test_segment *t) printf (FAILED "\n"); printf ("\n"); - aspath_unintern (asp); + + if (asp) + aspath_unintern (asp); } /* prepend testing */ @@ -1078,7 +1086,8 @@ empty_prepend_test (struct test_segment *t) printf (FAILED "!\n"); printf ("\n"); - aspath_unintern (asp1); + if (asp1) + aspath_unintern (asp1); aspath_free (asp2); } -- cgit v1.2.1