Merge remote-tracking branch 'origin/topic/jsiwek/gh-977-improve-pcap-error-handling'

* origin/topic/jsiwek/gh-977-improve-pcap-error-handling:
  Compare pcap_next_ex() result to PCAP_ERROR/PCAP_ERROR_BREAK
  GH-977: Improve pcap error handling
  Remove not-useful code in iosource::Manager::OpenPktSrc

CHANGES

@@ -1,4 +1,16 @@
 
+3.2.0-dev.783 | 2020-06-11 23:21:41 -0700
+
+  * Compare pcap_next_ex() result to PCAP_ERROR/PCAP_ERROR_BREAK (Jon Siwek, Corelight)
+
+  * GH-977: Improve pcap error handling (Jon Siwek, Corelight)
+
+    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.
+
+  * Remove not-useful code in iosource::Manager::OpenPktSrc (Jon Siwek, Corelight)
+
 3.2.0-dev.779 | 2020-06-11 23:17:46 -0700
 
   * Rename BroType to zeek::Type (Tim Wojtulewicz, Corelight)

VERSION

@@ -1 +1 @@
-3.2.0-dev.779
+3.2.0-dev.783

src/iosource/Manager.cc

@@ -371,10 +371,6 @@ PktSrc* Manager::OpenPktSrc(const std::string& path, bool is_live)
 	PktSrc* ps = (*component->Factory())(npath, is_live);
 	assert(ps);
 
-	if ( ! ps->IsOpen() && ps->IsError() )
-		// Set an error message if it didn't open successfully.
-		ps->Error("could not open");
-
 	DBG_LOG(DBG_PKTIO, "Created packet source of type %s for %s", component->Name().c_str(), npath.c_str());
 
 	Register(ps);

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 )
+	int res = pcap_next_ex(pd, &header, &data);
-		{
+
-		// Source has gone dry.  If it's a network interface, this just means
+	switch ( res ) {
-		// it's timed out. If it's a file, though, then the file has been
+	case PCAP_ERROR_BREAK: // -2
-		// exhausted.
+		// Exhausted pcap file, no more packets to read.
-		if ( ! props.is_live )
+		assert(! props.is_live);
 		Close();
-
+		return false;
+	case PCAP_ERROR: // -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;
 	}

src/iosource/pcap/Source.h

@@ -39,8 +39,6 @@ private:
 	Stats stats;
 
 	pcap_t *pd;
-
-	struct pcap_pkthdr current_hdr;
 };
 
 }