bgpd: Fix a couple of issues in BGP-LS NLRI encoding/decoding

[cscarpitta]

This PR solves a couple of issues in BGP-LS NLRI encoding/decoding.

See individual commits.

[greptile-apps]

Greptile Summary

This PR fixes several correctness and safety issues in the BGP-LS NLRI encoding and decoding pipeline. Changes span attribute encoding (bgp_attr.c), NLRI parsing (bgp_ls.c), and the low-level encode/decode routines (bgp_ls_nlri.c/bgp_ls_nlri.h).

Key fixes included:

• Extended-length attribute header (bgp_attr.c): The BGP-LS path attribute was being encoded with a 1-byte length field (stream_putc) but without setting BGP_ATTR_FLAG_EXTLEN. RFC 9552 requires a 2-byte length for this attribute. This mismatch would cause peer implementations to misparse the attribute length, breaking BGP-LS sessions. The fix sets the EXTLEN flag and uses stream_putw/stream_putw_at throughout, and adds a UINT16_MAX overflow guard.

• IGP Router ID buffer overflow (bgp_ls_nlri.h): igp_router_id[] was sized at 8 bytes, but BGP_LS_IGP_ROUTER_ID_MAX_SIZE (used as the accepted maximum during decoding) is 16 bytes (IPv6). Receiving a Direct/Static IPv6 Router ID would overflow into adjacent struct fields. The buffer is now correctly sized to BGP_LS_IGP_ROUTER_ID_MAX_SIZE.

• Double ntohl in Extended Admin Group decoding (bgp_ls_nlri.c): The old code applied ntohl() on top of stream_getl(), which already performs big-endian-to-host conversion internally. On little-endian systems (x86), this double-inversion silently produced byte-reversed admin group bitmaps. The redundant ntohl() is removed; the pattern is now consistent with other callers of admin_group_bulk_set in the codebase (e.g., lib/zclient.c, lib/link_state.c).

• Stream writability guards (bgp_ls_nlri.c): STREAM_WRITEABLE checks are added before every write to the stream in encode paths, allowing functions to return -1 cleanly rather than silently truncating or corrupting output.

• Duplicate TLV detection (bgp_ls_nlri.c): A wide set of node/link/prefix descriptor TLVs and attribute TLVs now reject duplicates with a warning, tightening protocol compliance and preventing partial overwrites of fields.

• NLRI temporary allocation (bgp_ls.c): The per-iteration stack-allocated struct bgp_ls_nlri nlri is replaced with a heap-allocated bgp_ls_nlri_alloc() pointer, which is properly freed after use. This change is safe because bgp_ls_nlri_get() always creates an independent deep copy when inserting into the hash, so ls_entry and the local nlri are distinct allocations.

Confidence Score: 5/5

• This PR is safe to merge; it fixes real bugs with no regressions introduced.
• All changes are targeted correctness fixes backed by the RFC and codebase conventions. The EXTLEN flag and 2-byte length fix is a protocol-level correctness requirement. The igp_router_id buffer expansion closes a latent stack/heap overflow. The ntohl removal eliminates a byte-order inversion on little-endian hosts, consistent with how every other admin_group_bulk_set caller in the tree behaves. Memory management in bgp_ls.c is correct: the temporary heap nlri is always freed, and ls_entry is an independent deep copy. The stream writability guards and duplicate TLV checks add defensive depth without changing correct-input behavior. No pre-existing interfaces are broken.
• No files require special attention.

Important Files Changed

Filename	Overview
bgpd/bgp_attr.c	Fixes BGP-LS attribute header to use EXTLEN flag with 2-byte length field per RFC 9552; adds UINT16_MAX overflow guard and rolls back stream on error
bgpd/bgp_ls.c	Changes NLRI decoding from stack-allocated struct to heap-allocated pointer with proper allocation/free lifecycle; adds NULL guards for peer/BGP/ls_info
bgpd/bgp_ls_nlri.c	Adds stream writability checks before every write, comprehensive duplicate TLV detection, correct removal of double ntohl conversion in parse_ext_admin_group, and goto-error cleanup for link/prefix descriptor parsing
bgpd/bgp_ls_nlri.h	Extends igp_router_id[] from 8 to BGP_LS_IGP_ROUTER_ID_MAX_SIZE (16) bytes to support IPv6 router IDs; adds BGP_LS_MAX_EXT_ADMIN_GROUPS constant

Sequence Diagram

sequenceDiagram
    participant Peer as BGP Peer
    participant Parse as bgp_nlri_parse_ls
    participant Decode as bgp_ls_decode_nlri
    participant Hash as nlri_hash
    participant RIB as BGP RIB

    Peer->>Parse: packet MP_REACH or UNREACH NLRI
    Note over Parse: Guard: peer/bgp/ls_info/packet not NULL

    loop for each NLRI in stream
        Parse->>Decode: bgp_ls_nlri_alloc() then decode into heap nlri
        Decode->>Decode: Parse TLVs with duplicate detection
        Decode->>Decode: STREAM_WRITEABLE and STREAM_READABLE checks
        Decode-->>Parse: decoded nlri or -1 on error

        alt decode failed
            Parse->>Parse: bgp_ls_nlri_free(nlri)
            Parse-->>Peer: BGP_NLRI_PARSE_ERROR
        else decode OK
            Parse->>Hash: bgp_ls_nlri_get(nlri)
            Note over Hash: deep-copy nlri into hash if new, refcnt incremented
            Hash-->>Parse: ls_entry which is hash-owned copy
            Parse->>RIB: bgp_update or bgp_withdraw
            Parse->>Parse: bgp_ls_nlri_free(nlri) to free temp alloc
        end
    end

    Parse-->>Peer: BGP_NLRI_PARSE_OK

Loading

_{Last reviewed commit: 03dbdfd}

[cscarpitta]

@greptile review

[cscarpitta]

@greptile review

[cscarpitta]

@greptile review

[cscarpitta]

@greptile review

[github-actions]

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

[riw777]

looks good