bgpd: add 'set delete [ext-/large-]comm-list' command in route-map by fdumontet6WIND · Pull Request #22291 · FRRouting/frr

fdumontet6WIND · 2026-06-09T15:45:46Z

The next series of commits introduces the usage of a community list in a
route-map for set operations. This implies that the delete keyword has
to become mandatory, so that an add keyword is added too.

Instead of creating backward compatiblities issues, duplicate the
current commands, and move the delete keyword at the head of the
command.

route-map RM_OUT permit 10
set comm-list AA delete

becomes:

route-map RM_OUT permit 10
set comm-list delete AA

Both commands are available for configuration until deprecation of the
first model. Running-config will fallback to the new option.

The next series of commits introduces the usage of a community list in a route-map for set operations. This implies that the delete keyword has to become mandatory, so that an add keyword is added too. Instead of creating backward compatiblities issues, duplicate the current commands, and move the delete keyword at the head of the command. > route-map RM_OUT permit 10 > set comm-list AA delete becomes: > route-map RM_OUT permit 10 > set comm-list delete AA Both commands are available for configuration until deprecation of the first model. Running-config will fallback to the new option. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com> Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>

greptile-apps · 2026-06-09T15:51:12Z

Greptile Summary

This PR introduces new set comm-list <delete|add|replace> NAME route-map commands for all three community-list types (standard, large, extended), duplicating the legacy set comm-list NAME delete form while keeping backward compatibility until a 2027-05-27 deprecation date.

New CLI forms: set comm-list delete|add|replace NAME, set large-comm-list delete NAME, and set extended-comm-list delete NAME are added alongside the old forms; running-config is saved in the new format only.
New semantics: A shared route_set_community_change handler handles delete/add/replace via flags; community_list_add() in bgp_clist.c iterates permit entries to build the merged community attribute.
NB/YANG wiring: Three new YANG identities and leaf cases (comm-list-name-delete/add/replace) are added, each with a new NB callback; a buffer-sizing bug in the new callback causes snprintf truncation for the "delete" and "replace" argument strings (see inline comment).

Confidence Score: 3/5

The PR should not merge as-is: the NB callback allocates a buffer one to two bytes too small for the 'delete' and 'replace' keywords, causing snprintf to silently truncate the argument string, which then misidentifies the action as 'replace' rather than 'delete'. The new topotest also has a duplicate function definition that leaves the replace case unexercised.

The argstr truncation in bgpd/bgp_routemap_nb_config.c is a present defect: every NETCONF-driven 'comm-list-delete' or 'comm-list-replace' operation applies the wrong action silently. Combined with a test suite gap that would not have caught this regression, the change carries real risk on the new code path.

bgpd/bgp_routemap_nb_config.c (argstr buffer sizing at line 3291) and tests/topotests/bgp_route_map_comm_list/test_bgp_route_map_comm_list.py (duplicate _verify_bgp_rib, missing replace assertion).

Important Files Changed

Filename	Overview
bgpd/bgp_routemap_nb_config.c	New NB callback for comm-list-name-change has a buffer sizing bug: argstr_len is strlen(name)+7 but 'delete' needs +8 and 'replace' needs +9, causing snprintf to truncate and the rule argument to mismatch.
bgpd/bgp_routemap.c	Core route-map set logic refactored to support add/replace/delete via a unified function; two uninitialized-variable paths and a memory leak in the compile function are present.
tests/topotests/bgp_route_map_comm_list/test_bgp_route_map_comm_list.py	New topotest for the feature, but _verify_bgp_rib is defined twice — the second definition silently overwrites the first, and Case 3 (replace) is never actually asserted.
yang/frr-bgp-route-map.yang	Adds new YANG identities (comm-list-add, comm-list-replace) and separate leaf cases for comm-list-name-delete/add/replace; straightforward schema extension.
lib/routemap_cli.c	Running-config output updated to the new 'set comm-list delete
bgpd/bgp_clist.c	Adds community_list_add() iterating permit entries; correctly skips expanded/regex entries.
bgpd/bgp_routemap_nb.c	Registers three new NB callbacks sharing the same handler pair; straightforward wiring.
bgpd/bgp_community.c	community_new() promoted from static to extern; trivial change needed by community_list_add.

Prompt To Fix All With AI

Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 5
bgpd/bgp_routemap_nb_config.c:3291-3306
**Buffer too small — snprintf silently truncates "delete" and "replace" arguments**

`argstr_len` is set to `strlen(name) + strlen("delete") + 1 = strlen(name) + 7`. Writing `"%s %s"` with "delete" requires `strlen(name) + 1 (space) + 6 (delete) + 1 (NUL) = strlen(name) + 8` bytes, so snprintf truncates the last character, producing e.g. `"AA delet"`. For "replace" (7 chars) two bytes are missing, giving `"AA replac"`. The compile step in `route_set_community_change_compile` then splits on the space and checks `splits[1]` against "delete" / "add" — neither matches the truncated form, so both flags stay `false` and the action is silently treated as "replace" instead of "delete". The fix is to size the buffer to the longest keyword: `strlen(name) + strlen("replace") + 2` (1 for the space, 1 for NUL).

### Issue 2 of 5
bgpd/bgp_routemap.c:7119-7123
**Potentially uninitialized `action_type` pointer**

If none of the three boolean parameters is `true`, `action_type` is uninitialized and the two subsequent `snprintf` calls invoke undefined behavior. While the VTY command definition guarantees one of `delete`/`add`/`replace` is always set, adding an `else` branch removes the UB and silences compiler warnings.

```suggestion
	if (delete) action_type = "-delete";
	else if (add) action_type = "-add";
	else if (replace) action_type = "-replace";
	else action_type = "-delete"; /* unreachable; silence uninitialized-variable warning */

	snprintf(xpath_value, XPATH_MAXLEN,
```

### Issue 3 of 5
bgpd/bgp_routemap.c:7205-7209
**Same uninitialized `action_type` pattern in `no_set_community_change`**

Same three-way if/else-if without a final `else`, leaving `action_type` potentially uninitialized before the `snprintf`.

```suggestion
	if (del) action_type = "comm-list-delete";
	else if (add) action_type = "comm-list-add";
	else if (rep) action_type = "comm-list-replace";
	else action_type = "comm-list-delete"; /* unreachable; silence uninitialized-variable warning */

	snprintf(xpath_value, XPATH_MAXLEN,
```

### Issue 4 of 5
bgpd/bgp_routemap.c:3186-3189
**Memory leak when `splits[1]` is NULL**

`frrstr_split` allocates both the `splits` pointer array and each `splits[i]` string. When `splits != NULL` but `splits[1] == NULL`, the function returns `NULL` before the cleanup loop at line 3200-3202, leaking `splits[0]` and the `splits` array.

### Issue 5 of 5
tests/topotests/bgp_route_map_comm_list/test_bgp_route_map_comm_list.py:165-169
**`_verify_bgp_rib` is defined twice — second definition shadows the first, and Case 3 (replace) is never validated**

Python silently replaces the first definition with the second. In the second definition, `expected_1` and `expected_2` are set for case 3 (replace) but are immediately overwritten by the case 4 assignments, so the `json_cmp` call at the end tests case 4, not case 3. The "replace" feature has no executed assertion at all.

_{Reviews (1): Last reviewed commit: "tests: add topotest for BGP route-map co..." | Re-trigger Greptile}

greptile-apps · 2026-06-09T15:51:16Z

