diff --git a/CHANGES b/CHANGES index 6c0cf06cdb..c115bead82 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,30 @@ +6.0.0-dev.467 | 2023-04-28 10:01:13 +0200 + + * GH-2791: pcap/Source: Allow more than 32bit for link and dropped stats (Arne Welzel, Corelight) + + 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. + + * record_fields: Include information about optionality of fields (Arne Welzel, Corelight) + + This was reported as a wish for log schema generation, so add it... + + * Fix a few warnings from recent changes (Tim Wojtulewicz) + 6.0.0-dev.461 | 2023-04-28 09:37:08 +0200 * Simplify btests using cluster_started event. (Jan Grashoefer, Corelight) diff --git a/NEWS b/NEWS index 3a6ae9b160..cbdddff657 100644 --- a/NEWS +++ b/NEWS @@ -300,6 +300,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/VERSION b/VERSION index 55d159fa30..39ca79e52c 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.0.0-dev.461 +6.0.0-dev.467 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