[iproute PATCH v3 0/6] Covscan: Misc fixes

[iproute PATCH v3 0/6] Covscan: Misc fixes

[Phil Sutter]

This series collects patches from v1 addressing miscellaneous issues
detected by covscan.

Changes since v2:
- Dropped patch 1 since v2 discussion is still inconclusive.
- Replaced patch 2 by a more appropriate one given feedback from v2.

Phil Sutter (6):
  ss: Make struct tcpstat fields 'timer' and 'timeout' unsigned
  ss: Make sure scanned index value to unix_state_map is sane
  netem/maketable: Check return value of fscanf()
  lib/bpf: Check return value of write()
  lib/fs: Fix and simplify make_path()
  lib/libnetlink: Don't pass NULL parameter to memcpy()

 lib/bpf.c         |  3 ++-
 lib/fs.c          | 20 +++++---------------
 lib/libnetlink.c  |  6 ++++--
 misc/ss.c         | 11 +++++------
 netem/maketable.c |  4 ++--
 5 files changed, 18 insertions(+), 26 deletions(-)

-- 
2.13.1

[iproute PATCH v3 1/6] ss: Make struct tcpstat fields 'timer' and 'timeout' unsigned

[Phil Sutter]

Both 'timer' and 'timeout' variables of struct tcpstat are either
scanned as unsigned values from /proc/net/tcp{,6} or copied from
'idiag_timer' and 'idiag_expries' fields of struct inet_diag_msg, which
itself are unsigned. Therefore they may be unsigned as well, which
eliminates the need to check for negative values.

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

diff --git a/misc/ss.c b/misc/ss.c
index 34c6da5443642..c41d5169aba52 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -697,8 +697,8 @@ struct dctcpstat {
 
 struct tcpstat {
 	struct sockstat	    ss;
-	int		    timer;
-	int		    timeout;
+	unsigned int	    timer;
+	unsigned int	    timeout;
 	int		    probes;
 	char		    cong_alg[16];
 	double		    rto, ato, rtt, rttvar;
@@ -869,13 +869,11 @@ static void sock_addr_print(const char *addr, char *delim, const char *port,
 	sock_addr_print_width(addr_width, addr, delim, serv_width, port, ifname);
 }
 
-static const char *print_ms_timer(int timeout)
+static const char *print_ms_timer(unsigned int timeout)
 {
 	static char buf[64];
 	int secs, msecs, minutes;
 
-	if (timeout < 0)
-		timeout = 0;
 	secs = timeout/1000;
 	minutes = secs/60;
 	secs = secs%60;
-- 
2.13.1

[iproute PATCH v3 2/6] 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 c41d5169aba52..951aa877bcb01 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -3148,7 +3148,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 v3 3/6] 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 v3 4/6] 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

[iproute PATCH v3 5/6] 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 c59ac564581d0..7083bf2e23bb7 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 v3 6/6] 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 874e660be7eb4..fbe719ee10449 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -871,7 +871,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;
 }
@@ -958,7 +959,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 v3 6/6] lib/libnetlink: Don't pass NULL parameter to memcpy()

[Stephen Hemminger]

On Thu, 24 Aug 2017 11:41:31 +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>
> ---
>  lib/libnetlink.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> index 874e660be7eb4..fbe719ee10449 100644
> --- a/lib/libnetlink.c
> +++ b/lib/libnetlink.c
> @@ -871,7 +871,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;
>  }
> @@ -958,7 +959,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;
>  }

Ok, applied. You never know when GCC language experts might decide
to exploit undefined behavior.

Re: [iproute PATCH v3 0/6] Covscan: Misc fixes

[Stephen Hemminger]

On Thu, 24 Aug 2017 11:41:25 +0200
Phil Sutter <phil@nwl.cc> wrote:

> This series collects patches from v1 addressing miscellaneous issues
> detected by covscan.
> 
> Changes since v2:
> - Dropped patch 1 since v2 discussion is still inconclusive.
> - Replaced patch 2 by a more appropriate one given feedback from v2.
> 
> Phil Sutter (6):
>   ss: Make struct tcpstat fields 'timer' and 'timeout' unsigned
>   ss: Make sure scanned index value to unix_state_map is sane
>   netem/maketable: Check return value of fscanf()
>   lib/bpf: Check return value of write()
>   lib/fs: Fix and simplify make_path()
>   lib/libnetlink: Don't pass NULL parameter to memcpy()
> 
>  lib/bpf.c         |  3 ++-
>  lib/fs.c          | 20 +++++---------------
>  lib/libnetlink.c  |  6 ++++--
>  misc/ss.c         | 11 +++++------
>  netem/maketable.c |  4 ++--
>  5 files changed, 18 insertions(+), 26 deletions(-)
> 

Applied this group