[PATCH iproute2] libnetlink.c, ss.c: properly handle fread() error

[PATCH iproute2] libnetlink.c, ss.c: properly handle fread() error

[Michał Łyszczek]

fread(3) returns size_t data type which is unsigned, thus check
`if (fread(...) < 0)' is always false. To check if fread(3) has
failed, user should check if return is 0 and then check error
indicator with ferror(3).

Signed-off-by: Michał Łyszczek <michal.lyszczek@bofc.pl>
---
 lib/libnetlink.c | 6 +++---
 misc/ss.c        | 7 ++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 6ce8b199..76c383f9 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -1174,7 +1174,7 @@ int rtnl_listen(struct rtnl_handle *rtnl,
 int rtnl_from_file(FILE *rtnl, rtnl_listen_filter_t handler,
 		   void *jarg)
 {
-	int status;
+	size_t status;
 	char buf[16384];
 	struct nlmsghdr *h = (struct nlmsghdr *)buf;
 
@@ -1184,7 +1184,7 @@ int rtnl_from_file(FILE *rtnl, rtnl_listen_filter_t handler,
 
 		status = fread(&buf, 1, sizeof(*h), rtnl);
 
-		if (status < 0) {
+		if (status == 0 && ferror(rtnl)) {
 			if (errno == EINTR)
 				continue;
 			perror("rtnl_from_file: fread");
@@ -1204,7 +1204,7 @@ int rtnl_from_file(FILE *rtnl, rtnl_listen_filter_t handler,
 
 		status = fread(NLMSG_DATA(h), 1, NLMSG_ALIGN(l), rtnl);
 
-		if (status < 0) {
+		if (status == 0 && ferror(rtnl)) {
 			perror("rtnl_from_file: fread");
 			return -1;
 		}
diff --git a/misc/ss.c b/misc/ss.c
index 363b4c8d..769332e9 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -3329,12 +3329,13 @@ static int tcp_show_netlink_file(struct filter *f)
 	}
 
 	while (1) {
-		int status, err2;
+		int err2;
+		size_t status;
 		struct nlmsghdr *h = (struct nlmsghdr *)buf;
 		struct sockstat s = {};
 
 		status = fread(buf, 1, sizeof(*h), fp);
-		if (status < 0) {
+		if (status == 0 && ferror(fp)) {
 			perror("Reading header from $TCPDIAG_FILE");
 			break;
 		}
@@ -3345,7 +3346,7 @@ static int tcp_show_netlink_file(struct filter *f)
 
 		status = fread(h+1, 1, NLMSG_ALIGN(h->nlmsg_len-sizeof(*h)), fp);
 
-		if (status < 0) {
+		if (status == 0 && ferror(fp)) {
 			perror("Reading $TCPDIAG_FILE");
 			break;
 		}
-- 
2.21.0

Re: [PATCH iproute2] libnetlink.c, ss.c: properly handle fread() error

[Stephen Hemminger]

On Thu, 24 Oct 2019 23:20:01 +0200
Michał Łyszczek <michal.lyszczek@bofc.pl> wrote:

> fread(3) returns size_t data type which is unsigned, thus check
> `if (fread(...) < 0)' is always false. To check if fread(3) has
> failed, user should check if return is 0 and then check error
> indicator with ferror(3).
> 
> Signed-off-by: Michał Łyszczek <michal.lyszczek@bofc.pl>

You did find something that probably has been broken for a long time.

First off, not sure why libnetlink is using fread here anyway.
It adds another copy to all I/O which can matter with 1M routes.

Also the man page for fread() implies that truncated reads (not
just zero) can happen on error. Better to check that full read was
completed or at least a valid netlink header?

[PATCH v2 iproute2] libnetlink.c, ss.c: properly handle fread() errors

[Michał Łyszczek]

