Skip to content

isisd: clear route-map reference on redistribute unset#22368

Open
Evelyn0828 wants to merge 1 commit into
FRRouting:masterfrom
Evelyn0828:fix/isis-redist-rmap-use-count
Open

isisd: clear route-map reference on redistribute unset#22368
Evelyn0828 wants to merge 1 commit into
FRRouting:masterfrom
Evelyn0828:fix/isis-redist-rmap-use-count

Conversation

@Evelyn0828

Copy link
Copy Markdown
Contributor

Release the route-map reference when an IS-IS redistribution entry is removed or when the area is torn down. This keeps show route-map unused consistent after the redistribution command is removed.

CLOSES #22361

@greptile-apps

greptile-apps Bot commented Jun 15, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes two related resource-management bugs in isisd/isis_redist.c: a memory leak of the map_name string and a missing route_map_counter_decrement call, both triggered when a redistribution entry is removed or an area is torn down.

  • isis_redist_unset: adds isis_redist_routemap_set(redist, NULL) before the struct is freed, which correctly frees map_name and decrements the route-map use counter (both were silently skipped in the original code).
  • isis_redist_area_finish: replaces the bare XFREE(MTYPE_ISIS_RMAP_NAME, redist->map_name) with isis_redist_routemap_set(redist, NULL), adding the missing counter decrement so show route-map unused reports correctly after area teardown.

Confidence Score: 5/5

Safe to merge — the change is a targeted cleanup of two resource-management paths with no behavioral changes to route advertisement logic.

Both hunks replace either a missing cleanup or an incomplete one with the existing isis_redist_routemap_set(redist, NULL) helper, which already handles the NULL case as a no-op. The struct is still freed after the helper returns, so there is no use-after-free. The area-finish loop uses ALL_LIST_ELEMENTS (safe for in-loop deletion). No new logic is introduced and the change is consistent with how the same helper is used elsewhere in the file.

No files require special attention.

Important Files Changed

Filename Overview
isisd/isis_redist.c Two targeted fixes: adds missing route-map counter decrement and map_name free in isis_redist_unset, and replaces bare XFREE with the full isis_redist_routemap_set helper in isis_redist_area_finish.

Sequence Diagram

sequenceDiagram
    participant CLI
    participant isis_redist_unset
    participant isis_redist_routemap_set
    participant route_map

    CLI->>isis_redist_unset: no redistribute ipv4 ospf
    isis_redist_unset->>isis_redist_unset: "redist->redist = 0"
    isis_redist_unset->>isis_redist_routemap_set: isis_redist_routemap_set(redist, NULL)
    isis_redist_routemap_set->>route_map: "route_map_counter_decrement(redist->map)"
    isis_redist_routemap_set->>isis_redist_routemap_set: "XFREE map_name, map = NULL"
    isis_redist_routemap_set-->>isis_redist_unset: done
    isis_redist_unset->>isis_redist_unset: listnode_delete + XFREE redist

    Note over CLI,route_map: isis_redist_area_finish follows the same path
Loading

Reviews (1): Last reviewed commit: "isisd: clear route-map ref on redistribu..." | Re-trigger Greptile

@Evelyn0828
Evelyn0828 force-pushed the fix/isis-redist-rmap-use-count branch 2 times, most recently from 83ccd93 to e4dc3d6 Compare June 15, 2026 10:33
@mjstapp

mjstapp commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Please use a real email address in your commit sign-offs

@riw777 riw777 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good ... but can you fix the sign off email?

Clear the route-map reference through the existing helper when IS-IS
redistribution is removed so the route-map use count is updated.

Signed-off-by: Evelyn0828 <jxshuibei@gmail.com>
@Evelyn0828
Evelyn0828 force-pushed the fix/isis-redist-rmap-use-count branch from e4dc3d6 to 755037a Compare June 18, 2026 06:50
@Evelyn0828

Copy link
Copy Markdown
Contributor Author

Sign-off email fixed (jxshuibei@gmail.com) and force-pushed.

@Evelyn0828

Copy link
Copy Markdown
Contributor Author

The [CI] Basic Tests failure is unrelated to this change: it's pim_cand_rp_bsr.test_pim_bsr_priority_modify (a PIM BSR candidate-RP priority test) failing on ASAN Debian 12 Part 9. This PR only touches isisd redistribute route-map cleanup and doesn't affect PIM. Looks like a flaky/unrelated test — could a maintainer kick off a re-run?

@mergify

mergify Bot commented Jun 19, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

isisd: no redistribute leaks route-map use count

3 participants