From 12a81f8eb104cb0cc3ee35230d1b623b4da289b0 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 12:26:57 -0400 Subject: [PATCH 01/13] ospfd: set external_info instance value to 0 This value is used but never set. Set it to zero to suppress static analysis errors. Signed-off-by: Quentin Young --- ospfd/ospf_lsa.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ospfd/ospf_lsa.c b/ospfd/ospf_lsa.c index fd3da45f78..6623790837 100644 --- a/ospfd/ospf_lsa.c +++ b/ospfd/ospf_lsa.c @@ -1756,6 +1756,7 @@ static struct ospf_lsa *ospf_lsa_translated_nssa_new(struct ospf *ospf, ei.route_map_set.metric = -1; ei.route_map_set.metric_type = -1; ei.tag = 0; + ei.instance = 0; if ((new = ospf_external_lsa_new(ospf, &ei, &type7->data->id)) == NULL) { From 44b301a24bc8cedf7e8fad95ea8c8b996ee7ef4b Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 12:43:00 -0400 Subject: [PATCH 02/13] ospf6d: remove ospf6_interface_if_del Unused and contains obvious NPD Signed-off-by: Quentin Young --- ospf6d/ospf6_interface.c | 22 ---------------------- ospf6d/ospf6_interface.h | 1 - ospf6d/ospf6_zebra.c | 7 ------- 3 files changed, 30 deletions(-) diff --git a/ospf6d/ospf6_interface.c b/ospf6d/ospf6_interface.c index 7c60749d7e..cc8577b94e 100644 --- a/ospf6d/ospf6_interface.c +++ b/ospf6d/ospf6_interface.c @@ -349,28 +349,6 @@ void ospf6_interface_if_add(struct interface *ifp) ospf6_interface_state_update(oi->interface); } -void ospf6_interface_if_del(struct interface *ifp) -{ - struct ospf6_interface *oi; - - oi = (struct ospf6_interface *)ifp->info; - if (oi == NULL) - return; - - /* interface stop */ - if (oi->area) - thread_execute(master, interface_down, oi, 0); - - listnode_delete(oi->area->if_list, oi); - oi->area = (struct ospf6_area *)NULL; - - /* cut link */ - oi->interface = NULL; - ifp->info = NULL; - - ospf6_interface_delete(oi); -} - void ospf6_interface_state_update(struct interface *ifp) { struct ospf6_interface *oi; diff --git a/ospf6d/ospf6_interface.h b/ospf6d/ospf6_interface.h index 553e89a227..8fd43f099a 100644 --- a/ospf6d/ospf6_interface.h +++ b/ospf6d/ospf6_interface.h @@ -176,7 +176,6 @@ extern void ospf6_interface_enable(struct ospf6_interface *); extern void ospf6_interface_disable(struct ospf6_interface *); extern void ospf6_interface_if_add(struct interface *); -extern void ospf6_interface_if_del(struct interface *); extern void ospf6_interface_state_update(struct interface *); extern void ospf6_interface_connected_route_update(struct interface *); diff --git a/ospf6d/ospf6_zebra.c b/ospf6d/ospf6_zebra.c index 8c2e706d17..8458d19952 100644 --- a/ospf6d/ospf6_zebra.c +++ b/ospf6d/ospf6_zebra.c @@ -127,13 +127,6 @@ static int ospf6_zebra_if_del(int command, struct zclient *zclient, zlog_debug("Zebra Interface delete: %s index %d mtu %d", ifp->name, ifp->ifindex, ifp->mtu6); -#if 0 - /* XXX: ospf6_interface_if_del is not the right way to handle this, - * because among other thinkable issues, it will also clear all - * settings as they are contained in the struct ospf6_interface. */ - ospf6_interface_if_del (ifp); -#endif /*0*/ - if_set_index(ifp, IFINDEX_INTERNAL); return 0; } From e0981960cdc9bf40d610267b0b743571db78ff35 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 12:54:42 -0400 Subject: [PATCH 03/13] bgpd: double-check notify data when debugging clang-analyze complains that data may be null, and since we didn't explicitly check it (although we did check the overall packet length minus the header length) it has a point. Signed-off-by: Quentin Young --- bgpd/bgp_packet.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index f0b30f0186..1a08a05126 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -697,13 +697,13 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code, bgp_notify.code = code; bgp_notify.subcode = sub_code; bgp_notify.data = NULL; - bgp_notify.length = length - BGP_MSG_NOTIFY_MIN_SIZE; + bgp_notify.length = datalen; bgp_notify.raw_data = data; peer->notify.code = bgp_notify.code; peer->notify.subcode = bgp_notify.subcode; - if (bgp_notify.length) { + if (bgp_notify.length && data) { bgp_notify.data = XMALLOC(MTYPE_TMP, bgp_notify.length * 3); for (i = 0; i < bgp_notify.length; i++) From 988258b4270733d1dcefd884256028f647c759e2 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 13:16:10 -0400 Subject: [PATCH 04/13] bgpd: move attr display into checked block Here we have a block conditional on the nullity of a pointer, followed by a dereferennce of the same pointer. Move the deref into the conditional block. Signed-off-by: Quentin Young --- bgpd/bgp_route.c | 52 ++++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index bd73edded3..a71f5ac956 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -7047,32 +7047,36 @@ void route_vty_out_overlay(struct vty *vty, struct prefix *p, default: vty_out(vty, "?"); } + + char *str = esi2str(&(attr->evpn_overlay.eth_s_id)); + + vty_out(vty, "%s", str); + XFREE(MTYPE_TMP, str); + + if (IS_EVPN_PREFIX_IPADDR_V4((struct prefix_evpn *)p)) { + vty_out(vty, "/%s", + inet_ntoa(attr->evpn_overlay.gw_ip.ipv4)); + } else if (IS_EVPN_PREFIX_IPADDR_V6((struct prefix_evpn *)p)) { + vty_out(vty, "/%s", + inet_ntop(AF_INET6, + &(attr->evpn_overlay.gw_ip.ipv6), buf, + BUFSIZ)); + } + if (attr->ecommunity) { + char *mac = NULL; + struct ecommunity_val *routermac = ecommunity_lookup( + attr->ecommunity, ECOMMUNITY_ENCODE_EVPN, + ECOMMUNITY_EVPN_SUBTYPE_ROUTERMAC); + if (routermac) + mac = ecom_mac2str((char *)routermac->val); + if (mac) { + vty_out(vty, "/%s", (char *)mac); + XFREE(MTYPE_TMP, mac); + } + } + vty_out(vty, "\n"); } - struct eth_segment_id *id = &(attr->evpn_overlay.eth_s_id); - char *str = esi2str(id); - vty_out(vty, "%s", str); - XFREE(MTYPE_TMP, str); - if (IS_EVPN_PREFIX_IPADDR_V4((struct prefix_evpn *)p)) { - vty_out(vty, "/%s", inet_ntoa(attr->evpn_overlay.gw_ip.ipv4)); - } else if (IS_EVPN_PREFIX_IPADDR_V6((struct prefix_evpn *)p)) { - vty_out(vty, "/%s", - inet_ntop(AF_INET6, &(attr->evpn_overlay.gw_ip.ipv6), - buf, BUFSIZ)); - } - if (attr->ecommunity) { - char *mac = NULL; - struct ecommunity_val *routermac = ecommunity_lookup( - attr->ecommunity, ECOMMUNITY_ENCODE_EVPN, - ECOMMUNITY_EVPN_SUBTYPE_ROUTERMAC); - if (routermac) - mac = ecom_mac2str((char *)routermac->val); - if (mac) { - vty_out(vty, "/%s", (char *)mac); - XFREE(MTYPE_TMP, mac); - } - } - vty_out(vty, "\n"); } /* dampening route */ From 1ec890a76b1f97e65ee2802a15ef8331f6b6a281 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 13:33:12 -0400 Subject: [PATCH 05/13] lib: ignore cli lexer in clang-analyze Lexer code is generated by Flex and we don't care about false positives in it. Signed-off-by: Quentin Young --- lib/command_lex.l | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/command_lex.l b/lib/command_lex.l index 530900659b..0d6e6ee7e5 100644 --- a/lib/command_lex.l +++ b/lib/command_lex.l @@ -23,6 +23,9 @@ */ %{ +/* ignore flex generated code in static analyzer */ +#ifndef __clang_analyzer__ + /* ignore harmless bugs in old versions of flex */ #pragma GCC diagnostic ignored "-Wsign-compare" #pragma GCC diagnostic ignored "-Wmissing-prototypes" @@ -91,3 +94,5 @@ void cleanup_lexer (yyscan_t *scn) // yy_delete_buffer (buffer, *scn); yylex_destroy(*scn); } + +#endif /* __clang_analyzer__ */ From bcfdc7878480f63b816811f2285862f47cde7340 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 13:58:53 -0400 Subject: [PATCH 06/13] ospfd: remove interface param npd OSPF_IF_PARAM_CONFIGURED(S, P) checks both the nullity of S and the value of P; assuming either one from the value of this macro is incorrect. Signed-off-by: Quentin Young --- ospfd/ospf_zebra.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ospfd/ospf_zebra.c b/ospfd/ospf_zebra.c index 23d00633d4..6487596706 100644 --- a/ospfd/ospf_zebra.c +++ b/ospfd/ospf_zebra.c @@ -118,7 +118,8 @@ static int ospf_interface_add(int command, struct zclient *zclient, assert(ifp->info); - if (!OSPF_IF_PARAM_CONFIGURED(IF_DEF_PARAMS(ifp), type)) { + if (IF_DEF_PARAMS(ifp) + && !OSPF_IF_PARAM_CONFIGURED(IF_DEF_PARAMS(ifp), type)) { SET_IF_PARAM(IF_DEF_PARAMS(ifp), type); IF_DEF_PARAMS(ifp)->type = ospf_default_iftype(ifp); } From 8934b81c69f43d81923133fba3989135692c9f34 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 14:05:32 -0400 Subject: [PATCH 07/13] ospf6d: assert that we set a variable Assert that prefix_lsa was set. Suppresses clang-analyze warnings. Signed-off-by: Quentin Young --- ospf6d/ospf6_abr.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ospf6d/ospf6_abr.c b/ospf6d/ospf6_abr.c index 01b8055b66..b895b5ad8b 100644 --- a/ospf6d/ospf6_abr.c +++ b/ospf6d/ospf6_abr.c @@ -786,6 +786,9 @@ void ospf6_abr_examin_summary(struct ospf6_lsa *lsa, struct ospf6_area *oa) /* (3) if the prefix is equal to an active configured address range */ /* or if the NU bit is set in the prefix */ if (lsa->header->type == htons(OSPF6_LSTYPE_INTER_PREFIX)) { + /* must have been set in previous block */ + assert(prefix_lsa); + range = ospf6_route_lookup(&prefix, oa->range_table); if (range) { if (is_debug) From 039c1e8d4f858661a4211ba16145d6c53c49b8ba Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 14:09:21 -0400 Subject: [PATCH 08/13] lib: add asserts on returned matcher vals These asserts verify that the status correlates with the expected result and fixes a clang-analyze warning. Signed-off-by: Quentin Young --- lib/command_match.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/command_match.c b/lib/command_match.c index f6b07a0b20..99ec03e0c2 100644 --- a/lib/command_match.c +++ b/lib/command_match.c @@ -99,6 +99,9 @@ enum matcher_rv command_match(struct graph *cmdgraph, vector vline, struct listnode *head = listhead(*argv); struct listnode *tail = listtail(*argv); + assert(head); + assert(tail); + // delete dummy start node cmd_token_del((struct cmd_token *)head->data); list_delete_node(*argv, head); From 316f27e169adaed23985c10e53572b23944e8470 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 14:14:06 -0400 Subject: [PATCH 09/13] bgpd: rfapi xcallocs guaranteed non-null The return value of XCALLOC will always be non-null. Even if it were to be null, this code would still crash with a NPD. Signed-off-by: Quentin Young --- bgpd/rfapi/bgp_rfapi_cfg.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/bgpd/rfapi/bgp_rfapi_cfg.c b/bgpd/rfapi/bgp_rfapi_cfg.c index 8c4d5ab043..3e9722d920 100644 --- a/bgpd/rfapi/bgp_rfapi_cfg.c +++ b/bgpd/rfapi/bgp_rfapi_cfg.c @@ -548,13 +548,12 @@ rfapi_group_new(struct bgp *bgp, rfapi_group_cfg_type_t type, const char *name) rfg = XCALLOC(MTYPE_RFAPI_GROUP_CFG, sizeof(struct rfapi_nve_group_cfg)); - if (rfg) { - rfg->type = type; - rfg->name = strdup(name); - /* add to tail of list */ - listnode_add(bgp->rfapi_cfg->nve_groups_sequential, rfg); - } + rfg->type = type; + rfg->name = strdup(name); + /* add to tail of list */ + listnode_add(bgp->rfapi_cfg->nve_groups_sequential, rfg); rfg->label = MPLS_LABEL_NONE; + QOBJ_REG(rfg, rfapi_nve_group_cfg); return rfg; From aaf24c74e44438a55decfc1e2f0930f800a3a650 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 15:19:23 -0400 Subject: [PATCH 10/13] babeld: be more explicit about route resize result Resizing the route array can fail. Although the error condition is already correctly handled, if we're more explicit about the variables we expect to be initialized then clang-analyze is happier. Signed-off-by: Quentin Young --- babeld/route.c | 1 + 1 file changed, 1 insertion(+) diff --git a/babeld/route.c b/babeld/route.c index 501dd1f4df..bc7590fb39 100644 --- a/babeld/route.c +++ b/babeld/route.c @@ -176,6 +176,7 @@ insert_route(struct babel_route *route) resize_route_table(max_route_slots < 1 ? 8 : 2 * max_route_slots); if(route_slots >= max_route_slots) return NULL; + assert(routes); route->next = NULL; if(n < route_slots) memmove(routes + n + 1, routes + n, From 267fa38ed36a5960e8cba5303dd3abee1ff7d7a6 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 15:21:55 -0400 Subject: [PATCH 11/13] ospf6d: assert nh list is non-null clang-analyze Signed-off-by: Quentin Young --- ospf6d/ospf6_intra.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ospf6d/ospf6_intra.c b/ospf6d/ospf6_intra.c index 519c5e170f..de4ee2e1ac 100644 --- a/ospf6d/ospf6_intra.c +++ b/ospf6d/ospf6_intra.c @@ -1844,6 +1844,7 @@ void ospf6_intra_prefix_lsa_remove(struct ospf6_lsa *lsa) INTRA_PREFIX)) { prefix2str(&route->prefix, buf, sizeof(buf)); + assert(route->nh_list); zlog_debug("%s: route %s update paths %u nh %u" , __PRETTY_FUNCTION__, buf, From 2165e2eaf97b648547c1143821d97b7f80467799 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 15:26:28 -0400 Subject: [PATCH 12/13] bgpd: verify that multipath infos are set Makes clang-analyze happy Signed-off-by: Quentin Young --- bgpd/bgp_mpath.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bgpd/bgp_mpath.c b/bgpd/bgp_mpath.c index e33f3d3477..915387ca2d 100644 --- a/bgpd/bgp_mpath.c +++ b/bgpd/bgp_mpath.c @@ -592,6 +592,8 @@ void bgp_info_mpath_update(struct bgp_node *rn, struct bgp_info *new_best, */ new_mpath = listgetdata(mp_node); list_delete_node(mp_list, mp_node); + assert(new_mpath); + assert(prev_mpath); if ((mpath_count < maxpaths) && (new_mpath != new_best) && bgp_info_nexthop_cmp(prev_mpath, new_mpath)) { if (new_mpath == next_mpath) From bd6b2706b31f49df13927a055e2e824d3357ce6a Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 11 Apr 2018 16:22:23 -0400 Subject: [PATCH 13/13] bgpd: remove unused variable Signed-off-by: Quentin Young --- bgpd/bgp_packet.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 1a08a05126..9ea2f22cec 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -649,7 +649,6 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code, uint8_t sub_code, uint8_t *data, size_t datalen) { struct stream *s; - int length; /* Lock I/O mutex to prevent other threads from pushing packets */ pthread_mutex_lock(&peer->io_mtx); @@ -670,7 +669,7 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code, stream_write(s, data, datalen); /* Set BGP packet length. */ - length = bgp_packet_set_size(s); + bgp_packet_set_size(s); /* wipe output buffer */ stream_fifo_clean(peer->obuf);