Memory leak problems.

[donaldsharp]

Remove all memory leaks on shutdown from all daemons. Also turn on test failure so we catch ne wmemory leaks

[donaldsharp]

@greptileai review

[donaldsharp]

@greptileai can you review skipping f4c4cbe that one does not need to be reviewed by you. That should get you under the limit

[greptile-apps]

Greptile Summary

This PR adds shutdown cleanup (_terminate() functions, hash drains, static-buffer frees, and ref-count fixes) across bgpd, pbrd, pimd, isisd, ldpd, zebra, and several lib modules to address memory leaks detected by valgrind/ASAN, while also tightening the topotest harness to surface those leaks as hard test failures.

• BGP teardown overhaul: EVPN cleanup ordering is restructured and extended from daemon-shutdown-only to every deletion of the EVPN-owner instance; bgp_clear_node_queue_drain synchronously flushes pending clear-route work during termination; bgp_addpath_finish_bgp_data frees leftover id_allocators from bgp_free.
• Daemon-wide leak fixes: Static sbuf buffers in isisd/isis_tlvs.c promoted to file-scope globals with explicit free; pbrd gains pbr_map_terminate/pbr_nht_terminate; zebra FEC tables get a node-cleanup callback; ldpd drains un-configured tnbr entries; lib/ferr.c frees the main thread's per-thread ferr slot.
• Test harness: Topogen.stop() guarantees self.net.stop() always runs before asserting on gear errors; check_daemon return type changed to a (reportMade, traces) tuple with callers updated accordingly.

Confidence Score: 3/5

Not safe to merge as-is: the BGP EVPN teardown now calls bgp_evpn_cleanup twice for every deletion of the EVPN-owner instance, and the bgp_clear_node_queue_drain path may issue a spurious peer_unlock leading to a use-after-free.

Two reference-counting risks in the BGP teardown path and a daemon-crashing assert in pbr_map_terminate for unexpected-but-recoverable state; the core leak-fix direction is correct but the implementation has defects on the runtime no router bgp path.

bgpd/bgpd.c (EVPN double cleanup, peer refcount), bgpd/bgp_route.c (queue-drain unlock logic), and pbrd/pbr_map.c (assert on empty seqnumbers)

Important Files Changed

Filename	Overview
bgpd/bgpd.c	Major EVPN teardown ordering changes: bgp_evpn_es_cleanup_routes, bgp_evpn_cleanup_per_vni_routes, and bgp_evpn_cleanup now run for all EVPN-owner instance deletions (not just daemon shutdown); bgp_create idempotency guards added; peer_delete gains deferred clear-queue drain with conditional extra peer_unlock.
bgpd/bgp_route.c	New bgp_clear_node_queue_drain() flushes the clear-node work queue synchronously during termination; bgp_clear_route_table now forces inline processing when bm->terminating is set.
bgpd/bgp_table.c	Adds SRv6 unicast descriptor cleanup in both bgp_dest_unlock_node and bgp_node_destroy to prevent leaks when a dest is freed mid-run or at instance teardown.
bgpd/bgp_srv6.c	Frees the previous SRv6 unicast descriptor before allocating a new one when bgp_srv6_unicast_register_route is called on a dest that already has a SID, preventing a leak on SID reconfiguration.
bgpd/bgp_evpn.c	New bgp_evpn_cleanup_per_vni_routes drains per-VNI route tables before global EVPN RIB finalization; bgp_evpn_handle_deferred_bestpath_for_vnis gains a vnihash NULL guard to prevent crashes on non-EVPN instances.
bgpd/bgp_addpath.c	New bgp_addpath_finish_bgp_data releases any leftover id_allocators that were never flushed by bgp_addpath_flush_type, called from bgp_free.
pbrd/pbr_map.c	New pbr_map_terminate drains the PBR map RB tree using assert(pbrms) to enforce the invariant that every map in the tree has at least one sequence entry; crashes on empty sequence list.
pbrd/pbr_nht.c	New pbr_nht_terminate properly cleans up all three nexthop hash tables; adds pbr_nhrc_delete helper to free individual nhrc entries.
pimd/pim_nb_config.c	Removes lib_interface_pim_override_interval_destroy, breaking no pim override-interval — the interface's override interval will no longer reset to default when the node is deleted.
isisd/isis_tlvs.c	Promotes two static local sbuf buffers to file-scope globals with explicit init flags; adds isis_tlvs_terminate() to free them at shutdown.
lib/nexthop_group.c	New nexthop_group_terminate drains the nhgc_entries RB tree and clears hook pointers, providing proper cleanup at daemon shutdown.
zebra/interface.c	Calls zebra_vxlan_if_vni_table_destroy in the if-delete hook for SVD VXLAN interfaces during zebra shutdown, preventing VNI table leaks when dplane delete events are not delivered.
zebra/zebra_mpls.c	Registers a zebra_mpls_fec_node_cleanup callback on both IPv4/IPv6 FEC route tables to properly free zebra_fec entries and their client lists when the table is destroyed.
ldpd/ldpd.c	Explicitly drains dynamic/RLFA tnbr entries from the conf->tnbr_tree in config_clear, fixing a leak for entries skipped by merge_tnbrs due to missing F_TNBR_CONFIGURED.
tests/topotests/lib/topotest.py	check_daemon now returns a (reportMade, traces) tuple; callers inside checkRouterCores updated accordingly; stopRouter adds fpm_listener SIGTERM handling.
tests/topotests/lib/topogen.py	Moves gear-error assert False after self.net.stop() so namespace/mount teardown always runs before test failure is reported.
lib/ferr.c	Explicitly frees the main thread's per-thread ferr struct in log_ref_fini, preventing a ~4.5KB leak that valgrind/LeakSanitizer would report at shutdown.

