In a removal operation do not allow a creation of the route_node in bgp

[donaldsharp]

No description provided.

[greptile-apps]

Greptile Summary

This PR replaces bgp_node_get (a "find-or-create" lookup) with bgp_node_lookup (find-only) in SAFI_MPLS_VPN/VNC code paths that operate during removal or withdrawal, preventing phantom route-node creation as a side effect of deletion. In bgp_static_set a new negate-aware branch is added that returns an early error when an RD node cannot be found during a delete operation.

• bgp_route.c / rfapi/rfapi.c: Four call sites in bgp_rib_withdraw, bgp_update, del_vnc_route, and add_vnc_route are changed from bgp_node_get to bgp_node_lookup; the bgp_dest_unlock_node is correctly moved inside the if (pdest) guard in every case.
• bgp_static_set: The RD-table node lookup is now done up front; the bgp_node_get / bgp_table_init block is only reached when !negate, and a user-visible error is returned when negate=true but the RD cannot be found.

Confidence Score: 3/5

The negate branch in bgp_static_set can reach bgp_node_lookup with a NULL table, which would crash bgpd.

The negate branch in bgp_static_set fetches table from pdest without a NULL check and immediately passes it to bgp_node_lookup; the original code always ensured the table was initialized before this point, so a corrupted or partially-initialized pdest node could crash bgpd on this path.

bgpd/bgp_route.c — specifically the negate branch in bgp_static_set around line 8758

Important Files Changed

Filename	Overview
bgpd/bgp_route.c	Replaces bgp_node_get with bgp_node_lookup in 4 VNC/SAFI_MPLS_VPN blocks to prevent node creation during removal; adds negate-aware logic in bgp_static_set, but the negate path may dereference a NULL table if pdest exists without initialized table info.
bgpd/rfapi/rfapi.c	Replaces bgp_node_get with bgp_node_lookup in 3 SAFI_MPLS_VPN blocks inside del_vnc_route and add_vnc_route; unlock is correctly moved inside the pdest != NULL guard; changes are safe and consistent.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[bgp_static_set called] --> B{safi == SAFI_MPLS_VPN or SAFI_EVPN?}
    B -- No --> C[table = bgp->static_routes]
    B -- Yes --> D[bgp_node_lookup prd]
    D --> E{negate?}
    E -- No --> F{pdest found?}
    F -- No --> G[bgp_node_get: create node]
    F -- Yes --> H[existing pdest]
    G --> I{has path info data?}
    H --> I
    I -- No --> J[bgp_table_init + set_table_info]
    I -- Yes --> K[table = get_bgp_table_info pdest]
    J --> K
    E -- Yes --> L{pdest found?}
    L -- No --> M[Error: RD not found, return CMD_WARNING]
    L -- Yes --> N[table = get_bgp_table_info pdest]
    N --> O{table NULL?}
    O -- Yes --> P[bgp_node_lookup NULL, crash risk]
    O -- No --> Q[bgp_node_lookup prefix in table]
    K --> Q

Loading

Comments Outside Diff (1)

1. bgpd/bgp_route.c, line 8758-8765 (link)

   Potential NULL table dereference in negate path

   When negate=true and pdest is found via bgp_node_lookup, bgp_dest_get_bgp_table_info(pdest) can return NULL (e.g., if the node was somehow created without its associated table, or via a race with cleanup). table is then passed directly to bgp_node_lookup(table, &p) at line 8765 with no NULL guard — dereferencing a NULL table here would crash the daemon. The original code avoided this by always initializing the table via bgp_table_init before reaching this point, but that guarantee no longer applies for the negate path in the new code.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: bgpd/bgp_route.c
   Line: 8758-8765
   
   Comment:
   **Potential NULL `table` dereference in negate path**
   
   When `negate=true` and `pdest` is found via `bgp_node_lookup`, `bgp_dest_get_bgp_table_info(pdest)` can return NULL (e.g., if the node was somehow created without its associated table, or via a race with cleanup). `table` is then passed directly to `bgp_node_lookup(table, &p)` at line 8765 with no NULL guard — dereferencing a NULL table here would crash the daemon. The original code avoided this by always initializing the table via `bgp_table_init` before reaching this point, but that guarantee no longer applies for the negate path in the new code.
   
   How can I resolve this? If you propose a fix, please make it concise.

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_route.c:8758-8765
**Potential NULL `table` dereference in negate path**

When `negate=true` and `pdest` is found via `bgp_node_lookup`, `bgp_dest_get_bgp_table_info(pdest)` can return NULL (e.g., if the node was somehow created without its associated table, or via a race with cleanup). `table` is then passed directly to `bgp_node_lookup(table, &p)` at line 8765 with no NULL guard — dereferencing a NULL table here would crash the daemon. The original code avoided this by always initializing the table via `bgp_table_init` before reaching this point, but that guarantee no longer applies for the negate path in the new code.

_{Reviews (1): Last reviewed commit: "bgpd: Use lookup not get for VNC pdest o..." | Re-trigger Greptile}

[donaldsharp]

ci:rerun

[mjstapp]

great cleanup, thanks

[donaldsharp]

ci:rerun

[donaldsharp]

ci:rerun

[riw777]

looks good