From 45559c4dfe3dca9312b9da4e78cf13563bd2acb8 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 21 Nov 2019 18:47:13 -0500 Subject: [PATCH 1/2] ospfd: nbr->oi is never null We test nbr->oi in a couple of places for null, but in the majority of places of the nbr->oi data is being used we just access it. Touch up code to trust this assertion and make the code more consistent in others. Found in Coverity. Signed-off-by: Donald Sharp --- ospfd/ospf_apiserver.c | 4 +--- ospfd/ospf_flood.c | 5 ++--- ospfd/ospf_neighbor.c | 4 +++- ospfd/ospf_nsm.c | 2 +- ospfd/ospf_packet.c | 4 +--- ospfd/ospf_snmp.c | 2 -- 6 files changed, 8 insertions(+), 13 deletions(-) diff --git a/ospfd/ospf_apiserver.c b/ospfd/ospf_apiserver.c index d6f1fba28b..bd703bc89d 100644 --- a/ospfd/ospf_apiserver.c +++ b/ospfd/ospf_apiserver.c @@ -2330,9 +2330,7 @@ void ospf_apiserver_clients_notify_nsm_change(struct ospf_neighbor *nbr) assert(nbr); - if (nbr->oi) { - ifaddr = nbr->oi->address->u.prefix4; - } + ifaddr = nbr->oi->address->u.prefix4; nbraddr = nbr->address.u.prefix4; diff --git a/ospfd/ospf_flood.c b/ospfd/ospf_flood.c index 381fb6820f..c29b464cab 100644 --- a/ospfd/ospf_flood.c +++ b/ospfd/ospf_flood.c @@ -328,8 +328,7 @@ int ospf_flood(struct ospf *ospf, struct ospf_neighbor *nbr, ospf_ls_retransmit_delete_nbr_as(ospf, current); break; default: - ospf_ls_retransmit_delete_nbr_area(nbr->oi->area, - current); + ospf_ls_retransmit_delete_nbr_area(oi->area, current); break; } } @@ -345,7 +344,7 @@ int ospf_flood(struct ospf *ospf, struct ospf_neighbor *nbr, procedure cannot overwrite the newly installed LSA until MinLSArrival seconds have elapsed. */ - if (!(new = ospf_lsa_install(ospf, nbr->oi, new))) + if (!(new = ospf_lsa_install(ospf, oi, new))) return -1; /* unknown LSA type or any other error condition */ /* Acknowledge the receipt of the LSA by sending a Link State diff --git a/ospfd/ospf_neighbor.c b/ospfd/ospf_neighbor.c index a9247dd0ec..46dfc505ef 100644 --- a/ospfd/ospf_neighbor.c +++ b/ospfd/ospf_neighbor.c @@ -141,6 +141,8 @@ void ospf_nbr_free(struct ospf_neighbor *nbr) thread_cancel_event(master, nbr); ospf_bfd_info_free(&nbr->bfd_info); + + nbr->oi = NULL; XFREE(MTYPE_OSPF_NEIGHBOR, nbr); } @@ -446,7 +448,7 @@ static struct ospf_neighbor *ospf_nbr_add(struct ospf_interface *oi, nbr->crypt_seqnum = ospfh->u.crypt.crypt_seqnum; if (IS_DEBUG_OSPF_EVENT) - zlog_debug("NSM[%s:%s]: start", IF_NAME(nbr->oi), + zlog_debug("NSM[%s:%s]: start", IF_NAME(oi), inet_ntoa(nbr->router_id)); return nbr; diff --git a/ospfd/ospf_nsm.c b/ospfd/ospf_nsm.c index 110738802c..9f6be3cbc7 100644 --- a/ospfd/ospf_nsm.c +++ b/ospfd/ospf_nsm.c @@ -224,7 +224,7 @@ static int ospf_db_summary_add(struct ospf_neighbor *nbr, struct ospf_lsa *lsa) case OSPF_OPAQUE_LINK_LSA: /* Exclude type-9 LSAs that does not have the same "oi" with * "nbr". */ - if (nbr->oi && ospf_if_exists(lsa->oi) != nbr->oi) + if (ospf_if_exists(lsa->oi) != nbr->oi) return 0; break; case OSPF_OPAQUE_AREA_LSA: diff --git a/ospfd/ospf_packet.c b/ospfd/ospf_packet.c index 8634589b11..80ffc3f361 100644 --- a/ospfd/ospf_packet.c +++ b/ospfd/ospf_packet.c @@ -3746,8 +3746,6 @@ int ospf_hello_reply_timer(struct thread *thread) nbr = THREAD_ARG(thread); nbr->t_hello_reply = NULL; - assert(nbr->oi); - if (IS_DEBUG_OSPF(nsm, NSM_TIMERS)) zlog_debug("NSM[%s:%s]: Timer (hello-reply timer expire)", IF_NAME(nbr->oi), inet_ntoa(nbr->router_id)); @@ -4335,7 +4333,7 @@ void ospf_proactively_arp(struct ospf_neighbor *nbr) char ping_nbr[OSPF_PING_NBR_STR_MAX]; int ret; - if (!nbr || !nbr->oi || !nbr->oi->ifp) + if (!nbr) return; snprintf(ping_nbr, sizeof(ping_nbr), diff --git a/ospfd/ospf_snmp.c b/ospfd/ospf_snmp.c index c26545344a..da3bc6f581 100644 --- a/ospfd/ospf_snmp.c +++ b/ospfd/ospf_snmp.c @@ -2257,8 +2257,6 @@ static uint8_t *ospfNbrEntry(struct variable *v, oid *name, size_t *length, if (!nbr) return NULL; oi = nbr->oi; - if (!oi) - return NULL; /* Return the current value of the variable */ switch (v->magic) { From 6d24b7cc08d6c95a538415bdc58614c076f1f9a7 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 21 Nov 2019 19:41:48 -0500 Subject: [PATCH 2/2] bgpd: Prevent possible SA thinking we'll divide by zero The half and reuse variables can never be 1 but the SA systems we have do not know this and think it is possible. Provide the kick in the snarples that the SA needs to know this is not true. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index fb2eb10dd9..48bf255994 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -12183,6 +12183,12 @@ DEFUN (bgp_damp_set, max = 4 * half; } + /* + * These can't be 0 but our SA doesn't understand the + * way our cli is constructed + */ + assert(reuse); + assert(half); if (suppress < reuse) { vty_out(vty, "Suppress value cannot be less than reuse value \n");