fread(3) returns size_t data type which is unsigned, thus check
`if (fread(...) < 0)' is always false. To check if fread(3) has
failed, user should check error indicator with ferror(3).

This commit also changes read logic a little bit by being less
forgiving for errors. Previous logic was checking if fread(3)
read *at least* required ammount of data, now code checks if
fread(3) read *exactly* expected ammount of data. This makes
sense because code parses very specific binary file, and reading
even 1 less/more byte than expected, will later corrupt data anyway.

Signed-off-by: Michał Łyszczek <michal.lyszczek@bofc.pl>

---
v1 -> v2: fread(3) can also return error on truncated reads and
            not only on 0bytes read (suggested by Stephen Hemminger)

---
 lib/libnetlink.c | 26 +++++++++++++-------------
 misc/ss.c        | 26 +++++++++++++-------------
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 6ce8b199..e02d6294 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -1174,7 +1174,7 @@ int rtnl_listen(struct rtnl_handle *rtnl,
 int rtnl_from_file(FILE *rtnl, rtnl_listen_filter_t handler,
 		   void *jarg)
 {
-	int status;
+	size_t status;
 	char buf[16384];
 	struct nlmsghdr *h = (struct nlmsghdr *)buf;
 
@@ -1184,14 +1184,15 @@ int rtnl_from_file(FILE *rtnl, rtnl_listen_filter_t handler,
 
 		status = fread(&buf, 1, sizeof(*h), rtnl);
 
-		if (status < 0) {
-			if (errno == EINTR)
-				continue;
-			perror("rtnl_from_file: fread");
+		if (status == 0 && feof(rtnl))
+			return 0;
+		if (status != sizeof(*h)) {
+			if (ferror(rtnl))
+				perror("rtnl_from_file: fread");
+			if (feof(rtnl))
+				fprintf(stderr, "rtnl-from_file: truncated message\n");
 			return -1;
 		}
-		if (status == 0)
-			return 0;
 
 		len = h->nlmsg_len;
 		l = len - sizeof(*h);
@@ -1204,12 +1205,11 @@ int rtnl_from_file(FILE *rtnl, rtnl_listen_filter_t handler,
 
 		status = fread(NLMSG_DATA(h), 1, NLMSG_ALIGN(l), rtnl);
 
-		if (status < 0) {
-			perror("rtnl_from_file: fread");
-			return -1;
-		}
-		if (status < l) {
-			fprintf(stderr, "rtnl-from_file: truncated message\n");
+		if (status != NLMSG_ALIGN(l)) {
+			if (ferror(rtnl))
+				perror("rtnl_from_file: fread");
+			if (feof(rtnl))
+				fprintf(stderr, "rtnl-from_file: truncated message\n");
 			return -1;
 		}
 
diff --git a/misc/ss.c b/misc/ss.c
index 363b4c8d..efa87781 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -3329,28 +3329,28 @@ static int tcp_show_netlink_file(struct filter *f)
 	}
 
 	while (1) {
-		int status, err2;
+		int err2;
+		size_t status, nitems;
 		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");
-			break;
-		}
 		if (status != sizeof(*h)) {
-			perror("Unexpected EOF reading $TCPDIAG_FILE");
+			if (ferror(fp))
+				perror("Reading header from $TCPDIAG_FILE");
+			if (feof(fp))
+				fprintf(stderr, "Unexpected EOF reading $TCPDIAG_FILE");
 			break;
 		}
 
-		status = fread(h+1, 1, NLMSG_ALIGN(h->nlmsg_len-sizeof(*h)), fp);
+		nitems = NLMSG_ALIGN(h->nlmsg_len - sizeof(*h));
+		status = fread(h+1, 1, nitems, fp);
 
-		if (status < 0) {
-			perror("Reading $TCPDIAG_FILE");
-			break;
-		}
-		if (status + sizeof(*h) < h->nlmsg_len) {
-			perror("Unexpected EOF reading $TCPDIAG_FILE");
+		if (status != nitems) {
+			if (ferror(fp))
+				perror("Reading $TCPDIAG_FILE");
+			if (feof(fp))
+				fprintf(stderr, "Unexpected EOF reading $TCPDIAG_FILE");
 			break;
 		}
 
-- 
2.23.0

Re: [PATCH iproute2] libnetlink.c, ss.c: properly handle fread() error

[michal.lyszczek]

[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]

Hello Stephen,
On 2019-10-28 21:21:28, Stephen Hemminger wrote:
> On Thu, 24 Oct 2019 23:20:01 +0200
> Michał Łyszczek <michal.lyszczek@bofc.pl> wrote:
>
> > fread(3) returns size_t data type which is unsigned, thus check
> > `if (fread(...) < 0)' is always false. To check if fread(3) has
> > failed, user should check if return is 0 and then check error
> > indicator with ferror(3).
>
> You did find something that probably has been broken for a long time.
>
> First off, not sure why libnetlink is using fread here anyway.
> It adds another copy to all I/O which can matter with 1M routes.

I don't this is a problem. Of course, this could be optimized with read(2)
but these functions are (or at least I think they are) called very rarely.
Optimal solution with read(2) will surely be much more complex than using
fread(3). I'm not sure if minor performance gain is worth bigger complexity.

> Also the man page for fread() implies that truncated reads (not
> just zero) can happen on error. Better to check that full read was
> completed or at least a valid netlink header?

Yes, you are right, I must have missed that. I've changed patch to
take this into account. I think, since this code parses precise binary
data, each call to fread(3) should return exact ammount of bytes as
what was requested as reading less then expected could lead to corrupt
read later anyway.

For example if `l = 3' and `NLMSG_ALIGN(l) == 4' doing

    status = fread(NLMSG_DATA(h), 1, NLMSG_ALIGN(l), rtnl);
    if (status < l)
        error;

Will result in error when fread(3) returns 3 bytes (and error), as
this will move stream pointer by 3 bytes instead of 4, and next
call to fread(3) will first read last DATA byte and then header
bytes, which will result in corrupted header and possible misleading
error later in execution - I belive errors should be reported as
soon as possible.


Please review newly attached patch (in another mail).

-- 
.-----------------.-------------------.---------------------.------------------.
| Michal Lyszczek | Embedded C, Linux |   Company Address   |  .-. open source |
| +48 727 564 419 | Software Engineer | Leszczynskiego 4/29 |  oo|  supporter  |
| https://bofc.pl `----.--------------: 50-078 Wroclaw, Pol | /`'\      &      |
| GPG FF1EBFE7E3A974B1 | Bits of Code | NIP:  813 349 58 78 |(\_;/) programer  |
`----------------------^--------------^---------------------^------------------'

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 iproute2] libnetlink.c, ss.c: properly handle fread() errors

[Stephen Hemminger]

On Tue, 29 Oct 2019 12:13:11 +0100
Michał Łyszczek <michal.lyszczek@bofc.pl> wrote:

> fread(3) returns size_t data type which is unsigned, thus check
> `if (fread(...) < 0)' is always false. To check if fread(3) has
> failed, user should check error indicator with ferror(3).
> 
> This commit also changes read logic a little bit by being less
> forgiving for errors. Previous logic was checking if fread(3)
> read *at least* required ammount of data, now code checks if
> fread(3) read *exactly* expected ammount of data. This makes
> sense because code parses very specific binary file, and reading
> even 1 less/more byte than expected, will later corrupt data anyway.
> 
> Signed-off-by: Michał Łyszczek <michal.lyszczek@bofc.pl>
> 
> ---
> v1 -> v2: fread(3) can also return error on truncated reads and
>             not only on 0bytes read (suggested by Stephen Hemminger)
> 

Thanks, applied.

Isn't error handling messy.