bgpd: Fix buffer/count inconsistency in SRv6 End.X SID TLV parsers

[cscarpitta]

When parsing SRv6 End.X SID TLVs, each new entry is added to a dynamically grown array. The array is resized first, and the new entry is filled in. The parser then processes optional sub-TLVs and increments the entry count only after sub-TLV parsing succeeds. If sub-TLV parsing fails, the function returns an error before the count is incremented, leaving the array one element larger than the count indicates.

Code that later copies the array allocates the destination based on the count, so it can allocate too little memory and miss the last entry.

Fix by incrementing the count and marking the attribute present immediately after filling in the entry, before parsing sub-TLVs. If sub-TLV parsing then fails, the count still matches the array size, and the error cleanup path frees the entire array safely.

This affects parse_srv6_endx_sid(), parse_isis_srv6_lan_endx_sid(), and parse_ospf_srv6_lan_endx_sid().

[greptile-apps]

Greptile Summary

This PR fixes a buffer/count inconsistency in three SRv6 End.X SID TLV parser functions where the array was grown by XREALLOC before the entry count was incremented, but the increment only happened after optional sub-TLV parsing succeeded. A sub-TLV parse failure left the allocated array one element larger than the count, causing downstream copy operations to under-allocate and miss the last entry.

• In parse_srv6_endx_sid, parse_isis_srv6_lan_endx_sid, and parse_ospf_srv6_lan_endx_sid, srv6_*_sid_count++ and SET_FLAG(present_tlvs, …) are moved to immediately after the entry is filled in, before sub-TLV parsing.
• This ensures that on sub-TLV parse failure the count always matches the allocated array size, so the error-path XFREE and any copy via bgp_ls_attr_dup (which sizes the destination by count) both behave correctly.

Confidence Score: 5/5

This is a targeted, correct fix to a real memory-accounting bug; no new logic is introduced and the change is safe to merge.

All three affected functions receive the same mechanical reordering of two lines — no new allocations, loops, or conditional paths are added. The copy path in bgp_ls_attr_dup and the iteration paths all use the count correctly after this change, and the free path was never count-dependent so it was unaffected before and after.

No files require special attention.

Important Files Changed

Filename	Overview
bgpd/bgp_ls_nlri.c	Count increment and present-TLV flag moved before sub-TLV parsing in all three SRv6 End.X SID parsers; fix is minimal, consistent, and correctly aligns array size with count on both success and failure paths.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[parse_srv6_endx_sid / parse_isis_srv6_lan_endx_sid / parse_ospf_srv6_lan_endx_sid] --> B{length check}
    B -- too short --> C[return -1]
    B -- ok --> D{count >= MAX?}
    D -- yes --> E[skip TLV, return 0]
    D -- no --> F[XREALLOC array to count+1]
    F --> G[Fill entry fields from stream]
    G --> H["count++ / SET_FLAG moved here"]
    H --> I{has sub-TLVs?}
    I -- no --> J[return 0]
    I -- yes --> K[parse_endx_sub_tlvs]
    K -- success --> J
    K -- fail --> L["return -1 (count already correct)"]

Loading

_{Reviews (1): Last reviewed commit: "bgpd: Fix buffer/count inconsistency in ..." | Re-trigger Greptile}

[ton31337]

Should we decrement/UNSET on error?