+		argstr_len = strlen(name) + strlen("delete") + 1;
+		argstr = XCALLOC(MTYPE_ROUTE_MAP_COMPILED, argstr_len);
+
+		action = yang_dnode_get_string(args->dnode,
+				"../../frr-route-map:action");
+		if (IS_SET_COMM_LIST_DEL(action)) {
+			rhc->rhc_rule = "comm-list-delete";
+			snprintf(argstr, argstr_len, "%s %s", name, "delete");
+		}
+		else if (IS_SET_COMM_LIST_ADD(action)) {
+			rhc->rhc_rule = "comm-list-add";
+			snprintf(argstr, argstr_len, "%s %s", name, "add");
+		}
+		else if (IS_SET_COMM_LIST_REPLACE(action)) {
+			rhc->rhc_rule = "comm-list-replace";
+			snprintf(argstr, argstr_len, "%s %s", name, "replace");


Buffer too small — snprintf silently truncates "delete" and "replace" arguments

argstr_len is set to strlen(name) + strlen("delete") + 1 = strlen(name) + 7. Writing "%s %s" with "delete" requires strlen(name) + 1 (space) + 6 (delete) + 1 (NUL) = strlen(name) + 8 bytes, so snprintf truncates the last character, producing e.g. "AA delet". For "replace" (7 chars) two bytes are missing, giving "AA replac". The compile step in route_set_community_change_compile then splits on the space and checks splits[1] against "delete" / "add" — neither matches the truncated form, so both flags stay false and the action is silently treated as "replace" instead of "delete". The fix is to size the buffer to the longest keyword: strlen(name) + strlen("replace") + 2 (1 for the space, 1 for NUL).

Prompt To Fix With AI

This is a comment left during a code review. Path: bgpd/bgp_routemap_nb_config.c Line: 3291-3306 Comment: **Buffer too small — snprintf silently truncates "delete" and "replace" arguments** `argstr_len` is set to `strlen(name) + strlen("delete") + 1 = strlen(name) + 7`. Writing `"%s %s"` with "delete" requires `strlen(name) + 1 (space) + 6 (delete) + 1 (NUL) = strlen(name) + 8` bytes, so snprintf truncates the last character, producing e.g. `"AA delet"`. For "replace" (7 chars) two bytes are missing, giving `"AA replac"`. The compile step in `route_set_community_change_compile` then splits on the space and checks `splits[1]` against "delete" / "add" — neither matches the truncated form, so both flags stay `false` and the action is silently treated as "replace" instead of "delete". The fix is to size the buffer to the longest keyword: `strlen(name) + strlen("replace") + 2` (1 for the space, 1 for NUL). How can I resolve this? If you propose a fix, please make it concise.

taken into account

greptile-apps · 2026-06-09T15:51:17Z

+	if (delete) action_type = "-delete";
+	else if (add) action_type = "-add";
+	else if (replace) action_type = "-replace";
+
+	snprintf(xpath_value, XPATH_MAXLEN,


Potentially uninitialized action_type pointer

If none of the three boolean parameters is true, action_type is uninitialized and the two subsequent snprintf calls invoke undefined behavior. While the VTY command definition guarantees one of delete/add/replace is always set, adding an else branch removes the UB and silences compiler warnings.

Suggested change

if (delete) action_type = "-delete";

else if (add) action_type = "-add";

else if (replace) action_type = "-replace";

snprintf(xpath_value, XPATH_MAXLEN,

if (delete) action_type = "-delete";

else if (add) action_type = "-add";

else if (replace) action_type = "-replace";

else action_type = "-delete"; /* unreachable; silence uninitialized-variable warning */

snprintf(xpath_value, XPATH_MAXLEN,

Prompt To Fix With AI

This is a comment left during a code review. Path: bgpd/bgp_routemap.c Line: 7119-7123 Comment: **Potentially uninitialized `action_type` pointer** If none of the three boolean parameters is `true`, `action_type` is uninitialized and the two subsequent `snprintf` calls invoke undefined behavior. While the VTY command definition guarantees one of `delete`/`add`/`replace` is always set, adding an `else` branch removes the UB and silences compiler warnings. ```suggestion if (delete) action_type = "-delete"; else if (add) action_type = "-add"; else if (replace) action_type = "-replace"; else action_type = "-delete"; /* unreachable; silence uninitialized-variable warning */ snprintf(xpath_value, XPATH_MAXLEN, ``` How can I resolve this? If you propose a fix, please make it concise.

call of that function implies that one of the 3 booleans is set to true. This is due to the sementic of the DEFPY_YANG command.

greptile-apps · 2026-06-09T15:51:18Z

+	if (del) action_type = "comm-list-delete";
+	else if (add) action_type = "comm-list-add";
+	else if(rep) action_type = "comm-list-replace";
+
+	snprintf(xpath_value, XPATH_MAXLEN,


Same uninitialized action_type pattern in no_set_community_change

Same three-way if/else-if without a final else, leaving action_type potentially uninitialized before the snprintf.

Suggested change

if (del) action_type = "comm-list-delete";

else if (add) action_type = "comm-list-add";

else if(rep) action_type = "comm-list-replace";

snprintf(xpath_value, XPATH_MAXLEN,

if (del) action_type = "comm-list-delete";

else if (add) action_type = "comm-list-add";

else if (rep) action_type = "comm-list-replace";

else action_type = "comm-list-delete"; /* unreachable; silence uninitialized-variable warning */

snprintf(xpath_value, XPATH_MAXLEN,

Prompt To Fix With AI

This is a comment left during a code review. Path: bgpd/bgp_routemap.c Line: 7205-7209 Comment: **Same uninitialized `action_type` pattern in `no_set_community_change`** Same three-way if/else-if without a final `else`, leaving `action_type` potentially uninitialized before the `snprintf`. ```suggestion if (del) action_type = "comm-list-delete"; else if (add) action_type = "comm-list-add"; else if (rep) action_type = "comm-list-replace"; else action_type = "comm-list-delete"; /* unreachable; silence uninitialized-variable warning */ snprintf(xpath_value, XPATH_MAXLEN, ``` How can I resolve this? If you propose a fix, please make it concise.

call of that function implies that one of the 3 booleans is set to true. This is due to the sementic of the DEFPY_YANG command.

greptile-apps · 2026-06-09T15:51:19Z

 	frrstr_split(arg, " ", &splits, &num);

-	if (splits == NULL)
+	if (splits == NULL || splits[1] == NULL)
 		return NULL;


Memory leak when splits[1] is NULL

frrstr_split allocates both the splits pointer array and each splits[i] string. When splits != NULL but splits[1] == NULL, the function returns NULL before the cleanup loop at line 3200-3202, leaking splits[0] and the splits array.

Prompt To Fix With AI

This is a comment left during a code review. Path: bgpd/bgp_routemap.c Line: 3186-3189 Comment: **Memory leak when `splits[1]` is NULL** `frrstr_split` allocates both the `splits` pointer array and each `splits[i]` string. When `splits != NULL` but `splits[1] == NULL`, the function returns `NULL` before the cleanup loop at line 3200-3202, leaking `splits[0]` and the `splits` array. How can I resolve this? If you propose a fix, please make it concise.

taken into account

take into account

greptile-apps · 2026-06-09T15:51:20Z

+                ]
+            }
+        }
+
+        expected_4 = {


_verify_bgp_rib is defined twice — second definition shadows the first, and Case 3 (replace) is never validated

Python silently replaces the first definition with the second. In the second definition, expected_1 and expected_2 are set for case 3 (replace) but are immediately overwritten by the case 4 assignments, so the json_cmp call at the end tests case 4, not case 3. The "replace" feature has no executed assertion at all.

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/topotests/bgp_route_map_comm_list/test_bgp_route_map_comm_list.py Line: 165-169 Comment: **`_verify_bgp_rib` is defined twice — second definition shadows the first, and Case 3 (replace) is never validated** Python silently replaces the first definition with the second. In the second definition, `expected_1` and `expected_2` are set for case 3 (replace) but are immediately overwritten by the case 4 assignments, so the `json_cmp` call at the end tests case 4, not case 3. The "replace" feature has no executed assertion at all. How can I resolve this? If you propose a fix, please make it concise.

taken into account

ton31337 · 2026-06-10T06:00:12Z

+
+    for _, (rname, router) in enumerate(router_list.items(), 1):
+        router.load_config(
+            TopoRouter.RD_ZEBRA, os.path.join(CWD, "{}/zebra.conf".format(rname))


Please use frr.conf, not per-daemon config files.

ton31337

I suggest making the old command "hidden" (DEFPY_*). It would work, but not suggested for "new commers".

…nities via list Currently, FRR supports deleting BGP communities using a community list via the `set comm-list delete` route-map command. This commit expands this functionality to allow adding and replacing communities using a community list. Specific changes include: * CLI: Updates the route-map configuration to support the `set comm-list <del|add|repl> <COMMUNITY_LIST_NAME>` syntax. * YANG: Modifies `frr-bgp-route-map.yang` to introduce `comm-list-add` and `comm-list-replace` identities and their corresponding nodes. * BGP Core: Implements `community_list_add()` in `bgp_clist.c` to handle the merging of communities defined in a standard list into an existing community structure. * Route-map: Refactors `route_set_community_delete` into a unified `route_set_community_change` callback to handle add, delete, and replace logic seamlessly. * Northbound: Adds the necessary configuration and destroy callbacks to support the new YANG paths. Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>

This commit introduces a new topotest (`bgp_route_map_comm_list`) to validate the newly added route-map `set comm-list <add|del|rep>` functionality. The test provisions a simple 2-node topology (r1, r2) where: * r1 advertises four prefixes (10.1.1.0/24 - 10.1.4.0/24) with baseline BGP communities. * r2 applies an inbound route-map using community-lists to modify these communities. The Python test script verifies the JSON output of the BGP RIB on r2 to ensure: * ADD: The new community is correctly appended to the existing ones (10.1.1.0/24). * DELETE: The specified community is removed while keeping the others (10.1.2.0/24). * REPLACE: The existing communities are completely overwritten by the new one (10.1.3.0/24). * COMBINED: Multiple operations (ADD + DELETE) applied to the same prefix work correctly in sequence (10.1.4.0/24). Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>

frrbot Bot added bgp libfrr tests Topotests, make check, etc yang labels Jun 9, 2026

github-actions Bot added size/XL master labels Jun 9, 2026

greptile-apps Bot reviewed Jun 9, 2026

View reviewed changes

fdumontet6WIND force-pushed the comm_list branch 11 times, most recently from e62c281 to ddd8aeb Compare June 9, 2026 21:07

ton31337 reviewed Jun 10, 2026

View reviewed changes

fdumontet6WIND force-pushed the comm_list branch 2 times, most recently from 28565a3 to 053e83e Compare June 10, 2026 08:47

fdumontet6WIND added 2 commits June 10, 2026 11:29

fdumontet6WIND force-pushed the comm_list branch from 053e83e to 06f7e4a Compare June 10, 2026 09:44

Conversation

fdumontet6WIND commented Jun 9, 2026

Uh oh!

greptile-apps Bot commented Jun 9, 2026

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Uh oh!

greptile-apps Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

fdumontet6WIND Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

fdumontet6WIND Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

fdumontet6WIND Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

fdumontet6WIND Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

fdumontet6WIND Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

fdumontet6WIND Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

fdumontet6WIND Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

ton31337 Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

ton31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fdumontet6WIND Jun 9, 2026 •

edited

Loading