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

1 files changed, 16 insertions, 10 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);