Skip to content

bgpd: call init, term, copy LS attr admin_group#21289

Merged
donaldsharp merged 2 commits into
FRRouting:masterfrom
mjstapp:fix_bgp_ls_admin_group
Mar 23, 2026
Merged

bgpd: call init, term, copy LS attr admin_group#21289
donaldsharp merged 2 commits into
FRRouting:masterfrom
mjstapp:fix_bgp_ls_admin_group

Conversation

@mjstapp
Copy link
Copy Markdown
Contributor

@mjstapp mjstapp commented Mar 23, 2026

There's an admin group struct embedded in the BGP LS attr; it needs to be init'd, freed, and copied properly.

There's an admin group struct embedded in the BGP LS attr;
it needs to be init'd and freed.

Reported-by: Haruto Kimura (Stella) <harutokimura0608@gmail.com>
Signed-off-by: Mark Stapp <mjs@cisco.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Mar 23, 2026

Greptile Summary

This PR fixes three memory-management bugs for the ext_admin_group field (struct admin_group, which wraps a heap-allocated bitfield_t) embedded inside struct bgp_ls_attr:

  • bgp_ls_attr_alloc — calls admin_group_init after XCALLOC so the internal bitmap.data pointer is properly allocated before any admin-group operations occur.
  • bgp_ls_attr_free — calls admin_group_term before the XFREE calls, preventing a memory leak of the bitmap.data allocation.
  • bgp_ls_attr_copy — resets the shallow-copied ext_admin_group with memset and then calls admin_group_copy to perform a proper deep copy, eliminating a double-free / use-after-free hazard introduced by the preceding memcpy(dst, src, sizeof(*dst)).

The fix is minimal and correct. All three lifecycle operations (admin_group_init, admin_group_term, admin_group_copy) are well-tested library functions; the pattern used here is consistent with how the same struct is handled elsewhere in the codebase (e.g., isisd).

Confidence Score: 5/5

  • This PR is safe to merge — it is a targeted, correct fix for clear memory-management bugs with no functional regressions.
  • All three changes are correct: admin_group_init after XCALLOC (bitmap starts zeroed so the !bf_is_inited assert passes), admin_group_term before XFREE (prevents bitmap leak), and memset + admin_group_copy in the copy path (eliminates the double-free/UAF from the previous shallow memcpy). Every caller of these three functions allocates via bgp_ls_attr_alloc, so the invariant that the admin_group is always initialized before use is upheld. The pattern is consistent with how admin_group is managed in isisd.
  • No files require special attention.

Important Files Changed

Filename Overview
bgpd/bgp_ls_nlri.c Adds proper init/term/copy lifecycle management for the embedded ext_admin_group (struct admin_group) field in bgp_ls_attr. Fixes a memory leak (missing admin_group_term on free), potential use of uninitialized bitmap (missing admin_group_init on alloc), and a double-free / use-after-free hazard (shallow memcpy in bgp_ls_attr_copy left both src and dst sharing the same bitmap.data pointer).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[bgp_ls_attr_alloc] -->|XCALLOC zeros memory| B[admin_group_init\next_admin_group]
    B -->|bf_init allocates bitmap.data| C[attr ready for use]

    D[bgp_ls_attr_free] -->|check attr != NULL| E[admin_group_term\next_admin_group]
    E -->|bf_free releases bitmap.data| F[XFREE other fields]
    F --> G[XFREE attr]

    H[bgp_ls_attr_copy] -->|XCALLOC + memcpy| I[shallow copy\next_admin_group shares pointer]
    I -->|memset ext_admin_group to 0| J[bitmap.data = NULL\nno longer shared]
    J -->|admin_group_copy| K[bf_copy creates deep copy\nof bitmap.data]
    K --> L[dst fully independent from src]
Loading

Reviews (2): Last reviewed commit: "bgpd: properly copy ls attr's admin_grou..." | Re-trigger Greptile

the bgp_ls_attr_copy() function must make a separate copy
of the embedded admin_group in the bgp_ls_attr.

Signed-off-by: Mark Stapp <mjs@cisco.com>
@mjstapp mjstapp changed the title bgpd: call init and term funcs for LS attr admin_group bgpd: call init, term, copy LS attr admin_group Mar 23, 2026
@mjstapp
Copy link
Copy Markdown
Contributor Author

mjstapp commented Mar 23, 2026

@greptileai review

@donaldsharp donaldsharp merged commit 3526e67 into FRRouting:master Mar 23, 2026
21 checks passed
@mjstapp mjstapp deleted the fix_bgp_ls_admin_group branch April 8, 2026 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants