bgpd: Avoid having a dangling pointer after we free NHC attribute

[ton31337]

No description provided.

[greptile-apps]

Greptile Summary

This PR fixes dangling pointer bugs in bgp_attr_nhc() by adding bgp_attr_set_nhc(attr, NULL) after every bgp_nhc_free(nhc) call in error paths — ensuring attr->nhc is not left pointing to freed memory. It also changes the link-local next-hop without BGPID TLV case from a non-fatal BGP_ATTR_PARSE_PROCEED return to bgp_attr_malformed(...), which is a behavioral change that aligns with the draft's MUST language but could trigger peer NOTIFICATION for previously-tolerated implementations.

Confidence Score: 5/5

Safe to merge; the dangling-pointer fixes are correct and complete, and the one behavioral change (link-local without BGPID TLV) is defensible per the draft's MUST language.

All remaining findings are P2 (log-level inconsistency). The core fix — adding bgp_attr_set_nhc(attr, NULL) after every bgp_nhc_free(nhc) — is mechanically correct. No data loss, no security issue, no broken primary path.

No files require special attention beyond the zlog_warn / zlog_err inconsistency at line 4025 of bgpd/bgp_attr.c.

Important Files Changed

Filename	Overview
bgpd/bgp_attr.c	Adds missing bgp_attr_set_nhc(attr, NULL) after each bgp_nhc_free(nhc) in error paths of bgp_attr_nhc(); also changes the link-local/BGPID-missing case from PROCEED to a malformed error (behavioral change).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["bgp_attr_nhc() entry\nnhc = bgp_attr_get_nhc(attr)"] --> B{discard/withdraw?}
    B -- yes --> Z[nhc_ignore: skip bytes]
    B -- no --> C{length < MIN_LEN?}
    C -- yes --> E1["bgp_nhc_free(nhc)\nbgp_attr_set_nhc(attr, NULL)\nreturn malformed ✅ fixed"]
    C -- no --> D{afi/safi valid?}
    D -- no --> E2["bgp_nhc_free(nhc)\nbgp_attr_set_nhc(attr, NULL)\nreturn PARSE_ERROR ✅ fixed"]
    D -- yes --> F{nh_length valid?}
    F -- no --> E3["bgp_nhc_free(nhc)\nbgp_attr_set_nhc(attr, NULL)\nreturn malformed ✅ fixed"]
    F -- yes --> G["if(!nhc) XCALLOC new nhc\nfill afi/safi/nh_length"]
    G --> H{nh_length == IPv4?}
    H -- no-else --> E4["bgp_nhc_free(nhc)\nbgp_attr_set_nhc(attr, NULL)\nreturn PARSE_ERROR_NOTIFYPLS ✅ fixed"]
    H -- yes/IPv6 --> I[Parse TLV loop]
    I --> J{TLV length OOB?}
    J -- yes --> E5["bgp_nhc_free(nhc)\nbgp_attr_set_nhc(attr, NULL)\nreturn malformed ✅ fixed"]
    J -- no --> K{NNHN TLV invalid?}
    K -- yes --> E6["bgp_nhc_tlv_free + bgp_nhc_free\nbgp_attr_set_nhc(attr, NULL)\nreturn malformed ✅ fixed"]
    K -- no --> I
    I --> L{link-local && no BGPID TLV?}
    L -- yes --> E7["bgp_nhc_free(nhc)\nbgp_attr_set_nhc(attr, NULL)\nreturn malformed\n⚠️ behavior change: was PROCEED"]
    L -- no --> M["bgp_attr_set_nhc(attr, bgp_nhc_intern(nhc))\nreturn PROCEED"]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: bgpd/bgp_attr.c
Line: 4025-4028

Comment:
**Log level inconsistency after severity upgrade**

The link-local/missing-BGPID-TLV path was upgraded from `BGP_ATTR_PARSE_PROCEED` (non-fatal) to `bgp_attr_malformed()` (triggers a NOTIFICATION to the peer), but the log statement was left as `zlog_warn`. Every other hard error in this function uses `zlog_err`. Keeping `zlog_warn` here means operators won't see a consistent severity signal when a session is about to be torn down.

```suggestion
		zlog_err("%pBP sent link-local next-hop in NHC without required BGPID TLV", peer);
		bgp_nhc_free(nhc);
		bgp_attr_set_nhc(attr, NULL);
		return bgp_attr_malformed(args, BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, args->total);
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (2): Last reviewed commit: "bgpd: Avoid having a dangling pointer af..." | Re-trigger Greptile}

[ton31337]

@greptileai review

[donaldsharp]

@Mergifyio backport stable/10.6 stable/10.5 stable/10.4

[mergify]

backport stable/10.6 stable/10.5 stable/10.4

✅ Backports have been created

Details

• #21679 bgpd: Avoid having a dangling pointer after we free NHC attribute (backport #21605) has been created for branch stable/10.6 but encountered conflicts

Cherry-pick of c5b215e has failed:

On branch mergify/bp/stable/10.6/pr-21605
Your branch is up to date with 'origin/stable/10.6'.

You are currently cherry-picking commit c5b215ef9.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   bgpd/bgp_attr.c

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

• #21680 bgpd: Avoid having a dangling pointer after we free NHC attribute (backport #21605) has been created for branch stable/10.5 but encountered conflicts

Cherry-pick of c5b215e has failed:

On branch mergify/bp/stable/10.5/pr-21605
Your branch is up to date with 'origin/stable/10.5'.

You are currently cherry-picking commit c5b215ef9.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   bgpd/bgp_attr.c

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

• #21681 bgpd: Avoid having a dangling pointer after we free NHC attribute (backport #21605) has been created for branch stable/10.4 but encountered conflicts

Cherry-pick of c5b215e has failed:

On branch mergify/bp/stable/10.4/pr-21605
Your branch is up to date with 'origin/stable/10.4'.

You are currently cherry-picking commit c5b215ef9.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   bgpd/bgp_attr.c

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally