[quagga-users 10575] Re: update on bgpd AS4 crashes

Chris Caputo ccaputo at alt.net
Thu Apr 30 21:35:24 IST 2009


On Thu, 30 Apr 2009, Mo Shivji wrote:
> > Hello, all.
> >
> > There is an issue with Quagga 0.99.11 bgpd crashing on today's version
> > of Internet, and I confirm, that the cause of the problem is in Quagga
> > code. A patch by Chris Caputo, which addresses this specific
> > issue, has been available since February, and today I have committed it
> > into the main development repository. This patch can be applied to the
> > source tree of 0.99.11 release, and it seems to be the best hotfix we
> > have today. Try it and let me know, if it helped or not. Below is an
> > URL for your convenience:
> 
> Will this patch work with 0.99.10 ?

I've put up an isolated patch for the 4-byte ASN "len < str_size" assert 
problem.  Patches and compiles cleanly against 0.99.10 and 0.99.11.  Have 
not runtime tested it, but it should work fine.:

 https://www.caputo.com/foss/quagga-0.99.10-BGP-4-byte-ASN-bug-fixes.patch 
 https://www.caputo.com/foss/quagga-0.99.11-BGP-4-byte-ASN-bug-fixes.patch

 (the patches are identical.  naming is just for clarity.)

The patch is also copied below.

(This is not intended for the git repository.  Separate patches have been 
submitted for that.)

Chris

--- quagga-0.99.10-stock/bgpd/bgp_aspath.c	2008-04-10 11:47:45.000000000 +0000
+++ quagga-0.99.10/bgpd/bgp_aspath.c	2009-04-30 20:12:22.000000000 +0000
@@ -393,25 +393,6 @@ aspath_delimiter_char (u_char type, u_ch
   return ' ';
 }
 
-/* countup asns from this segment and index onward */
-static int
-assegment_count_asns (struct assegment *seg, int from)
-{
-  int count = 0;
-  while (seg)
-    {
-      if (!from)
-        count += seg->length;
-      else
-        {
-          count += (seg->length - from);
-          from = 0;
-        }
-      seg = seg->next;
-    }
-  return count;
-}
-
 unsigned int
 aspath_count_confeds (struct aspath *aspath)
 {
@@ -521,13 +502,23 @@ aspath_count_numas (struct aspath *aspat
   return num;
 }
 
+static char *
+aspath_expand (char *path,
+	       size_t *space,
+	       size_t needed)
+{
+  while (*space < needed)
+    *space *= 2;
+  return XREALLOC (MTYPE_AS_STR, path, *space);
+}
+
 /* Convert aspath structure to string expression. */
 static char *
 aspath_make_str_count (struct aspath *as)
 {
   struct assegment *seg;
-  int str_size;
-  int len = 0;
+  size_t str_size;
+  size_t len = 0;
   char *str_buf;
 
   /* Empty aspath. */
@@ -540,18 +531,7 @@ aspath_make_str_count (struct aspath *as
   
   seg = as->segments;
   
-  /* ASN takes 5 chars at least, plus seperator, see below.
-   * If there is one differing segment type, we need an additional
-   * 2 chars for segment delimiters, and the final '\0'.
-   * Hopefully this is large enough to avoid hitting the realloc
-   * code below for most common sequences.
-   *
-   * With 32bit ASNs, this range will increase, but only worth changing
-   * once there are significant numbers of ASN >= 100000
-   */
-#define ASN_STR_LEN (5 + 1)
-  str_size = MAX (assegment_count_asns (seg, 0) * ASN_STR_LEN + 2 + 1,
-                  ASPATH_STR_DEFAULT_LEN);
+  str_size = ASPATH_STR_DEFAULT_LEN;
   str_buf = XMALLOC (MTYPE_AS_STR, str_size);
 
   while (seg)
@@ -575,32 +555,25 @@ aspath_make_str_count (struct aspath *as
             return NULL;
         }
       
-      /* We might need to increase str_buf, particularly if path has
-       * differing segments types, our initial guesstimate above will
-       * have been wrong.  need 5 chars for ASN, a seperator each and
-       * potentially two segment delimiters, plus a space between each
-       * segment and trailing zero.
-       *
-       * This may need to revised if/when significant numbers of
-       * ASNs >= 100000 are assigned and in-use on the internet...
-       */
-#define SEGMENT_STR_LEN(X) (((X)->length * ASN_STR_LEN) + 2 + 1 + 1)
-      if ( (len + SEGMENT_STR_LEN(seg)) > str_size)
-        {
-          str_size = len + SEGMENT_STR_LEN(seg);
-          str_buf = XREALLOC (MTYPE_AS_STR, str_buf, str_size);
-        }
-#undef ASN_STR_LEN
-#undef SEGMENT_STR_LEN
-      
       if (seg->type != AS_SEQUENCE)
-        len += snprintf (str_buf + len, str_size - len, 
-			 "%c", 
-                         aspath_delimiter_char (seg->type, AS_SEG_START));
+	{
+	  str_buf = aspath_expand (str_buf,
+				   &str_size,
+				   len + 2); /* %c + '\0' */
+	  len += snprintf (str_buf + len, str_size - len, 
+			   "%c", 
+			   aspath_delimiter_char (seg->type, AS_SEG_START));
+	}
       
       /* write out the ASNs, with their seperators, bar the last one*/
       for (i = 0; i < seg->length; i++)
         {
+#define APPROX_DIGIT_COUNT(x) (x < 100000U ? 5 : 10)
+	  /* %u + %c + %c + " " + '\0' (last two are below loop) */
+	  str_buf = aspath_expand (str_buf,
+				   &str_size,
+				   len + APPROX_DIGIT_COUNT(seg->as[i]) + 4);
+				   
           len += snprintf (str_buf + len, str_size - len, "%u", seg->as[i]);
           
           if (i < (seg->length - 1))


More information about the Quagga-users mailing list