Here are some fixes and cleanups caught by inspecting code and/or compiling by new compilers.
Cheers, Vita
v1 -> v2: drop ipfrag changes (will be done later)
Vitezslav Samel (10): packet.c: simplify and rename in_cksum() to verify_ipv4_hdr_chksum() verify_ipv4_hdr_chksum(): remove unneded check ifstats.c: simplify destroyiflist() bugfix: detstats(), ifstats(): handle packets with incorrect header checksum bugfix: positionptr(): properly allocate newly created interfaces packet_get(): optimization build: use wide version of -lpanel when needed ifaces.c: fix warning packet.c: fix "use of uninitialized variable" warning iptraf.c: fix "use of uninitialized variable" warning
Makefile | 22 ++++++++++++++-------- src/detstats.c | 18 ++++++++++-------- src/ifaces.c | 2 +- src/ifaces.h | 2 +- src/ifstats.c | 53 +++++++++++++++++++++++++++++------------------------ src/iptraf.c | 9 +++------ src/packet.c | 29 ++++++++++------------------- 7 files changed, 68 insertions(+), 67 deletions(-)
All in_cksum() does is IPv4 header checksum verification, so name it accordingly and in process simplify it a bit.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/packet.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/packet.c b/src/packet.c index bc8ed21..38d424f 100644 --- a/src/packet.c +++ b/src/packet.c @@ -38,12 +38,14 @@ packet.c - routines to open the raw socket, read socket data and pkt_cast_hdrp_l3off_t(hdr, pkt, 0)
/* code taken from http://www.faqs.org/rfcs/rfc1071.html. See section 4.1 "C" */ -static int in_cksum(u_short * addr, int len) +static int verify_ipv4_hdr_chksum(struct iphdr *ip) { register int sum = 0; + u_short *addr = (u_short *)ip; + int len = ip->ihl * 4;
while (len > 1) { - sum += *(u_short *) addr++; + sum += *addr++; len -= 2; }
@@ -53,7 +55,7 @@ static int in_cksum(u_short * addr, int len) while (sum >> 16) sum = (sum & 0xffff) + (sum >> 16);
- return (u_short) (~sum); + return ((u_short) ~sum) == 0; }
static int packet_adjust(struct pkt_hdr *pkt) @@ -198,19 +200,9 @@ again: switch (pkt->pkt_protocol) { case ETH_P_IP: { struct iphdr *ip = pkt->iphdr; - int hdr_check; - register int ip_checksum; in_port_t f_sport = 0, f_dport = 0;
- /* - * Compute and verify IP header checksum. - */ - - ip_checksum = ip->check; - ip->check = 0; - hdr_check = in_cksum((u_short *) pkt->iphdr, pkt_iph_len(pkt)); - - if ((hdr_check != ip_checksum)) + if (!verify_ipv4_hdr_chksum(ip)) return CHECKSUM_ERROR;
if ((ip->protocol == IPPROTO_TCP || ip->protocol == IPPROTO_UDP)
len is always even (due to multiplication by 4) so we can omit the check for odd len.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/packet.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/src/packet.c b/src/packet.c index 38d424f..3f1f1da 100644 --- a/src/packet.c +++ b/src/packet.c @@ -49,9 +49,6 @@ static int verify_ipv4_hdr_chksum(struct iphdr *ip) len -= 2; }
- if (len > 0) - sum += *(unsigned char *) addr; - while (sum >> 16) sum = (sum & 0xffff) + (sum >> 16);
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/ifstats.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/ifstats.c b/src/ifstats.c index 479c243..2b009e5 100644 --- a/src/ifstats.c +++ b/src/ifstats.c @@ -231,20 +231,14 @@ static struct iflist *positionptr(struct iflist *iflist, const int ifindex)
static void destroyiflist(struct iflist *list) { - struct iflist *ctmp; - struct iflist *ptmp; + struct iflist *ptmp = list; + + while (ptmp != NULL) { + struct iflist *ctmp = ptmp->next_entry;
- if (list != NULL) { - ptmp = list; - ctmp = ptmp->next_entry; - - do { - rate_destroy(&ptmp->rate); - free(ptmp); - ptmp = ctmp; - if (ctmp != NULL) - ctmp = ctmp->next_entry; - } while (ptmp != NULL); + rate_destroy(&ptmp->rate); + free(ptmp); + ptmp = ctmp; } }
detstats() and ifstats() could not account bad IPv4 packets (packets with invalid header checksum) because since version iptraf-3.0.0 we were dropping them. This caused the BadIP column to be always zero.
Fix this by allowing pkt_result value CHECKSUM_ERROR to continue down the code.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/detstats.c | 18 ++++++++++-------- src/ifstats.c | 17 ++++++++++------- 2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/src/detstats.c b/src/detstats.c index 5783fd6..646893f 100644 --- a/src/detstats.c +++ b/src/detstats.c @@ -253,8 +253,6 @@ void detstats(char *iface, time_t facilitytime) WINDOW *statwin; PANEL *statpanel;
- int pkt_result = 0; - FILE *logfile = NULL;
unsigned int iplen = 0; @@ -518,14 +516,18 @@ void detstats(char *iface, time_t facilitytime)
int outgoing;
- pkt_result = - packet_process(&pkt, NULL, NULL, NULL, - MATCH_OPPOSITE_USECONFIG, - options.v6inv4asv6); + int pkt_result = packet_process(&pkt, NULL, NULL, NULL, + MATCH_OPPOSITE_USECONFIG, + options.v6inv4asv6);
- if (pkt_result != PACKET_OK - && pkt_result != MORE_FRAGMENTS) + switch (pkt_result) { + case PACKET_OK: /* we only handle these */ + case MORE_FRAGMENTS: + case CHECKSUM_ERROR: + break; + default: /* drop others */ continue; + }
outgoing = (pkt.pkt_pkttype == PACKET_OUTGOING); update_proto_counter(&ifcounts.total, outgoing, pkt.pkt_len); diff --git a/src/ifstats.c b/src/ifstats.c index 2b009e5..2eb51b6 100644 --- a/src/ifstats.c +++ b/src/ifstats.c @@ -411,8 +411,6 @@ void ifstats(time_t facilitytime) int logging = options.logging; struct iftab table;
- int pkt_result = 0; - struct iflist *ptmp = NULL;
FILE *logfile = NULL; @@ -570,13 +568,18 @@ void ifstats(time_t facilitytime) if (pkt.pkt_len <= 0) continue;
- pkt_result = packet_process(&pkt, NULL, NULL, NULL, - MATCH_OPPOSITE_USECONFIG, - options.v6inv4asv6); + int pkt_result = packet_process(&pkt, NULL, NULL, NULL, + MATCH_OPPOSITE_USECONFIG, + options.v6inv4asv6);
- if (pkt_result != PACKET_OK - && pkt_result != MORE_FRAGMENTS) + switch (pkt_result) { + case PACKET_OK: /* we only handle these */ + case MORE_FRAGMENTS: + case CHECKSUM_ERROR: + break; + default: /* drop others */ continue; + }
ptmp = positionptr(table.head, pkt.pkt_ifindex); if (!ptmp)
When creating new entry in interface list (for interface created when ifstats() already running) we must allocate/init the rate too.
Fix this bug by creating new function alloc_iflist_entry() and use it where appropriate.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/ifstats.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/ifstats.c b/src/ifstats.c index 2eb51b6..2a5bba4 100644 --- a/src/ifstats.c +++ b/src/ifstats.c @@ -126,6 +126,15 @@ static int ifinlist(struct iflist *list, char *ifname) return result; }
+static struct iflist *alloc_iflist_entry(void) +{ + struct iflist *tmp = xmallocz(sizeof(struct iflist)); + + rate_alloc(&tmp->rate, 5); + + return tmp; +} + /* * Initialize the list of interfaces. This linked list is used in the * selection boxes as well as in the general interface statistics screen. @@ -171,10 +180,9 @@ static void initiflist(struct iflist **list) * At this point, the interface is now sure to be up and running. */
- struct iflist *itmp = xmallocz(sizeof(struct iflist)); - strcpy(itmp->ifname, ifname); + struct iflist *itmp = alloc_iflist_entry(); itmp->ifindex = ifindex; - rate_alloc(&itmp->rate, 5); + strcpy(itmp->ifname, ifname);
/* make the linked list sorted by ifindex */ struct iflist *cur = *list, *last = NULL; @@ -211,7 +219,7 @@ static struct iflist *positionptr(struct iflist *iflist, const int ifindex) } /* no interface was found, try to create new one */ if (ptmp == NULL) { - struct iflist *itmp = xmallocz(sizeof(struct iflist)); + struct iflist *itmp = alloc_iflist_entry(); itmp->ifindex = ifindex; itmp->index = last->index + 1; int r = dev_get_ifname(ifindex, itmp->ifname);
There's no need to initialize whole struct pkt_hdr; we only need to signalize we have no packet ready by setting pkt_len to 0. Other fields are either initialized in allocation time or set by packet_adjust() and packet_process() / packet_set_l3_hdrp() functions.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/packet.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/packet.c b/src/packet.c index 3f1f1da..224b17d 100644 --- a/src/packet.c +++ b/src/packet.c @@ -143,7 +143,9 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win) ss = poll(pfds, nfds, DEFAULT_UPDATE_DELAY); } while ((ss == -1) && (errno == EINTR));
- PACKET_INIT_STRUCT(pkt); + /* no packet ready yet */ + pkt->pkt_len = 0; + if ((ss > 0) && (pfds[0].revents & POLLIN) != 0) { struct sockaddr_ll from; struct iovec iov;
When linking with -lncurses[56]w we need to link with -lpanelw.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- Makefile | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/Makefile b/Makefile index 958b0fb..59b7c6f 100644 --- a/Makefile +++ b/Makefile @@ -218,7 +218,7 @@ ifdef NEEDS_NCURSESW5 NCURSES_CFLAGS := $(shell ncursesw5-config --cflags 2>/dev/null) NCURSES_LDFLAGS := $(shell ncursesw5-config --libs 2>/dev/null) ifndef NO_PANEL - NCURSES_LDFLAGS += -lpanel + NCURSES_LDFLAGS += -lpanelw endif endif endif @@ -238,7 +238,7 @@ ifdef NEEDS_NCURSESW6 NCURSES_CFLAGS := $(shell ncursesw6-config --cflags 2>/dev/null) NCURSES_LDFLAGS := $(shell ncursesw6-config --libs 2>/dev/null) ifndef NO_PANEL - NCURSES_LDFLAGS += -lpanel + NCURSES_LDFLAGS += -lpanelw endif endif endif @@ -248,21 +248,27 @@ ifndef NCURSES_LDFLAGS ifneq ($(shell ncursesw6-config --libs 2>/dev/null),) NCURSES_CFLAGS := $(shell ncursesw6-config --cflags 2>/dev/null) NCURSES_LDFLAGS := $(shell ncursesw6-config --libs 2>/dev/null) + ifndef NO_PANEL + NCURSES_LDFLAGS += -lpanelw + endif else ifneq ($(shell ncurses6-config --libs 2>/dev/null),) NCURSES_CFLAGS := $(shell ncurses6-config --cflags 2>/dev/null) NCURSES_LDFLAGS := $(shell ncurses6-config --libs 2>/dev/null) + ifndef NO_PANEL + NCURSES_LDFLAGS += -lpanel + endif else ifneq ($(shell ncursesw5-config --libs 2>/dev/null),) NCURSES_CFLAGS := $(shell ncursesw5-config --cflags 2>/dev/null) NCURSES_LDFLAGS := $(shell ncursesw5-config --libs 2>/dev/null) + ifndef NO_PANEL + NCURSES_LDFLAGS += -lpanelw + endif else ifneq ($(shell ncurses5-config --libs 2>/dev/null),) NCURSES_CFLAGS := $(shell ncurses5-config --cflags 2>/dev/null) NCURSES_LDFLAGS := $(shell ncurses5-config --libs 2>/dev/null) - endif - - ifneq ($(NCURSES_LDFLAGS),) - ifndef NO_PANEL - NCURSES_LDFLAGS += -lpanel - endif + ifndef NO_PANEL + NCURSES_LDFLAGS += -lpanel + endif endif endif
Fix warning: duplicate 'const' declaration specifier reported by clang-3.4.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/ifaces.c | 2 +- src/ifaces.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/ifaces.c b/src/ifaces.c index a3a7b08..d88e3ab 100644 --- a/src/ifaces.c +++ b/src/ifaces.c @@ -248,7 +248,7 @@ int dev_bind_ifindex(int fd, const int ifindex) return bind(fd, (struct sockaddr *) &fromaddr, addrlen); }
-int dev_bind_ifname(int fd, const char const *ifname) +int dev_bind_ifname(int fd, const char * const ifname) { int ir; struct ifreq ifr; diff --git a/src/ifaces.h b/src/ifaces.h index 97d01ba..da4a680 100644 --- a/src/ifaces.h +++ b/src/ifaces.h @@ -15,7 +15,7 @@ int dev_set_flags(const char *iface, int flags); int dev_clear_flags(const char *iface, int flags); int dev_get_ifname(int ifindex, char *ifname); int dev_bind_ifindex(const int fd, const int ifindex); -int dev_bind_ifname(const int fd, const char const *ifname); +int dev_bind_ifname(const int fd, const char * const ifname); char *gen_iface_msg(char *ifptr); int dev_promisc_flag(const char *dev_name);
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/packet.c b/src/packet.c index 224b17d..4556516 100644 --- a/src/packet.c +++ b/src/packet.c @@ -212,7 +212,7 @@ again: * Process TCP/UDP fragments */ if ((ntohs(ip->frag_off) & 0x3fff) != 0) { - int firstin; + int firstin = 0;
/* * total_br contains total byte count of all fragments
We don't need the pidfile_created variable, just jump to another label. Reported by clang-3.4.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/iptraf.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/iptraf.c b/src/iptraf.c index 2dc1759..0178147 100644 --- a/src/iptraf.c +++ b/src/iptraf.c @@ -427,9 +427,7 @@ int main(int argc, char **argv)
if (create_pidfile() < 0) - goto cleanup; - - int pidfile_created = 1; + goto bailout;
/* * If a facility is directly invoked from the command line, check for @@ -537,8 +535,7 @@ int main(int argc, char **argv) endwin();
cleanup: - if (pidfile_created) - unlink(IPTRAF_PIDFILE); - + unlink(IPTRAF_PIDFILE); +bailout: return 0; }
Vitezslav Samel vitezslav@samel.cz writes:
Here are some fixes and cleanups caught by inspecting code and/or compiling by new compilers.
Cheers, Vitav1 -> v2: drop ipfrag changes (will be done later)
Vitezslav Samel (10): packet.c: simplify and rename in_cksum() to verify_ipv4_hdr_chksum() verify_ipv4_hdr_chksum(): remove unneded check ifstats.c: simplify destroyiflist() bugfix: detstats(), ifstats(): handle packets with incorrect header checksum bugfix: positionptr(): properly allocate newly created interfaces packet_get(): optimization build: use wide version of -lpanel when needed ifaces.c: fix warning packet.c: fix "use of uninitialized variable" warning iptraf.c: fix "use of uninitialized variable" warning
Makefile | 22 ++++++++++++++-------- src/detstats.c | 18 ++++++++++-------- src/ifaces.c | 2 +- src/ifaces.h | 2 +- src/ifstats.c | 53 +++++++++++++++++++++++++++++------------------------ src/iptraf.c | 9 +++------ src/packet.c | 29 ++++++++++------------------- 7 files changed, 68 insertions(+), 67 deletions(-)
applied. thanks.
On Fri, Apr 18, 2014 at 12:19:56PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
Here are some fixes and cleanups caught by inspecting code and/or compiling by new compilers.
Cheers, Vitav1 -> v2: drop ipfrag changes (will be done later)
Vitezslav Samel (10): packet.c: simplify and rename in_cksum() to verify_ipv4_hdr_chksum() verify_ipv4_hdr_chksum(): remove unneded check ifstats.c: simplify destroyiflist() bugfix: detstats(), ifstats(): handle packets with incorrect header checksum bugfix: positionptr(): properly allocate newly created interfaces packet_get(): optimization build: use wide version of -lpanel when needed ifaces.c: fix warning packet.c: fix "use of uninitialized variable" warning iptraf.c: fix "use of uninitialized variable" warning
Makefile | 22 ++++++++++++++-------- src/detstats.c | 18 ++++++++++-------- src/ifaces.c | 2 +- src/ifaces.h | 2 +- src/ifstats.c | 53 +++++++++++++++++++++++++++++------------------------ src/iptraf.c | 9 +++------ src/packet.c | 29 ++++++++++------------------- 7 files changed, 68 insertions(+), 67 deletions(-)
applied. thanks.
Will you push it to public repo?
Thanks, Vita
Vitezslav Samel vitezslav@samel.cz writes:
On Fri, Apr 18, 2014 at 12:19:56PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
Here are some fixes and cleanups caught by inspecting code and/or compiling by new compilers.
Cheers, Vitav1 -> v2: drop ipfrag changes (will be done later)
Vitezslav Samel (10): packet.c: simplify and rename in_cksum() to verify_ipv4_hdr_chksum() verify_ipv4_hdr_chksum(): remove unneded check ifstats.c: simplify destroyiflist() bugfix: detstats(), ifstats(): handle packets with incorrect header checksum bugfix: positionptr(): properly allocate newly created interfaces packet_get(): optimization build: use wide version of -lpanel when needed ifaces.c: fix warning packet.c: fix "use of uninitialized variable" warning iptraf.c: fix "use of uninitialized variable" warning
Makefile | 22 ++++++++++++++-------- src/detstats.c | 18 ++++++++++-------- src/ifaces.c | 2 +- src/ifaces.h | 2 +- src/ifstats.c | 53 +++++++++++++++++++++++++++++------------------------ src/iptraf.c | 9 +++------ src/packet.c | 29 ++++++++++------------------- 7 files changed, 68 insertions(+), 67 deletions(-)
applied. thanks.
Will you push it to public repo?
can't push to my own repo :)
I have filled ticket to fedorahosted infrastructure. Let you know, when they resolve it.
Nikola Pajkovsky n.pajkovsky@gmail.com writes:
Vitezslav Samel vitezslav@samel.cz writes:
On Fri, Apr 18, 2014 at 12:19:56PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
Here are some fixes and cleanups caught by inspecting code and/or compiling by new compilers.
Cheers, Vitav1 -> v2: drop ipfrag changes (will be done later)
Vitezslav Samel (10): packet.c: simplify and rename in_cksum() to verify_ipv4_hdr_chksum() verify_ipv4_hdr_chksum(): remove unneded check ifstats.c: simplify destroyiflist() bugfix: detstats(), ifstats(): handle packets with incorrect header checksum bugfix: positionptr(): properly allocate newly created interfaces packet_get(): optimization build: use wide version of -lpanel when needed ifaces.c: fix warning packet.c: fix "use of uninitialized variable" warning iptraf.c: fix "use of uninitialized variable" warning
Makefile | 22 ++++++++++++++-------- src/detstats.c | 18 ++++++++++-------- src/ifaces.c | 2 +- src/ifaces.h | 2 +- src/ifstats.c | 53 +++++++++++++++++++++++++++++------------------------ src/iptraf.c | 9 +++------ src/packet.c | 29 ++++++++++------------------- 7 files changed, 68 insertions(+), 67 deletions(-)
applied. thanks.
Will you push it to public repo?
can't push to my own repo :)
I have filled ticket to fedorahosted infrastructure. Let you know, when they resolve it.
pull
iptraf-ng@lists.fedorahosted.org
-
Nikola Pajkovsky -
Vitezslav Samel