Merge remote-tracking branch 'origin/topic/awelzel/2791-pcap-stat-overflow'

* origin/topic/awelzel/2791-pcap-stat-overflow:
  pcap/Source: Allow more than 32bit for link and dropped stats

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)

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
 ---------------------
 

VERSION

@@ -1 +1 @@
-6.0.0-dev.461
+6.0.0-dev.467

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 <pcap-int.h>
 #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;
+			update_pktsrc_stats(&stats, &pstat, &prev_pstat);
-			s->link = pstat.ps_recv;
+			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

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