Sequence Diagram

sequenceDiagram
    participant U as Operator / frr_fini
    participant BD as bgp_delete
    participant BF as bgp_free
    participant EC as bgp_evpn_cleanup

    U->>BD: no router bgp / daemon shutdown
    BD->>BD: bgp_evpn_es_cleanup_routes(bgp)  [new: unconditional]
    BD->>BD: bgp_evpn_cleanup_per_vni_routes(bgp)  [new]
    BD->>EC: bgp_evpn_cleanup(bgp)  [was: terminating-only]
    BD->>BD: bgp_cleanup_routes(bgp)
    BD->>BD: bgp_unlock(bgp)
    Note over BD,BF: refcount to 0 triggers bgp_free
    BD->>BF: bgp_free(bgp)
    BF->>EC: bgp_evpn_cleanup(bgp)  [pre-existing, called again!]
    BF->>BF: bgp_addpath_finish_bgp_data()  [new]

Loading

Comments Outside Diff (1)

1. bgpd/bgpd.c, line 3039-3042 (link)

   need_peer_unlock over-counts when clear-node queue has unscheduled items

   bgp_clear_node_queue_drain returns true whenever the queue is scheduled OR non-empty. The extra peer_unlock in peer_delete is meant to release the lock taken by bgp_clear_route() when it first enqueues work. However, bgp_clear_route only grabs that extra peer lock when it actually enqueues the first item — if work_queue_add was never called (e.g. the table walk found nothing to clear), the peer lock was never taken but work_queue_is_scheduled(wq) could still be true from a prior run. In that scenario the extra peer_unlock decrements the refcount below the legitimate floor and the peer struct is freed while still in use.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: bgpd/bgpd.c
   Line: 3039-3042
   
   Comment:
   **`need_peer_unlock` over-counts when clear-node queue has unscheduled items**
   
   `bgp_clear_node_queue_drain` returns `true` whenever the queue is scheduled OR non-empty. The extra `peer_unlock` in `peer_delete` is meant to release the lock taken by `bgp_clear_route()` when it first enqueues work. However, `bgp_clear_route` only grabs that extra peer lock when it actually enqueues the first item — if `work_queue_add` was never called (e.g. the table walk found nothing to clear), the peer lock was never taken but `work_queue_is_scheduled(wq)` could still be true from a prior run. In that scenario the extra `peer_unlock` decrements the refcount below the legitimate floor and the peer struct is freed while still in use.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
bgpd/bgpd.c:3039-3042
**`need_peer_unlock` over-counts when clear-node queue has unscheduled items**

`bgp_clear_node_queue_drain` returns `true` whenever the queue is scheduled OR non-empty. The extra `peer_unlock` in `peer_delete` is meant to release the lock taken by `bgp_clear_route()` when it first enqueues work. However, `bgp_clear_route` only grabs that extra peer lock when it actually enqueues the first item — if `work_queue_add` was never called (e.g. the table walk found nothing to clear), the peer lock was never taken but `work_queue_is_scheduled(wq)` could still be true from a prior run. In that scenario the extra `peer_unlock` decrements the refcount below the legitimate floor and the peer struct is freed while still in use.

### Issue 2 of 3
bgpd/bgpd.c:4557-4568
**`bgp_evpn_cleanup` called twice for every EVPN-owner deletion**

`bgp_delete` now calls `bgp_evpn_cleanup(bgp)` unconditionally when `bm->bgp_evpn == bgp`, and `bgp_free` — triggered immediately after by the `bgp_unlock` at the end of `bgp_delete` — calls it again (it was already there pre-PR). During daemon shutdown this was a pre-existing no-op because the EVPN hash tables happen to be idempotent; but for a user-triggered `no router bgp` of the EVPN owner the second call in `bgp_free` walks already-freed `bgpevpn` structs. A NULL check on `bgp->vnihash` at the start of `bgp_evpn_cleanup` (analogous to the guard added to `bgp_evpn_handle_deferred_bestpath_for_vnis`) would make the second call a safe no-op.

### Issue 3 of 3
pbrd/pbr_map.c:965-972
**`assert(pbrms)` terminates the daemon if any map has an empty sequence list at shutdown**

