From aa5ced0ac866d1645075bef6325884dcb71a3703 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 31 Mar 2022 15:56:24 -0400 Subject: [PATCH] isisd, lib, ospfd, pathd: Null out free'd pointer The commands: router isis 1 mpls-te on no mpls-te on mpls-te on no mpls-te on ! Will crash Valgrind gives us this: ==652336== Invalid read of size 8 ==652336== at 0x49AB25C: typed_rb_min (typerb.c:495) ==652336== by 0x4943B54: vertices_const_first (link_state.h:424) ==652336== by 0x493DCE4: vertices_first (link_state.h:424) ==652336== by 0x493DADC: ls_ted_del_all (link_state.c:1010) ==652336== by 0x47E77B: isis_instance_mpls_te_destroy (isis_nb_config.c:1871) ==652336== by 0x495BE20: nb_callback_destroy (northbound.c:1131) ==652336== by 0x495B5AC: nb_callback_configuration (northbound.c:1356) ==652336== by 0x4958127: nb_transaction_process (northbound.c:1473) ==652336== by 0x4958275: nb_candidate_commit_apply (northbound.c:906) ==652336== by 0x49585B8: nb_candidate_commit (northbound.c:938) ==652336== by 0x495CE4A: nb_cli_classic_commit (northbound_cli.c:64) ==652336== by 0x495D6C5: nb_cli_apply_changes_internal (northbound_cli.c:250) ==652336== Address 0x6f928e0 is 272 bytes inside a block of size 320 free'd ==652336== at 0x48399AB: free (vg_replace_malloc.c:538) ==652336== by 0x494BA30: qfree (memory.c:141) ==652336== by 0x493D99D: ls_ted_del (link_state.c:997) ==652336== by 0x493DC20: ls_ted_del_all (link_state.c:1018) ==652336== by 0x47E77B: isis_instance_mpls_te_destroy (isis_nb_config.c:1871) ==652336== by 0x495BE20: nb_callback_destroy (northbound.c:1131) ==652336== by 0x495B5AC: nb_callback_configuration (northbound.c:1356) ==652336== by 0x4958127: nb_transaction_process (northbound.c:1473) ==652336== by 0x4958275: nb_candidate_commit_apply (northbound.c:906) ==652336== by 0x49585B8: nb_candidate_commit (northbound.c:938) ==652336== by 0x495CE4A: nb_cli_classic_commit (northbound_cli.c:64) ==652336== by 0x495D6C5: nb_cli_apply_changes_internal (northbound_cli.c:250) ==652336== Block was alloc'd at ==652336== at 0x483AB65: calloc (vg_replace_malloc.c:760) ==652336== by 0x494B6F8: qcalloc (memory.c:116) ==652336== by 0x493D7D2: ls_ted_new (link_state.c:967) ==652336== by 0x47E4DD: isis_instance_mpls_te_create (isis_nb_config.c:1832) ==652336== by 0x495BB29: nb_callback_create (northbound.c:1034) ==652336== by 0x495B547: nb_callback_configuration (northbound.c:1348) ==652336== by 0x4958127: nb_transaction_process (northbound.c:1473) ==652336== by 0x4958275: nb_candidate_commit_apply (northbound.c:906) ==652336== by 0x49585B8: nb_candidate_commit (northbound.c:938) ==652336== by 0x495CE4A: nb_cli_classic_commit (northbound_cli.c:64) ==652336== by 0x495D6C5: nb_cli_apply_changes_internal (northbound_cli.c:250) ==652336== by 0x495D23E: nb_cli_apply_changes (northbound_cli.c:268) Let's null out the pointer. After this change. Valgrind no longer reports issues and isisd no longer crashes. Fixes: #10939 Signed-off-by: Donald Sharp --- isisd/isis_nb_config.c | 2 +- lib/link_state.c | 19 ++++++++++--------- lib/link_state.h | 2 +- ospfd/ospf_te.c | 2 +- pathd/path_ted.c | 4 ++-- 5 files changed, 15 insertions(+), 14 deletions(-) diff --git a/isisd/isis_nb_config.c b/isisd/isis_nb_config.c index 019c26687b..de7797813a 100644 --- a/isisd/isis_nb_config.c +++ b/isisd/isis_nb_config.c @@ -1868,7 +1868,7 @@ int isis_instance_mpls_te_destroy(struct nb_cb_destroy_args *args) return NB_OK; /* Remove Link State Database */ - ls_ted_del_all(area->mta->ted); + ls_ted_del_all(&area->mta->ted); /* Flush LSP if circuit engage */ for (ALL_LIST_ELEMENTS_RO(area->circuit_list, node, circuit)) { diff --git a/lib/link_state.c b/lib/link_state.c index e4ccd0fb65..639a1d37d8 100644 --- a/lib/link_state.c +++ b/lib/link_state.c @@ -997,25 +997,26 @@ void ls_ted_del(struct ls_ted *ted) XFREE(MTYPE_LS_DB, ted); } -void ls_ted_del_all(struct ls_ted *ted) +void ls_ted_del_all(struct ls_ted **ted) { struct ls_vertex *vertex; struct ls_edge *edge; struct ls_subnet *subnet; - if (ted == NULL) + if (*ted == NULL) return; /* First remove Vertices, Edges and Subnets and associated Link State */ - frr_each_safe (vertices, &ted->vertices, vertex) - ls_vertex_del_all(ted, vertex); - frr_each_safe (edges, &ted->edges, edge) - ls_edge_del_all(ted, edge); - frr_each_safe (subnets, &ted->subnets, subnet) - ls_subnet_del_all(ted, subnet); + frr_each_safe (vertices, &(*ted)->vertices, vertex) + ls_vertex_del_all(*ted, vertex); + frr_each_safe (edges, &(*ted)->edges, edge) + ls_edge_del_all(*ted, edge); + frr_each_safe (subnets, &(*ted)->subnets, subnet) + ls_subnet_del_all(*ted, subnet); /* then remove TED itself */ - ls_ted_del(ted); + ls_ted_del(*ted); + *ted = NULL; } void ls_ted_clean(struct ls_ted *ted) diff --git a/lib/link_state.h b/lib/link_state.h index 761e8b6a27..f46a2068a1 100644 --- a/lib/link_state.h +++ b/lib/link_state.h @@ -746,7 +746,7 @@ extern void ls_ted_del(struct ls_ted *ted); * * @param ted Link State Data Base */ -extern void ls_ted_del_all(struct ls_ted *ted); +extern void ls_ted_del_all(struct ls_ted **ted); /** * Clean Link State Data Base by removing all Vertices, Edges and SubNets marked diff --git a/ospfd/ospf_te.c b/ospfd/ospf_te.c index 999bc49d91..2679873674 100644 --- a/ospfd/ospf_te.c +++ b/ospfd/ospf_te.c @@ -3908,7 +3908,7 @@ DEFUN (no_ospf_mpls_te, ote_debug("MPLS-TE: ON -> OFF"); /* Remove TED */ - ls_ted_del_all(OspfMplsTE.ted); + ls_ted_del_all(&OspfMplsTE.ted); OspfMplsTE.enabled = false; /* Flush all TE Opaque LSAs */ diff --git a/pathd/path_ted.c b/pathd/path_ted.c index 3440b93399..b30f38f6fb 100644 --- a/pathd/path_ted.c +++ b/pathd/path_ted.c @@ -66,7 +66,7 @@ uint32_t path_ted_teardown(void) PATH_TED_DEBUG("%s : TED [%p]", __func__, ted_state_g.ted); path_ted_unregister_vty(); path_ted_stop_importing_igp(); - ls_ted_del_all(ted_state_g.ted); + ls_ted_del_all(&ted_state_g.ted); path_ted_timer_sync_cancel(); path_ted_timer_refresh_cancel(); return 0; @@ -391,7 +391,7 @@ DEFUN (no_path_ted, } /* Remove TED */ - ls_ted_del_all(ted_state_g.ted); + ls_ted_del_all(&ted_state_g.ted); ted_state_g.enabled = false; PATH_TED_DEBUG("%s: PATHD-TED: ON -> OFF", __func__); ted_state_g.import = IMPORT_UNKNOWN;