[iproute PATCH 00/51] Fix potential issues detected by Coverity tool

[iproute PATCH 00/51] Fix potential issues detected by Coverity tool

[Phil Sutter]

Covscan really wasn't amused (indicated by the number of patches in this
series). Try to make it happy.

Phil Sutter (51):
  devlink: Check return code of strslashrsplit()
  devlink: No need for this self-assignment
  ipaddress: Make buffer for filter.flushb static
  ipaddress: Avoid accessing uninitialized variable lcl
  iplink_can: Prevent overstepping array bounds
  iplink_vrf: Complain if main table is not found
  ipmaddr: Avoid accessing uninitialized data
  ipntable: No need to check and assign to parms_rta
  ipntable: Make sure filter.name is NULL-terminated
  iproute: Fix for missing 'Oifs:' display
  iproute: Check mark value input
  iproute_lwtunnel: csum_mode value checking was ineffective
  iproute_lwtunnel: Argument to strerror must be positive
  ipvrf: Don't try to close an invalid fd
  ipvrf: Fix error path of vrf_switch()
  xfrm_state: Make sure alg_name is NULL-terminated
  lib/bpf: Don't leak fp in bpf_find_mntpt()
  lib/fs: Fix format string in find_fs_mount()
  lib/fs: Fix and simplify make_path()
  lib/inet_proto: Make sure destination buffers are NULL-terminated
  lib/libnetlink: Don't pass NULL parameter to memcpy()
  lib/rt_names: Drop dead code in rtnl_rttable_n2a()
  ifstat: Fix memleak in error case
  ifstat, nstat: Check fdopen() return value
  ifstat: Fix memleak in dump_kern_db() for json output
  lnstat_util: Simplify alloc_and_open() a bit
  nstat: Fix for potential NULL pointer dereference
  nstat: Avoid passing negative fd to fdopen()
  ss: Use C99 initializer in netlink_show_one()
  ss: Skip useless check in parse_hostcond()
  ss: Drop useless assignment
  ss: Make sure index variable is >= 0
  ss: Don't leak fd in tcp_show_netlink_file()
  ss: Make sure scanned index value to unix_state_map is sane
  ss: Fix potential memleak in unix_stats_print()
  netem/maketable: Check return value of fstat()
  netem/maketable: Check return value of fscanf()
  tc/em_ipset: Don't leak sockfd on error path
  tc/m_gact: Drop dead code
  tc/m_xt: Fix for potential string buffer overflows
  tc/q_multiq: Don't pass garbage in TCA_OPTIONS
  tc/q_netem: Don't dereference possibly NULL pointer
  tc/tc_filter: Make sure filter name is not empty
  tipc/bearer: Fix resource leak in error path
  tipc/bearer: Prevent NULL pointer dereference
  tipc/node: Fix socket fd check in cmd_node_get_addr()
  examples: Some shell fixes to cbq.init
  ifcfg: Quote left-hand side of [ ] expression
  lib/ll_map: Make sure im->name is NULL-terminated
  Check user supplied interface name lengths
  lib/bpf: Check return value of write()

 devlink/devlink.c        | 18 ++++++++++-----
 examples/cbq.init-v0.7.3 | 24 ++++++++++----------
 include/utils.h          |  1 +
 ip/ifcfg                 |  2 +-
 ip/ip6tunnel.c           |  6 +++--
 ip/ipaddress.c           |  4 ++--
 ip/ipl2tp.c              |  1 +
 ip/iplink.c              | 27 +++++++----------------
 ip/iplink_can.c          |  4 ++--
 ip/iplink_vrf.c          |  5 ++++-
 ip/ipmaddr.c             |  3 ++-
 ip/ipntable.c            |  5 ++---
 ip/iproute.c             | 14 +++++++-----
 ip/iproute_lwtunnel.c    |  9 ++++----
 ip/iprule.c              |  4 ++++
 ip/iptunnel.c            | 12 ++++++----
 ip/iptuntap.c            |  4 +++-
 ip/ipvrf.c               | 16 ++++++++------
 ip/xfrm_state.c          |  3 ++-
 lib/bpf.c                |  8 +++++--
 lib/fs.c                 | 22 +++++--------------
 lib/inet_proto.c         |  9 +++++---
 lib/libnetlink.c         |  6 +++--
 lib/ll_map.c             |  4 ++--
 lib/rt_names.c           |  4 ----
 lib/utils.c              |  8 +++++++
 misc/arpd.c              |  1 +
 misc/ifstat.c            | 28 +++++++++++++++++-------
 misc/lnstat_util.c       |  7 ++----
 misc/nstat.c             | 33 +++++++++++++++++++---------
 misc/ss.c                | 57 +++++++++++++++++++++++++++++-------------------
 netem/maketable.c        |  8 +++----
 tc/em_ipset.c            |  1 +
 tc/m_gact.c              | 14 +++---------
 tc/m_xt.c                |  7 +++---
 tc/q_multiq.c            |  2 +-
 tc/q_netem.c             |  4 +++-
 tc/tc_filter.c           |  3 +++
 tipc/bearer.c            |  7 ++++--
 tipc/node.c              |  3 ++-
 40 files changed, 230 insertions(+), 168 deletions(-)

-- 
2.13.1

[iproute PATCH 01/51] devlink: Check return code of strslashrsplit()

[Phil Sutter]

This function shouldn't fail because all callers of
__dl_argv_handle_port() make sure the passed string contains enough
slashes already, but better make sure if this changes in future the
function won't access uninitialized data.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 devlink/devlink.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index f9bc16c350c40..de41a9f4aae10 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -526,18 +526,26 @@ static int __dl_argv_handle_port(char *str,
 				 char **p_bus_name, char **p_dev_name,
 				 uint32_t *p_port_index)
 {
-	char *handlestr = handlestr;
-	char *portstr = portstr;
+	char *handlestr;
+	char *portstr;
 	int err;
 
-	strslashrsplit(str, &handlestr, &portstr);
+	err = strslashrsplit(str, &handlestr, &portstr);
+	if (err) {
+		pr_err("Port identification \"%s\" is invalid\n", str);
+		return err;
+	}
 	err = strtouint32_t(portstr, p_port_index);
 	if (err) {
 		pr_err("Port index \"%s\" is not a number or not within range\n",
 		       portstr);
 		return err;
 	}
-	strslashrsplit(handlestr, p_bus_name, p_dev_name);
+	err = strslashrsplit(handlestr, p_bus_name, p_dev_name);
+	if (err) {
+		pr_err("Port identification \"%s\" is invalid\n", str);
+		return err;
+	}
 	return 0;
 }
 
-- 
2.13.1

Re: [iproute PATCH 01/51] devlink: Check return code of strslashrsplit()

[Stephen Hemminger]

On Sat, 12 Aug 2017 14:04:20 +0200
Phil Sutter <phil@nwl.cc> wrote:

> This function shouldn't fail because all callers of
> __dl_argv_handle_port() make sure the passed string contains enough
> slashes already, but better make sure if this changes in future the
> function won't access uninitialized data.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Ok, but please don't go too far down the dead end of addressing all possible
"if code changes in the future this would be a bug". Keep to finding
and fixing the real bugs that exist today.

[iproute PATCH 02/51] devlink: No need for this self-assignment

[Phil Sutter]

dl_argv_handle_both() will either assign to handle_bit or error out in
which case the variable is not used by the caller.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 devlink/devlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index de41a9f4aae10..9fb3c7d2a91e9 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -787,7 +787,7 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
 	int err;
 
 	if (o_required & DL_OPT_HANDLE && o_required & DL_OPT_HANDLEP) {
-		uint32_t handle_bit = handle_bit;
+		uint32_t handle_bit;
 
 		err = dl_argv_handle_both(dl, &opts->bus_name, &opts->dev_name,
 					  &opts->port_index, &handle_bit);
-- 
2.13.1

[iproute PATCH 03/51] ipaddress: Make buffer for filter.flushb static

[Phil Sutter]

The buffer is accessed outside of the function defining it, so make it
static.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 4d37c5e045071..3c9decb51b412 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1488,7 +1488,7 @@ static void ipaddr_filter(struct nlmsg_chain *linfo, struct nlmsg_chain *ainfo)
 static int ipaddr_flush(void)
 {
 	int round = 0;
-	char flushb[4096-512];
+	static char flushb[4096-512];
 
 	filter.flushb = flushb;
 	filter.flushp = 0;
-- 
2.13.1

Re: [iproute PATCH 03/51] ipaddress: Make buffer for filter.flushb static

[Stephen Hemminger]

On Sat, 12 Aug 2017 14:04:22 +0200
Phil Sutter <phil@nwl.cc> wrote:

> The buffer is accessed outside of the function defining it, so make it
> static.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Where does a function access this buffer which is not a sibling?

Re: [iproute PATCH 03/51] ipaddress: Make buffer for filter.flushb static

[Phil Sutter]

On Tue, Aug 15, 2017 at 08:13:08AM -0700, Stephen Hemminger wrote:
> On Sat, 12 Aug 2017 14:04:22 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > The buffer is accessed outside of the function defining it, so make it
> > static.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> Where does a function access this buffer which is not a sibling?

flushb is (was) an auto-variable of ipaddr_flush() which makes
filter.flushb point to it. That pointer is dereferenced from
rtnl_send_check() called by flush_update() and from print_addrinfo(). I
have to admit, the code around flushing addresses is a bit of a mystery
to me, but I don't think it's safe to access function local storage from
outside of it's scope.

Cheers, Phil

[iproute PATCH 04/51] ipaddress: Avoid accessing uninitialized variable lcl

[Phil Sutter]

If no address was given, ipaddr_modify() accesses uninitialized data
when assigning to req.ifa.ifa_prefixlen.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 3c9decb51b412..9307c9416dde3 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1888,7 +1888,7 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
 	char  *lcl_arg = NULL;
 	char  *valid_lftp = NULL;
 	char  *preferred_lftp = NULL;
-	inet_prefix lcl;
+	inet_prefix lcl = {};
 	inet_prefix peer;
 	int local_len = 0;
 	int peer_len = 0;
-- 
2.13.1

[iproute PATCH 05/51] iplink_can: Prevent overstepping array bounds

[Phil Sutter]

can_state_names array contains at most CAN_STATE_MAX fields, so allowing
an index to it to be equal to that number is wrong. While here, also
make sure the array is indeed that big so nothing bad happens if
CAN_STATE_MAX ever increases.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iplink_can.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index 5df56b2bbbb3b..2954010fefa22 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -251,7 +251,7 @@ static int can_parse_opt(struct link_util *lu, int argc, char **argv,
 	return 0;
 }
 
-static const char *can_state_names[] = {
+static const char *can_state_names[CAN_STATE_MAX] = {
 	[CAN_STATE_ERROR_ACTIVE] = "ERROR-ACTIVE",
 	[CAN_STATE_ERROR_WARNING] = "ERROR-WARNING",
 	[CAN_STATE_ERROR_PASSIVE] = "ERROR-PASSIVE",
@@ -275,7 +275,7 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (tb[IFLA_CAN_STATE]) {
 		uint32_t state = rta_getattr_u32(tb[IFLA_CAN_STATE]);
 
-		fprintf(f, "state %s ", state <= CAN_STATE_MAX ?
+		fprintf(f, "state %s ", state < CAN_STATE_MAX ?
 			can_state_names[state] : "UNKNOWN");
 	}
 
-- 
2.13.1

Re: [iproute PATCH 05/51] iplink_can: Prevent overstepping array bounds

[Stephen Hemminger]

On Sat, 12 Aug 2017 14:04:24 +0200
Phil Sutter <phil@nwl.cc> wrote:

> can_state_names array contains at most CAN_STATE_MAX fields, so allowing
> an index to it to be equal to that number is wrong. While here, also
> make sure the array is indeed that big so nothing bad happens if
> CAN_STATE_MAX ever increases.

No more speculative bug fixes.

Re: [iproute PATCH 05/51] iplink_can: Prevent overstepping array bounds

[Phil Sutter]

On Tue, Aug 15, 2017 at 08:10:49AM -0700, Stephen Hemminger wrote:
> On Sat, 12 Aug 2017 14:04:24 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > can_state_names array contains at most CAN_STATE_MAX fields, so allowing
> > an index to it to be equal to that number is wrong. While here, also
> > make sure the array is indeed that big so nothing bad happens if
> > CAN_STATE_MAX ever increases.
> 
> No more speculative bug fixes.

I don't think a bit of speculation regarding forwards-compatibility is a
bad thing per se. In this case it is about the possibility for kernel
code to add a new state to enum can_state.

Older ip binaries will allow an index of CAN_STATE_MAX and therefore
access data beyond end of can-state_names array.

If you update linux headers but forget to add the new state to
can_state_names array, the same will happen even if the sanity check for
'state' value is being fixed as by my patch.

By specifying the size of can_state_names array upon definition,
can_print_opt() will just print a null pointer which printf() can
handle.

Cheers, Phil

[iproute PATCH 06/51] iplink_vrf: Complain if main table is not found

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iplink_vrf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ip/iplink_vrf.c b/ip/iplink_vrf.c
index 917630e853375..809eda5de8f6e 100644
--- a/ip/iplink_vrf.c
+++ b/ip/iplink_vrf.c
@@ -131,7 +131,10 @@ __u32 ipvrf_get_table(const char *name)
 					   &answer.n, sizeof(answer)) < 0) {
 		/* special case "default" vrf to be the main table */
 		if (errno == ENODEV && !strcmp(name, "default"))
-			rtnl_rttable_a2n(&tb_id, "main");
+			if (rtnl_rttable_a2n(&tb_id, "main"))
+				fprintf(stderr,
+					"BUG: RTTable \"main\" not found.\n");
+
 
 		return tb_id;
 	}
-- 
2.13.1

Re: [iproute PATCH 06/51] iplink_vrf: Complain if main table is not found

[David Ahern]

On 8/12/17 6:04 AM, Phil Sutter wrote:
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  ip/iplink_vrf.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/ip/iplink_vrf.c b/ip/iplink_vrf.c
> index 917630e853375..809eda5de8f6e 100644
> --- a/ip/iplink_vrf.c
> +++ b/ip/iplink_vrf.c
> @@ -131,7 +131,10 @@ __u32 ipvrf_get_table(const char *name)
>  					   &answer.n, sizeof(answer)) < 0) {
>  		/* special case "default" vrf to be the main table */
>  		if (errno == ENODEV && !strcmp(name, "default"))
> -			rtnl_rttable_a2n(&tb_id, "main");
> +			if (rtnl_rttable_a2n(&tb_id, "main"))
> +				fprintf(stderr,
> +					"BUG: RTTable \"main\" not found.\n");
> +
>  
>  		return tb_id;
>  	}
> 

Acked-by: David Ahern <dsahern@gmail.com>

[iproute PATCH 07/51] ipmaddr: Avoid accessing uninitialized data

[Phil Sutter]

Looks like this can only happen if /proc/net/igmp is malformed, but
better be sure.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipmaddr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c
index 4f726fdd976f1..85a69e779563d 100644
--- a/ip/ipmaddr.c
+++ b/ip/ipmaddr.c
@@ -136,7 +136,7 @@ static void read_igmp(struct ma_info **result_p)
 
 	while (fgets(buf, sizeof(buf), fp)) {
 		struct ma_info *ma;
-		size_t len;
+		size_t len = 0;
 
 		if (buf[0] != '\t') {
 			sscanf(buf, "%d%s", &m.index, m.name);
-- 
2.13.1

[iproute PATCH 08/51] ipntable: No need to check and assign to parms_rta

[Phil Sutter]

This variable is initialized at declaration and nowhere else does any
assignment to it happen, so just drop the check.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipntable.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ip/ipntable.c b/ip/ipntable.c
index 879626ee4f491..1837909fa42e7 100644
--- a/ip/ipntable.c
+++ b/ip/ipntable.c
@@ -202,8 +202,6 @@ static int ipntable_modify(int cmd, int flags, int argc, char **argv)
 			if (get_u32(&queue, *argv, 0))
 				invarg("\"queue\" value is invalid", *argv);
 
-			if (!parms_rta)
-				parms_rta = (struct rtattr *)&parms_buf;
 			rta_addattr32(parms_rta, sizeof(parms_buf),
 				      NDTPA_QUEUE_LEN, queue);
 			parms_change = 1;
-- 
2.13.1

[iproute PATCH 09/51] ipntable: Make sure filter.name is NULL-terminated

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipntable.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ip/ipntable.c b/ip/ipntable.c
index 1837909fa42e7..30907146e85a3 100644
--- a/ip/ipntable.c
+++ b/ip/ipntable.c
@@ -631,7 +631,8 @@ static int ipntable_show(int argc, char **argv)
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();
 
-			strncpy(filter.name, *argv, sizeof(filter.name));
+			strncpy(filter.name, *argv, sizeof(filter.name) - 1);
+			filter.name[sizeof(filter.name) - 1] = '\0';
 		} else
 			invarg("unknown", *argv);
 
-- 
2.13.1

[iproute PATCH 10/51] iproute: Fix for missing 'Oifs:' display

[Phil Sutter]

Covscan complained about dead code but after reading it, I assume the
author's intention was to prefix the interface list with 'Oifs: '.
Initializing first to 1 and setting it to 0 after above prefix was
printed should fix it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iproute.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ip/iproute.c b/ip/iproute.c
index cb695ad4141a7..89caac124f489 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -624,7 +624,7 @@ int print_route(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
 	}
 	if (tb[RTA_MULTIPATH]) {
 		struct rtnexthop *nh = RTA_DATA(tb[RTA_MULTIPATH]);
-		int first = 0;
+		int first = 1;
 
 		len = RTA_PAYLOAD(tb[RTA_MULTIPATH]);
 
@@ -634,10 +634,12 @@ int print_route(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
 			if (nh->rtnh_len > len)
 				break;
 			if (r->rtm_flags&RTM_F_CLONED && r->rtm_type == RTN_MULTICAST) {
-				if (first)
+				if (first) {
 					fprintf(fp, "Oifs: ");
-				else
+					first = 0;
+				} else {
 					fprintf(fp, " ");
+				}
 			} else
 				fprintf(fp, "%s\tnexthop ", _SL_);
 			if (nh->rtnh_len > sizeof(*nh)) {
-- 
2.13.1

[iproute PATCH 11/51] iproute: Check mark value input

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iproute.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ip/iproute.c b/ip/iproute.c
index 89caac124f489..5fe8a3a75d5b7 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1495,7 +1495,8 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 			id = *argv;
 		} else if (strcmp(*argv, "mark") == 0) {
 			NEXT_ARG();
-			get_unsigned(&mark, *argv, 0);
+			if (get_unsigned(&mark, *argv, 0))
+				invarg("invalid mark value\n", *argv);
 			filter.markmask = -1;
 		} else if (strcmp(*argv, "via") == 0) {
 			int family;
@@ -1712,7 +1713,8 @@ static int iproute_get(int argc, char **argv)
 			idev = *argv;
 		} else if (matches(*argv, "mark") == 0) {
 			NEXT_ARG();
-			get_unsigned(&mark, *argv, 0);
+			if (get_unsigned(&mark, *argv, 0))
+				invarg("invalid mark value\n", *argv);
 		} else if (matches(*argv, "oif") == 0 ||
 			   strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-- 
2.13.1

[iproute PATCH 12/51] iproute_lwtunnel: csum_mode value checking was ineffective

[Phil Sutter]

ila_csum_name2mode() returning -1 on error but being declared as
returning __u8 doesn't make much sense. Change the code to correctly
detect this issue. Checking for __u8 overruns shouldn't be necessary
though since ila_csum_name2mode() return values are well-defined.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iproute_lwtunnel.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c
index 5c0c7d110d23e..398ab5e077ed8 100644
--- a/ip/iproute_lwtunnel.c
+++ b/ip/iproute_lwtunnel.c
@@ -171,7 +171,7 @@ static char *ila_csum_mode2name(__u8 csum_mode)
 	}
 }
 
-static __u8 ila_csum_name2mode(char *name)
+static int ila_csum_name2mode(char *name)
 {
 	if (strcmp(name, "adj-transport") == 0)
 		return ILA_CSUM_ADJUST_TRANSPORT;
@@ -517,7 +517,7 @@ static int parse_encap_ila(struct rtattr *rta, size_t len,
 
 	while (argc > 0) {
 		if (strcmp(*argv, "csum-mode") == 0) {
-			__u8 csum_mode;
+			int csum_mode;
 
 			NEXT_ARG();
 
@@ -526,7 +526,8 @@ static int parse_encap_ila(struct rtattr *rta, size_t len,
 				invarg("\"csum-mode\" value is invalid\n",
 				       *argv);
 
-			rta_addattr8(rta, 1024, ILA_ATTR_CSUM_MODE, csum_mode);
+			rta_addattr8(rta, 1024, ILA_ATTR_CSUM_MODE,
+				     (__u8)csum_mode);
 
 			argc--; argv++;
 		} else {
-- 
2.13.1

[iproute PATCH 13/51] iproute_lwtunnel: Argument to strerror must be positive

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iproute_lwtunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c
index 398ab5e077ed8..1a3dc4d4c0ed9 100644
--- a/ip/iproute_lwtunnel.c
+++ b/ip/iproute_lwtunnel.c
@@ -643,7 +643,7 @@ static int lwt_parse_bpf(struct rtattr *rta, size_t len,
 	err = bpf_parse_common(bpf_type, &cfg, &bpf_cb_ops, &x);
 	if (err < 0) {
 		fprintf(stderr, "Failed to parse eBPF program: %s\n",
-			strerror(err));
+			strerror(-err));
 		return -1;
 	}
 	rta_nest_end(rta, nest);
-- 
2.13.1

[iproute PATCH 14/51] ipvrf: Don't try to close an invalid fd

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipvrf.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index 0094cf8557cd7..92e2db98ca7d7 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -268,7 +268,7 @@ static int vrf_configure_cgroup(const char *path, int ifindex)
 		fprintf(stderr,
 			"Failed to open cgroup path: '%s'\n",
 			strerror(errno));
-		goto out;
+		return rc;
 	}
 
 	/*
@@ -290,13 +290,14 @@ static int vrf_configure_cgroup(const char *path, int ifindex)
 	if (bpf_prog_attach_fd(prog_fd, cg_fd, BPF_CGROUP_INET_SOCK_CREATE)) {
 		fprintf(stderr, "Failed to attach prog to cgroup: '%s'\n",
 			strerror(errno));
-		goto out;
+		goto out2;
 	}
 
 	rc = 0;
+out2:
+	close(prog_fd);
 out:
 	close(cg_fd);
-	close(prog_fd);
 
 	return rc;
 }
-- 
2.13.1

Re: [iproute PATCH 14/51] ipvrf: Don't try to close an invalid fd

[David Ahern]

On 8/12/17 6:04 AM, Phil Sutter wrote:
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  ip/ipvrf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/ip/ipvrf.c b/ip/ipvrf.c
> index 0094cf8557cd7..92e2db98ca7d7 100644
> --- a/ip/ipvrf.c
> +++ b/ip/ipvrf.c
> @@ -268,7 +268,7 @@ static int vrf_configure_cgroup(const char *path, int ifindex)
>  		fprintf(stderr,
>  			"Failed to open cgroup path: '%s'\n",
>  			strerror(errno));
> -		goto out;
> +		return rc;
>  	}
>  
>  	/*
> @@ -290,13 +290,14 @@ static int vrf_configure_cgroup(const char *path, int ifindex)
>  	if (bpf_prog_attach_fd(prog_fd, cg_fd, BPF_CGROUP_INET_SOCK_CREATE)) {
>  		fprintf(stderr, "Failed to attach prog to cgroup: '%s'\n",
>  			strerror(errno));
> -		goto out;
> +		goto out2;
>  	}
>  
>  	rc = 0;
> +out2:
> +	close(prog_fd);
>  out:
>  	close(cg_fd);
> -	close(prog_fd);
>  
>  	return rc;
>  }
> 

Acked-by: David Ahern <dsahern@gmail.com>

Re: [iproute PATCH 14/51] ipvrf: Don't try to close an invalid fd

[Stephen Hemminger]

On Sat, 12 Aug 2017 14:04:33 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Signed-off-by: Phil Sutter <phil@nwl.cc>

close of -1 is a harmless.

[iproute PATCH 15/51] ipvrf: Fix error path of vrf_switch()

[Phil Sutter]

Apart from trying to close(-1), this also leaked memory.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipvrf.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index 92e2db98ca7d7..75cc026d072b8 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -373,12 +373,12 @@ static int vrf_switch(const char *name)
 
 	/* -1 on length to add '/' to the end */
 	if (ipvrf_get_netns(netns, sizeof(netns) - 1) < 0)
-		return -1;
+		goto out;
 
 	if (vrf_path(vpath, sizeof(vpath)) < 0) {
 		fprintf(stderr, "Failed to get base cgroup path: %s\n",
 			strerror(errno));
-		return -1;
+		goto out;
 	}
 
 	/* if path already ends in netns then don't add it again */
@@ -429,13 +429,14 @@ static int vrf_switch(const char *name)
 	snprintf(pid, sizeof(pid), "%d", getpid());
 	if (write(fd, pid, strlen(pid)) < 0) {
 		fprintf(stderr, "Failed to join cgroup\n");
-		goto out;
+		goto out2;
 	}
 
 	rc = 0;
+out2:
+	close(fd);
 out:
 	free(mnt);
-	close(fd);
 
 	return rc;
 }
-- 
2.13.1

Re: [iproute PATCH 15/51] ipvrf: Fix error path of vrf_switch()

[David Ahern]

On 8/12/17 6:04 AM, Phil Sutter wrote:
> Apart from trying to close(-1), this also leaked memory.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  ip/ipvrf.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/ip/ipvrf.c b/ip/ipvrf.c
> index 92e2db98ca7d7..75cc026d072b8 100644
> --- a/ip/ipvrf.c
> +++ b/ip/ipvrf.c
> @@ -373,12 +373,12 @@ static int vrf_switch(const char *name)
>  
>  	/* -1 on length to add '/' to the end */
>  	if (ipvrf_get_netns(netns, sizeof(netns) - 1) < 0)
> -		return -1;
> +		goto out;
>  
>  	if (vrf_path(vpath, sizeof(vpath)) < 0) {
>  		fprintf(stderr, "Failed to get base cgroup path: %s\n",
>  			strerror(errno));
> -		return -1;
> +		goto out;
>  	}
>  
>  	/* if path already ends in netns then don't add it again */
> @@ -429,13 +429,14 @@ static int vrf_switch(const char *name)
>  	snprintf(pid, sizeof(pid), "%d", getpid());
>  	if (write(fd, pid, strlen(pid)) < 0) {
>  		fprintf(stderr, "Failed to join cgroup\n");
> -		goto out;
> +		goto out2;
>  	}
>  
>  	rc = 0;
> +out2:
> +	close(fd);
>  out:
>  	free(mnt);
> -	close(fd);
>  
>  	return rc;
>  }
> 

Acked-by: David Ahern <dsahern@gmail.com>

[iproute PATCH 16/51] xfrm_state: Make sure alg_name is NULL-terminated

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/xfrm_state.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index e11c93bf1c3b5..7c0389038986e 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c
@@ -125,7 +125,8 @@ static int xfrm_algo_parse(struct xfrm_algo *alg, enum xfrm_attr_type_t type,
 	fprintf(stderr, "warning: ALGO-NAME/ALGO-KEYMAT values will be sent to the kernel promiscuously! (verifying them isn't implemented yet)\n");
 #endif
 
-	strncpy(alg->alg_name, name, sizeof(alg->alg_name));
+	strncpy(alg->alg_name, name, sizeof(alg->alg_name) - 1);
+	alg->alg_name[sizeof(alg->alg_name) - 1] = '\0';
 
 	if (slen > 2 && strncmp(key, "0x", 2) == 0) {
 		/* split two chars "0x" from the top */
-- 
2.13.1

[iproute PATCH 17/51] lib/bpf: Don't leak fp in bpf_find_mntpt()

[Phil Sutter]

If fopen() succeeded but len != PATH_MAX, the function leaks the open
FILE pointer. Fix this by checking len value before calling fopen().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/bpf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/bpf.c b/lib/bpf.c
index 4f52ad4a8f023..1dcb261dc915f 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -537,8 +537,11 @@ static const char *bpf_find_mntpt(const char *fstype, unsigned long magic,
 		}
 	}
 
+	if (len != PATH_MAX)
+		return NULL;
+
 	fp = fopen("/proc/mounts", "r");
-	if (fp == NULL || len != PATH_MAX)
+	if (fp == NULL)
 		return NULL;
 
 	while (fscanf(fp, "%*s %" textify(PATH_MAX) "s %99s %*s %*d %*d\n",
-- 
2.13.1

Re: [iproute PATCH 17/51] lib/bpf: Don't leak fp in bpf_find_mntpt()

[Daniel Borkmann]

On 08/12/2017 02:04 PM, Phil Sutter wrote:
> If fopen() succeeded but len != PATH_MAX, the function leaks the open
> FILE pointer. Fix this by checking len value before calling fopen().
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

[iproute PATCH 18/51] lib/fs: Fix format string in find_fs_mount()

[Phil Sutter]

A field width of 4096 allows fscanf() to store that amount of characters
into the given buffer, though that doesn't include the terminating NULL
byte. Decrease the value by one to leave space for it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fs.c b/lib/fs.c
index c59ac564581d0..1ff881ecfcd8c 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -45,7 +45,7 @@ static char *find_fs_mount(const char *fs_to_find)
 		return NULL;
 	}
 
-	while (fscanf(fp, "%*s %4096s %127s %*s %*d %*d\n",
+	while (fscanf(fp, "%*s %4095s %127s %*s %*d %*d\n",
 		      path, fstype) == 2) {
 		if (strcmp(fstype, fs_to_find) == 0) {
 			mnt = strdup(path);
-- 
2.13.1

[iproute PATCH 19/51] lib/fs: Fix and simplify make_path()

[Phil Sutter]

Calling stat() before mkdir() is racey: The entry might change in
between. Also, the call to stat() seems to exist only to check if the
directory exists already. So simply call mkdir() unconditionally and
catch only errors other than EEXIST.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/fs.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/lib/fs.c b/lib/fs.c
index 1ff881ecfcd8c..ebe05cd44e11b 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -102,7 +102,6 @@ out:
 int make_path(const char *path, mode_t mode)
 {
 	char *dir, *delim;
-	struct stat sbuf;
 	int rc = -1;
 
 	delim = dir = strdup(path);
@@ -120,20 +119,11 @@ int make_path(const char *path, mode_t mode)
 		if (delim)
 			*delim = '\0';
 
-		if (stat(dir, &sbuf) != 0) {
-			if (errno != ENOENT) {
-				fprintf(stderr,
-					"stat failed for %s: %s\n",
-					dir, strerror(errno));
-				goto out;
-			}
-
-			if (mkdir(dir, mode) != 0) {
-				fprintf(stderr,
-					"mkdir failed for %s: %s\n",
-					dir, strerror(errno));
-				goto out;
-			}
+		rc = mkdir(dir, mode);
+		if (mkdir(dir, mode) != 0 && errno != EEXIST) {
+			fprintf(stderr, "mkdir failed for %s: %s\n",
+				dir, strerror(errno));
+			goto out;
 		}
 
 		if (delim == NULL)
-- 
2.13.1

[iproute PATCH 20/51] lib/inet_proto: Make sure destination buffers are NULL-terminated

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/inet_proto.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/inet_proto.c b/lib/inet_proto.c
index ceda082b12a2e..87ed4769fc3da 100644
--- a/lib/inet_proto.c
+++ b/lib/inet_proto.c
@@ -35,8 +35,10 @@ const char *inet_proto_n2a(int proto, char *buf, int len)
 	pe = getprotobynumber(proto);
 	if (pe) {
 		icache = proto;
-		strncpy(ncache, pe->p_name, 16);
-		strncpy(buf, pe->p_name, len);
+		strncpy(ncache, pe->p_name, 15);
+		ncache[15] = '\0';
+		strncpy(buf, pe->p_name, len - 1);
+		buf[len] = '\0';
 		return buf;
 	}
 	snprintf(buf, len, "ipproto-%d", proto);
@@ -62,7 +64,8 @@ int inet_proto_a2n(const char *buf)
 	pe = getprotobyname(buf);
 	if (pe) {
 		icache = pe->p_proto;
-		strncpy(ncache, pe->p_name, 16);
+		strncpy(ncache, pe->p_name, 15);
+		ncache[15] = '\0';
 		return pe->p_proto;
 	}
 	return -1;
-- 
2.13.1

[iproute PATCH 21/51] lib/libnetlink: Don't pass NULL parameter to memcpy()

[Phil Sutter]

Both addattr_l() and rta_addattr_l() may be called with NULL data
pointer and 0 alen parameters. Avoid calling memcpy() in that case.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/libnetlink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 81a344abff27a..5e3adade77077 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -866,7 +866,8 @@ int addattr_l(struct nlmsghdr *n, int maxlen, int type, const void *data,
 	rta = NLMSG_TAIL(n);
 	rta->rta_type = type;
 	rta->rta_len = len;
-	memcpy(RTA_DATA(rta), data, alen);
+	if (alen)
+		memcpy(RTA_DATA(rta), data, alen);
 	n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(len);
 	return 0;
 }
@@ -953,7 +954,8 @@ int rta_addattr_l(struct rtattr *rta, int maxlen, int type,
 	subrta = (struct rtattr *)(((char *)rta) + RTA_ALIGN(rta->rta_len));
 	subrta->rta_type = type;
 	subrta->rta_len = len;
-	memcpy(RTA_DATA(subrta), data, alen);
+	if (alen)
+		memcpy(RTA_DATA(subrta), data, alen);
 	rta->rta_len = NLMSG_ALIGN(rta->rta_len) + RTA_ALIGN(len);
 	return 0;
 }
-- 
2.13.1

Re: [iproute PATCH 21/51] lib/libnetlink: Don't pass NULL parameter to memcpy()

[Stephen Hemminger]

On Sat, 12 Aug 2017 14:04:40 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Both addattr_l() and rta_addattr_l() may be called with NULL data
> pointer and 0 alen parameters. Avoid calling memcpy() in that case.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

What are you fixing. memcpy(dest, NULL, 0) should be harmless NOP

Re: [iproute PATCH 21/51] lib/libnetlink: Don't pass NULL parameter to memcpy()

[Phil Sutter]

On Tue, Aug 15, 2017 at 08:15:55AM -0700, Stephen Hemminger wrote:
> On Sat, 12 Aug 2017 14:04:40 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > Both addattr_l() and rta_addattr_l() may be called with NULL data
> > pointer and 0 alen parameters. Avoid calling memcpy() in that case.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> What are you fixing. memcpy(dest, NULL, 0) should be harmless NOP

Yes, if that turns into a NOP this patch is not needed.

Thanks, Phil

Re: [iproute PATCH 21/51] lib/libnetlink: Don't pass NULL parameter to memcpy()

[Lance Richardson]

> From: "Phil Sutter" <phil@nwl.cc>
> To: "Stephen Hemminger" <stephen@networkplumber.org>
> Cc: netdev@vger.kernel.org
> Sent: Tuesday, August 15, 2017 12:42:55 PM
> Subject: Re: [iproute PATCH 21/51] lib/libnetlink: Don't pass NULL parameter to memcpy()
> 
> On Tue, Aug 15, 2017 at 08:15:55AM -0700, Stephen Hemminger wrote:
> > On Sat, 12 Aug 2017 14:04:40 +0200
> > Phil Sutter <phil@nwl.cc> wrote:
> > 
> > > Both addattr_l() and rta_addattr_l() may be called with NULL data
> > > pointer and 0 alen parameters. Avoid calling memcpy() in that case.
> > > 
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > 
> > What are you fixing. memcpy(dest, NULL, 0) should be harmless NOP
> 
> Yes, if that turns into a NOP this patch is not needed.
> 
> Thanks, Phil
> 

It is a NOP in this case, but it is also "undefined behavior" and can lead
to the compiler assuming that dest != NULL, which would be problematic
if dest were dereferenced later in the code (it isn't in this case, but
might be in general).

A small example with current gcc:

foo.c:
    #include <stdio.h>

    extern void foo(char *, size_t);

    int main(int argc, char **argv)
    {
            char x[128];

            foo(x, sizeof x);
            foo(NULL, 0);

            return 0;
    }

bar.c:
    #include <stdio.h>
    #include <string.h>

    void foo(char *ptr, size_t len)
    {
            memset(ptr, 0, len);

            if (ptr)
                    printf("ptr is non-null: %p\n", ptr);
    }

Compile the code:

    $ gcc -o foobar -O2 foo.c bar.c

Execute it (note second line of output, which might be surprising):

    $ ./foobar
    ptr is non-null: 0x7ffdc47daef0
    ptr is non-null: (nil)


Regards,

    Lance Richardson

[iproute PATCH 22/51] lib/rt_names: Drop dead code in rtnl_rttable_n2a()

[Phil Sutter]

Since 'id' is 32bit unsigned, it can never exceed RT_TABLE_MAX (which is
defined to 0xFFFFFFFF). Therefore drop that never matching conditional.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/rt_names.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/rt_names.c b/lib/rt_names.c
index 04c15ff5b15f8..e5efd78e6f810 100644
--- a/lib/rt_names.c
+++ b/lib/rt_names.c
@@ -410,10 +410,6 @@ const char *rtnl_rttable_n2a(__u32 id, char *buf, int len)
 {
 	struct rtnl_hash_entry *entry;
 
-	if (id > RT_TABLE_MAX) {
-		snprintf(buf, len, "%u", id);
-		return buf;
-	}
 	if (!rtnl_rttable_init)
 		rtnl_rttable_initialize();
 	entry = rtnl_rttable_hash[id & 255];
-- 
2.13.1

[iproute PATCH 23/51] ifstat: Fix memleak in error case

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ifstat.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index a853ee6d7e3b3..8fa354265a9a1 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -143,8 +143,10 @@ static int get_nlmsg_extended(const struct sockaddr_nl *who,
 		struct rtattr *attr;
 
 		attr = parse_rtattr_one_nested(sub_type, tb[filter_type]);
-		if (attr == NULL)
+		if (attr == NULL) {
+			free(n);
 			return 0;
+		}
 		memcpy(&n->val, RTA_DATA(attr), sizeof(n->val));
 	}
 	memset(&n->rate, 0, sizeof(n->rate));
-- 
2.13.1

[iproute PATCH 24/51] ifstat, nstat: Check fdopen() return value

[Phil Sutter]

Prevent passing NULL FILE pointer to fgets() later.

Fix both tools in a single patch since the code changes are basically
identical.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ifstat.c | 16 +++++++++++-----
 misc/nstat.c  | 16 +++++++++++-----
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 8fa354265a9a1..6c8cfa0bd94f5 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -988,12 +988,18 @@ int main(int argc, char *argv[])
 	    && verify_forging(fd) == 0) {
 		FILE *sfp = fdopen(fd, "r");
 
-		load_raw_table(sfp);
-		if (hist_db && source_mismatch) {
-			fprintf(stderr, "ifstat: history is stale, ignoring it.\n");
-			hist_db = NULL;
+		if (!sfp) {
+			fprintf(stderr, "ifstat: fdopen failed: %s\n",
+				strerror(errno));
+			close(fd);
+		} else  {
+			load_raw_table(sfp);
+			if (hist_db && source_mismatch) {
+				fprintf(stderr, "ifstat: history is stale, ignoring it.\n");
+				hist_db = NULL;
+			}
+			fclose(sfp);
 		}
-		fclose(sfp);
 	} else {
 		if (fd >= 0)
 			close(fd);
diff --git a/misc/nstat.c b/misc/nstat.c
index 1212b1f2c8128..a4dd405d43a93 100644
--- a/misc/nstat.c
+++ b/misc/nstat.c
@@ -706,12 +706,18 @@ int main(int argc, char *argv[])
 	    && verify_forging(fd) == 0) {
 		FILE *sfp = fdopen(fd, "r");
 
-		load_good_table(sfp);
-		if (hist_db && source_mismatch) {
-			fprintf(stderr, "nstat: history is stale, ignoring it.\n");
-			hist_db = NULL;
+		if (!sfp) {
+			fprintf(stderr, "nstat: fdopen failed: %s\n",
+				strerror(errno));
+			close(fd);
+		} else {
+			load_good_table(sfp);
+			if (hist_db && source_mismatch) {
+				fprintf(stderr, "nstat: history is stale, ignoring it.\n");
+				hist_db = NULL;
+			}
+			fclose(sfp);
 		}
-		fclose(sfp);
 	} else {
 		if (fd >= 0)
 			close(fd);
-- 
2.13.1

[iproute PATCH 25/51] ifstat: Fix memleak in dump_kern_db() for json output

[Phil Sutter]

Looks like this was forgotten when converting to common json output
formatter.

Fixes: fcc16c2287bf8 ("provide common json output formatter")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ifstat.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 6c8cfa0bd94f5..ac3eff6b870a9 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -535,8 +535,12 @@ static void dump_kern_db(FILE *fp)
 		else
 			print_one_if(fp, n, n->val);
 	}
-	if (json_output)
-		fprintf(fp, "\n} }\n");
+	if (jw) {
+		jsonw_end_object(jw);
+
+		jsonw_end_object(jw);
+		jsonw_destroy(&jw);
+	}
 }
 
 static void dump_incr_db(FILE *fp)
-- 
2.13.1

[iproute PATCH 26/51] lnstat_util: Simplify alloc_and_open() a bit

[Phil Sutter]

Relying upon callers and using unsafe strcpy() is probably not the best
idea. Aside from that, using snprintf() allows to format the string for
lf->path in one go.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/lnstat_util.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/misc/lnstat_util.c b/misc/lnstat_util.c
index cc54598fe1bef..ec19238c24b94 100644
--- a/misc/lnstat_util.c
+++ b/misc/lnstat_util.c
@@ -180,11 +180,8 @@ static struct lnstat_file *alloc_and_open(const char *path, const char *file)
 	}
 
 	/* initialize */
-	/* de->d_name is guaranteed to be <= NAME_MAX */
-	strcpy(lf->basename, file);
-	strcpy(lf->path, path);
-	strcat(lf->path, "/");
-	strcat(lf->path, lf->basename);
+	snprintf(lf->basename, sizeof(lf->basename), "%s", file);
+	snprintf(lf->path, sizeof(lf->path), "%s/%s", path, file);
 
 	/* initialize to default */
 	lf->interval.tv_sec = 1;
-- 
2.13.1

[iproute PATCH 27/51] nstat: Fix for potential NULL pointer dereference

[Phil Sutter]

If the string at 'p' contains neither space not newline, 'p' will become
NULL. Make sure this isn't the case before dereferencing it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/nstat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/nstat.c b/misc/nstat.c
index a4dd405d43a93..23e1569d7872b 100644
--- a/misc/nstat.c
+++ b/misc/nstat.c
@@ -198,7 +198,7 @@ static void load_ugly_table(FILE *fp)
 		off = p - buf;
 		p += 2;
 
-		while (*p) {
+		while (p && *p) {
 			char *next;
 
 			if ((next = strchr(p, ' ')) != NULL)
-- 
2.13.1

[iproute PATCH 28/51] nstat: Avoid passing negative fd to fdopen()

[Phil Sutter]

Introduce a wrapper which does the sanity checking and returns NULL
in case fd is invalid.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/nstat.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/misc/nstat.c b/misc/nstat.c
index 23e1569d7872b..c1e7ddec271e2 100644
--- a/misc/nstat.c
+++ b/misc/nstat.c
@@ -252,9 +252,16 @@ static void load_ugly_table(FILE *fp)
 	}
 }
 
+static FILE *fdopen_null(int fd, const char *mode)
+{
+	if (fd < 0)
+		return NULL;
+	return fdopen(fd, mode);
+}
+
 static void load_sctp_snmp(void)
 {
-	FILE *fp = fdopen(net_sctp_snmp_open(), "r");
+	FILE *fp = fdopen_null(net_sctp_snmp_open(), "r");
 
 	if (fp) {
 		load_good_table(fp);
@@ -264,7 +271,7 @@ static void load_sctp_snmp(void)
 
 static void load_snmp(void)
 {
-	FILE *fp = fdopen(net_snmp_open(), "r");
+	FILE *fp = fdopen_null(net_snmp_open(), "r");
 
 	if (fp) {
 		load_ugly_table(fp);
@@ -274,7 +281,7 @@ static void load_snmp(void)
 
 static void load_snmp6(void)
 {
-	FILE *fp = fdopen(net_snmp6_open(), "r");
+	FILE *fp = fdopen_null(net_snmp6_open(), "r");
 
 	if (fp) {
 		load_good_table(fp);
@@ -284,7 +291,7 @@ static void load_snmp6(void)
 
 static void load_netstat(void)
 {
-	FILE *fp = fdopen(net_netstat_open(), "r");
+	FILE *fp = fdopen_null(net_netstat_open(), "r");
 
 	if (fp) {
 		load_ugly_table(fp);
-- 
2.13.1

[iproute PATCH 29/51] ss: Use C99 initializer in netlink_show_one()

[Phil Sutter]

This has the additional benefit of initializing st.ino to zero which is
used later in is_sctp_assoc() function.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index f0d1c22f75cff..b4f89c85c2d52 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -3470,17 +3470,18 @@ static int netlink_show_one(struct filter *f,
 				int rq, int wq,
 				unsigned long long sk, unsigned long long cb)
 {
-	struct sockstat st;
+	struct sockstat st = {
+		.state		= SS_CLOSE,
+		.rq		= rq,
+		.wq		= wq,
+		.local.family	= AF_NETLINK,
+		.remote.family	= AF_NETLINK,
+	};
 
 	SPRINT_BUF(prot_buf) = {};
 	const char *prot_name;
 	char procname[64] = {};
 
-	st.state = SS_CLOSE;
-	st.rq	 = rq;
-	st.wq	 = wq;
-	st.local.family = st.remote.family = AF_NETLINK;
-
 	if (f->f) {
 		st.rport = -1;
 		st.lport = pid;
-- 
2.13.1

[iproute PATCH 30/51] ss: Skip useless check in parse_hostcond()

[Phil Sutter]

The passed 'addr' parameter is dereferenced by caller before and in
parse_hostcond() multiple times before this check, so assume it is
always true.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/ss.c b/misc/ss.c
index b4f89c85c2d52..5ea388fbf1c1a 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -1671,7 +1671,7 @@ void *parse_hostcond(char *addr, bool is_port)
 			}
 		}
 	}
-	if (!is_port && addr && *addr && *addr != '*') {
+	if (!is_port && *addr && *addr != '*') {
 		if (get_prefix_1(&a.addr, addr, fam)) {
 			if (get_dns_host(&a, addr, fam)) {
 				fprintf(stderr, "Error: an inet prefix is expected rather than \"%s\".\n", addr);
-- 
2.13.1

[iproute PATCH 31/51] ss: Drop useless assignment

[Phil Sutter]

After '*b = *a', 'b->next' already has the same value as 'a->next'.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/misc/ss.c b/misc/ss.c
index 5ea388fbf1c1a..d767b1103ea81 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -1440,7 +1440,6 @@ static int remember_he(struct aafilter *a, struct hostent *he)
 			if ((b = malloc(sizeof(*b))) == NULL)
 				return cnt;
 			*b = *a;
-			b->next = a->next;
 			a->next = b;
 		}
 		memcpy(b->addr.data, *ptr, len);
-- 
2.13.1

[iproute PATCH 32/51] ss: Make sure index variable is >= 0

[Phil Sutter]

This shouldn't happen but relying upon external data without checking
may lead to unexpected results.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/ss.c b/misc/ss.c
index d767b1103ea81..4d2f75b571ea6 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2003,7 +2003,7 @@ static void tcp_timer_print(struct tcpstat *s)
 		"unknown"
 	};
 
-	if (s->timer) {
+	if (s->timer >= 0) {
 		if (s->timer > 4)
 			s->timer = 5;
 		printf(" timer:(%s,%s,%d)",
-- 
2.13.1

[iproute PATCH 33/51] ss: Don't leak fd in tcp_show_netlink_file()

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index 4d2f75b571ea6..cda5e3b6a2d6f 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2687,41 +2687,44 @@ static int tcp_show_netlink_file(struct filter *f)
 {
 	FILE	*fp;
 	char	buf[16384];
+	int	err = -1;
 
 	if ((fp = fopen(getenv("TCPDIAG_FILE"), "r")) == NULL) {
 		perror("fopen($TCPDIAG_FILE)");
-		return -1;
+		return err;
 	}
 
 	while (1) {
-		int status, err;
+		int status, err2;
 		struct nlmsghdr *h = (struct nlmsghdr *)buf;
 		struct sockstat s = {};
 
 		status = fread(buf, 1, sizeof(*h), fp);
 		if (status < 0) {
 			perror("Reading header from $TCPDIAG_FILE");
-			return -1;
+			break;
 		}
 		if (status != sizeof(*h)) {
 			perror("Unexpected EOF reading $TCPDIAG_FILE");
-			return -1;
+			break;
 		}
 
 		status = fread(h+1, 1, NLMSG_ALIGN(h->nlmsg_len-sizeof(*h)), fp);
 
 		if (status < 0) {
 			perror("Reading $TCPDIAG_FILE");
-			return -1;
+			break;
 		}
 		if (status + sizeof(*h) < h->nlmsg_len) {
 			perror("Unexpected EOF reading $TCPDIAG_FILE");
-			return -1;
+			break;
 		}
 
 		/* The only legal exit point */
-		if (h->nlmsg_type == NLMSG_DONE)
-			return 0;
+		if (h->nlmsg_type == NLMSG_DONE) {
+			err = 0;
+			break;
+		}
 
 		if (h->nlmsg_type == NLMSG_ERROR) {
 			struct nlmsgerr *err = (struct nlmsgerr *)NLMSG_DATA(h);
@@ -2732,7 +2735,7 @@ static int tcp_show_netlink_file(struct filter *f)
 				errno = -err->error;
 				perror("TCPDIAG answered");
 			}
-			return -1;
+			break;
 		}
 
 		parse_diag_msg(h, &s);
@@ -2741,10 +2744,15 @@ static int tcp_show_netlink_file(struct filter *f)
 		if (f && f->f && run_ssfilter(f->f, &s) == 0)
 			continue;
 
-		err = inet_show_sock(h, &s);
-		if (err < 0)
-			return err;
+		err2 = inet_show_sock(h, &s);
+		if (err2 < 0) {
+			err = err2;
+			break;
+		}
 	}
+
+	fclose(fp);
+	return err;
 }
 
 static int tcp_show(struct filter *f, int socktype)
-- 
2.13.1

[iproute PATCH 34/51] ss: Make sure scanned index value to unix_state_map is sane

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/misc/ss.c b/misc/ss.c
index cda5e3b6a2d6f..667b8faad6528 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -3150,7 +3150,8 @@ static int unix_show(struct filter *f)
 
 		if (flags & (1 << 16)) {
 			u->state = SS_LISTEN;
-		} else {
+		} else if (u->state > 0 &&
+			   u->state <= ARRAY_SIZE(unix_state_map)) {
 			u->state = unix_state_map[u->state-1];
 			if (u->type == SOCK_DGRAM && u->state == SS_CLOSE && u->rport)
 				u->state = SS_ESTABLISHED;
-- 
2.13.1

[iproute PATCH 35/51] ss: Fix potential memleak in unix_stats_print()

[Phil Sutter]

Fixes: 2d0e538f3e1cd ("ss: Drop list traversal from unix_stats_print()")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/misc/ss.c b/misc/ss.c
index 667b8faad6528..7d84b83c8ad71 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -3170,8 +3170,10 @@ static int unix_show(struct filter *f)
 
 		if (name[0]) {
 			u->name = strdup(name);
-			if (!u->name)
+			if (!u->name) {
+				free(u);
 				break;
+			}
 		}
 
 		if (u->rport) {
-- 
2.13.1

[iproute PATCH 36/51] netem/maketable: Check return value of fstat()

[Phil Sutter]

Otherwise info.st_size may contain garbage.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 netem/maketable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/netem/maketable.c b/netem/maketable.c
index 6aff927be7040..ad660e7d457f0 100644
--- a/netem/maketable.c
+++ b/netem/maketable.c
@@ -24,8 +24,8 @@ readdoubles(FILE *fp, int *number)
 	int limit;
 	int n=0, i;
 
-	fstat(fileno(fp), &info);
-	if (info.st_size > 0) {
+	if (!fstat(fileno(fp), &info) &&
+	    info.st_size > 0) {
 		limit = 2*info.st_size/sizeof(double);	/* @@ approximate */
 	} else {
 		limit = 10000;
-- 
2.13.1

[iproute PATCH 37/51] netem/maketable: Check return value of fscanf()

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 netem/maketable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/netem/maketable.c b/netem/maketable.c
index ad660e7d457f0..ccb8f0c68b062 100644
--- a/netem/maketable.c
+++ b/netem/maketable.c
@@ -38,8 +38,8 @@ readdoubles(FILE *fp, int *number)
 	}
 
 	for (i=0; i<limit; ++i){
-		fscanf(fp, "%lf", &x[i]);
-		if (feof(fp))
+		if (fscanf(fp, "%lf", &x[i]) != 1 ||
+		    feof(fp))
 			break;
 		++n;
 	}
-- 
2.13.1

[iproute PATCH 38/51] tc/em_ipset: Don't leak sockfd on error path

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/em_ipset.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tc/em_ipset.c b/tc/em_ipset.c
index fab975f5ea563..b59756515d239 100644
--- a/tc/em_ipset.c
+++ b/tc/em_ipset.c
@@ -84,6 +84,7 @@ static int get_version(unsigned int *version)
 	res = getsockopt(sockfd, SOL_IP, SO_IP_SET, &req_version, &size);
 	if (res != 0) {
 		perror("xt_set getsockopt");
+		close(sockfd);
 		return -1;
 	}
 
-- 
2.13.1

[iproute PATCH 39/51] tc/m_gact: Drop dead code

[Phil Sutter]

The use of 'ok' variable in parse_gact() is ineffective: The second
conditional increments it either if *argv is 'gact' or if
parse_action_control() doesn't fail (in which case exit() is called).
So this is effectively an unconditional increment and since no decrement
happens anywhere, all remaining checks for 'ok != 0' can be dropped.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/m_gact.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/tc/m_gact.c b/tc/m_gact.c
index 1a2583372c34e..df143c9e0953e 100644
--- a/tc/m_gact.c
+++ b/tc/m_gact.c
@@ -76,7 +76,6 @@ parse_gact(struct action_util *a, int *argc_p, char ***argv_p,
 {
 	int argc = *argc_p;
 	char **argv = *argv_p;
-	int ok = 0;
 	struct tc_gact p = { 0 };
 #ifdef CONFIG_GACT_PROB
 	int rd = 0;
@@ -89,17 +88,14 @@ parse_gact(struct action_util *a, int *argc_p, char ***argv_p,
 
 
 	if (matches(*argv, "gact") == 0) {
-		ok++;
 		argc--;
 		argv++;
-	} else {
-		if (parse_action_control(&argc, &argv, &p.action, false) == -1)
-			usage();
-		ok++;
+	} else if (parse_action_control(&argc, &argv, &p.action, false) == -1) {
+		usage();	/* does not return */
 	}
 
 #ifdef CONFIG_GACT_PROB
-	if (ok && argc > 0) {
+	if (argc > 0) {
 		if (matches(*argv, "random") == 0) {
 			rd = 1;
 			NEXT_ARG();
@@ -142,15 +138,11 @@ parse_gact(struct action_util *a, int *argc_p, char ***argv_p,
 			}
 			argc--;
 			argv++;
-			ok++;
 		} else if (matches(*argv, "help") == 0) {
 				usage();
 		}
 	}
 
-	if (!ok)
-		return -1;
-
 	tail = NLMSG_TAIL(n);
 	addattr_l(n, MAX_MSG, tca_id, NULL, 0);
 	addattr_l(n, MAX_MSG, TCA_GACT_PARMS, &p, sizeof(p));
-- 
2.13.1

[iproute PATCH 40/51] tc/m_xt: Fix for potential string buffer overflows

[Phil Sutter]

- Use strncpy() when writing to target->t->u.user.name and make sure the
  final byte remains untouched (xtables_calloc() set it to zero).
- 'tname' length sanitization was completely wrong: If it's length
  exceeded the 16 bytes available in 'k', passing a length value of 16
  to strncpy() would overwrite the previously NULL'ed 'k[15]'. Also, the
  sanitization has to happen if 'tname' is exactly 16 bytes long as
  well.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/m_xt.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tc/m_xt.c b/tc/m_xt.c
index ad52d239caf61..9218b14594403 100644
--- a/tc/m_xt.c
+++ b/tc/m_xt.c
@@ -95,7 +95,8 @@ build_st(struct xtables_target *target, struct xt_entry_target *t)
 	if (t == NULL) {
 		target->t = xtables_calloc(1, size);
 		target->t->u.target_size = size;
-		strcpy(target->t->u.user.name, target->name);
+		strncpy(target->t->u.user.name, target->name,
+			sizeof(target->t->u.user.name) - 1);
 		target->t->u.user.revision = target->revision;
 
 		if (target->init != NULL)
@@ -277,8 +278,8 @@ static int parse_ipt(struct action_util *a, int *argc_p,
 	}
 	fprintf(stdout, " index %d\n", index);
 
-	if (strlen(tname) > 16) {
-		size = 16;
+	if (strlen(tname) >= 16) {
+		size = 15;
 		k[15] = 0;
 	} else {
 		size = 1 + strlen(tname);
-- 
2.13.1

[iproute PATCH 41/51] tc/q_multiq: Don't pass garbage in TCA_OPTIONS

[Phil Sutter]

multiq_parse_opt() doesn't change 'opt' at all. So at least make sure
it doesn't fill TCA_OPTIONS attribute with garbage from stack.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/q_multiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/q_multiq.c b/tc/q_multiq.c
index 7823931494563..9c09c9a7748f6 100644
--- a/tc/q_multiq.c
+++ b/tc/q_multiq.c
@@ -43,7 +43,7 @@ static void explain(void)
 static int multiq_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 			    struct nlmsghdr *n)
 {
-	struct tc_multiq_qopt opt;
+	struct tc_multiq_qopt opt = {};
 
 	if (argc) {
 		if (strcmp(*argv, "help") == 0) {
-- 
2.13.1

[iproute PATCH 42/51] tc/q_netem: Don't dereference possibly NULL pointer

[Phil Sutter]

Assuming 'opt' might be NULL, move the call to RTA_PAYLOAD to after the
check since it dereferences its parameter.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/q_netem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tc/q_netem.c b/tc/q_netem.c
index 0975ae111de97..7e3330512041a 100644
--- a/tc/q_netem.c
+++ b/tc/q_netem.c
@@ -538,7 +538,7 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	int *ecn = NULL;
 	struct tc_netem_qopt qopt;
 	const struct tc_netem_rate *rate = NULL;
-	int len = RTA_PAYLOAD(opt) - sizeof(qopt);
+	int len;
 	__u64 rate64 = 0;
 
 	SPRINT_BUF(b1);
@@ -546,6 +546,8 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	if (opt == NULL)
 		return 0;
 
+	len = RTA_PAYLOAD(opt) - sizeof(qopt);
+
 	if (len < 0) {
 		fprintf(stderr, "options size error\n");
 		return -1;
-- 
2.13.1

[iproute PATCH 43/51] tc/tc_filter: Make sure filter name is not empty

[Phil Sutter]

The later check for 'k[0] != 0' requires a non-empty filter name,
otherwise NULL pointer dereference in 'q' might happen.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/tc_filter.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tc/tc_filter.c b/tc/tc_filter.c
index b13fb9185d4fd..a799edb35886d 100644
--- a/tc/tc_filter.c
+++ b/tc/tc_filter.c
@@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv)
 			usage();
 			return 0;
 		} else {
+			if (!strlen(*argv))
+				invarg("invalid filter name", *argv);
+
 			strncpy(k, *argv, sizeof(k)-1);
 
 			q = get_filter_kind(k);
-- 
2.13.1

[iproute PATCH 44/51] tipc/bearer: Fix resource leak in error path

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tipc/bearer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tipc/bearer.c b/tipc/bearer.c
index 810344f672af1..c3d4491f8f6ef 100644
--- a/tipc/bearer.c
+++ b/tipc/bearer.c
@@ -163,6 +163,7 @@ static int nl_add_udp_enable_opts(struct nlmsghdr *nlh, struct opt *opts,
 	if (!remip) {
 		if (generate_multicast(loc->ai_family, buf, sizeof(buf))) {
 			fprintf(stderr, "Failed to generate multicast address\n");
+			freeaddrinfo(loc);
 			return -EINVAL;
 		}
 		remip = buf;
@@ -177,6 +178,8 @@ static int nl_add_udp_enable_opts(struct nlmsghdr *nlh, struct opt *opts,
 
 	if (rem->ai_family != loc->ai_family) {
 		fprintf(stderr, "UDP local and remote AF mismatch\n");
+		freeaddrinfo(rem);
+		freeaddrinfo(loc);
 		return -EINVAL;
 	}
 
-- 
2.13.1

[iproute PATCH 45/51] tipc/bearer: Prevent NULL pointer dereference

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tipc/bearer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tipc/bearer.c b/tipc/bearer.c
index c3d4491f8f6ef..0598328ab1f1b 100644
--- a/tipc/bearer.c
+++ b/tipc/bearer.c
@@ -438,8 +438,8 @@ static int cmd_bearer_enable(struct nlmsghdr *nlh, const struct cmd *cmd,
 	if (err)
 		return err;
 
-	opt = get_opt(opts, "media");
-	if (strcmp(opt->val, "udp") == 0) {
+	if ((opt = get_opt(opts, "media")) &&
+	    strcmp(opt->val, "udp") == 0) {
 		err = nl_add_udp_enable_opts(nlh, opts, cmdl);
 		if (err)
 			return err;
-- 
2.13.1

[iproute PATCH 46/51] tipc/node: Fix socket fd check in cmd_node_get_addr()

[Phil Sutter]

socket() returns -1 on error, not 0.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tipc/node.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tipc/node.c b/tipc/node.c
index 201fe1a4df3bd..fe085aec9b4ac 100644
--- a/tipc/node.c
+++ b/tipc/node.c
@@ -109,7 +109,8 @@ static int cmd_node_get_addr(struct nlmsghdr *nlh, const struct cmd *cmd,
 	socklen_t sz = sizeof(struct sockaddr_tipc);
 	struct sockaddr_tipc addr;
 
-	if (!(sk = socket(AF_TIPC, SOCK_RDM, 0))) {
+	sk = socket(AF_TIPC, SOCK_RDM, 0);
+	if (sk < 0) {
 		fprintf(stderr, "opening TIPC socket: %s\n", strerror(errno));
 		return -1;
 	}
-- 
2.13.1

[iproute PATCH 47/51] examples: Some shell fixes to cbq.init

[Phil Sutter]

This addresses the following issues:

- $@ is an array, so don't use it in quoted strings - use $* instead.

- Add missing quotes to components of [ ] expressions. These are not
  strictly necessary since the output of 'wc -l' should be a single word
  only, but in case of errors, bash prints "integer expression expected"
  instead of "too many arguments".

- Use -print0/-0 when piping from find to xargs to allow for filenames
  which contain whitespace.

- Quote arguments to 'eval' to prevent word-splitting.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 examples/cbq.init-v0.7.3 | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/examples/cbq.init-v0.7.3 b/examples/cbq.init-v0.7.3
index 1bc0d446f8983..66448d88f0053 100644
--- a/examples/cbq.init-v0.7.3
+++ b/examples/cbq.init-v0.7.3
@@ -532,7 +532,7 @@ cbq_off () {
 
 ### Prefixed message
 cbq_message () {
-	echo -e "**CBQ: $@"
+	echo -e "**CBQ: $*"
 } # cbq_message
 
 ### Failure message
@@ -560,15 +560,15 @@ cbq_time2abs () {
 ### Display CBQ setup
 cbq_show () {
 	for dev in `cbq_device_list`; do
-		[ `tc qdisc show dev $dev| wc -l` -eq 0 ] && continue
+		[ "`tc qdisc show dev $dev| wc -l`" -eq 0 ] && continue
 		echo -e "### $dev: queueing disciplines\n"
 		tc $1 qdisc show dev $dev; echo
 
-		[ `tc class show dev $dev| wc -l` -eq 0 ] && continue
+		[ "`tc class show dev $dev| wc -l`" -eq 0 ] && continue
 		echo -e "### $dev: traffic classes\n"
 		tc $1 class show dev $dev; echo
 
-		[ `tc filter show dev $dev| wc -l` -eq 0 ] && continue
+		[ "`tc filter show dev $dev| wc -l`" -eq 0 ] && continue
 		echo -e "### $dev: filtering rules\n"
 		tc $1 filter show dev $dev; echo
 	done
@@ -585,7 +585,7 @@ cbq_init () {
 
 	### Gather all DEVICE fields from $1/cbq-*
 	DEVFIELDS=`find $1 -maxdepth 1 \( -type f -or -type l \) -name 'cbq-*' \
-		  -not -name '*~' | xargs sed -n 's/#.*//; \
+		  -not -name '*~' -print0 | xargs -0 sed -n 's/#.*//; \
 		  s/[[:space:]]//g; /^DEVICE=[^,]*,[^,]*\(,[^,]*\)\?/ \
 		  { s/.*=//; p; }'| sort -u`
 	[ -z "$DEVFIELDS" ] &&
@@ -593,7 +593,7 @@ cbq_init () {
 
 	### Check for different DEVICE fields for the same device
 	DEVICES=`echo "$DEVFIELDS"| sed 's/,.*//'| sort -u`
-	[ `echo "$DEVICES"| wc -l` -ne `echo "$DEVFIELDS"| wc -l` ] &&
+	[ "`echo "$DEVICES"| wc -l`" -ne "`echo "$DEVFIELDS"| wc -l`" ] &&
 		cbq_failure "different DEVICE fields for single device!\n$DEVFIELDS"
 } # cbq_init
 
@@ -618,7 +618,7 @@ cbq_load_class () {
 	PRIO_MARK=$PRIO_MARK_DEFAULT
 	PRIO_REALM=$PRIO_REALM_DEFAULT
 
-	eval `echo "$CFILE"| grep -E "^($CBQ_WORDS)="`
+	eval "`echo "$CFILE"| grep -E "^($CBQ_WORDS)="`"
 
 	### Require RATE/WEIGHT
 	[ -z "$RATE" -o -z "$WEIGHT" ] &&
@@ -661,7 +661,7 @@ if [ "$1" = "compile" ]; then
 
 	### echo-only version of "tc" command
 	tc () {
-		echo "$TC $@"
+		echo "$TC $*"
 	} # tc
 
 elif [ -n "$CBQ_DEBUG" ]; then
@@ -669,13 +669,13 @@ elif [ -n "$CBQ_DEBUG" ]; then
 
 	### Logging version of "ip" command
 	ip () {
-		echo -e "\n# ip $@" >> $CBQ_DEBUG
+		echo -e "\n# ip $*" >> $CBQ_DEBUG
 		$IP "$@" 2>&1 | tee -a $CBQ_DEBUG
 	} # ip
 
 	### Logging version of "tc" command
 	tc () {
-		echo -e "\n# tc $@" >> $CBQ_DEBUG
+		echo -e "\n# tc $*" >> $CBQ_DEBUG
 		$TC "$@" 2>&1 | tee -a $CBQ_DEBUG
 	} # tc
 else
@@ -711,8 +711,8 @@ if [ "$1" != "compile" -a "$2" != "nocache" -a -z "$CBQ_DEBUG" ]; then
 	### validate the cache
 	[ "$2" = "invalidate" -o ! -f $CBQ_CACHE ] && VALID=0
 	if [ $VALID -eq 1 ]; then
-		[ `find $CBQ_PATH -maxdepth 1 -newer $CBQ_CACHE| \
-		  wc -l` -gt 0 ] && VALID=0
+		[ "`find $CBQ_PATH -maxdepth 1 -newer $CBQ_CACHE| \
+		  wc -l`" -gt 0 ] && VALID=0
 	fi
 
 	### compile the config if the cache is invalid
-- 
2.13.1

[iproute PATCH 48/51] ifcfg: Quote left-hand side of [ ] expression

[Phil Sutter]

This prevents word-splitting and therefore leads to more accurate error
message in case 'grep -c' prints something other than a number.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ifcfg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/ifcfg b/ip/ifcfg
index 083d9df36742f..30a2dc49816cd 100644
--- a/ip/ifcfg
+++ b/ip/ifcfg
@@ -131,7 +131,7 @@ noarp=$?
 
 ip route add unreachable 224.0.0.0/24 >& /dev/null
 ip route add unreachable 255.255.255.255 >& /dev/null
-if [ `ip link ls $dev | grep -c MULTICAST` -ge 1 ]; then
+if [ "`ip link ls $dev | grep -c MULTICAST`" -ge 1 ]; then
   ip route add 224.0.0.0/4 dev $dev scope global >& /dev/null
 fi
 
-- 
2.13.1

[iproute PATCH 49/51] lib/ll_map: Make sure im->name is NULL-terminated

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/ll_map.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/ll_map.c b/lib/ll_map.c
index 4e4556c9ac80b..4d06eb69f138a 100644
--- a/lib/ll_map.c
+++ b/lib/ll_map.c
@@ -120,11 +120,11 @@ int ll_remember_index(const struct sockaddr_nl *who,
 		return 0;
 	}
 
-	im = malloc(sizeof(*im));
+	im = calloc(1, sizeof(*im));
 	if (im == NULL)
 		return 0;
 	im->index = ifi->ifi_index;
-	strcpy(im->name, ifname);
+	strncpy(im->name, ifname, IFNAMSIZ - 1);
 	im->type = ifi->ifi_type;
 	im->flags = ifi->ifi_flags;
 
-- 
2.13.1

[iproute PATCH 50/51] Check user supplied interface name lengths

[Phil Sutter]

The original problem was that something like:

| strncpy(ifr.ifr_name, *argv, IFNAMSIZ);

might leave ifr.ifr_name unterminated if length of *argv exceeds
IFNAMSIZ. In order to fix this, I thought about replacing all those
cases with (equivalent) calls to snprintf() or even introducing
strlcpy(). But as Ulrich Drepper correctly pointed out when rejecting
the latter from being added to glibc, truncating a string without
notifying the user is not to be considered good practice. So let's
excercise what he suggested and reject empty or overlong interface names
right from the start - this way calls to strncpy() like shown above
become safe and the user has a chance to reconsider what he was trying
to do.

Note that this doesn't add calls to assert_valid_dev_name() to all
places where user supplied interface name is parsed. In many cases, the
interface must exist already and is therefore being looked up using
ll_name_to_index(), so if_nametoindex() will perform the necessary
checks already.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/utils.h |  1 +
 ip/ip6tunnel.c  |  6 ++++--
 ip/ipl2tp.c     |  1 +
 ip/iplink.c     | 27 ++++++++-------------------
 ip/ipmaddr.c    |  1 +
 ip/iprule.c     |  4 ++++
 ip/iptunnel.c   | 12 ++++++++----
 ip/iptuntap.c   |  4 +++-
 lib/utils.c     |  8 ++++++++
 misc/arpd.c     |  1 +
 10 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index 6080b962fb411..103def33c92f3 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -132,6 +132,7 @@ void missarg(const char *) __attribute__((noreturn));
 void invarg(const char *, const char *) __attribute__((noreturn));
 void duparg(const char *, const char *) __attribute__((noreturn));
 void duparg2(const char *, const char *) __attribute__((noreturn));
+void assert_valid_dev_name(const char *, const char *);
 int matches(const char *arg, const char *pattern);
 int inet_addr_match(const inet_prefix *a, const inet_prefix *b, int bits);
 
diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index b4a7def144226..b3ccfcf87be8f 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -180,7 +180,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 			memcpy(&p->laddr, &laddr.data, sizeof(p->laddr));
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(medium, *argv, IFNAMSIZ - 1);
+			assert_valid_dev_name("dev", *argv);
+			strncpy(medium, *argv, IFNAMSIZ);
 		} else if (strcmp(*argv, "encaplimit") == 0) {
 			NEXT_ARG();
 			if (strcmp(*argv, "none") == 0) {
@@ -273,7 +274,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 				usage();
 			if (p->name[0])
 				duparg2("name", *argv);
-			strncpy(p->name, *argv, IFNAMSIZ - 1);
+			assert_valid_dev_name("name", *argv);
+			strncpy(p->name, *argv, IFNAMSIZ);
 			if (cmd == SIOCCHGTUNNEL && count == 0) {
 				struct ip6_tnl_parm2 old_p = {};
 
diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c
index 88664c909e11f..cdf9f4d8425d4 100644
--- a/ip/ipl2tp.c
+++ b/ip/ipl2tp.c
@@ -545,6 +545,7 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
 			}
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();
+			assert_valid_dev_name("name", *argv);
 			p->ifname = *argv;
 		} else if (strcmp(*argv, "remote") == 0) {
 			NEXT_ARG();
diff --git a/ip/iplink.c b/ip/iplink.c
index 5aff2fde38dae..8f76f0467fc9c 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -869,7 +869,6 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 
 static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 {
-	int len;
 	char *dev = NULL;
 	char *name = NULL;
 	char *link = NULL;
@@ -959,13 +958,9 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 	}
 
 	if (name) {
-		len = strlen(name) + 1;
-		if (len == 1)
-			invarg("\"\" is not a valid device identifier\n",
-			       "name");
-		if (len > IFNAMSIZ)
-			invarg("\"name\" too long\n", name);
-		addattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);
+		assert_valid_dev_name("name", name);
+		addattr_l(&req.n, sizeof(req),
+			  IFLA_IFNAME, name, strlen(name) + 1);
 	}
 
 	if (type) {
@@ -1015,7 +1010,6 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 
 int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
 {
-	int len;
 	struct iplink_req req = {
 		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
 		.n.nlmsg_flags = NLM_F_REQUEST | flags,
@@ -1028,13 +1022,9 @@ int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
 	} answer;
 
 	if (name) {
-		len = strlen(name) + 1;
-		if (len == 1)
-			invarg("\"\" is not a valid device identifier\n",
-				   "name");
-		if (len > IFNAMSIZ)
-			invarg("\"name\" too long\n", name);
-		addattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);
+		assert_valid_dev_name("name", name);
+		addattr_l(&req.n, sizeof(req),
+			  IFLA_IFNAME, name, strlen(name) + 1);
 	}
 	addattr32(&req.n, sizeof(req), IFLA_EXT_MASK, filt_mask);
 
@@ -1357,6 +1347,7 @@ static int do_set(int argc, char **argv)
 			"Not enough of information: \"dev\" argument is required.\n");
 		exit(-1);
 	}
+	assert_valid_dev_name("dev", dev);
 
 	if (newaddr || newbrd) {
 		halen = get_address(dev, &htype);
@@ -1375,9 +1366,7 @@ static int do_set(int argc, char **argv)
 	}
 
 	if (newname && strcmp(dev, newname)) {
-		if (strlen(newname) == 0)
-			invarg("\"\" is not a valid device identifier\n",
-			       "name");
+		assert_valid_dev_name("name", newname);
 		if (do_changename(dev, newname) < 0)
 			return -1;
 		dev = newname;
diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c
index 85a69e779563d..753e32d5807c0 100644
--- a/ip/ipmaddr.c
+++ b/ip/ipmaddr.c
@@ -284,6 +284,7 @@ static int multiaddr_modify(int cmd, int argc, char **argv)
 			NEXT_ARG();
 			if (ifr.ifr_name[0])
 				duparg("dev", *argv);
+			assert_valid_dev_name("dev", *argv);
 			strncpy(ifr.ifr_name, *argv, IFNAMSIZ);
 		} else {
 			if (matches(*argv, "address") == 0) {
diff --git a/ip/iprule.c b/ip/iprule.c
index 8313138db815f..0184e922053ae 100644
--- a/ip/iprule.c
+++ b/ip/iprule.c
@@ -472,10 +472,12 @@ static int iprule_list_flush_or_save(int argc, char **argv, int action)
 		} else if (strcmp(*argv, "dev") == 0 ||
 			   strcmp(*argv, "iif") == 0) {
 			NEXT_ARG();
+			assert_valid_dev_name("iif", *argv);
 			strncpy(filter.iif, *argv, IFNAMSIZ);
 			filter.iifmask = 1;
 		} else if (strcmp(*argv, "oif") == 0) {
 			NEXT_ARG();
+			assert_valid_dev_name("oif", *argv);
 			strncpy(filter.oif, *argv, IFNAMSIZ);
 			filter.oifmask = 1;
 		} else if (strcmp(*argv, "l3mdev") == 0) {
@@ -695,10 +697,12 @@ static int iprule_modify(int cmd, int argc, char **argv)
 		} else if (strcmp(*argv, "dev") == 0 ||
 			   strcmp(*argv, "iif") == 0) {
 			NEXT_ARG();
+			assert_valid_dev_name("iif", *argv);
 			addattr_l(&req.n, sizeof(req), FRA_IFNAME,
 				  *argv, strlen(*argv)+1);
 		} else if (strcmp(*argv, "oif") == 0) {
 			NEXT_ARG();
+			assert_valid_dev_name("oif", *argv);
 			addattr_l(&req.n, sizeof(req), FRA_OIFNAME,
 				  *argv, strlen(*argv)+1);
 		} else if (strcmp(*argv, "l3mdev") == 0) {
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 105d0f5576f1a..b9dd19c6f83c5 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -139,7 +139,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 				p->iph.saddr = htonl(INADDR_ANY);
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(medium, *argv, IFNAMSIZ - 1);
+			assert_valid_dev_name("dev", *argv);
+			strncpy(medium, *argv, IFNAMSIZ);
 		} else if (strcmp(*argv, "ttl") == 0 ||
 			   strcmp(*argv, "hoplimit") == 0 ||
 			   strcmp(*argv, "hlim") == 0) {
@@ -178,7 +179,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 
 			if (p->name[0])
 				duparg2("name", *argv);
-			strncpy(p->name, *argv, IFNAMSIZ - 1);
+			assert_valid_dev_name("name", *argv);
+			strncpy(p->name, *argv, IFNAMSIZ);
 			if (cmd == SIOCCHGTUNNEL && count == 0) {
 				struct ip_tunnel_parm old_p = {};
 
@@ -488,7 +490,8 @@ static int do_prl(int argc, char **argv)
 			count++;
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(medium, *argv, IFNAMSIZ-1);
+			assert_valid_dev_name("dev", *argv);
+			strncpy(medium, *argv, IFNAMSIZ);
 			devname++;
 		} else {
 			fprintf(stderr,
@@ -537,7 +540,8 @@ static int do_6rd(int argc, char **argv)
 			cmd = SIOCDEL6RD;
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(medium, *argv, IFNAMSIZ-1);
+			assert_valid_dev_name("dev", *argv);
+			strncpy(medium, *argv, IFNAMSIZ);
 			devname++;
 		} else {
 			fprintf(stderr,
diff --git a/ip/iptuntap.c b/ip/iptuntap.c
index 451f7f0eac6bb..062f3d6ed036e 100644
--- a/ip/iptuntap.c
+++ b/ip/iptuntap.c
@@ -176,7 +176,8 @@ static int parse_args(int argc, char **argv,
 			ifr->ifr_flags |= IFF_MULTI_QUEUE;
 		} else if (matches(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(ifr->ifr_name, *argv, IFNAMSIZ-1);
+			assert_valid_dev_name("dev", *argv);
+			strncpy(ifr->ifr_name, *argv, IFNAMSIZ);
 		} else {
 			if (matches(*argv, "name") == 0) {
 				NEXT_ARG();
@@ -184,6 +185,7 @@ static int parse_args(int argc, char **argv,
 				usage();
 			if (ifr->ifr_name[0])
 				duparg2("name", *argv);
+			assert_valid_dev_name("name", *argv);
 			strncpy(ifr->ifr_name, *argv, IFNAMSIZ);
 		}
 		count++;
diff --git a/lib/utils.c b/lib/utils.c
index 9143ed2284870..002063075fd61 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -699,6 +699,14 @@ void duparg2(const char *key, const char *arg)
 	exit(-1);
 }
 
+void assert_valid_dev_name(const char *arg, const char *name)
+{
+	size_t len = strlen(name);
+
+	if (len < 1 || len >= IFNAMSIZ)
+		invarg("empty or overlong interface name.", len ? name : arg);
+}
+
 int matches(const char *cmd, const char *pattern)
 {
 	int len = strlen(cmd);
diff --git a/misc/arpd.c b/misc/arpd.c
index bfab44544ee1d..4b834868e0ec7 100644
--- a/misc/arpd.c
+++ b/misc/arpd.c
@@ -664,6 +664,7 @@ int main(int argc, char **argv)
 		struct ifreq ifr = {};
 
 		for (i = 0; i < ifnum; i++) {
+			assert_valid_dev_name(ifnames[i], ifnames[i]);
 			strncpy(ifr.ifr_name, ifnames[i], IFNAMSIZ);
 			if (ioctl(udp_sock, SIOCGIFINDEX, &ifr)) {
 				perror("ioctl(SIOCGIFINDEX)");
-- 
2.13.1

Re: [iproute PATCH 50/51] Check user supplied interface name lengths

[Stephen Hemminger]

On Sat, 12 Aug 2017 14:05:09 +0200
Phil Sutter <phil@nwl.cc> wrote:

> +void assert_valid_dev_name(const char *, const char *);

Not a fan of long function names.
“I have only made this letter longer because I have not had the time to make it shorter."

Maybe just add a new function addattr_ifname() and add the checking there?

Re: [iproute PATCH 50/51] Check user supplied interface name lengths

[Phil Sutter]

On Tue, Aug 15, 2017 at 09:09:45AM -0700, Stephen Hemminger wrote:
> On Sat, 12 Aug 2017 14:05:09 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > +void assert_valid_dev_name(const char *, const char *);
> 
> Not a fan of long function names.
> “I have only made this letter longer because I have not had the time to make it shorter."

:)

> Maybe just add a new function addattr_ifname() and add the checking there?

It is not only about netlink attributes - these are in fact
unproblematic, since they allow for interface names longer than
IFNAMSIZ-1 and the kernel will then reject. The length check has to
happen before copying into any IFNAMSIZ-sized buffer takes place (e.g.
ifr_name of struct ifreq). Logically I would prefer to perform the
checks right at the point user input parsing takes place since it
belongs there.

I could rename the function to 'ifname_valid' instead? I liked having
'assert' in the name though, since the function calls exit() on error.

Thanks, Phil

Re: [iproute PATCH 50/51] Check user supplied interface name lengths

[Phil Sutter]

Hi Stephen,

On Tue, Aug 15, 2017 at 06:51:32PM +0200, Phil Sutter wrote:
> On Tue, Aug 15, 2017 at 09:09:45AM -0700, Stephen Hemminger wrote:
> > On Sat, 12 Aug 2017 14:05:09 +0200
> > Phil Sutter <phil@nwl.cc> wrote:
> > 
> > > +void assert_valid_dev_name(const char *, const char *);
> > 
> > Not a fan of long function names.
> > “I have only made this letter longer because I have not had the time to make it shorter."
> 
> :)
> 
> > Maybe just add a new function addattr_ifname() and add the checking there?
> 
> It is not only about netlink attributes - these are in fact
> unproblematic, since they allow for interface names longer than
> IFNAMSIZ-1 and the kernel will then reject. The length check has to
> happen before copying into any IFNAMSIZ-sized buffer takes place (e.g.
> ifr_name of struct ifreq). Logically I would prefer to perform the
> checks right at the point user input parsing takes place since it
> belongs there.
> 
> I could rename the function to 'ifname_valid' instead? I liked having
> 'assert' in the name though, since the function calls exit() on error.

Any thoughts on this?

Cheers, Phil

[iproute PATCH 51/51] lib/bpf: Check return value of write()

[Phil Sutter]

This is merely to silence the compiler warning. If write to stderr
failed, assume that printing an error message will fail as well so don't
even try.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/bpf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/bpf.c b/lib/bpf.c
index 1dcb261dc915f..825e071cea572 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -591,7 +591,8 @@ int bpf_trace_pipe(void)
 
 		ret = read(fd, buff, sizeof(buff) - 1);
 		if (ret > 0) {
-			write(2, buff, ret);
+			if (write(STDERR_FILENO, buff, ret) != ret)
+				return -1;
 			fflush(stderr);
 		}
 	}
-- 
2.13.1

Re: [iproute PATCH 51/51] lib/bpf: Check return value of write()

[Daniel Borkmann]

On 08/12/2017 02:05 PM, Phil Sutter wrote:
> This is merely to silence the compiler warning. If write to stderr
> failed, assume that printing an error message will fail as well so don't
> even try.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>   lib/bpf.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 1dcb261dc915f..825e071cea572 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -591,7 +591,8 @@ int bpf_trace_pipe(void)
>
>   		ret = read(fd, buff, sizeof(buff) - 1);
>   		if (ret > 0) {
> -			write(2, buff, ret);
> +			if (write(STDERR_FILENO, buff, ret) != ret)
> +				return -1;

Quite unlikely to fail, but we should probably bark loudly
here instead of just returning -1. Perhaps assert() would
suit better.

>   			fflush(stderr);
>   		}
>   	}
>

Re: [iproute PATCH 51/51] lib/bpf: Check return value of write()

[Phil Sutter]

On Mon, Aug 14, 2017 at 11:17:39AM +0200, Daniel Borkmann wrote:
> On 08/12/2017 02:05 PM, Phil Sutter wrote:
> > This is merely to silence the compiler warning. If write to stderr
> > failed, assume that printing an error message will fail as well so don't
> > even try.
> >
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >   lib/bpf.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/bpf.c b/lib/bpf.c
> > index 1dcb261dc915f..825e071cea572 100644
> > --- a/lib/bpf.c
> > +++ b/lib/bpf.c
> > @@ -591,7 +591,8 @@ int bpf_trace_pipe(void)
> >
> >   		ret = read(fd, buff, sizeof(buff) - 1);
> >   		if (ret > 0) {
> > -			write(2, buff, ret);
> > +			if (write(STDERR_FILENO, buff, ret) != ret)
> > +				return -1;
> 
> Quite unlikely to fail, but we should probably bark loudly
> here instead of just returning -1. Perhaps assert() would
> suit better.

Well, according to assert(3), it will print to stderr before aborting
the program. So if writing STDERR_FILENO failed, I guess it won't
provide much more detail to the user, either. If bpf_trace_pipe()
returns non-zero, parse_bpf() prints an error message to stderr and
returns -1. Ultimately tc will return non-zero. With stderr unfit for
writing into, I doubt there's anything left we could do besides that.

But I really think we shouldn't make such a fuss about it - writing to
stderr either always works or we're in trouble everywhere. This patch
was merely to shut gcc up, so no need to waste much energy on a scenario
which won't happen anyway.

Thanks, Phil

Re: [iproute PATCH 51/51] lib/bpf: Check return value of write()

[Daniel Borkmann]

On 08/14/2017 07:25 PM, Phil Sutter wrote:
[...]
> But I really think we shouldn't make such a fuss about it - writing to
> stderr either always works or we're in trouble everywhere. This patch
> was merely to shut gcc up, so no need to waste much energy on a scenario
> which won't happen anyway.

Yup, fair enough, makes sense.

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

RE: [iproute PATCH 51/51] lib/bpf: Check return value of write()

[David Laight]

From: Phil Sutter
> Sent: 12 August 2017 13:05
> This is merely to silence the compiler warning. If write to stderr
> failed, assume that printing an error message will fail as well so don't
> even try.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  lib/bpf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 1dcb261dc915f..825e071cea572 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -591,7 +591,8 @@ int bpf_trace_pipe(void)
> 
>  		ret = read(fd, buff, sizeof(buff) - 1);
>  		if (ret > 0) {
> -			write(2, buff, ret);
> +			if (write(STDERR_FILENO, buff, ret) != ret)
> +				return -1;
>  			fflush(stderr);
>  		}

WTF is this code doing anyway?
write() is a system call, fflush() writes out any data buffered in the
stdio stream.
If there was anything buffered you'd want to output it earlier.
Otherwise if it is going to use fflush() it should be using fwrite().

I presume the function is allowed to write to stderr - since in general
library functions shouldn't assume fd 0/1/2 or stdin/out/err are valid.
There is a lot of code out there that does close(0); close(1); close(2);
but leaves stdout/err valid. Call printf() instead of sprint() and eventually
10k of data gets written somewhere rather unexpected.

If it is a copy loop, what is wrong with the last byte of buff[].
It is valid for write() to return a partial length - the code should
probably loop until all the data is accepted (or error).

	David

Re: [iproute PATCH 51/51] lib/bpf: Check return value of write()

[Daniel Borkmann]

On 08/15/2017 02:31 PM, David Laight wrote:
[...]
> WTF is this code doing anyway?
> write() is a system call, fflush() writes out any data buffered in the
> stdio stream.
> If there was anything buffered you'd want to output it earlier.
> Otherwise if it is going to use fflush() it should be using fwrite().
>
> I presume the function is allowed to write to stderr - since in general
> library functions shouldn't assume fd 0/1/2 or stdin/out/err are valid.
> There is a lot of code out there that does close(0); close(1); close(2);
> but leaves stdout/err valid. Call printf() instead of sprint() and eventually
> 10k of data gets written somewhere rather unexpected.
>
> If it is a copy loop, what is wrong with the last byte of buff[].
> It is valid for write() to return a partial length - the code should
> probably loop until all the data is accepted (or error).

Just send a patch if you really care; would have probably been faster
than typing up your email. ;) Thank you!

Re: [iproute PATCH 00/51] Fix potential issues detected by Coverity tool

[Stephen Hemminger]

On Sat, 12 Aug 2017 14:04:19 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Covscan really wasn't amused (indicated by the number of patches in this
> series). Try to make it happy.
> 
> Phil Sutter (51):
>   devlink: Check return code of strslashrsplit()
>   devlink: No need for this self-assignment
>   ipaddress: Make buffer for filter.flushb static
>   ipaddress: Avoid accessing uninitialized variable lcl
>   iplink_can: Prevent overstepping array bounds
>   iplink_vrf: Complain if main table is not found
>   ipmaddr: Avoid accessing uninitialized data
>   ipntable: No need to check and assign to parms_rta
>   ipntable: Make sure filter.name is NULL-terminated
>   iproute: Fix for missing 'Oifs:' display
>   iproute: Check mark value input
>   iproute_lwtunnel: csum_mode value checking was ineffective
>   iproute_lwtunnel: Argument to strerror must be positive
>   ipvrf: Don't try to close an invalid fd
>   ipvrf: Fix error path of vrf_switch()
>   xfrm_state: Make sure alg_name is NULL-terminated
>   lib/bpf: Don't leak fp in bpf_find_mntpt()
>   lib/fs: Fix format string in find_fs_mount()
>   lib/fs: Fix and simplify make_path()
>   lib/inet_proto: Make sure destination buffers are NULL-terminated
>   lib/libnetlink: Don't pass NULL parameter to memcpy()
>   lib/rt_names: Drop dead code in rtnl_rttable_n2a()
>   ifstat: Fix memleak in error case
>   ifstat, nstat: Check fdopen() return value
>   ifstat: Fix memleak in dump_kern_db() for json output
>   lnstat_util: Simplify alloc_and_open() a bit
>   nstat: Fix for potential NULL pointer dereference
>   nstat: Avoid passing negative fd to fdopen()
>   ss: Use C99 initializer in netlink_show_one()
>   ss: Skip useless check in parse_hostcond()
>   ss: Drop useless assignment
>   ss: Make sure index variable is >= 0
>   ss: Don't leak fd in tcp_show_netlink_file()
>   ss: Make sure scanned index value to unix_state_map is sane
>   ss: Fix potential memleak in unix_stats_print()
>   netem/maketable: Check return value of fstat()
>   netem/maketable: Check return value of fscanf()
>   tc/em_ipset: Don't leak sockfd on error path
>   tc/m_gact: Drop dead code
>   tc/m_xt: Fix for potential string buffer overflows
>   tc/q_multiq: Don't pass garbage in TCA_OPTIONS
>   tc/q_netem: Don't dereference possibly NULL pointer
>   tc/tc_filter: Make sure filter name is not empty
>   tipc/bearer: Fix resource leak in error path
>   tipc/bearer: Prevent NULL pointer dereference
>   tipc/node: Fix socket fd check in cmd_node_get_addr()
>   examples: Some shell fixes to cbq.init
>   ifcfg: Quote left-hand side of [ ] expression
>   lib/ll_map: Make sure im->name is NULL-terminated
>   Check user supplied interface name lengths
>   lib/bpf: Check return value of write()
> 
>  devlink/devlink.c        | 18 ++++++++++-----
>  examples/cbq.init-v0.7.3 | 24 ++++++++++----------
>  include/utils.h          |  1 +
>  ip/ifcfg                 |  2 +-
>  ip/ip6tunnel.c           |  6 +++--
>  ip/ipaddress.c           |  4 ++--
>  ip/ipl2tp.c              |  1 +
>  ip/iplink.c              | 27 +++++++----------------
>  ip/iplink_can.c          |  4 ++--
>  ip/iplink_vrf.c          |  5 ++++-
>  ip/ipmaddr.c             |  3 ++-
>  ip/ipntable.c            |  5 ++---
>  ip/iproute.c             | 14 +++++++-----
>  ip/iproute_lwtunnel.c    |  9 ++++----
>  ip/iprule.c              |  4 ++++
>  ip/iptunnel.c            | 12 ++++++----
>  ip/iptuntap.c            |  4 +++-
>  ip/ipvrf.c               | 16 ++++++++------
>  ip/xfrm_state.c          |  3 ++-
>  lib/bpf.c                |  8 +++++--
>  lib/fs.c                 | 22 +++++--------------
>  lib/inet_proto.c         |  9 +++++---
>  lib/libnetlink.c         |  6 +++--
>  lib/ll_map.c             |  4 ++--
>  lib/rt_names.c           |  4 ----
>  lib/utils.c              |  8 +++++++
>  misc/arpd.c              |  1 +
>  misc/ifstat.c            | 28 +++++++++++++++++-------
>  misc/lnstat_util.c       |  7 ++----
>  misc/nstat.c             | 33 +++++++++++++++++++---------
>  misc/ss.c                | 57 +++++++++++++++++++++++++++++-------------------
>  netem/maketable.c        |  8 +++----
>  tc/em_ipset.c            |  1 +
>  tc/m_gact.c              | 14 +++---------
>  tc/m_xt.c                |  7 +++---
>  tc/q_multiq.c            |  2 +-
>  tc/q_netem.c             |  4 +++-
>  tc/tc_filter.c           |  3 +++
>  tipc/bearer.c            |  7 ++++--
>  tipc/node.c              |  3 ++-
>  40 files changed, 230 insertions(+), 168 deletions(-)
> 

I am not amused by large patchsets either.
It takes more time to review, and one comment means the whole series
has to be redone.

Re: [iproute PATCH 00/51] Fix potential issues detected by Coverity tool

[Phil Sutter]

On Tue, Aug 15, 2017 at 08:07:25AM -0700, Stephen Hemminger wrote:
> On Sat, 12 Aug 2017 14:04:19 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > Covscan really wasn't amused (indicated by the number of patches in this
> > series). Try to make it happy.
> > 
> > Phil Sutter (51):
[...]
> >  40 files changed, 230 insertions(+), 168 deletions(-)
> > 
> 
> I am not amused by large patchsets either.
> It takes more time to review, and one comment means the whole series
> has to be redone.

OK, so should I send each patch individually (unless related to others)?

Thanks, Phil

Re: [iproute PATCH 00/51] Fix potential issues detected by Coverity tool

[Stephen Hemminger]

On Tue, 15 Aug 2017 18:04:32 +0200
Phil Sutter <phil@nwl.cc> wrote:

> On Tue, Aug 15, 2017 at 08:07:25AM -0700, Stephen Hemminger wrote:
> > On Sat, 12 Aug 2017 14:04:19 +0200
> > Phil Sutter <phil@nwl.cc> wrote:
> >   
> > > Covscan really wasn't amused (indicated by the number of patches in this
> > > series). Try to make it happy.
> > > 
> > > Phil Sutter (51):  
> [...]
> > >  40 files changed, 230 insertions(+), 168 deletions(-)
> > >   
> > 
> > I am not amused by large patchsets either.
> > It takes more time to review, and one comment means the whole series
> > has to be redone.  
> 
> OK, so should I send each patch individually (unless related to others)?
> 
> Thanks, Phil


Group like patches as needed. For example, all the ss patches.