bgpd: separate lcommunity validation from tokenizer

`lcommunity_gettoken` expects a space-delimeted list of 0 or more large
communities. `lcommunity_list_valid` can perform this check.
`lcommunity_list_valid` now validates large community lists more
accurately based on the following condition: Each quantity in a standard bgp
large community must:

1. Contain at least one digit
2. Fit within 4 octets
3. Contain only digits unless the lcommunity is "expanded"
4. Contain a valid regex if the lcommunity is "expanded"

Moreover we validate that each large community list contains exactly 3
such values separated by a single colon each.

One quirk of our validation which is worth documenting is:

```
bgp large-community-list standard test2 permit 1:c:3
bgp large-community-list expanded test1 permit 1:c:3
```

The first line will throw an error complaining about a "malformed community-list
value". The second line will be accepted because the each value is each treated as
a regex when matching large communities, it simply will never match anything so
it's rather useless.

Signed-off-by: Wesley Coakley <wcoakley@nvidia.com>

bgpd/bgp_clist.c

@@ -1110,11 +1110,14 @@ struct lcommunity *lcommunity_list_match_delete(struct lcommunity *lcom,
 }
 
 /* Helper to check if every octet do not exceed UINT_MAX */
-static bool lcommunity_list_valid(const char *community)
+bool lcommunity_list_valid(const char *community, int style)
 {
 	int octets;
 	char **splits, **communities;
+	char *endptr;
 	int num, num_communities;
+	regex_t *regres;
+	int invalid = 0;
 
 	frrstr_split(community, " ", &communities, &num_communities);
 
@@ -1123,25 +1126,43 @@ static bool lcommunity_list_valid(const char *community)
 		frrstr_split(communities[j], ":", &splits, &num);
 
 		for (int i = 0; i < num; i++) {
-			if (strtoul(splits[i], NULL, 10) > UINT_MAX)
-				return false;
-
 			if (strlen(splits[i]) == 0)
-				return false;
+				/* There is no digit to check */
+				invalid++;
+
+			if (style == LARGE_COMMUNITY_LIST_STANDARD) {
+				if (*splits[i] == '-')
+					/* Must not be negative */
+					invalid++;
+				else if (strtoul(splits[i], &endptr, 10)
+					 > UINT_MAX)
+					/* Larger than 4 octets */
+					invalid++;
+				else if (*endptr)
+					/* Not all characters were digits */
+					invalid++;
+			} else {
+				regres = bgp_regcomp(communities[j]);
+				if (!regres)
+					/* malformed regex */
+					invalid++;
+				else
+					bgp_regex_free(regres);
+			}
 
 			octets++;
 			XFREE(MTYPE_TMP, splits[i]);
 		}
 		XFREE(MTYPE_TMP, splits);
 
-		if (octets < 3)
+		if (octets != 3)
-			return false;
+			invalid++;
 
 		XFREE(MTYPE_TMP, communities[j]);
 	}
 	XFREE(MTYPE_TMP, communities);
 
-	return true;
+	return (invalid > 0) ? false : true;
 }
 
 /* Set lcommunity-list.  */
