[quagga-dev 6535] Re: update on bgpd AS4 crashes

[Chris Caputo]

On Thu, 30 Apr 2009, Stephen Hemminger wrote:
> On Thu, 30 Apr 2009 17:44:54 +0400 Denis Ovsienko <pilot at etcnet.org> 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:
> > 
> > http://code.quagga.net/cgi-bin/gitweb.cgi?p=quagga.git;a=commitdiff_plain;h=aea339f72807c34a7916d8614e030069815e144c
> > 
> > This issue will get further treatment after necessary research.
> > Thank you.
> > _______________________________________________
> > Quagga-dev mailing list
> > Quagga-dev at lists.quagga.net
> > http://lists.quagga.net/mailman/listinfo/quagga-dev
> 
> Thank you for fixing the bug but this looks like one of those ugly
> maintenance patches, not like the original code.
> 
>   1. Multiple fixes should not have been rolled into one patch. The unsigned
>      print arguments should be separate
>   2. #define TERMINATOR 1 is unhelpful ugliness
>   3. it is cleaner to pass value than to use pass by reference
>   4. better naming
>   5. don't loop reallocing, just get size right first
>   6. don't use int when should be using size_t or int
>   7. put size math in caller not the growth function.
> 
>     i.e
>     static char *aspath_expand(char *path, size_t *space, size_t needed)
>     {
>          while (*space < needed)
> 	     *space *= 2;
>          return XREALLOC(path, *space);
>      }

Hey Stephen,

I appreciate the feedback.

Please consider the following patch against git-current based on your 
suggestions.  (the realloc in the while loop was really silly on my part - 
thanks especially for catching that)

Chris

---

Subject: [PATCH] Cleanup previous 'BGP 4-byte ASN bug fixes' patch

* bgpd/bgp_aspath.c: (aspath_make_str_count) Incorporate suggestions
  from code review.  Fix egregious realloc() while in while loop
  inefficiency.

Signed-off-by: Chris Caputo <ccaputo at alt.net>
---
 bgpd/bgp_aspath.c |   40 ++++++++++++++++++----------------------
 1 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c
index a1e4608..9f55305 100644
--- a/bgpd/bgp_aspath.c
+++ b/bgpd/bgp_aspath.c
@@ -502,19 +502,14 @@ aspath_count_numas (struct aspath *aspath)
   return num;
 }
 
-static void
-aspath_make_str_big_enough (int len,
-			    char **str_buf,
-			    int *str_size,
-			    int count_to_be_added)
+static char *
+aspath_expand (char *path,
+	       size_t *space,
+	       size_t needed)
 {
-#define TERMINATOR 1
-  while (len + count_to_be_added + TERMINATOR > *str_size)
-    {
-      *str_size *= 2;
-      *str_buf = XREALLOC (MTYPE_AS_STR, *str_buf, *str_size);
-    }
-#undef TERMINATOR
+  while (*space < needed)
+    *space *= 2;
+  return XREALLOC (MTYPE_AS_STR, path, *space);
 }
 
 /* Convert aspath structure to string expression. */
@@ -522,8 +517,8 @@ 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. */
@@ -562,7 +557,9 @@ aspath_make_str_count (struct aspath *as)
       
       if (seg->type != AS_SEQUENCE)
 	{
-	  aspath_make_str_big_enough (len, &str_buf, &str_size, 1); /* %c */
+	  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));
@@ -571,13 +568,12 @@ aspath_make_str_count (struct aspath *as)
       /* write out the ASNs, with their seperators, bar the last one*/
       for (i = 0; i < seg->length; i++)
         {
-#define APPROX_DIG_CNT(x) (x < 100000U ? 5 : 10)
-	  /* %u + %c + %c + " " (last two are below loop) */
-	  aspath_make_str_big_enough (len,
-				      &str_buf,
-				      &str_size,
-				      APPROX_DIG_CNT(seg->as[i]) + 1 + 1 + 1);
-
+#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))
-- 
1.6.0.6