Bgp capability common by donaldsharp · Pull Request #22254 · FRRouting/frr

donaldsharp · 2026-06-08T14:43:37Z

There was a bunch of duplicated code, reduce it so we don't have this problem in the future. This probably should go in after #22249

greptile-apps · 2026-06-08T14:46:45Z

Greptile Summary

This PR eliminates two identical capability-length-check switch blocks (one in bgp_open.c, one in bgp_packet.c) by extracting them into a new shared helper bgp_capability_length_check(). It also adds CAPABILITY_CODE_LINK_LOCAL to both the cap_modsizes array and the new function, preparing for the upcoming link-local next-hop capability work in #22249.

Refactoring: The duplicated 40-line switch is replaced with a single call to bgp_capability_length_check() in both call sites; return semantics are preserved (return -1 in bgp_open.c, goto done in bgp_packet.c).
New capability coverage: CAPABILITY_CODE_LINK_LOCAL is now length-checked in both OPEN and dynamic capability paths, which was previously unguarded.
Pre-existing gap: CAPABILITY_CODE_LLGR has entries in cap_minsizes/cap_modsizes but is absent from the new (and both old) switch statements, so its length is still never validated.

Confidence Score: 4/5

Safe to merge; the refactoring is a straightforward extraction with no behavioral regression for existing capabilities.

The logic in both call sites is faithfully preserved — bgp_notify_send is still called on failure, and the callers use their original exit paths (return -1 vs goto done). The only new coverage is CAPABILITY_CODE_LINK_LOCAL, which is intentional. CAPABILITY_CODE_LLGR was and remains absent from the check switch, meaning malformed LLGR lengths go unvalidated; this is pre-existing but becomes more conspicuous now that the check lives in one place.

bgpd/bgp_open.c — specifically the CAPABILITY_CODE_LLGR omission in bgp_capability_length_check.

Important Files Changed

Filename	Overview
bgpd/bgp_open.c	Extracts the duplicate length-check switch into bgp_capability_length_check(); adds CAPABILITY_CODE_LINK_LOCAL to both cap_modsizes and the new check. Pre-existing omission of CAPABILITY_CODE_LLGR from the switch is now more apparent.
bgpd/bgp_open.h	Adds extern declaration for the new bgp_capability_length_check() helper; no issues.
bgpd/bgp_packet.c	Replaces the local copy of the length-check switch with a call to the new shared helper; semantically equivalent to the removed code.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["bgp_capability_parse (bgp_open.c)"] -->|"caphdr.code, caphdr.length"| C
    B["bgp_capability_msg_parse (bgp_packet.c)"] -->|"hdr->code, hdr->length"| C
    C["bgp_capability_length_check()"]
    C --> D{"code in known capability set?"}
    D -- No --> E["return true (unknown, skip)"]
    D -- Yes --> F{"length less than cap_minsizes[code]?"}
    F -- Yes --> G["zlog_info + bgp_notify_send, return false"]
    F -- No --> H{"length mod cap_modsizes[code] != 0?"}
    H -- Yes --> G
    H -- No --> I["return true"]
    G -->|"false, bgp_open.c caller"| A2["return -1"]
    G -->|"false, bgp_packet.c caller"| B2["goto done"]
    I --> J["continue capability processing"]

Prompt To Fix All With AI

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

---

### Issue 1 of 1
bgpd/bgp_open.c:99-134
**`CAPABILITY_CODE_LLGR` missing from shared length-check**

`CAPABILITY_CODE_LLGR` has entries in both `cap_minsizes` and `cap_modsizes` (lines 64 and 88 respectively), but it is absent from the switch in `bgp_capability_length_check`. This was pre-existing in both original switch blocks, but now that those blocks are consolidated into a single authoritative function, it becomes more visible: LLGR capability lengths are never validated. Any peer sending a malformed LLGR capability can bypass the sanity check entirely.

_{Reviews (1): Last reviewed commit: "bgpd: validate CAPABILITY_CODE_LINK_LOCA..." | Re-trigger Greptile}

greptile-apps · 2026-06-08T14:46:49Z

