From 5541066660995ae4f98018b0290f4351417c52e4 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Thu, 27 Apr 2023 18:25:56 +0200 Subject: [PATCH] pcap/Source: Allow more than 32bit for link and dropped stats The PktSrc::Stats object works with 64bit unsigned integers. Unfortunately, libpcap's struct pcap_stat is using 32bit values and users have reported the wrapping of these values being visible in their stats.log roughly every 7.5 hours (~160kpps). This change moves tracking of link and drop counters into the PktSrc::Stats object (like is done for received and bytes_received) and updates them on a call to PcapSource::Statistics() with the difference to the previous stats values to prevent the wrap from becoming visible to script land. This doesn't cover the case of the stats counters wrapping around multiple times between two invocations of PktSrc::Statistics(). With the default interval of 5 minutes for the stats script, this seems acceptable. Closes #2791. --- NEWS | 3 ++ src/iosource/pcap/Source.cc | 88 ++++++++++++++++++++++++++++++++++++- src/iosource/pcap/Source.h | 1 + 3 files changed, 90 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 77b53a6dea..df6b35f91f 100644 --- a/NEWS +++ b/NEWS @@ -297,6 +297,9 @@ Changed Functionality masked to exclude PCP and DEI bits. Previously, these bits were included and could cause invalid vlan values > 4095 to be reported. +- Libpcap based packet source now avoids the 32bit wraparound of link and + dropped packet counters as reported by users. + Removed Functionality --------------------- diff --git a/src/iosource/pcap/Source.cc b/src/iosource/pcap/Source.cc index fd01c19554..c8869a0c7c 100644 --- a/src/iosource/pcap/Source.cc +++ b/src/iosource/pcap/Source.cc @@ -4,6 +4,8 @@ #include "zeek/zeek-config.h" +#include "zeek/3rdparty/doctest.h" + #ifdef HAVE_PCAP_INT_H #include #endif @@ -326,6 +328,23 @@ bool PcapSource::SetFilter(int index) return true; } +// Given two pcap_stat structures, compute the difference of linked and dropped +// and add it to the given Stats object. +static void update_pktsrc_stats(PktSrc::Stats* stats, const struct pcap_stat* now, + const struct pcap_stat* prev) + { + decltype(now->ps_drop) ps_drop_diff = 0; + decltype(now->ps_recv) ps_recv_diff = 0; + + // This is subtraction of unsigned ints: It's not undefined + // and results in modulo arithmetic. + ps_recv_diff = now->ps_recv - prev->ps_recv; + ps_drop_diff = now->ps_drop - prev->ps_drop; + + stats->link += ps_recv_diff; + stats->dropped += ps_drop_diff; + } + void PcapSource::Statistics(Stats* s) { char errbuf[PCAP_ERRBUF_SIZE]; @@ -344,11 +363,13 @@ void PcapSource::Statistics(Stats* s) else { - s->dropped = pstat.ps_drop; - s->link = pstat.ps_recv; + update_pktsrc_stats(&stats, &pstat, &prev_pstat); + prev_pstat = pstat; } } + s->link = stats.link; + s->dropped = stats.dropped; s->received = stats.received; s->bytes_received = stats.bytes_received; @@ -376,4 +397,67 @@ iosource::PktSrc* PcapSource::Instantiate(const std::string& path, bool is_live) return new PcapSource(path, is_live); } +TEST_CASE("pcap source update_pktsrc_stats") + { + PktSrc::Stats stats; + struct pcap_stat now = {0}; + struct pcap_stat prev = {0}; + + SUBCASE("all zero") + { + update_pktsrc_stats(&stats, &now, &prev); + CHECK(stats.link == 0); + CHECK(stats.dropped == 0); + } + + SUBCASE("no overflow") + { + now.ps_recv = 7; + now.ps_drop = 3; + update_pktsrc_stats(&stats, &now, &prev); + CHECK(stats.link == 7); + CHECK(stats.dropped == 3); + } + + SUBCASE("no overflow prev") + { + stats.link = 2; + stats.dropped = 1; + prev.ps_recv = 2; + prev.ps_drop = 1; + now.ps_recv = 7; + now.ps_drop = 3; + + update_pktsrc_stats(&stats, &now, &prev); + CHECK(stats.link == 7); + CHECK(stats.dropped == 3); + } + + SUBCASE("overflow") + { + prev.ps_recv = 4294967295; + prev.ps_drop = 4294967294; + now.ps_recv = 0; + now.ps_drop = 1; + + update_pktsrc_stats(&stats, &now, &prev); + CHECK(stats.link == 1); + CHECK(stats.dropped == 3); + } + + SUBCASE("overflow 2") + { + stats.link = 4294967295; + stats.dropped = 4294967294; + prev.ps_recv = 4294967295; + prev.ps_drop = 4294967294; + now.ps_recv = 10; + now.ps_drop = 3; + + update_pktsrc_stats(&stats, &now, &prev); + CHECK(stats.link == 4294967306); // 2**32 - 1 + 11 + CHECK(stats.dropped == 4294967299); // 2**32 - 2 + 5 + } + } + } // namespace zeek::iosource::pcap diff --git a/src/iosource/pcap/Source.h b/src/iosource/pcap/Source.h index b02828ae1c..cbff27963a 100644 --- a/src/iosource/pcap/Source.h +++ b/src/iosource/pcap/Source.h @@ -43,6 +43,7 @@ private: Stats stats; pcap_t* pd; + struct pcap_stat prev_pstat = {0}; }; } // namespace zeek::iosource::pcap