[bgpd] review 32-bit AS-path hotfix for 0.99.12
The patch by Chris Caputo, which was used to prepare 0.99.12
release, consists of three parts:
1. memory allocation fix itself
2. fix for warnings about constant variables
3. fix for printf format specs (%d was used instead of %u)
It was confirmed later, that:
a. a much simpler bugfix was available for memory allocation
b. committed version of the bugfix wasn't optimal CPU-wise
At this point I consider reasonable to revert the allocation
portion of that patch and to replace it with the shorter
version, which is:
-#define ASN_STR_LEN (5 + 1)
+#define ASN_STR_LEN (10 + 1)
Other two parts of Mr. Caputo's patch remain intact.
diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c
index 5881abe..002fff9 100644
--- a/bgpd/bgp_aspath.c
+++ b/bgpd/bgp_aspath.c
@@ -389,6 +389,25 @@
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)
{
@@ -498,21 +517,6 @@
return num;
}
-static void
-aspath_make_str_big_enough (int len,
- char **str_buf,
- int *str_size,
- int count_to_be_added)
-{
-#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
-}
-
/* Convert aspath structure to string expression. */
static char *
aspath_make_str_count (struct aspath *as)
@@ -532,7 +536,18 @@
seg = as->segments;
- str_size = ASPATH_STR_DEFAULT_LEN;
+ /* ASN takes 5 to 10 chars 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.
+ *
+ * This was changed to 10 after the well-known BGP assertion, which
+ * had hit some parts of the Internet in May of 2009.
+ */
+#define ASN_STR_LEN (10 + 1)
+ str_size = MAX (assegment_count_asns (seg, 0) * ASN_STR_LEN + 2 + 1,
+ ASPATH_STR_DEFAULT_LEN);
str_buf = XMALLOC (MTYPE_AS_STR, str_size);
while (seg)
@@ -556,24 +571,32 @@
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 10 chars for ASN, a seperator each and
+ * potentially two segment delimiters, plus a space between each
+ * segment and trailing zero.
+ *
+ * This definitely didn't work with the value of 5 bytes and
+ * 32-bit ASNs.
+ */
+#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)
- {
- aspath_make_str_big_enough (len, &str_buf, &str_size, 1); /* %c */
- len += snprintf (str_buf + len, str_size - len,
- "%c",
- aspath_delimiter_char (seg->type, AS_SEG_START));
- }
+ 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_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);
-
len += snprintf (str_buf + len, str_size - len, "%u", seg->as[i]);
if (i < (seg->length - 1))