+	switch (code) {
+	case CAPABILITY_CODE_MP:
+	case CAPABILITY_CODE_REFRESH:
+	case CAPABILITY_CODE_ORF:
+	case CAPABILITY_CODE_RESTART:
+	case CAPABILITY_CODE_AS4:
+	case CAPABILITY_CODE_ADDPATH:
+	case CAPABILITY_CODE_DYNAMIC:
+	case CAPABILITY_CODE_ENHE:
+	case CAPABILITY_CODE_FQDN:
+	case CAPABILITY_CODE_ENHANCED_RR:
+	case CAPABILITY_CODE_EXT_MESSAGE:
+	case CAPABILITY_CODE_ROLE:
+	case CAPABILITY_CODE_SOFT_VERSION:
+	case CAPABILITY_CODE_PATHS_LIMIT:
+	case CAPABILITY_CODE_LINK_LOCAL:
+		if (length < cap_minsizes[code]) {
+			zlog_info("%pBP: %s Capability length error: got %u, expected at least %u",
+				  peer, lookup_msg(capcode_str, code, NULL), length,
+				  (unsigned int)cap_minsizes[code]);
+			bgp_notify_send(connection, BGP_NOTIFY_OPEN_ERR,
+					BGP_NOTIFY_OPEN_MALFORMED_ATTR);
+			return false;
+		}
+		if (length && length % cap_modsizes[code] != 0) {
+			zlog_info("%pBP: %s Capability length error: got %u, expected a multiple of %u",
+				  peer, lookup_msg(capcode_str, code, NULL), length,
+				  (unsigned int)cap_modsizes[code]);
+			bgp_notify_send(connection, BGP_NOTIFY_OPEN_ERR,
+					BGP_NOTIFY_OPEN_MALFORMED_ATTR);
+			return false;
+		}
+		break;
+	default:
+		break;
+	}


CAPABILITY_CODE_LLGR missing from shared length-check

CAPABILITY_CODE_LLGR has entries in both cap_minsizes and cap_modsizes (lines 64 and 88 respectively), but it is absent from the switch in bgp_capability_length_check. This was pre-existing in both original switch blocks, but now that those blocks are consolidated into a single authoritative function, it becomes more visible: LLGR capability lengths are never validated. Any peer sending a malformed LLGR capability can bypass the sanity check entirely.

Prompt To Fix With AI

This is a comment left during a code review. Path: bgpd/bgp_open.c Line: 99-134 Comment: **`CAPABILITY_CODE_LLGR` missing from shared length-check** `CAPABILITY_CODE_LLGR` has entries in both `cap_minsizes` and `cap_modsizes` (lines 64 and 88 respectively), but it is absent from the switch in `bgp_capability_length_check`. This was pre-existing in both original switch blocks, but now that those blocks are consolidated into a single authoritative function, it becomes more visible: LLGR capability lengths are never validated. Any peer sending a malformed LLGR capability can bypass the sanity check entirely. How can I resolve this? If you propose a fix, please make it concise.

github-actions · 2026-06-09T05:44:06Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

The OPEN message capability parser (bgp_capability_parse) and the Dynamic Capability message parser (bgp_capability_msg_parse) each had an identical type-specific length-sanity switch over the known capability codes, checking against cap_minsizes[]/cap_modsizes[] and sending the same OPEN error notification on failure. Extract that logic into a single bgp_capability_length_check() helper in bgp_open.c (declared in bgp_open.h) and call it from both parsers. Each caller keeps its own control flow on failure: bgp_capability_parse aborts (return -1) while bgp_capability_msg_parse skips the individual capability (goto done). Signed-off-by: Donald Sharp <sharpd@nvidia.com>

The Link-Local capability code was present in cap_minsizes[] but missing from cap_modsizes[], and it was not covered by the type-specific length sanity switch in bgp_capability_length_check(). Add the matching cap_modsizes[] entry and include the code in the switch so the capability length is validated consistently with the others. Signed-off-by: Donald Sharp <sharpd@nvidia.com>

ton31337 · 2026-06-10T06:50:07Z

 	[CAPABILITY_CODE_ROLE] = 1,
 	[CAPABILITY_CODE_SOFT_VERSION] = 1,
 	[CAPABILITY_CODE_PATHS_LIMIT] = 5,
+	[CAPABILITY_CODE_LINK_LOCAL] = 1,


link-local capability is length of 0, why do we need it here?

frrbot Bot added the bgp label Jun 8, 2026

github-actions Bot added size/L master labels Jun 8, 2026

greptile-apps Bot reviewed Jun 8, 2026

View reviewed changes

github-actions Bot added the conflicts label Jun 9, 2026

riw777 self-requested a review June 9, 2026 11:51

donaldsharp added 2 commits June 9, 2026 13:12

donaldsharp force-pushed the bgp_capability_common branch from 8011427 to 38ec1b0 Compare June 9, 2026 17:12

github-actions Bot removed the conflicts label Jun 9, 2026

ton31337 reviewed Jun 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bgp capability common#22254

Bgp capability common#22254
donaldsharp wants to merge 2 commits into
FRRouting:masterfrom
donaldsharp:bgp_capability_common

donaldsharp commented Jun 8, 2026

Uh oh!

greptile-apps Bot commented Jun 8, 2026

Uh oh!

greptile-apps Bot Jun 8, 2026

Uh oh!

github-actions Bot commented Jun 9, 2026

Uh oh!

ton31337 Jun 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

donaldsharp commented Jun 8, 2026

Uh oh!

greptile-apps Bot commented Jun 8, 2026

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 9, 2026

Uh oh!

ton31337 Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants