From 2000e2a4245632ca4a5018e8ec1497652d566502 Mon Sep 17 00:00:00 2001
From: Jon Siwek <jsiwek@corelight.com>
Date: Mon, 8 Jun 2020 18:11:58 -0700
Subject: [PATCH] GH-977: Improve pcap error handling

Switches from pcap_next() to pcap_next_ex() to better handle all error
conditions.  This allows, for example, to have a non-zero exit code for
a Zeek process that fails to fully process all packets in a pcap file.
---
 src/iosource/pcap/Source.cc               |  43 +++++++++++++++-------
 src/iosource/pcap/Source.h                |   2 -
 testing/btest/Traces/http/no-uri.pcap     | Bin 6336 -> 6335 bytes
 testing/btest/Traces/trunc/trunc-hdr.pcap | Bin 49 -> 48 bytes
 4 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/src/iosource/pcap/Source.cc b/src/iosource/pcap/Source.cc
index 8df5111da0..e064181d58 100644
--- a/src/iosource/pcap/Source.cc
+++ b/src/iosource/pcap/Source.cc
@@ -26,7 +26,6 @@ PcapSource::PcapSource(const std::string& path, bool is_live)
 	props.path = path;
 	props.is_live = is_live;
 	pd = nullptr;
-	memset(&current_hdr, 0, sizeof(current_hdr));
 	}
 
 void PcapSource::Open()
@@ -197,29 +196,47 @@ bool PcapSource::ExtractNextPacket(Packet* pkt)
 	if ( ! pd )
 		return false;
 
-	const u_char* data = pcap_next(pd, &current_hdr);
+	const u_char* data;
+	pcap_pkthdr* header;
 
-	if ( ! data )
-		{
-		// Source has gone dry.  If it's a network interface, this just means
-		// it's timed out. If it's a file, though, then the file has been
-		// exhausted.
-		if ( ! props.is_live )
-			Close();
+	int res = pcap_next_ex(pd, &header, &data);
 
+	switch ( res ) {
+	case -2:
+		// Exhausted pcap file, no more packets to read.
+		assert(! props.is_live);
+		Close();
 		return false;
-		}
+	case -1:
+		// Error occurred while reading the packet.
+		if ( props.is_live )
+			reporter->Error("failed to read a packet from %s: %s",
+			                props.path.data(), pcap_geterr(pd));
+		else
+			reporter->FatalError("failed to read a packet from %s: %s",
+			                     props.path.data(), pcap_geterr(pd));
+		return false;
+	case 0:
+		// Read from live interface timed out (ok).
+		return false;
+	case 1:
+		// Read a packet without problem.
+		break;
+	default:
+		reporter->InternalError("unhandled pcap_next_ex return value: %d", res);
+		return false;
+	}
 
-	pkt->Init(props.link_type, &current_hdr.ts, current_hdr.caplen, current_hdr.len, data);
+	pkt->Init(props.link_type, &header->ts, header->caplen, header->len, data);
 
-	if ( current_hdr.len == 0 || current_hdr.caplen == 0 )
+	if ( header->len == 0 || header->caplen == 0 )
 		{
 		Weird("empty_pcap_header", pkt);
 		return false;
 		}
 
 	++stats.received;
-	stats.bytes_received += current_hdr.len;
+	stats.bytes_received += header->len;
 
 	return true;
 	}
diff --git a/src/iosource/pcap/Source.h b/src/iosource/pcap/Source.h
index 24271b6613..8ad53b6731 100644
--- a/src/iosource/pcap/Source.h
+++ b/src/iosource/pcap/Source.h
@@ -39,8 +39,6 @@ private:
 	Stats stats;
 
 	pcap_t *pd;
-
-	struct pcap_pkthdr current_hdr;
 };
 
 }
diff --git a/testing/btest/Traces/http/no-uri.pcap b/testing/btest/Traces/http/no-uri.pcap
index 86c42097a147d7a6020a79a76da475fbf42f297b..47a81fb2253571e4816db1780561c309e8c7e943 100644
GIT binary patch
delta 7
OcmX?LxZiNYehB~$k^^Y~

delta 9
QcmdmQc))POehEe{02OWnbN~PV

diff --git a/testing/btest/Traces/trunc/trunc-hdr.pcap b/testing/btest/Traces/trunc/trunc-hdr.pcap
index 689128f2bb5db52f0b06d74f641fa534b5bb3530..c7820a3685514759ead0de950c707b6d7173a519 100644
GIT binary patch
delta 4
LcmXpsm|y?^0;mBZ

delta 6
NcmXpooM6Dn1poy10WAOk