From 5b8524f5c29d0fc8a5ea2f6355e90b85b3439709 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Sat, 21 Oct 2017 10:24:21 -0200 Subject: [PATCH 1/6] lib: fix coverity warnings introduced by the iface rb-tree conversion Signed-off-by: Renato Westphal --- lib/if.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/if.c b/lib/if.c index 320dfba4b5..0fe7da1c0d 100644 --- a/lib/if.c +++ b/lib/if.c @@ -194,7 +194,10 @@ void if_delete_retain(struct interface *ifp) /* Delete and free interface structure. */ void if_delete(struct interface *ifp) { - struct vrf *vrf = vrf_lookup_by_id(ifp->vrf_id); + struct vrf *vrf; + + vrf = vrf_lookup_by_id(ifp->vrf_id); + assert(vrf); IFNAME_RB_REMOVE(vrf, ifp); if (ifp->ifindex != IFINDEX_INTERNAL) @@ -213,9 +216,13 @@ void if_delete(struct interface *ifp) /* Interface existance check by index. */ struct interface *if_lookup_by_index(ifindex_t ifindex, vrf_id_t vrf_id) { - struct vrf *vrf = vrf_lookup_by_id(vrf_id); + struct vrf *vrf; struct interface if_tmp; + vrf = vrf_lookup_by_id(vrf_id); + if (!vrf) + return NULL; + if_tmp.ifindex = ifindex; return RB_FIND(if_index_head, &vrf->ifaces_by_index, &if_tmp); } @@ -244,7 +251,8 @@ struct interface *if_lookup_by_name(const char *name, vrf_id_t vrf_id) struct vrf *vrf = vrf_lookup_by_id(vrf_id); struct interface if_tmp; - if (!name || strnlen(name, INTERFACE_NAMSIZ) == INTERFACE_NAMSIZ) + if (!vrf || !name + || strnlen(name, INTERFACE_NAMSIZ) == INTERFACE_NAMSIZ) return NULL; strlcpy(if_tmp.name, name, sizeof(if_tmp.name)); @@ -388,7 +396,10 @@ struct interface *if_get_by_name(const char *name, vrf_id_t vrf_id, int vty) void if_set_index(struct interface *ifp, ifindex_t ifindex) { - struct vrf *vrf = vrf_lookup_by_id(ifp->vrf_id); + struct vrf *vrf; + + vrf = vrf_lookup_by_id(ifp->vrf_id); + assert(vrf); if (ifp->ifindex == ifindex) return; From efd7904eabd3a79e14301b1a2005e36347293d4a Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Fri, 20 Oct 2017 22:16:57 -0200 Subject: [PATCH 2/6] *: add missing \n in some help strings Signed-off-by: Renato Westphal --- bgpd/bgp_vty.c | 6 ++-- bgpd/rfp-example/librfp/rfp_example.c | 2 +- eigrpd/eigrp_vty.c | 5 ++- isisd/isisd.c | 2 +- lib/command.c | 2 +- lib/keychain.c | 8 ++--- nhrpd/nhrp_vty.c | 4 +-- ospfd/ospf_ri.c | 2 +- ospfd/ospf_vty.c | 52 ++++++++++++++++----------- zebra/debug.c | 2 +- zebra/interface.c | 2 +- zebra/zebra_pw.c | 2 +- zebra/zserv.c | 8 ++--- 13 files changed, 54 insertions(+), 43 deletions(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 02a194050c..749c9d25d4 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -3794,7 +3794,7 @@ DEFUN (neighbor_remove_private_as_all, NEIGHBOR_STR NEIGHBOR_ADDR_STR2 "Remove private ASNs in outbound updates\n" - "Apply to all AS numbers") + "Apply to all AS numbers\n") { int idx_peer = 1; return peer_af_flag_set_vty(vty, argv[idx_peer]->arg, bgp_node_afi(vty), @@ -5733,7 +5733,7 @@ DEFUN (neighbor_maximum_prefix_restart, "Maximum number of prefix accept from this peer\n" "maximum no. of prefix limit\n" "Restart bgp connection after limit is exceeded\n" - "Restart interval in minutes") + "Restart interval in minutes\n") { int idx_peer = 1; int idx_number = 3; @@ -5751,7 +5751,7 @@ ALIAS_HIDDEN( "Maximum number of prefix accept from this peer\n" "maximum no. of prefix limit\n" "Restart bgp connection after limit is exceeded\n" - "Restart interval in minutes") + "Restart interval in minutes\n") DEFUN (neighbor_maximum_prefix_threshold_restart, neighbor_maximum_prefix_threshold_restart_cmd, diff --git a/bgpd/rfp-example/librfp/rfp_example.c b/bgpd/rfp-example/librfp/rfp_example.c index fb112a8865..cde2d7b352 100644 --- a/bgpd/rfp-example/librfp/rfp_example.c +++ b/bgpd/rfp-example/librfp/rfp_example.c @@ -42,7 +42,7 @@ DEFUN (rfp_example_config_value, "rfp example-config-value VALUE", RFP_SHOW_STR "Example value to be configured\n" - "Value to display") + "Value to display\n") { uint32_t value = 0; struct rfp_instance_t *rfi = NULL; diff --git a/eigrpd/eigrp_vty.c b/eigrpd/eigrp_vty.c index 34a07f5fe3..5fc7818166 100644 --- a/eigrpd/eigrp_vty.c +++ b/eigrpd/eigrp_vty.c @@ -870,11 +870,10 @@ DEFUN (no_eigrp_ip_summary_address, DEFUN (no_eigrp_if_ip_holdinterval, no_eigrp_if_ip_holdinterval_cmd, "no ip hold-time eigrp", - "No" + NO_STR "Interface Internet Protocol config commands\n" "Configures EIGRP hello interval\n" - "Enhanced Interior Gateway Routing Protocol (EIGRP)\n" - "Seconds before neighbor is considered down\n") + "Enhanced Interior Gateway Routing Protocol (EIGRP)\n") { VTY_DECLVAR_CONTEXT(interface, ifp); struct eigrp_interface *ei = ifp->info; diff --git a/isisd/isisd.c b/isisd/isisd.c index 5dd348089d..d4e5a4e29b 100644 --- a/isisd/isisd.c +++ b/isisd/isisd.c @@ -1509,7 +1509,7 @@ DEFUN_NOSH (router_isis, "router isis WORD", ROUTER_STR "ISO IS-IS\n" - "ISO Routing area tag") + "ISO Routing area tag\n") { int idx_word = 2; return isis_area_get(vty, argv[idx_word]->arg); diff --git a/lib/command.c b/lib/command.c index 97eba96c3a..2e91d84bf3 100644 --- a/lib/command.c +++ b/lib/command.c @@ -1367,7 +1367,7 @@ DEFUN (config_quit, DEFUN (config_end, config_end_cmd, "end", - "End current mode and change to enable mode.") + "End current mode and change to enable mode.\n") { switch (vty->node) { case VIEW_NODE: diff --git a/lib/keychain.c b/lib/keychain.c index 23a2d72b17..bb2c173354 100644 --- a/lib/keychain.c +++ b/lib/keychain.c @@ -633,7 +633,7 @@ DEFUN (accept_lifetime_infinite_day_month, "Day of th month to start\n" "Month of the year to start\n" "Year to start\n" - "Never expires") + "Never expires\n") { int idx_hhmmss = 1; int idx_number = 2; @@ -654,7 +654,7 @@ DEFUN (accept_lifetime_infinite_month_day, "Month of the year to start\n" "Day of th month to start\n" "Year to start\n" - "Never expires") + "Never expires\n") { int idx_hhmmss = 1; int idx_month = 2; @@ -843,7 +843,7 @@ DEFUN (send_lifetime_infinite_day_month, "Day of th month to start\n" "Month of the year to start\n" "Year to start\n" - "Never expires") + "Never expires\n") { int idx_hhmmss = 1; int idx_number = 2; @@ -864,7 +864,7 @@ DEFUN (send_lifetime_infinite_month_day, "Month of the year to start\n" "Day of th month to start\n" "Year to start\n" - "Never expires") + "Never expires\n") { int idx_hhmmss = 1; int idx_month = 2; diff --git a/nhrpd/nhrp_vty.c b/nhrpd/nhrp_vty.c index ab052ac04a..e0d0268e41 100644 --- a/nhrpd/nhrp_vty.c +++ b/nhrpd/nhrp_vty.c @@ -438,7 +438,7 @@ DEFUN(if_nhrp_mtu, if_nhrp_mtu_cmd, NHRP_STR "Configure NHRP advertised MTU\n" "MTU value\n" - "Advertise bound interface MTU similar to OpenNHRP") + "Advertise bound interface MTU similar to OpenNHRP\n") { VTY_DECLVAR_CONTEXT(interface, ifp); struct nhrp_interface *nifp = ifp->info; @@ -461,7 +461,7 @@ DEFUN(if_no_nhrp_mtu, if_no_nhrp_mtu_cmd, NHRP_STR "Configure NHRP advertised MTU\n" "MTU value\n" - "Advertise bound interface MTU similar to OpenNHRP") + "Advertise bound interface MTU similar to OpenNHRP\n") { VTY_DECLVAR_CONTEXT(interface, ifp); struct nhrp_interface *nifp = ifp->info; diff --git a/ospfd/ospf_ri.c b/ospfd/ospf_ri.c index 69f6883186..ead6923435 100644 --- a/ospfd/ospf_ri.c +++ b/ospfd/ospf_ri.c @@ -1119,7 +1119,7 @@ DEFUN (router_info, OSPF_RI_STR "Enable the Router Information functionality with AS flooding scope\n" "Enable the Router Information functionality with Area flooding scope\n" - "OSPF area ID in IP format") + "OSPF area ID in IP format\n") { int idx_ipv4 = 2; char *area = (argc == 3) ? argv[idx_ipv4]->arg : NULL; diff --git a/ospfd/ospf_vty.c b/ospfd/ospf_vty.c index df3cd16c1c..f349979d18 100644 --- a/ospfd/ospf_vty.c +++ b/ospfd/ospf_vty.c @@ -6243,7 +6243,7 @@ DEFUN (ip_ospf_authentication, "IP Information\n" "OSPF interface commands\n" "Enable authentication on this interface\n" - "Address of interface") + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx_ipv4 = 3; @@ -6280,7 +6280,7 @@ DEFUN (no_ip_ospf_authentication_args, "Enable authentication on this interface\n" "Use null authentication\n" "Use message-digest authentication\n" - "Address of interface") + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx_encryption = 4; @@ -6360,7 +6360,7 @@ DEFUN (no_ip_ospf_authentication, "IP Information\n" "OSPF interface commands\n" "Enable authentication on this interface\n" - "Address of interface") + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx_ipv4 = 4; @@ -6437,7 +6437,7 @@ DEFUN (ip_ospf_authentication_key, "OSPF interface commands\n" "Authentication password (key)\n" "The OSPF password (key)\n" - "Address of interface") + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx = 0; @@ -6649,7 +6649,9 @@ DEFUN_HIDDEN (no_ospf_message_digest_key, "OSPF interface commands\n" "Message digest authentication password (key)\n" "Key ID\n" - "Address of interface") + "Use MD5 algorithm\n" + "The OSPF password (key)\n" + "Address of interface\n") { return no_ip_ospf_message_digest_key(self, vty, argc, argv); } @@ -6712,9 +6714,11 @@ DEFUN (no_ip_ospf_cost, no_ip_ospf_cost_cmd, "no ip ospf cost [(1-65535)] [A.B.C.D]", NO_STR + "IP Information\n" "OSPF interface commands\n" "Interface cost\n" - "Address of interface") + "Cost\n" + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx = 0; @@ -6907,7 +6911,10 @@ DEFUN (no_ip_ospf_dead_interval, "OSPF interface commands\n" "Interval time after which a neighbor is declared down\n" "Seconds\n" - "Address of interface") + "Minimal 1s dead-interval with fast sub-second hellos\n" + "Hello multiplier factor\n" + "Number of Hellos to send each second\n" + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx_ipv4 = argc - 1; @@ -6969,7 +6976,10 @@ DEFUN_HIDDEN (no_ospf_dead_interval, "OSPF interface commands\n" "Interval time after which a neighbor is declared down\n" "Seconds\n" - "Address of interface") + "Minimal 1s dead-interval with fast sub-second hellos\n" + "Hello multiplier factor\n" + "Number of Hellos to send each second\n" + "Address of interface\n") { return no_ip_ospf_dead_interval(self, vty, argc, argv); } @@ -7198,7 +7208,7 @@ DEFUN (ip_ospf_priority, "OSPF interface commands\n" "Router priority\n" "Priority\n" - "Address of interface") + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx = 0; @@ -7246,7 +7256,7 @@ DEFUN_HIDDEN (ospf_priority, "OSPF interface commands\n" "Router priority\n" "Priority\n" - "Address of interface") + "Address of interface\n") { return ip_ospf_priority(self, vty, argc, argv); } @@ -7259,7 +7269,7 @@ DEFUN (no_ip_ospf_priority, "OSPF interface commands\n" "Router priority\n" // ignored "Priority\n" - "Address of interface") + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx = 0; @@ -7311,7 +7321,7 @@ DEFUN_HIDDEN (no_ospf_priority, "OSPF interface commands\n" "Router priority\n" "Priority\n" - "Address of interface") + "Address of interface\n") { return no_ip_ospf_priority(self, vty, argc, argv); } @@ -7323,7 +7333,7 @@ DEFUN (ip_ospf_retransmit_interval, "OSPF interface commands\n" "Time between retransmitting lost link state advertisements\n" "Seconds\n" - "Address of interface") + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx = 0; @@ -7358,7 +7368,7 @@ DEFUN_HIDDEN (ospf_retransmit_interval, "OSPF interface commands\n" "Time between retransmitting lost link state advertisements\n" "Seconds\n" - "Address of interface") + "Address of interface\n") { return ip_ospf_retransmit_interval(self, vty, argc, argv); } @@ -7422,7 +7432,7 @@ DEFUN (ip_ospf_transmit_delay, "OSPF interface commands\n" "Link state transmit delay\n" "Seconds\n" - "Address of interface") + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx = 0; @@ -7457,7 +7467,7 @@ DEFUN_HIDDEN (ospf_transmit_delay, "OSPF interface commands\n" "Link state transmit delay\n" "Seconds\n" - "Address of interface") + "Address of interface\n") { return ip_ospf_transmit_delay(self, vty, argc, argv); } @@ -7469,7 +7479,8 @@ DEFUN (no_ip_ospf_transmit_delay, "IP Information\n" "OSPF interface commands\n" "Link state transmit delay\n" - "Address of interface") + "Seconds\n" + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx = 0; @@ -7509,7 +7520,7 @@ DEFUN_HIDDEN (no_ospf_transmit_delay, "OSPF interface commands\n" "Link state transmit delay\n" "Seconds\n" - "Address of interface") + "Address of interface\n") { return no_ip_ospf_transmit_delay(self, vty, argc, argv); } @@ -8203,7 +8214,7 @@ DEFUN (ip_ospf_mtu_ignore, "IP Information\n" "OSPF interface commands\n" "Disable MTU mismatch detection on this interface\n" - "Address of interface") + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx_ipv4 = 3; @@ -8239,10 +8250,11 @@ DEFUN (ip_ospf_mtu_ignore, DEFUN (no_ip_ospf_mtu_ignore, no_ip_ospf_mtu_ignore_addr_cmd, "no ip ospf mtu-ignore [A.B.C.D]", + NO_STR "IP Information\n" "OSPF interface commands\n" "Disable MTU mismatch detection on this interface\n" - "Address of interface") + "Address of interface\n") { VTY_DECLVAR_CONTEXT(interface, ifp); int idx_ipv4 = 4; diff --git a/zebra/debug.c b/zebra/debug.c index ac96051abd..1df547a023 100644 --- a/zebra/debug.c +++ b/zebra/debug.c @@ -138,7 +138,7 @@ DEFUN (debug_zebra_vxlan, DEFUN (debug_zebra_pw, debug_zebra_pw_cmd, "[no] debug zebra pseudowires", - "Negate a command or set its defaults\n" + NO_STR DEBUG_STR "Zebra configuration\n" "Debug option set for zebra pseudowires\n") diff --git a/zebra/interface.c b/zebra/interface.c index e912b2dcf8..dd1050ee7f 100644 --- a/zebra/interface.c +++ b/zebra/interface.c @@ -2539,7 +2539,7 @@ DEFUN (no_ip_address, NO_STR "Interface Internet Protocol config commands\n" "Set the IP address of an interface\n" - "IP Address (e.g. 10.0.0.1/8)") + "IP Address (e.g. 10.0.0.1/8)\n") { int idx_ipv4_prefixlen = 3; VTY_DECLVAR_CONTEXT(interface, ifp); diff --git a/zebra/zebra_pw.c b/zebra/zebra_pw.c index 0ca7e34b23..d3492fb41c 100644 --- a/zebra/zebra_pw.c +++ b/zebra/zebra_pw.c @@ -436,7 +436,7 @@ DEFUN (show_pseudowires, "show mpls pseudowires", SHOW_STR MPLS_STR - "Pseudowires") + "Pseudowires\n") { struct zebra_vrf *zvrf; struct zebra_pw *pw; diff --git a/zebra/zserv.c b/zebra/zserv.c index 6295de0c2e..15bfd37da8 100644 --- a/zebra/zserv.c +++ b/zebra/zserv.c @@ -2800,7 +2800,7 @@ DEFUN (ip_forwarding, ip_forwarding_cmd, "ip forwarding", IP_STR - "Turn on IP forwarding") + "Turn on IP forwarding\n") { int ret; @@ -2821,7 +2821,7 @@ DEFUN (no_ip_forwarding, "no ip forwarding", NO_STR IP_STR - "Turn off IP forwarding") + "Turn off IP forwarding\n") { int ret; @@ -2967,7 +2967,7 @@ DEFUN (ipv6_forwarding, ipv6_forwarding_cmd, "ipv6 forwarding", IPV6_STR - "Turn on IPv6 forwarding") + "Turn on IPv6 forwarding\n") { int ret; @@ -2988,7 +2988,7 @@ DEFUN (no_ipv6_forwarding, "no ipv6 forwarding", NO_STR IPV6_STR - "Turn off IPv6 forwarding") + "Turn off IPv6 forwarding\n") { int ret; From a1d6bbb1f30d638a039db2b5e5ef3ea542590b62 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Sun, 22 Oct 2017 16:36:13 -0200 Subject: [PATCH 3/6] ospfd: fix coverity warnings - security best practices violations Signed-off-by: Renato Westphal --- ospfd/ospf_lsa.c | 9 +++++---- ospfd/ospf_te.c | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/ospfd/ospf_lsa.c b/ospfd/ospf_lsa.c index 74d5178f55..4e67694059 100644 --- a/ospfd/ospf_lsa.c +++ b/ospfd/ospf_lsa.c @@ -284,12 +284,12 @@ const char *dump_lsa_key(struct ospf_lsa *lsa) if (lsa != NULL && (lsah = lsa->data) != NULL) { char id[INET_ADDRSTRLEN], ar[INET_ADDRSTRLEN]; - strcpy(id, inet_ntoa(lsah->id)); - strcpy(ar, inet_ntoa(lsah->adv_router)); + strlcpy(id, inet_ntoa(lsah->id), sizeof(id)); + strlcpy(ar, inet_ntoa(lsah->adv_router), sizeof(ar)); sprintf(buf, "Type%d,id(%s),ar(%s)", lsah->type, id, ar); } else - strcpy(buf, "NULL"); + strlcpy(buf, "NULL", sizeof(buf)); return buf; } @@ -2713,7 +2713,8 @@ struct ospf_lsa *ospf_lsa_install(struct ospf *ospf, struct ospf_interface *oi, new->data->type, NULL)); break; default: - strcpy(area_str, inet_ntoa(new->area->area_id)); + strlcpy(area_str, inet_ntoa(new->area->area_id), + sizeof(area_str)); zlog_debug("LSA[%s]: Install %s to Area %s", dump_lsa_key(new), lookup_msg(ospf_lsa_type_msg, diff --git a/ospfd/ospf_te.c b/ospfd/ospf_te.c index b13e833afd..253b272df6 100644 --- a/ospfd/ospf_te.c +++ b/ospfd/ospf_te.c @@ -1252,7 +1252,7 @@ static int ospf_mpls_te_lsa_originate1(struct ospf_area *area, if (IS_DEBUG_OSPF(lsa, LSA_GENERATE)) { char area_id[INET_ADDRSTRLEN]; - strcpy(area_id, inet_ntoa(area->area_id)); + strlcpy(area_id, inet_ntoa(area->area_id), sizeof(area_id)); zlog_debug( "LSA[Type%d:%s]: Originate Opaque-LSA/MPLS-TE: Area(%s), Link(%s)", new->data->type, inet_ntoa(new->data->id), area_id, From 44f12f209f7019c0abbec0f919cb18a136cd7bee Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Sun, 22 Oct 2017 21:14:21 -0200 Subject: [PATCH 4/6] *: fix coverity warnings - resource leaks These are mostly trivial fixes for leaks in the error path of some functions. The changes in bgpd/bgp_mpath.c deserves a bit of explanation though. In the bgp_info_mpath_aggregate_update() function, we were allocating memory for the lcomm variable but doing nothing with it. Since the code for communities, extended communities and large communities is pretty much the same in this function, it's clear that this was a copy and paste error where most of the ext. community code was copied but not all of it as it should have been. Signed-off-by: Renato Westphal --- bgpd/bgp_mpath.c | 4 ++++ lib/hash.c | 1 + ripngd/ripngd.c | 16 ++++++++++------ tools/start-stop-daemon.c | 5 +---- vtysh/vtysh.c | 5 +++-- zebra/rtadv.c | 1 + 6 files changed, 20 insertions(+), 12 deletions(-) diff --git a/bgpd/bgp_mpath.c b/bgpd/bgp_mpath.c index d3ee140bb4..9d32c4bee1 100644 --- a/bgpd/bgp_mpath.c +++ b/bgpd/bgp_mpath.c @@ -751,6 +751,10 @@ void bgp_info_mpath_aggregate_update(struct bgp_info *new_best, attr.ecommunity = ecomm; attr.flag |= ATTR_FLAG_BIT(BGP_ATTR_EXT_COMMUNITIES); } + if (lcomm) { + attr.lcommunity = lcomm; + attr.flag |= ATTR_FLAG_BIT(BGP_ATTR_LARGE_COMMUNITIES); + } /* Zap multipath attr nexthop so we set nexthop to self */ attr.nexthop.s_addr = 0; diff --git a/lib/hash.c b/lib/hash.c index f222279216..4894b65aff 100644 --- a/lib/hash.c +++ b/lib/hash.c @@ -395,6 +395,7 @@ DEFUN_NOSH(show_hash_stats, pthread_mutex_lock(&_hashes_mtx); if (!_hashes) { pthread_mutex_unlock(&_hashes_mtx); + ttable_del(tt); vty_out(vty, "No hash tables in use.\n"); return CMD_SUCCESS; } diff --git a/ripngd/ripngd.c b/ripngd/ripngd.c index daa2526a0c..673f0637c7 100644 --- a/ripngd/ripngd.c +++ b/ripngd/ripngd.c @@ -101,21 +101,21 @@ static int ripng_make_socket(void) setsockopt_so_recvbuf(sock, 8096); ret = setsockopt_ipv6_pktinfo(sock, 1); if (ret < 0) - return ret; + goto error; #ifdef IPTOS_PREC_INTERNETCONTROL ret = setsockopt_ipv6_tclass(sock, IPTOS_PREC_INTERNETCONTROL); if (ret < 0) - return ret; + goto error; #endif ret = setsockopt_ipv6_multicast_hops(sock, 255); if (ret < 0) - return ret; + goto error; ret = setsockopt_ipv6_multicast_loop(sock, 0); if (ret < 0) - return ret; + goto error; ret = setsockopt_ipv6_hoplimit(sock, 1); if (ret < 0) - return ret; + goto error; memset(&ripaddr, 0, sizeof(ripaddr)); ripaddr.sin6_family = AF_INET6; @@ -132,11 +132,15 @@ static int ripng_make_socket(void) zlog_err("Can't bind ripng socket: %s.", safe_strerror(errno)); if (ripngd_privs.change(ZPRIVS_LOWER)) zlog_err("ripng_make_socket: could not lower privs"); - return ret; + goto error; } if (ripngd_privs.change(ZPRIVS_LOWER)) zlog_err("ripng_make_socket: could not lower privs"); return sock; + +error: + close(sock); + return ret; } /* Send RIPng packet. */ diff --git a/tools/start-stop-daemon.c b/tools/start-stop-daemon.c index 30dc6484ae..8dc16f4209 100644 --- a/tools/start-stop-daemon.c +++ b/tools/start-stop-daemon.c @@ -538,10 +538,7 @@ static void parse_options(int argc, char *const *argv) execname = optarg; break; case 'c': /* --chuid | */ - /* we copy the string just in case we need the - * argument later. */ - changeuser = strdup(optarg); - changeuser = strtok(changeuser, ":"); + changeuser = strtok(optarg, ":"); changegroup = strtok(NULL, ":"); break; case 'r': /* --chroot /new/root */ diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c index 061c25cea5..92c6f64e1c 100644 --- a/vtysh/vtysh.c +++ b/vtysh/vtysh.c @@ -573,6 +573,7 @@ int vtysh_mark_file(const char *filename) * appropriate */ if (strlen(vty_buf_trimmed) == 3 && strncmp("end", vty_buf_trimmed, 3) == 0) { + cmd_free_strvec(vline); continue; } @@ -804,8 +805,6 @@ static int vtysh_rl_describe(void) } else if (rl_end && isspace((int)rl_line_buffer[rl_end - 1])) vector_set(vline, NULL); - describe = cmd_describe_command(vline, vty, &ret); - fprintf(stdout, "\n"); /* Ambiguous and no match error. */ @@ -824,6 +823,8 @@ static int vtysh_rl_describe(void) break; } + describe = cmd_describe_command(vline, vty, &ret); + /* Get width of command string. */ width = 0; for (i = 0; i < vector_active(describe); i++) diff --git a/zebra/rtadv.c b/zebra/rtadv.c index 7f9bc47315..a1e1602bf8 100644 --- a/zebra/rtadv.c +++ b/zebra/rtadv.c @@ -675,6 +675,7 @@ static int rtadv_make_socket(void) sizeof(struct icmp6_filter)); if (ret < 0) { zlog_info("ICMP6_FILTER set fail: %s", safe_strerror(errno)); + close(sock); return ret; } From cbb65f5ef53d9029811e248608125ffa293753eb Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Mon, 23 Oct 2017 19:19:26 -0200 Subject: [PATCH 5/6] *: fix coverity warnings - error handling issues Ignore the return value of some functions in the places we know they can't fail, and other small fixes. Regarding the change in bgpd/rfapi/rfapi_rib.c, asserting that rfapiRaddr2Qprefix() didn't fail is the common idiom inside the rfapi code. Signed-off-by: Renato Westphal --- bgpd/bgp_evpn.c | 2 +- bgpd/bgp_route.c | 2 +- bgpd/bgp_updgrp_adv.c | 15 +++++++-------- bgpd/bgp_updgrp_packet.c | 14 ++++++-------- bgpd/rfapi/rfapi_rib.c | 2 +- eigrpd/eigrp_vty.c | 4 ++-- vtysh/vtysh_main.c | 2 +- 7 files changed, 19 insertions(+), 22 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index a09d966d7b..182a6c64f2 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2542,7 +2542,7 @@ void bgp_evpn_derive_auto_rd(struct bgp *bgp, struct bgpevpn *vpn) vpn->prd.family = AF_UNSPEC; vpn->prd.prefixlen = 64; sprintf(buf, "%s:%hu", inet_ntoa(bgp->router_id), vpn->rd_id); - str2prefix_rd(buf, &vpn->prd); + (void)str2prefix_rd(buf, &vpn->prd); UNSET_FLAG(vpn->flags, VNI_FLAG_RD_CFGD); } diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 0c2a2f6fe9..af71088b7d 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -10545,7 +10545,7 @@ DEFUN (show_bgp_afi_vpn_rd_route, afi_t afi = AFI_MAX; int idx = 0; - argv_find_and_parse_afi(argv, argc, &idx, &afi); + (void)argv_find_and_parse_afi(argv, argc, &idx, &afi); ret = str2prefix_rd(argv[5]->arg, &prd); if (!ret) { vty_out(vty, "%% Malformed Route Distinguisher\n"); diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index fbbeaee9b2..8a24cba598 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -687,11 +687,11 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) attr.local_pref = bgp->default_local_pref; - if (afi == AFI_IP) - str2prefix("0.0.0.0/0", &p); - else if (afi == AFI_IP6) { - str2prefix("::/0", &p); + memset(&p, 0, sizeof(p)); + p.family = afi2family(afi); + p.prefixlen = 0; + if (afi == AFI_IP6) { /* IPv6 global nexthop must be included. */ attr.mp_nexthop_len = BGP_ATTR_NHLEN_IPV6_GLOBAL; @@ -759,10 +759,9 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) * clear adj_out for the 0.0.0.0/0 prefix in the BGP * table. */ - if (afi == AFI_IP) - str2prefix("0.0.0.0/0", &p); - else - str2prefix("::/0", &p); + memset(&p, 0, sizeof(p)); + p.family = afi2family(afi); + p.prefixlen = 0; rn = bgp_afi_node_get(bgp->rib[afi][safi], afi, safi, &p, NULL); diff --git a/bgpd/bgp_updgrp_packet.c b/bgpd/bgp_updgrp_packet.c index 433c5ce512..a35d814e47 100644 --- a/bgpd/bgp_updgrp_packet.c +++ b/bgpd/bgp_updgrp_packet.c @@ -1090,10 +1090,9 @@ void subgroup_default_update_packet(struct update_subgroup *subgrp, bpacket_attr_vec_arr_reset(&vecarr); addpath_encode = bgp_addpath_encode_tx(peer, afi, safi); - if (afi == AFI_IP) - str2prefix("0.0.0.0/0", &p); - else - str2prefix("::/0", &p); + memset(&p, 0, sizeof(p)); + p.family = afi2family(afi); + p.prefixlen = 0; /* Logging the attribute. */ if (bgp_debug_update(NULL, &p, subgrp->update_group, 0)) { @@ -1176,10 +1175,9 @@ void subgroup_default_withdraw_packet(struct update_subgroup *subgrp) safi = SUBGRP_SAFI(subgrp); addpath_encode = bgp_addpath_encode_tx(peer, afi, safi); - if (afi == AFI_IP) - str2prefix("0.0.0.0/0", &p); - else - str2prefix("::/0", &p); + memset(&p, 0, sizeof(p)); + p.family = afi2family(afi); + p.prefixlen = 0; if (bgp_debug_update(NULL, &p, subgrp->update_group, 0)) { char buf[PREFIX_STRLEN]; diff --git a/bgpd/rfapi/rfapi_rib.c b/bgpd/rfapi/rfapi_rib.c index 92cd1888ee..36ae6e7273 100644 --- a/bgpd/rfapi/rfapi_rib.c +++ b/bgpd/rfapi/rfapi_rib.c @@ -1668,7 +1668,7 @@ void rfapiRibUpdatePendingNode( struct prefix pfx_vn; - rfapiRaddr2Qprefix(&rfd->vn_addr, &pfx_vn); + assert(!rfapiRaddr2Qprefix(&rfd->vn_addr, &pfx_vn)); if (prefix_same(&pfx_vn, &pfx_nh)) continue; } diff --git a/eigrpd/eigrp_vty.c b/eigrpd/eigrp_vty.c index 5fc7818166..4e76428535 100644 --- a/eigrpd/eigrp_vty.c +++ b/eigrpd/eigrp_vty.c @@ -398,7 +398,7 @@ DEFUN (eigrp_network, struct prefix p; int ret; - str2prefix(argv[1]->arg, &p); + (void)str2prefix(argv[1]->arg, &p); ret = eigrp_network_set(eigrp, &p); @@ -421,7 +421,7 @@ DEFUN (no_eigrp_network, struct prefix p; int ret; - str2prefix(argv[2]->arg, &p); + (void)str2prefix(argv[2]->arg, &p); ret = eigrp_network_unset(eigrp, &p); diff --git a/vtysh/vtysh_main.c b/vtysh/vtysh_main.c index 8509a8a05a..57042f8e62 100644 --- a/vtysh/vtysh_main.c +++ b/vtysh/vtysh_main.c @@ -533,7 +533,7 @@ int main(int argc, char **argv, char **env) fp = open(history_file, O_CREAT | O_EXCL, S_IRUSR | S_IWUSR); - if (fp) + if (fp != -1) close(fp); read_history(history_file); From b11b57723b963fb5d0ab11748b7957f0b7fa5d37 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Mon, 23 Oct 2017 19:24:07 -0200 Subject: [PATCH 6/6] lib: optimize sockunion_connect() This function is only called with non-blocking sockets [1], so there's no need to worry about setting O_NONBLOCK and unsetting it later if the given fd was a blocking socket. This saves us 4 syscalls per connect, which is not much but is something. Also, remove an outdated comment about the return values of this function. It returns a 'connect_result' enum now, whose values are self-explanatory (connect_error, connect_success and connect_in_progress). This also fixes a coverity scan warning where we weren't checking the return value of the fcntl() syscall. [1] bgp_connect() and pim_msdp_sock_connect(). Signed-off-by: Renato Westphal --- lib/sockunion.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/lib/sockunion.c b/lib/sockunion.c index 559ae37ffb..ab8d8be3e5 100644 --- a/lib/sockunion.c +++ b/lib/sockunion.c @@ -175,15 +175,11 @@ static int sockunion_sizeof(const union sockunion *su) return ret; } -/* sockunion_connect returns - -1 : error occured - 0 : connect success - 1 : connect is in progress */ +/* Performs a non-blocking connect(). */ enum connect_result sockunion_connect(int fd, const union sockunion *peersu, unsigned short port, ifindex_t ifindex) { int ret; - int val; union sockunion su; memcpy(&su, peersu, sizeof(union sockunion)); @@ -203,18 +199,12 @@ enum connect_result sockunion_connect(int fd, const union sockunion *peersu, break; } - /* Make socket non-block. */ - val = fcntl(fd, F_GETFL, 0); - fcntl(fd, F_SETFL, val | O_NONBLOCK); - /* Call connect function. */ ret = connect(fd, (struct sockaddr *)&su, sockunion_sizeof(&su)); /* Immediate success */ - if (ret == 0) { - fcntl(fd, F_SETFL, val); + if (ret == 0) return connect_success; - } /* If connect is in progress then return 1 else it's real error. */ if (ret < 0) { @@ -227,8 +217,6 @@ enum connect_result sockunion_connect(int fd, const union sockunion *peersu, } } - fcntl(fd, F_SETFL, val); - return connect_in_progress; }