[iproute PATCH v2 0/7] Covscan: Misc fixes

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

No changes to the actual patches, just splitting into smaller series.

Phil Sutter (7):
  nstat: Avoid passing negative fd to fdopen()
  ss: Make sure index variable is >= 0
  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/nstat.c      | 15 +++++++++++----
 misc/ss.c         |  5 +++--
 netem/maketable.c |  4 ++--
 6 files changed, 27 insertions(+), 26 deletions(-)

-- 
2.13.1

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..517f1dc461e8a 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

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 4f52ad4a8f023..be42348b3cc37 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c

@@ -588,7 +588,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

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 1212b1f2c8128..7cdde75a56e4e 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

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

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 1ee02d73b2d7f..6c091a694231e 100644
--- a/misc/ss.c
+++ b/misc/ss.c

@@ -3151,7 +3151,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

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 10360e5a04ff8..1ee02d73b2d7f 100644
--- a/misc/ss.c
+++ b/misc/ss.c

@@ -2004,7 +2004,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

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

On Mon, 21 Aug 2017 19:08:07 +0200
Phil Sutter [off-list ref] wrote:

Why not just fix it at the source of the open.
I.e 
static FILE *generic_proc_open(condt char * env, const char *name)
{
...
	return fopen(p, "r");
}

On Mon, 21 Aug 2017 19:08:08 +0200
Phil Sutter [off-list ref] wrote:

Let's go one step deeper on this.
Why is s->timer an int, it should be unsigned. In which case the code in
print would not have to change.

On Mon, Aug 21, 2017 at 05:23:23PM -0700, Stephen Hemminger wrote:

On Mon, 21 Aug 2017 19:08:07 +0200
Phil Sutter [off-list ref] wrote:

quoted

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 1212b1f2c8128..7cdde75a56e4e 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);

[...]

Why not just fix it at the source of the open.
I.e 
static FILE *generic_proc_open(condt char * env, const char *name)
{
...
	return fopen(p, "r");
}

What should it do? All the load_*() functions are supposed to be
fault-tolerant, so calling abort() or something is not an option.

Thanks, Phil