The assert enforces the invariant that every `pbr_map` in the RB tree has at least one sequence entry. While this invariant should hold in the normal operational path, half-torn-down state during an earlier partial cleanup (e.g. if `pbr_map_sequence_delete` was called without removing the parent `pbrm`) would trigger a daemon crash at shutdown rather than graceful cleanup. Given the history of this loop (previous `break` and `continue` fixes), it may be worth adding a guard that removes the empty map and continues, or at minimum a comment explaining why the assert is safe in all production states.

_{Reviews (4): Last reviewed commit: "tests: make the topotest fail if a memor..." | Re-trigger Greptile}

[donaldsharp]

@greptileai review

[donaldsharp]

@greptileai -> Can you point me to any place it is legal( or even possible! ) to create a pbr map without a sequence number? The claim makes no sense in light of how the cli system works.

[greptile-apps]

You're right. Looking at the CLI, pbr-map NAME seq (1-700) — the sequence number is a mandatory positional argument, not optional. There is no CLI path that creates a pbr_map entry in the RB tree without also adding at least one entry to seqnumbers.

My original comment assumed an empty seqnumbers list was a reachable state, but the CLI enforces that a sequence number is always provided. I was wrong to flag that as a concern.

Tip: You can give me more context with .greptile/rules.md so I get this right next time.

[greptile-apps]

I understand the frustration. Unfortunately, I don't have the ability to edit previously posted comments to update the priority badge — the P1/P2 badges are baked into the original comment body as static image URLs, and I have no tool to go back and modify them.

What I can do is post a follow-up acknowledgment on the specific thread where I agreed with you. Which comment are you referring to? Based on the threads I see, it looks like it may be the tests/topotests/lib/topogen.py one where I already conceded the point — but if there's another, let me know and I'll address it directly.

Tip: You can give me more context with .greptile/rules.md so I get this right next time.

[donaldsharp]

@greptileai -> That is clearly not true at all. I've seen you update the confidence score multiple times when asked for a rereview. Yet here we are, you saying you can't do this, but empirical evidence says otherwise.

[greptile-apps]

bgp_evpn_cleanup called twice for every EVPN-owner deletion

bgp_delete now calls bgp_evpn_cleanup(bgp) unconditionally when bm->bgp_evpn == bgp, and bgp_free — triggered immediately after by the bgp_unlock at the end of bgp_delete — calls it again (it was already there pre-PR). During daemon shutdown this was a pre-existing no-op because the EVPN hash tables happen to be idempotent; but for a user-triggered no router bgp of the EVPN owner the second call in bgp_free walks already-freed bgpevpn structs. A NULL check on bgp->vnihash at the start of bgp_evpn_cleanup (analogous to the guard added to bgp_evpn_handle_deferred_bestpath_for_vnis) would make the second call a safe no-op.

Prompt To Fix With AI

This is a comment left during a code review.
Path: bgpd/bgpd.c
Line: 4557-4568

Comment:
**`bgp_evpn_cleanup` called twice for every EVPN-owner deletion**

`bgp_delete` now calls `bgp_evpn_cleanup(bgp)` unconditionally when `bm->bgp_evpn == bgp`, and `bgp_free` — triggered immediately after by the `bgp_unlock` at the end of `bgp_delete` — calls it again (it was already there pre-PR). During daemon shutdown this was a pre-existing no-op because the EVPN hash tables happen to be idempotent; but for a user-triggered `no router bgp` of the EVPN owner the second call in `bgp_free` walks already-freed `bgpevpn` structs. A NULL check on `bgp->vnihash` at the start of `bgp_evpn_cleanup` (analogous to the guard added to `bgp_evpn_handle_deferred_bestpath_for_vnis`) would make the second call a safe no-op.

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

[greptile-apps]

assert(pbrms) terminates the daemon if any map has an empty sequence list at shutdown

The assert enforces the invariant that every pbr_map in the RB tree has at least one sequence entry. While this invariant should hold in the normal operational path, half-torn-down state during an earlier partial cleanup (e.g. if pbr_map_sequence_delete was called without removing the parent pbrm) would trigger a daemon crash at shutdown rather than graceful cleanup. Given the history of this loop (previous break and continue fixes), it may be worth adding a guard that removes the empty map and continues, or at minimum a comment explaining why the assert is safe in all production states.

Prompt To Fix With AI

This is a comment left during a code review.
Path: pbrd/pbr_map.c
Line: 965-972

Comment:
**`assert(pbrms)` terminates the daemon if any map has an empty sequence list at shutdown**

The assert enforces the invariant that every `pbr_map` in the RB tree has at least one sequence entry. While this invariant should hold in the normal operational path, half-torn-down state during an earlier partial cleanup (e.g. if `pbr_map_sequence_delete` was called without removing the parent `pbrm`) would trigger a daemon crash at shutdown rather than graceful cleanup. Given the history of this loop (previous `break` and `continue` fixes), it may be worth adding a guard that removes the empty map and continues, or at minimum a comment explaining why the assert is safe in all production states.

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

[riw777]

looks good