isisd clean up: migrate lists to typesafe DLIST and fix minor memory leaks #20351
+4,288
−6,971
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Note: I did forget snmp, let's me provide fixes. |
Contributor
Author
|
ci:rerun |
Member
|
I'm seeing an issue with the CI not reporting the error correctly (we are investigating). But there are checkpatch failures: |
vjardin
force-pushed
the
isis_cleanup
branch
3 times, most recently
from
0df4056 to
627ad47
Compare
vjardin
added a commit
to vjardin/frr
that referenced
this pull request
Jan 4, 2026
Apply clang-format to source files that will be modified by subsequent commits in this branch. This ensures consistent code style before introducing new functionality. XXX Drop this commit once FRRouting#20351 will be merged. Signed-off-by: Vincent Jardin <[email protected]>
vjardin
added a commit
to vjardin/frr
that referenced
this pull request
Jan 4, 2026
Temporary squash of the pull request 20351 to fix checkpatch issues. This commit will be dropped when rebasing onto master after FRRouting#20351 merges. Original commits from 20351: - isisd: fix code formatting in files to be modified - isisd: migrate lists to typesafe DLIST - isisd: fix crash in isis_area_destroy destruction order - isisd: fix memory leak for MT settings static buffers - tests: fix minor memory leak in test_isis_lspdb Signed-off-by: Vincent Jardin <[email protected]>
Contributor
|
ci:rerun |
vjardin
force-pushed
the
isis_cleanup
branch
2 times, most recently
from
240db79 to
06fa666
Compare
donaldsharp
reviewed
Jan 5, 2026
donaldsharp
reviewed
Jan 5, 2026
donaldsharp
reviewed
Jan 5, 2026
riw777
approved these changes
Jan 6, 2026
Member
riw777
left a comment
riw777
left a comment
There was a problem hiding this comment.
looks good ... waiting on @donaldsharp 's comments
vjardin
force-pushed
the
isis_cleanup
branch
2 times, most recently
from
89f8f91 to
b925877
Compare
Member
|
My comment about the const frr_each was wrong, when I went back and looked at it closer. |
Contributor
Author
maybe not ;) I did try to unwind it... still WIP |
vjardin
force-pushed
the
isis_cleanup
branch
2 times, most recently
from
963a5cf to
f5a42d1
Compare
Add an explicit NULL check in the DLIST _pop function to satisfy static analyzers that cannot track the DLIST invariant. The DLIST implementation guarantees that hitem.next is never NULL - it either points to the first element or back to the sentinel for an empty list. However, static analyzers see a pointer loaded from a struct field and assume it could be NULL. The added NULL check: - Satisfies static analyzers that flag potential null dereferences - Zero runtime cost (compiler optimizes it away) Signed-off-by: Vincent Jardin <[email protected]>
Apply clang-format to source files that will be modified by subsequent commits in this branch. This ensures consistent code style before introducing new functionality. Signed-off-by: Vincent Jardin <[email protected]>
Migrate IS-IS lists from the legacy list API to FRR's typesafe.h DLIST macros: - isis_instance_list (im->isis) - isis_area_list (isis->area_list) - isis_circuit_list (area->circuit_list) - isis_area_adj_list (area->adjacency_list) Update iteration patterns from ALL_LIST_ELEMENTS_RO/ALL_LIST_ELEMENTS to frr_each/frr_each_safe, and replace listnode_add/listnode_delete with typesafe list operations. Also add spf_adj_list typesafe list for temporary SPF adjacency references, replacing the temporary list_new()/list_delete() pattern. Signed-off-by: Vincent Jardin <[email protected]>
Move lsp_db_fini() before circuit list cleanup in isis_area_destroy(). The lsp_destroy() function iterates over circuit_list to remove LSPs from tx queues. With typesafe lists, a finalized list is invalid (not NULL), so accessing it causes a crash. Signed-off-by: Vincent Jardin <[email protected]>
The multi-topology code in isis_mt.c uses three static buffers to avoid repeated memory allocations during MT settings lookups. These buffers are allocated using XREALLOC in area_mt_settings(), circuit_mt_settings(), and circuit_bcast_mt_set(), growing as needed but never freed during daemon shutdown. This causes the memory leak checker to report three "ISIS MT Area Setting" allocations per router at exit time, as the static buffers persist until process termination without being properly released. The fix moves these function-local static variables to file scope and adds a new mt_fini() cleanup function that frees all three buffers during daemon termination. This function is called from terminate() in isis_main.c alongside the other cleanup functions, ensuring proper memory release when isisd exits. Signed-off-by: Vincent Jardin <[email protected]>
AddressSanitizer detected the isis structure allocated in main() was never freed. This is a minor test code issue with no impact on isisd itself. Signed-off-by: Vincent Jardin <[email protected]>
Convert the isis_area->area_addrs field from the legacy struct list * to the FRR typesafe DLIST container. This change is part of the ongoing effort to migrate IS-IS data structures to typesafe containers for improved type safety and code clarity. Signed-off-by: Vincent Jardin <[email protected]>
Fix destruction order in isis_area_destroy() to prevent crashes when terminating areas with typesafe DLIST. Two issues were addressed: 1. Move isis_mpls_te_term() before circuit list cleanup because it iterates over circuit_list to delete extended sub-TLVs. 2. Move isis_sr_area_term() and isis_srv6_area_term() before isis_mpls_te_term() because SR adj_sids reference structures inside circuit->ext which isis_mpls_te_term() frees. By terminating SR first: - adj_sids are cleaned up while circuit->ext is still valid - SR gets disabled so subsequent adjacency hooks return early With typesafe DLIST, a finalized list is invalid (not NULL), and accessing freed structures causes heap-use-after-free. This fixes AddressSanitizer crashes in isis_mpls_te_term() and sr_adj_sid_del() during daemon termination. Signed-off-by: Vincent Jardin <[email protected]>
Move vrf_terminate() and prefix_list_reset() before isis_master_terminate(). Their callbacks (isis_vrf_disable, isis_prefix_list_update) iterate over im->isis list which is finalized by isis_master_terminate(). With typesafe DLIST, iterating a finalized list causes a SEGV crash. Signed-off-by: Vincent Jardin <[email protected]>
Contributor
Author
|
ci:rerun |
Contributor
Author
|
THANK YOU for those who did the reviews |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
the purpose: to align with more FRR's patterns.
Let's modernise the ISIS daemon by migrating from the legacy linklist API to FRR's typesafe DLIST macros, and fixes several memory issues discovered during testing with address sanitize.
The former legacy list API (list_new, ALL_LIST_ELEMENTS_RO, etc.) lacks type safety and has been superseded by the typesafe container macros in lib/typesafe.h. This migration improves code quality by enabling compile-time type checking and eliminating potential type confusion bugs.
During ASAN testing, three memory issues fixed: a destruction order bug causing crashes when deleting ISIS areas, leaked static buffers in the multi-topology code that were never freed at shutdown, and a minor leak in a unit test.
All changes have been validated with the ISIS topotest suite using AddressSanitizer-enabled binaries. The SPF, LSPDB, vertex queue, and TLV fuzz unit tests also pass cleanly.
This pull request will conflict with the TI-LFA, but I'd like to use those patterns for TI-LFA, so let's merge it before (#20348)