From 4a07939b68450a2fa4dbf808316da5cc3149d754 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 17 May 2017 20:33:43 -0400 Subject: [PATCH 1/2] pimd: Thread changes allow pim to crash a boo When we add a thread pointer to thread_add_XXX functions when the specified function is called, thread.c is setting the thread pointer to NULL. This was causing pim to liberally pull it's zassert grenade pin's. Additionally clean up code to not set the NULL pointer. Signed-off-by: Donald Sharp --- pimd/pim_ifchannel.c | 4 ---- pimd/pim_igmp.c | 10 ---------- pimd/pim_igmpv3.c | 11 ----------- pimd/pim_mroute.c | 3 --- pimd/pim_msdp.c | 5 ----- pimd/pim_neighbor.c | 3 --- pimd/pim_pim.c | 2 -- pimd/pim_ssmpingd.c | 5 +---- pimd/pim_upstream.c | 11 +---------- pimd/pim_zebra.c | 4 ---- pimd/pim_zlookup.c | 5 ----- 11 files changed, 2 insertions(+), 61 deletions(-) diff --git a/pimd/pim_ifchannel.c b/pimd/pim_ifchannel.c index 77ff2aaa4e..d4dbcb383d 100644 --- a/pimd/pim_ifchannel.c +++ b/pimd/pim_ifchannel.c @@ -586,8 +586,6 @@ static int on_ifjoin_expiry_timer(struct thread *t) ch = THREAD_ARG(t); - ch->t_ifjoin_expiry_timer = NULL; - ifjoin_to_noinfo(ch); /* ch may have been deleted */ @@ -603,8 +601,6 @@ static int on_ifjoin_prune_pending_timer(struct thread *t) ch = THREAD_ARG(t); - ch->t_ifjoin_prune_pending_timer = NULL; - if (ch->ifjoin_state == PIM_IFJOIN_PRUNE_PENDING) { /* Send PruneEcho(S,G) ? */ diff --git a/pimd/pim_igmp.c b/pimd/pim_igmp.c index 66dfc069c9..76ec812b3f 100644 --- a/pimd/pim_igmp.c +++ b/pimd/pim_igmp.c @@ -155,7 +155,6 @@ static int pim_igmp_other_querier_expire(struct thread *t) igmp = THREAD_ARG(t); - zassert(igmp->t_other_querier_timer); zassert(!igmp->t_igmp_query_timer); if (PIM_DEBUG_IGMP_TRACE) { @@ -166,8 +165,6 @@ static int pim_igmp_other_querier_expire(struct thread *t) ifaddr_str); } - igmp->t_other_querier_timer = NULL; - /* We are the current querier, then re-start sending general queries. @@ -200,9 +197,7 @@ void pim_igmp_other_querier_timer_on(struct igmp_sock *igmp) zlog_debug("Querier %s resetting TIMER event for Other-Querier-Present", ifaddr_str); } - THREAD_OFF(igmp->t_other_querier_timer); - zassert(!igmp->t_other_querier_timer); } else { /* @@ -264,7 +259,6 @@ void pim_igmp_other_querier_timer_off(struct igmp_sock *igmp) } } THREAD_OFF(igmp->t_other_querier_timer); - zassert(!igmp->t_other_querier_timer); } static int @@ -964,7 +958,6 @@ static int igmp_group_timer(struct thread *t) zassert(group->group_filtermode_isexcl); - group->t_group_timer = NULL; group->group_filtermode_isexcl = 0; /* Any source (*,G) is forwarded only if mode is EXCLUDE {empty} */ @@ -972,7 +965,6 @@ static int igmp_group_timer(struct thread *t) igmp_source_delete_expired(group->group_source_list); - zassert(!group->t_group_timer); zassert(!group->group_filtermode_isexcl); /* @@ -999,9 +991,7 @@ static void group_timer_off(struct igmp_group *group) zlog_debug("Cancelling TIMER event for group %s on %s", group_str, group->group_igmp_sock->interface->name); } - THREAD_OFF(group->t_group_timer); - zassert(!group->t_group_timer); } void igmp_group_timer_on(struct igmp_group *group, diff --git a/pimd/pim_igmpv3.c b/pimd/pim_igmpv3.c index 51db9ac3b7..ad37ad8762 100644 --- a/pimd/pim_igmpv3.c +++ b/pimd/pim_igmpv3.c @@ -128,9 +128,6 @@ static int igmp_source_timer(struct thread *t) group->group_igmp_sock->interface->name); } - zassert(source->t_source_timer); - source->t_source_timer = NULL; - /* RFC 3376: 6.3. IGMPv3 Source-Specific Forwarding Rules @@ -151,8 +148,6 @@ static int igmp_source_timer(struct thread *t) Source timer switched from (T > 0) to (T == 0): disable forwarding. */ - zassert(!source->t_source_timer); - if (group->group_filtermode_isexcl) { /* EXCLUDE mode */ @@ -193,7 +188,6 @@ static void source_timer_off(struct igmp_group *group, } THREAD_OFF(source->t_source_timer); - zassert(!source->t_source_timer); } static void igmp_source_timer_on(struct igmp_group *group, @@ -216,7 +210,6 @@ static void igmp_source_timer_on(struct igmp_group *group, thread_add_timer_msec(master, igmp_source_timer, source, interval_msec, &source->t_source_timer); - zassert(source->t_source_timer); /* RFC 3376: 6.3. IGMPv3 Source-Specific Forwarding Rules @@ -470,8 +463,6 @@ source_new (struct igmp_group *group, listnode_add(group->group_source_list, src); - zassert(!src->t_source_timer); /* source timer == 0 */ - /* Any source (*,G) is forwarded only if mode is EXCLUDE {empty} */ igmp_anysource_forward_stop(group); @@ -1281,8 +1272,6 @@ static int igmp_group_retransmit(struct thread *t) num_retransmit_sources_left = group_retransmit_sources(group, send_with_sflag_set); - group->t_group_query_retransmit_timer = NULL; - /* Keep group retransmit timer running if there is any retransmit counter pending diff --git a/pimd/pim_mroute.c b/pimd/pim_mroute.c index c3d68e24d4..01747268a3 100644 --- a/pimd/pim_mroute.c +++ b/pimd/pim_mroute.c @@ -625,7 +625,6 @@ static int mroute_read(struct thread *t) } /* Keep reading */ done: - qpim_mroute_socket_reader = NULL; mroute_read_on(); return result; @@ -633,8 +632,6 @@ static int mroute_read(struct thread *t) static void mroute_read_on() { - zassert(!qpim_mroute_socket_reader); - thread_add_read(master, mroute_read, 0, qpim_mroute_socket_fd, &qpim_mroute_socket_reader); } diff --git a/pimd/pim_msdp.c b/pimd/pim_msdp.c index 001e080d5d..ed1284a95d 100644 --- a/pimd/pim_msdp.c +++ b/pimd/pim_msdp.c @@ -67,7 +67,6 @@ pim_msdp_sa_timer_expiry_log(struct pim_msdp_sa *sa, const char *timer_str) static int pim_msdp_sa_adv_timer_cb(struct thread *t) { - msdp->sa_adv_timer = NULL; if (PIM_DEBUG_MSDP_EVENTS) { zlog_debug("MSDP SA advertisment timer expired"); } @@ -93,7 +92,6 @@ pim_msdp_sa_state_timer_cb(struct thread *t) struct pim_msdp_sa *sa; sa = THREAD_ARG(t); - sa->sa_state_timer = NULL; if (PIM_DEBUG_MSDP_EVENTS) { pim_msdp_sa_timer_expiry_log(sa, "state"); @@ -898,7 +896,6 @@ pim_msdp_peer_hold_timer_cb(struct thread *t) struct pim_msdp_peer *mp; mp = THREAD_ARG(t); - mp->hold_timer = NULL; if (PIM_DEBUG_MSDP_EVENTS) { pim_msdp_peer_timer_expiry_log(mp, "hold"); @@ -932,7 +929,6 @@ pim_msdp_peer_ka_timer_cb(struct thread *t) struct pim_msdp_peer *mp; mp = THREAD_ARG(t); - mp->ka_timer = NULL; if (PIM_DEBUG_MSDP_EVENTS) { pim_msdp_peer_timer_expiry_log(mp, "ka"); @@ -994,7 +990,6 @@ pim_msdp_peer_cr_timer_cb(struct thread *t) struct pim_msdp_peer *mp; mp = THREAD_ARG(t); - mp->cr_timer = NULL; if (PIM_DEBUG_MSDP_EVENTS) { pim_msdp_peer_timer_expiry_log(mp, "connect-retry"); diff --git a/pimd/pim_neighbor.c b/pimd/pim_neighbor.c index 00190bd830..7583be858d 100644 --- a/pimd/pim_neighbor.c +++ b/pimd/pim_neighbor.c @@ -223,8 +223,6 @@ static int on_neighbor_timer(struct thread *t) neigh->holdtime, src_str, ifp->name); } - neigh->t_expire_timer = NULL; - snprintf(msg, sizeof(msg), "%d-sec holdtime expired", neigh->holdtime); pim_neighbor_delete(ifp, neigh, msg); @@ -278,7 +276,6 @@ on_neighbor_jp_timer (struct thread *t) zlog_debug("%s:Sending JP Agg to %s on %s with %d groups", __PRETTY_FUNCTION__, src_str, neigh->interface->name, neigh->upstream_jp_agg->count); } - neigh->jp_timer = NULL; rpf.source_nexthop.interface = neigh->interface; rpf.rpf_addr.u.prefix4 = neigh->source_addr; diff --git a/pimd/pim_pim.c b/pimd/pim_pim.c index 2a2f3060ab..f6a5bb1226 100644 --- a/pimd/pim_pim.c +++ b/pimd/pim_pim.c @@ -721,7 +721,6 @@ static int on_pim_hello_send(struct thread *t) /* * Schedule next hello */ - pim_ifp->t_pim_hello_timer = NULL; hello_resched(ifp); /* @@ -801,7 +800,6 @@ void pim_hello_restart_triggered(struct interface *ifp) } THREAD_OFF(pim_ifp->t_pim_hello_timer); - pim_ifp->t_pim_hello_timer = NULL; } random_msec = triggered_hello_delay_msec; diff --git a/pimd/pim_ssmpingd.c b/pimd/pim_ssmpingd.c index 1249e0ee57..c907d66d5a 100644 --- a/pimd/pim_ssmpingd.c +++ b/pimd/pim_ssmpingd.c @@ -316,12 +316,10 @@ static int ssmpingd_sock_read(struct thread *t) ss = THREAD_ARG(t); sock_fd = THREAD_FD(t); - zassert(sock_fd == ss->sock_fd); result = ssmpingd_read_msg(ss); /* Keep reading */ - ss->t_sock_read = 0; ssmpingd_read_on(ss); return result; @@ -329,7 +327,6 @@ static int ssmpingd_sock_read(struct thread *t) static void ssmpingd_read_on(struct ssmpingd_sock *ss) { - zassert(!ss->t_sock_read); thread_add_read(master, ssmpingd_sock_read, ss, ss->sock_fd, &ss->t_sock_read); } @@ -370,7 +367,7 @@ static struct ssmpingd_sock *ssmpingd_new(struct in_addr source_addr) } ss->sock_fd = sock_fd; - ss->t_sock_read = 0; + ss->t_sock_read = NULL; ss->source_addr = source_addr; ss->creation = pim_time_monotonic_sec(); ss->requests = 0; diff --git a/pimd/pim_upstream.c b/pimd/pim_upstream.c index c139c91a8c..49c0ad3fd9 100644 --- a/pimd/pim_upstream.c +++ b/pimd/pim_upstream.c @@ -280,8 +280,6 @@ static int on_join_timer(struct thread *t) up = THREAD_ARG(t); - up->t_join_timer = NULL; - /* * In the case of a HFR we will not ahve anyone to send this to. */ @@ -1075,7 +1073,6 @@ pim_upstream_keep_alive_timer (struct thread *t) struct pim_upstream *up; up = THREAD_ARG(t); - up->t_ka_timer = NULL; if (I_am_RP (up->sg.grp)) { @@ -1131,7 +1128,6 @@ pim_upstream_msdp_reg_timer(struct thread *t) struct pim_upstream *up; up = THREAD_ARG(t); - up->t_msdp_reg_timer = NULL; /* source is no longer active - pull the SA from MSDP's cache */ pim_msdp_sa_local_del(&up->sg); @@ -1322,8 +1318,6 @@ pim_upstream_register_stop_timer (struct thread *t) struct ip ip_hdr; up = THREAD_ARG (t); - up->t_rs_timer = NULL; - if (PIM_DEBUG_TRACE) { char state_str[PIM_REG_STATE_STR_LEN]; @@ -1384,10 +1378,7 @@ pim_upstream_start_register_stop_timer (struct pim_upstream *up, int null_regist uint32_t time; if (up->t_rs_timer) - { - THREAD_TIMER_OFF (up->t_rs_timer); - up->t_rs_timer = NULL; - } + THREAD_TIMER_OFF (up->t_rs_timer); if (!null_register) { diff --git a/pimd/pim_zebra.c b/pimd/pim_zebra.c index 4506e8cdb5..1f2b47c7db 100644 --- a/pimd/pim_zebra.c +++ b/pimd/pim_zebra.c @@ -579,10 +579,6 @@ void pim_scan_oil() static int on_rpf_cache_refresh(struct thread *t) { - zassert(qpim_rpf_cache_refresher); - - qpim_rpf_cache_refresher = 0; - /* update PIM protocol state */ scan_upstream_rpf_cache(); diff --git a/pimd/pim_zlookup.c b/pimd/pim_zlookup.c index 9c26745e77..e4ca046818 100644 --- a/pimd/pim_zlookup.c +++ b/pimd/pim_zlookup.c @@ -46,7 +46,6 @@ static int zclient_lookup_connect(struct thread *t) struct zclient *zlookup; zlookup = THREAD_ARG(t); - zlookup->t_connect = NULL; if (zlookup->sock >= 0) { return 0; @@ -61,7 +60,6 @@ static int zclient_lookup_connect(struct thread *t) zlookup->fail = 0; /* reset counter on connection */ } - zassert(!zlookup->t_connect); if (zlookup->sock < 0) { /* Since last connect failed, retry within 10 secs */ zclient_lookup_sched(zlookup, 10); @@ -74,8 +72,6 @@ static int zclient_lookup_connect(struct thread *t) /* Schedule connection with delay. */ static void zclient_lookup_sched(struct zclient *zlookup, int delay) { - zassert(!zlookup->t_connect); - thread_add_timer(master, zclient_lookup_connect, zlookup, delay, &zlookup->t_connect); @@ -86,7 +82,6 @@ static void zclient_lookup_sched(struct zclient *zlookup, int delay) /* Schedule connection for now. */ static void zclient_lookup_sched_now(struct zclient *zlookup) { - zassert(!zlookup->t_connect); thread_add_event(master, zclient_lookup_connect, zlookup, 0, &zlookup->t_connect); From 25c58d6d2bbec1de1ab21d76f8bd2cd6cd65df0b Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 18 May 2017 09:44:09 -0400 Subject: [PATCH 2/2] pimd: Address PR Comments Remove a bit more dead code and unused variable. Signed-off-by: Donald Sharp --- pimd/pim_ssmpingd.c | 3 --- pimd/pim_upstream.c | 3 +-- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/pimd/pim_ssmpingd.c b/pimd/pim_ssmpingd.c index c907d66d5a..dd92ff1b28 100644 --- a/pimd/pim_ssmpingd.c +++ b/pimd/pim_ssmpingd.c @@ -310,13 +310,10 @@ static int ssmpingd_read_msg(struct ssmpingd_sock *ss) static int ssmpingd_sock_read(struct thread *t) { struct ssmpingd_sock *ss; - int sock_fd; int result; ss = THREAD_ARG(t); - sock_fd = THREAD_FD(t); - result = ssmpingd_read_msg(ss); /* Keep reading */ diff --git a/pimd/pim_upstream.c b/pimd/pim_upstream.c index 49c0ad3fd9..d06703ccb3 100644 --- a/pimd/pim_upstream.c +++ b/pimd/pim_upstream.c @@ -1377,8 +1377,7 @@ pim_upstream_start_register_stop_timer (struct pim_upstream *up, int null_regist { uint32_t time; - if (up->t_rs_timer) - THREAD_TIMER_OFF (up->t_rs_timer); + THREAD_TIMER_OFF (up->t_rs_timer); if (!null_register) {