bgpd: initialise nh_flag attribute

[suphawitwanl]

in "leak_update" function
nh_flag is not initialized when nexthop is valid at route creation.

Original PR21498 by fdumontet6WIND

[greptile-apps]

Greptile Summary

This PR refactors leak_update to accept a non-interned static_attr and perform bgp_attr_intern internally, with the goal of initialising BGP_ATTR_NH_VALID in nh_flags before the attr is interned. The new-route creation path receives the fix, but the update path (existing bpi found) still calls bgp_attr_intern(static_attr) at line 1354 before the nexthop-validity check at line 1373, leaving BGP_ATTR_NH_VALID absent from the interned attr on that code path.

• The fix is incomplete: the update path (bpi found loop) interns static_attr with nh_flags == 0 before leak_update_nexthop_valid runs, so bpi->attr->nh_flags will never carry BGP_ATTR_NH_VALID after an update event.
• In the new-route path, SET_FLAG targets new->attr which is still the raw static_attr pointer at that point — mutation goes to the caller's stack variable rather than a clearly named local, making the ordering fragile.

Confidence Score: 3/5

The PR partially fixes the reported bug; the update path still has the same uninitialized nh_flags defect, and the new-route fix relies on an implicit alias that is easy to break.

Two P1-class findings: the update path never sets BGP_ATTR_NH_VALID (original bug survives on that branch), and the new-route path mutates static_attr through new->attr as an implicit alias before interning. Both affect production nexthop-tracking state in MPLS VPN deployments. Multiple P1s pull the score below the single-P1 ceiling of 4.

bgpd/bgp_mplsvpn.c — specifically the update path at lines 1354–1378 and the new-route path at lines 1441–1447

Important Files Changed

Filename	Overview
bgpd/bgp_mplsvpn.c	Refactors leak_update to accept a non-interned static_attr and perform interning internally; fixes BGP_ATTR_NH_VALID initialization in the new-route path, but the update path still interns before the nexthop check, and the flag mutation at line 1444 relies on new->attr == static_attr as an implicit alias.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["leak_update(static_attr)"] --> B{existing bpi?}
    B -- yes --> C["bgp_attr_intern(static_attr)\nbpi->attr = new_attr\n⚠ nh_flags == 0 here"]
    C --> D["leak_update_nexthop_valid()"]
    D -- valid --> E["bgp_path_info_set_flag VALID\n❌ BGP_ATTR_NH_VALID NOT set on bpi->attr"]
    D -- invalid --> F["bgp_path_info_unset_flag VALID"]
    B -- no --> G["info_make(static_attr)\nnew->attr = static_attr"]
    G --> H["leak_update_nexthop_valid()"]
    H -- valid --> I["bgp_path_info_set_flag VALID\nSET_FLAG(new->attr->nh_flags)\n= mutates static_attr via alias"]
    H -- invalid --> J["bgp_path_info_unset_flag VALID"]
    I --> K["new->attr = bgp_attr_intern(static_attr)\n✅ nh_flags encoded correctly"]
    J --> K

Loading

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_mplsvpn.c:1444-1447
**`BGP_ATTR_NH_VALID` set on wrong attr instance**

`new->attr` at line 1444 still points to `static_attr` (the raw stack pointer assigned by `info_make`), so `SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID)` mutates `static_attr` directly. `bgp_attr_intern(static_attr)` then interns the modified struct, so the interned attr does end up with the flag set — but only because mutating the caller's stack variable through `new->attr` happens to work here.

The intent would be clearer and safer if the flag were set on `static_attr` explicitly before the intern:

```c
if (leak_update_nexthop_valid(to_bgp, bn, static_attr, afi, safi,
                              source_bpi, new, bgp_orig, p, debug)) {
    bgp_path_info_set_flag(bn, new, BGP_PATH_VALID);
    SET_FLAG(static_attr->nh_flags, BGP_ATTR_NH_VALID);
} else {
    bgp_path_info_unset_flag(bn, new, BGP_PATH_VALID);
}
new->attr = bgp_attr_intern(static_attr);
```

This makes the mutation target unambiguous and avoids relying on `new->attr == static_attr` as an implicit side-channel.

_{Reviews (2): Last reviewed commit: "bgpd: initialise nh_flag attribute" | Re-trigger Greptile}

[suphawitwanl]

@greptileai

[greptile-apps]

BGP_ATTR_NH_VALID set on wrong attr instance

new->attr at line 1444 still points to static_attr (the raw stack pointer assigned by info_make), so SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID) mutates static_attr directly. bgp_attr_intern(static_attr) then interns the modified struct, so the interned attr does end up with the flag set — but only because mutating the caller's stack variable through new->attr happens to work here.

The intent would be clearer and safer if the flag were set on static_attr explicitly before the intern:

if (leak_update_nexthop_valid(to_bgp, bn, static_attr, afi, safi,
                              source_bpi, new, bgp_orig, p, debug)) {
    bgp_path_info_set_flag(bn, new, BGP_PATH_VALID);
    SET_FLAG(static_attr->nh_flags, BGP_ATTR_NH_VALID);
} else {
    bgp_path_info_unset_flag(bn, new, BGP_PATH_VALID);
}
new->attr = bgp_attr_intern(static_attr);

This makes the mutation target unambiguous and avoids relying on new->attr == static_attr as an implicit side-channel.

Prompt To Fix With AI

This is a comment left during a code review.
Path: bgpd/bgp_mplsvpn.c
Line: 1444-1447

Comment:
**`BGP_ATTR_NH_VALID` set on wrong attr instance**

`new->attr` at line 1444 still points to `static_attr` (the raw stack pointer assigned by `info_make`), so `SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID)` mutates `static_attr` directly. `bgp_attr_intern(static_attr)` then interns the modified struct, so the interned attr does end up with the flag set — but only because mutating the caller's stack variable through `new->attr` happens to work here.

The intent would be clearer and safer if the flag were set on `static_attr` explicitly before the intern:

```c
if (leak_update_nexthop_valid(to_bgp, bn, static_attr, afi, safi,
                              source_bpi, new, bgp_orig, p, debug)) {
    bgp_path_info_set_flag(bn, new, BGP_PATH_VALID);
    SET_FLAG(static_attr->nh_flags, BGP_ATTR_NH_VALID);
} else {
    bgp_path_info_unset_flag(bn, new, BGP_PATH_VALID);
}
new->attr = bgp_attr_intern(static_attr);
```

This makes the mutation target unambiguous and avoids relying on `new->attr == static_attr` as an implicit side-channel.

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