From 4f011a65f4313e8dcf351900b7570ff5ab7b0bc0 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 8 Jun 2020 18:02:44 -0700 Subject: [PATCH 1/3] Remove not-useful code in iosource::Manager::OpenPktSrc It's generally expected for a PktSrc to not be Open yet right after instantiation, but rather from InitSource() called during the registration process. Besides that, the logic in question would potentially replace an error message that is useful/detailed with one that is not. --- src/iosource/Manager.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/iosource/Manager.cc b/src/iosource/Manager.cc index d97b585894..ec13f32871 100644 --- a/src/iosource/Manager.cc +++ b/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); From 2000e2a4245632ca4a5018e8ec1497652d566502 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 8 Jun 2020 18:11:58 -0700 Subject: [PATCH 2/3] 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(¤t_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, ¤t_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, ¤t_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 From 65ae4d732a34dd98a0f91ddd24b6e114a8c7f98d Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Thu, 11 Jun 2020 15:01:06 -0700 Subject: [PATCH 3/3] Compare pcap_next_ex() result to PCAP_ERROR/PCAP_ERROR_BREAK --- src/iosource/pcap/Source.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/iosource/pcap/Source.cc b/src/iosource/pcap/Source.cc index e064181d58..897cea0f25 100644 --- a/src/iosource/pcap/Source.cc +++ b/src/iosource/pcap/Source.cc @@ -202,12 +202,12 @@ bool PcapSource::ExtractNextPacket(Packet* pkt) int res = pcap_next_ex(pd, &header, &data); switch ( res ) { - case -2: + case PCAP_ERROR_BREAK: // -2 // Exhausted pcap file, no more packets to read. assert(! props.is_live); Close(); return false; - case -1: + case PCAP_ERROR: // -1 // Error occurred while reading the packet. if ( props.is_live ) reporter->Error("failed to read a packet from %s: %s",