@@ -1176,7 +1197,7 @@ int lcommunity_list_set(struct community_list_handler *ch, const char *name,
 	}
 
 	if (str) {
-		if (!lcommunity_list_valid(str))
+		if (!lcommunity_list_valid(str, style))
 			return COMMUNITY_LIST_ERR_MALFORMED_VAL;
 
 		if (style == LARGE_COMMUNITY_LIST_STANDARD)

bgpd/bgp_clist.h

@@ -154,6 +154,7 @@ extern int extcommunity_list_unset(struct community_list_handler *ch,
 extern int lcommunity_list_set(struct community_list_handler *ch,
 			       const char *name, const char *str,
 			       const char *seq, int direct, int style);
+extern bool lcommunity_list_valid(const char *community, int style);
 extern int lcommunity_list_unset(struct community_list_handler *ch,
 				 const char *name, const char *str,
 				 const char *seq, int direct, int style);

bgpd/bgp_lcommunity.c

@@ -348,16 +348,12 @@ void lcommunity_finish(void)
 	lcomhash = NULL;
 }
 
-/* Large Communities token enum. */
+/* Get next Large Communities token from the string.
-enum lcommunity_token {
+ * Assumes str is space-delimeted and describes 0 or more
-	lcommunity_token_unknown = 0,
+ * valid large communities
-	lcommunity_token_val,
+ */
-};
-
-/* Get next Large Communities token from the string. */
 static const char *lcommunity_gettoken(const char *str,
-				       struct lcommunity_val *lval,
+				       struct lcommunity_val *lval)
-				       enum lcommunity_token *token)
 {
 	const char *p = str;
 
@@ -372,60 +368,55 @@ static const char *lcommunity_gettoken(const char *str,
 		return NULL;
 
 	/* Community value. */
-	if (isdigit((unsigned char)*p)) {
+	int separator = 0;
-		int separator = 0;
+	int digit = 0;
-		int digit = 0;
+	uint32_t globaladmin = 0;
-		uint32_t globaladmin = 0;
+	uint32_t localdata1 = 0;
-		uint32_t localdata1 = 0;
+	uint32_t localdata2 = 0;
-		uint32_t localdata2 = 0;
 
-		while (isdigit((unsigned char)*p) || *p == ':') {
+	while (*p && *p != ' ') {
-			if (*p == ':') {
+		/* large community valid chars */
-				if (separator == 2) {
+		assert(isdigit((unsigned char)*p) || *p == ':');
-					*token = lcommunity_token_unknown;
+
-					return NULL;
+		if (*p == ':') {
-				} else {
+			separator++;
-					separator++;
+			digit = 0;
-					digit = 0;
+			if (separator == 1) {
-					if (separator == 1) {
+				globaladmin = localdata2;
-						globaladmin = localdata2;
-					} else {
-						localdata1 = localdata2;
-					}
-					localdata2 = 0;
-				}
 			} else {
-				digit = 1;
+				localdata1 = localdata2;
-				localdata2 *= 10;
-				localdata2 += (*p - '0');
 			}
-			p++;
+			localdata2 = 0;
+		} else {
+			digit = 1;
+			/* left shift the accumulated value and add current
+			 * digit
+			 */
+			localdata2 *= 10;
+			localdata2 += (*p - '0');
 		}
-		if (!digit) {
+		p++;
-			*token = lcommunity_token_unknown;
-			return NULL;
-		}
-
-		/*
-		 * Copy the large comm.
-		 */
-		lval->val[0] = (globaladmin >> 24) & 0xff;
-		lval->val[1] = (globaladmin >> 16) & 0xff;
-		lval->val[2] = (globaladmin >> 8) & 0xff;
-		lval->val[3] = globaladmin & 0xff;
-		lval->val[4] = (localdata1 >> 24) & 0xff;
-		lval->val[5] = (localdata1 >> 16) & 0xff;
-		lval->val[6] = (localdata1 >> 8) & 0xff;
-		lval->val[7] = localdata1 & 0xff;
-		lval->val[8] = (localdata2 >> 24) & 0xff;
-		lval->val[9] = (localdata2 >> 16) & 0xff;
-		lval->val[10] = (localdata2 >> 8) & 0xff;
-		lval->val[11] = localdata2 & 0xff;
-
-		*token = lcommunity_token_val;
-		return p;
 	}
-	*token = lcommunity_token_unknown;
+
+	/* Assert str was a valid large community */
+	assert(separator == 2 && digit == 1);
+
+	/*
+	 * Copy the large comm.
+	 */
+	lval->val[0] = (globaladmin >> 24) & 0xff;
+	lval->val[1] = (globaladmin >> 16) & 0xff;
+	lval->val[2] = (globaladmin >> 8) & 0xff;
+	lval->val[3] = globaladmin & 0xff;
+	lval->val[4] = (localdata1 >> 24) & 0xff;
+	lval->val[5] = (localdata1 >> 16) & 0xff;
+	lval->val[6] = (localdata1 >> 8) & 0xff;
+	lval->val[7] = localdata1 & 0xff;
+	lval->val[8] = (localdata2 >> 24) & 0xff;
+	lval->val[9] = (localdata2 >> 16) & 0xff;
+	lval->val[10] = (localdata2 >> 8) & 0xff;
+	lval->val[11] = localdata2 & 0xff;
+
 	return p;
 }
 
@@ -439,23 +430,16 @@ static const char *lcommunity_gettoken(const char *str,
 struct lcommunity *lcommunity_str2com(const char *str)
 {
 	struct lcommunity *lcom = NULL;
-	enum lcommunity_token token = lcommunity_token_unknown;
 	struct lcommunity_val lval;
 
+	if (!lcommunity_list_valid(str, LARGE_COMMUNITY_LIST_STANDARD))
+		return NULL;
+
 	do {
-		str = lcommunity_gettoken(str, &lval, &token);
+		str = lcommunity_gettoken(str, &lval);
-		switch (token) {
+		if (lcom == NULL)
-		case lcommunity_token_val:
+			lcom = lcommunity_new();
-			if (lcom == NULL)
+		lcommunity_add_val(lcom, &lval);
-				lcom = lcommunity_new();
-			lcommunity_add_val(lcom, &lval);
-			break;
-		case lcommunity_token_unknown:
-		default:
-			if (lcom)
-				lcommunity_free(&lcom);
-			return NULL;
-		}
 	} while (str);
 
 	return lcom;

bgpd/bgp_lcommunity.h

@@ -23,6 +23,7 @@
 
 #include "lib/json.h"
 #include "bgpd/bgp_route.h"
+#include "bgpd/bgp_clist.h"
 
 /* Large Communities value is twelve octets long.  */
 #define LCOMMUNITY_SIZE                        12

tests/topotests/bgp_large_community/test_bgp_large_community_topo_1.py

@@ -1174,6 +1174,39 @@ def test_large_community_boundary_values(request):
     )
 
 
+def test_large_community_invalid_chars(request):
+    """
+    BGP canonical lcommunities must only be digits
+    """
+    tc_name = request.node.name
+    write_test_header(tc_name)
+    tgen = get_topogen()
+
+    # Don"t run this test if we have any failure.
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    input_dict = {
+        "r4": {
+            "bgp_community_lists": [
+                {
+                    "community_type": "standard",
+                    "action": "permit",
+                    "name": "ANY",
+                    "value": "1:a:2",
+                    "large": True,
+                }
+            ]
+        }
+    }
+
+    step("Checking boundary value for community 1:a:2")
+    result = create_bgp_community_lists(tgen, input_dict)
+    assert result is not True, "Test case {} : Failed \n Error: {}".format(
+        tc_name, result
+    )
+
+
 def test_large_community_after_clear_bgp(request):
     """
     Clear BGP neighbor-ship and check if large community and community