From 787f14dbe30ca22c9e9c7e9b1ff0ce6dfb00db7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=9A=D0=BE=D1=80=D0=B5=D0=BD=D0=B1=D0=B5=D1=80=D0=B3=20?= =?UTF-8?q?=D0=9C=D0=B0=D1=80=D0=BA=20=28=D0=BD=D0=BE=D1=83=D1=82=D0=B1?= =?UTF-8?q?=D1=83=D0=BA=20=D0=B4=D0=BE=D0=BC=D0=B0=29?= Date: Sun, 9 Sep 2012 22:30:20 +0600 Subject: [PATCH 1/9] genl/family flags can be damaged during the auto-indentation. "-" was never used in the names of the flags. "_" was used in all places of the library. So, I just changed the undescore to the minus. Automatic indentation can insert spaces on either side of the minus, so the library will be compiled, but will not be usable (in this part of the code), as the parser will split words by white space, and the flag "admin - perm" will never work. --- lib/genl/family.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/genl/family.c b/lib/genl/family.c index 64b98cd..05e45ac 100644 --- a/lib/genl/family.c +++ b/lib/genl/family.c @@ -96,10 +96,10 @@ static void family_dump_line(struct nl_object *obj, struct nl_dump_params *p) } static const struct trans_tbl ops_flags[] = { - __ADD(GENL_ADMIN_PERM, admin-perm) - __ADD(GENL_CMD_CAP_DO, has-doit) - __ADD(GENL_CMD_CAP_DUMP, has-dump) - __ADD(GENL_CMD_CAP_HASPOL, has-policy) + __ADD(GENL_ADMIN_PERM, admin_perm) + __ADD(GENL_CMD_CAP_DO, has_doit) + __ADD(GENL_CMD_CAP_DUMP, has_dump) + __ADD(GENL_CMD_CAP_HASPOL, has_policy) }; static char *ops_flags2str(int flags, char *buf, size_t len) From fedb862ea5a46df327a4de75e613e2d7732e8b46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=9A=D0=BE=D1=80=D0=B5=D0=BD=D0=B1=D0=B5=D1=80=D0=B3=20?= =?UTF-8?q?=D0=9C=D0=B0=D1=80=D0=BA=20=28=D0=BD=D0=BE=D1=83=D1=82=D0=B1?= =?UTF-8?q?=D1=83=D0=BA=20=D0=B4=D0=BE=D0=BC=D0=B0=29?= Date: Mon, 10 Sep 2012 01:21:52 +0600 Subject: [PATCH 2/9] ROUTE_DIFF result was not used in some place in route_compare --- lib/route/route_obj.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/route/route_obj.c b/lib/route/route_obj.c index 7ea4fff..54df023 100644 --- a/lib/route/route_obj.c +++ b/lib/route/route_obj.c @@ -332,7 +332,7 @@ static int route_compare(struct nl_object *_a, struct nl_object *_b, if (a->rt_metrics_mask & (1 << i) && (!(b->rt_metrics_mask & (1 << i)) || a->rt_metrics[i] != b->rt_metrics[i])) - ROUTE_DIFF(METRICS, 1); + diff |= ROUTE_DIFF(METRICS, 1); } diff |= ROUTE_DIFF(FLAGS, From e1b67fb23f8f802f3b287c66fda531e002ca20d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=9A=D0=BE=D1=80=D0=B5=D0=BD=D0=B1=D0=B5=D1=80=D0=B3=20?= =?UTF-8?q?=D0=9C=D0=B0=D1=80=D0=BA=20=28=D0=BD=D0=BE=D1=83=D1=82=D0=B1?= =?UTF-8?q?=D1=83=D0=BA=20=D0=B4=D0=BE=D0=BC=D0=B0=29?= Date: Mon, 10 Sep 2012 01:33:04 +0600 Subject: [PATCH 3/9] Clang diagnostics Based on clang diagnostics: 1. lib/nl.c: recvmsgs(): nla filling with zeros commented. 2. lib/route/classid.c: & lib/route/pktloc.c: remove zero-filling of struct stat 3. lib/route/qdisc/htb.c: Fix htb_qdisc_msg_fill(): fix zero-filling 4. ematch/container.c: container_parse: commented why only 4 bytes are copied len marked as unused to eliminate compiler warning --- lib/nl.c | 6 ++++++ lib/route/classid.c | 2 +- lib/route/cls/ematch/container.c | 8 +++++++- lib/route/pktloc.c | 2 +- lib/route/qdisc/htb.c | 8 ++++---- 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/nl.c b/lib/nl.c index 5b37c2a..2fa741f 100644 --- a/lib/nl.c +++ b/lib/nl.c @@ -540,6 +540,12 @@ static int recvmsgs(struct nl_sock *sk, struct nl_cb *cb) int n, err = 0, multipart = 0, interrupted = 0, nrecv = 0; unsigned char *buf = NULL; struct nlmsghdr *hdr; + + /* + nla is passed on to not only to nl_recv() but may also be passed + to a function pointer provided by the caller which may or may not + initialize the variable. Thomas Graf. + */ struct sockaddr_nl nla = {0}; struct nl_msg *msg = NULL; struct ucred *creds = NULL; diff --git a/lib/route/classid.c b/lib/route/classid.c index a128773..87025a0 100644 --- a/lib/route/classid.c +++ b/lib/route/classid.c @@ -311,7 +311,7 @@ static int classid_map_add(uint32_t classid, const char *name) int rtnl_tc_read_classid_file(void) { static time_t last_read; - struct stat st = {0}; + struct stat st; char buf[256], *path; FILE *fd; int err; diff --git a/lib/route/cls/ematch/container.c b/lib/route/cls/ematch/container.c index ddbdce0..6d73ab6 100644 --- a/lib/route/cls/ematch/container.c +++ b/lib/route/cls/ematch/container.c @@ -14,8 +14,14 @@ #include #include -static int container_parse(struct rtnl_ematch *e, void *data, size_t len) +static int container_parse(struct rtnl_ematch *e, void *data, size_t len __attribute__((unused))) { + /* + The kernel may provide more than 4 bytes of data in the future and we want + older libnl versions to be ok with that. We want interfaces to be growable + so we only ever enforce a minimum data length and copy as much as we are + aware of. Thomas Graf. + */ memcpy(e->e_data, data, sizeof(uint32_t)); return 0; diff --git a/lib/route/pktloc.c b/lib/route/pktloc.c index e7dffe5..70d552b 100644 --- a/lib/route/pktloc.c +++ b/lib/route/pktloc.c @@ -90,7 +90,7 @@ static int read_pktlocs(void) YY_BUFFER_STATE buf = NULL; yyscan_t scanner = NULL; static time_t last_read; - struct stat st = {0}; + struct stat st; char *path; int i, err; FILE *fd; diff --git a/lib/route/qdisc/htb.c b/lib/route/qdisc/htb.c index 4298580..9fb0bf6 100644 --- a/lib/route/qdisc/htb.c +++ b/lib/route/qdisc/htb.c @@ -190,10 +190,10 @@ static int htb_qdisc_msg_fill(struct rtnl_tc *tc, void *data, struct nl_msg *msg) { struct rtnl_htb_qdisc *htb = data; - struct tc_htb_glob opts = {0}; - - opts.version = TC_HTB_PROTOVER; - opts.rate2quantum = 10; + struct tc_htb_glob opts = { + .version = TC_HTB_PROTOVER, + .rate2quantum = 10, + }; if (htb) { if (htb->qh_mask & SCH_HTB_HAS_RATE2QUANTUM) From e7ec197da35d5f8efc44af0afbe21b8612478a57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=9A=D0=BE=D1=80=D0=B5=D0=BD=D0=B1=D0=B5=D1=80=D0=B3=20?= =?UTF-8?q?=D0=9C=D0=B0=D1=80=D0=BA=20=28=D0=B4=D0=BE=D0=BC=D0=B0=29?= Date: Fri, 21 Sep 2012 23:10:26 +0600 Subject: [PATCH 4/9] nf-log example: correct copy-range parsing --- src/nf-log.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nf-log.c b/src/nf-log.c index 26bae6d..913ba16 100644 --- a/src/nf-log.c +++ b/src/nf-log.c @@ -96,7 +96,7 @@ int main(int argc, char *argv[]) copy_range = 0xFFFF; if (argc > 4) - copy_mode = atoi(argv[4]); + copy_range = atoi(argv[4]); nfnl_log_set_copy_range(log, copy_range); if ((err = nfnl_log_create(nf_sock, log)) < 0) From 9d6b104ec8b4dc8ce182158bf6c81db23e937c5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=9A=D0=BE=D1=80=D0=B5=D0=BD=D0=B1=D0=B5=D1=80=D0=B3=20?= =?UTF-8?q?=D0=9C=D0=B0=D1=80=D0=BA=20=28=D0=BD=D0=BE=D1=83=D1=82=D0=B1?= =?UTF-8?q?=D1=83=D0=BA=20=D0=B4=D0=BE=D0=BC=D0=B0=29?= Date: Mon, 10 Sep 2012 02:25:07 +0600 Subject: [PATCH 5/9] nl_recv(): "else if" logick simplified and refined --- lib/nl.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/lib/nl.c b/lib/nl.c index 2fa741f..8426c58 100644 --- a/lib/nl.c +++ b/lib/nl.c @@ -459,25 +459,31 @@ retry: n = recvmsg(sk->s_fd, &msg, flags); if (!n) goto abort; - else if (n < 0) { + + if (n < 0) { + if (errno == EINTR) { NL_DBG(3, "recvmsg() returned EINTR, retrying\n"); goto retry; - } else if (errno == EAGAIN) { + } + + if (errno == EAGAIN) { NL_DBG(3, "recvmsg() returned EAGAIN, aborting\n"); goto abort; - } else { - free(msg.msg_control); - free(*buf); - return -nl_syserr2nlerr(errno); } + + free(msg.msg_control); + free(*buf); + return -nl_syserr2nlerr(errno); } if (msg.msg_flags & MSG_CTRUNC) { msg.msg_controllen *= 2; msg.msg_control = realloc(msg.msg_control, msg.msg_controllen); goto retry; - } else if (iov.iov_len < n || msg.msg_flags & MSG_TRUNC) { + } + + if (iov.iov_len < n || msg.msg_flags & MSG_TRUNC) { /* Provided buffer is not long enough, enlarge it * to size of n (which should be total length of the message) * and try again. */ @@ -485,7 +491,9 @@ retry: iov.iov_base = *buf = realloc(*buf, iov.iov_len); flags = 0; goto retry; - } else if (flags != 0) { + } + + if (flags != 0) { /* Buffer is big enough, do the actual reading */ flags = 0; goto retry; From 69468517d0de1675d80f24661ff57a5dbac7275c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=9A=D0=BE=D1=80=D0=B5=D0=BD=D0=B1=D0=B5=D1=80=D0=B3=20?= =?UTF-8?q?=D0=9C=D0=B0=D1=80=D0=BA=20=28=D0=B4=D0=BE=D0=BC=D0=B0=29?= Date: Fri, 19 Oct 2012 22:58:58 +0600 Subject: [PATCH 6/9] nl_recv(): Memory allocation errors are handled properly now 1. all cleanup actions (like free()) now located at the end of function 2. in case of error or EOF, *buf and *creds (if given) set to NULL This protect from invalid code at user's side, like: char *buf; x = nl_recv(..., &buf, ...); if (x<=0) goto cleanup; cleanup: free(buf); 3. all intermediate buffers are stored into local variables, and user's variables only touches at the end. --- lib/nl.c | 75 +++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 22 deletions(-) diff --git a/lib/nl.c b/lib/nl.c index 8426c58..d08f7e1 100644 --- a/lib/nl.c +++ b/lib/nl.c @@ -437,9 +437,9 @@ int nl_recv(struct nl_sock *sk, struct sockaddr_nl *nla, .msg_controllen = 0, .msg_flags = 0, }; - struct cmsghdr *cmsg; - memset(nla, 0, sizeof(*nla)); + struct ucred* tmpcreds = NULL; + int retval = 0; if (sk->s_flags & NL_MSG_PEEK) flags |= MSG_PEEK | MSG_TRUNC; @@ -448,20 +448,29 @@ int nl_recv(struct nl_sock *sk, struct sockaddr_nl *nla, page_size = getpagesize(); iov.iov_len = sk->s_bufsize ? : page_size; - iov.iov_base = *buf = malloc(iov.iov_len); + iov.iov_base = malloc(iov.iov_len); + + if (!iov.iov_base) { + retval = -NLE_NOMEM; + goto abort; + } if (sk->s_flags & NL_SOCK_PASSCRED) { msg.msg_controllen = CMSG_SPACE(sizeof(struct ucred)); msg.msg_control = calloc(1, msg.msg_controllen); + if (!msg.msg_control) { + retval = -NLE_NOMEM; + goto abort; + } } retry: n = recvmsg(sk->s_fd, &msg, flags); - if (!n) + if (!n) { + retval = 0; goto abort; - + } if (n < 0) { - if (errno == EINTR) { NL_DBG(3, "recvmsg() returned EINTR, retrying\n"); goto retry; @@ -469,26 +478,37 @@ retry: if (errno == EAGAIN) { NL_DBG(3, "recvmsg() returned EAGAIN, aborting\n"); + retval = 0; goto abort; } - - free(msg.msg_control); - free(*buf); - return -nl_syserr2nlerr(errno); + retval = -nl_syserr2nlerr(errno); + goto abort; } if (msg.msg_flags & MSG_CTRUNC) { + void *tmp; msg.msg_controllen *= 2; - msg.msg_control = realloc(msg.msg_control, msg.msg_controllen); + tmp = realloc(msg.msg_control, msg.msg_controllen); + if (!tmp) { + retval = -NLE_NOMEM; + goto abort; + } + msg.msg_control = tmp; goto retry; } if (iov.iov_len < n || msg.msg_flags & MSG_TRUNC) { + void *tmp; /* Provided buffer is not long enough, enlarge it * to size of n (which should be total length of the message) * and try again. */ iov.iov_len = n; - iov.iov_base = *buf = realloc(*buf, iov.iov_len); + tmp = realloc(iov.iov_base, iov.iov_len); + if (!tmp) { + retval = -NLE_NOMEM; + goto abort; + } + iov.iov_base = tmp; flags = 0; goto retry; } @@ -500,29 +520,40 @@ retry: } if (msg.msg_namelen != sizeof(struct sockaddr_nl)) { - free(msg.msg_control); - free(*buf); - return -NLE_NOADDR; + retval = -NLE_NOADDR; + goto abort; } for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) { if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_CREDENTIALS) { if (creds) { - *creds = calloc(1, sizeof(struct ucred)); - memcpy(*creds, CMSG_DATA(cmsg), sizeof(struct ucred)); + tmpcreds = malloc(sizeof(*tmpcreds)); + if (!tmpcreds) { + retval = -NLE_NOMEM; + goto abort; + } + memcpy(tmpcreds, CMSG_DATA(cmsg), sizeof(*tmpcreds)); } break; } } - free(msg.msg_control); - return n; - + retval = n; abort: free(msg.msg_control); - free(*buf); - return 0; + + if (retval <= 0) { + free(iov.iov_base); iov.iov_base = NULL; + free(tmpcreds); tmpcreds = NULL; + } + + *buf = iov.iov_base; + + if (creds) + *creds = tmpcreds; + + return retval; } /** @cond SKIP */ From 2249eaebd4bde07e33f265e7eaac5ad85b5f1253 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=9A=D0=BE=D1=80=D0=B5=D0=BD=D0=B1=D0=B5=D1=80=D0=B3=20?= =?UTF-8?q?=D0=9C=D0=B0=D1=80=D0=BA=20=28=D0=B4=D0=BE=D0=BC=D0=B0=29?= Date: Fri, 19 Oct 2012 23:04:23 +0600 Subject: [PATCH 7/9] nl_recv(): EWOULDBLOCK return value also checked --- lib/nl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/nl.c b/lib/nl.c index d08f7e1..7f47223 100644 --- a/lib/nl.c +++ b/lib/nl.c @@ -475,9 +475,8 @@ retry: NL_DBG(3, "recvmsg() returned EINTR, retrying\n"); goto retry; } - - if (errno == EAGAIN) { - NL_DBG(3, "recvmsg() returned EAGAIN, aborting\n"); + if (errno == EAGAIN || errno == EWOULDBLOCK) { + NL_DBG(3, "recvmsg() returned EAGAIN||EWOULDBLOCK, aborting\n"); retval = 0; goto abort; } From da694e6c7b11d475d84ca6addeae34a6dac8c949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=9A=D0=BE=D1=80=D0=B5=D0=BD=D0=B1=D0=B5=D1=80=D0=B3=20?= =?UTF-8?q?=D0=9C=D0=B0=D1=80=D0=BA=20=28=D0=B4=D0=BE=D0=BC=D0=B0=29?= Date: Fri, 19 Oct 2012 23:06:23 +0600 Subject: [PATCH 8/9] nl_recv(): small code cleanups 1. memset around nla is unnecessary 2. calloc() is unnecessary. malloc() used instead. --- lib/nl.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/nl.c b/lib/nl.c index 7f47223..284d23c 100644 --- a/lib/nl.c +++ b/lib/nl.c @@ -405,8 +405,8 @@ errout: /** * Receive data from netlink socket * @arg sk Netlink socket. - * @arg nla Destination pointer for peer's netlink address. - * @arg buf Destination pointer for message content. + * @arg nla Destination pointer for peer's netlink address. (required) + * @arg buf Destination pointer for message content. (required) * @arg creds Destination pointer for credentials. * * Receives a netlink message, allocates a buffer in \c *buf and @@ -433,11 +433,7 @@ int nl_recv(struct nl_sock *sk, struct sockaddr_nl *nla, .msg_namelen = sizeof(struct sockaddr_nl), .msg_iov = &iov, .msg_iovlen = 1, - .msg_control = NULL, - .msg_controllen = 0, - .msg_flags = 0, }; - memset(nla, 0, sizeof(*nla)); struct ucred* tmpcreds = NULL; int retval = 0; @@ -457,7 +453,7 @@ int nl_recv(struct nl_sock *sk, struct sockaddr_nl *nla, if (sk->s_flags & NL_SOCK_PASSCRED) { msg.msg_controllen = CMSG_SPACE(sizeof(struct ucred)); - msg.msg_control = calloc(1, msg.msg_controllen); + msg.msg_control = malloc(msg.msg_controllen); if (!msg.msg_control) { retval = -NLE_NOMEM; goto abort; @@ -496,7 +492,7 @@ retry: goto retry; } - if (iov.iov_len < n || msg.msg_flags & MSG_TRUNC) { + if (iov.iov_len < n || (msg.msg_flags & MSG_TRUNC)) { void *tmp; /* Provided buffer is not long enough, enlarge it * to size of n (which should be total length of the message) From 420e4623fd1ebf2a14e2977d82fd477f1e913460 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=9A=D0=BE=D1=80=D0=B5=D0=BD=D0=B1=D0=B5=D1=80=D0=B3=20?= =?UTF-8?q?=D0=9C=D0=B0=D1=80=D0=BA=20=28=D0=B4=D0=BE=D0=BC=D0=B0=29?= Date: Fri, 19 Oct 2012 23:23:10 +0600 Subject: [PATCH 9/9] nl_recv(): work with credentials only if "creds" given and NL_SOCK_PASSCRED set --- lib/nl.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/nl.c b/lib/nl.c index 284d23c..9858307 100644 --- a/lib/nl.c +++ b/lib/nl.c @@ -407,7 +407,7 @@ errout: * @arg sk Netlink socket. * @arg nla Destination pointer for peer's netlink address. (required) * @arg buf Destination pointer for message content. (required) - * @arg creds Destination pointer for credentials. + * @arg creds Destination pointer for credentials. (optional) * * Receives a netlink message, allocates a buffer in \c *buf and * stores the message content. The peer's netlink address is stored @@ -451,7 +451,7 @@ int nl_recv(struct nl_sock *sk, struct sockaddr_nl *nla, goto abort; } - if (sk->s_flags & NL_SOCK_PASSCRED) { + if (creds && (sk->s_flags & NL_SOCK_PASSCRED)) { msg.msg_controllen = CMSG_SPACE(sizeof(struct ucred)); msg.msg_control = malloc(msg.msg_controllen); if (!msg.msg_control) { @@ -519,17 +519,20 @@ retry: goto abort; } - for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) { - if (cmsg->cmsg_level == SOL_SOCKET && - cmsg->cmsg_type == SCM_CREDENTIALS) { - if (creds) { - tmpcreds = malloc(sizeof(*tmpcreds)); - if (!tmpcreds) { - retval = -NLE_NOMEM; - goto abort; - } - memcpy(tmpcreds, CMSG_DATA(cmsg), sizeof(*tmpcreds)); + if (creds && (sk->s_flags & NL_SOCK_PASSCRED)) { + struct cmsghdr *cmsg; + + for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) { + if (cmsg->cmsg_level != SOL_SOCKET) + continue; + if (cmsg->cmsg_type != SCM_CREDENTIALS) + continue; + tmpcreds = malloc(sizeof(*tmpcreds)); + if (!tmpcreds) { + retval = -NLE_NOMEM; + goto abort; } + memcpy(tmpcreds, CMSG_DATA(cmsg), sizeof(*tmpcreds)); break; } }