| Message ID | 20191024212001.7020-1-michal.lyszczek@bofc.pl |
|---|---|
| State | Changes Requested |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2] libnetlink.c, ss.c: properly handle fread() error | expand |
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?
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).
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; }
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(-)