From b828a6ddc7a8d6f07f10e4a017f2a5540c072169 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Thu, 10 Oct 2013 14:45:06 -0500 Subject: [PATCH 01/11] Review usage of Reporter::InternalError, addresses BIT-1045. Replaced some with InternalWarning or InternalAnalyzerError, the later being a new method which signals the analyzer to not process further input. Some usages I just removed if they didn't make sense or clearly couldn't happen. Also did some minor refactors of related code while reviewing/exploring ways to get rid of InternalError usages. Also, for TCP content file write failures there's a new event: "contents_file_write_failure". --- src/Conn.cc | 28 ++++--- src/DNS_Mgr.cc | 7 +- src/Desc.cc | 10 ++- src/Event.cc | 5 +- src/EventRegistry.cc | 11 ++- src/File.cc | 56 ++++++++++---- src/Frag.cc | 60 ++++++++++----- src/Frag.h | 1 + src/ID.cc | 3 +- src/IP.cc | 11 ++- src/IP.h | 80 ++++++++++---------- src/OpaqueVal.cc | 2 +- src/PrefixTable.cc | 13 ++-- src/Reassem.cc | 2 - src/RemoteSerializer.cc | 4 +- src/Reporter.cc | 16 ++++ src/Reporter.h | 5 ++ src/RuleMatcher.cc | 6 +- src/Serializer.cc | 2 +- src/Sessions.cc | 46 +++++++---- src/Stmt.cc | 3 +- src/Val.cc | 15 ++-- src/analyzer/Analyzer.h | 2 +- src/analyzer/Manager.cc | 34 ++++++--- src/analyzer/Manager.h | 3 +- src/analyzer/protocol/http/HTTP.cc | 24 +++++- src/analyzer/protocol/icmp/ICMP.cc | 12 ++- src/analyzer/protocol/login/Login.cc | 27 +++++-- src/analyzer/protocol/login/Login.h | 4 +- src/analyzer/protocol/login/NVT.cc | 22 +++++- src/analyzer/protocol/login/RSH.cc | 14 +++- src/analyzer/protocol/login/Rlogin.cc | 8 +- src/analyzer/protocol/mime/MIME.cc | 26 +++++-- src/analyzer/protocol/mime/MIME.h | 3 +- src/analyzer/protocol/pop3/POP3.cc | 7 +- src/analyzer/protocol/rpc/RPC.cc | 17 ++++- src/analyzer/protocol/smb/SMB.cc | 4 +- src/analyzer/protocol/tcp/ContentLine.cc | 9 ++- src/analyzer/protocol/tcp/TCP.cc | 3 +- src/analyzer/protocol/tcp/TCP_Endpoint.cc | 33 +++++--- src/analyzer/protocol/tcp/TCP_Reassembler.cc | 36 +++++++-- src/analyzer/protocol/tcp/events.bif | 10 +++ src/analyzer/protocol/tcp/functions.bif | 4 +- src/file_analysis/Manager.cc | 13 +++- src/input/Manager.cc | 63 +++++++-------- src/input/readers/Binary.cc | 4 +- src/input/readers/Raw.cc | 6 +- src/plugin/Component.cc | 4 +- src/plugin/ComponentManager.h | 9 ++- src/threading/SerialTypes.cc | 8 +- src/util.cc | 4 +- 51 files changed, 532 insertions(+), 267 deletions(-) diff --git a/src/Conn.cc b/src/Conn.cc index e221bb20e0..d0a932e373 100644 --- a/src/Conn.cc +++ b/src/Conn.cc @@ -27,9 +27,6 @@ void ConnectionTimer::Init(Connection* arg_conn, timer_func arg_timer, ConnectionTimer::~ConnectionTimer() { - if ( conn->RefCnt() < 1 ) - reporter->InternalError("reference count inconsistency in ~ConnectionTimer"); - conn->RemoveTimer(this); Unref(conn); } @@ -44,9 +41,6 @@ void ConnectionTimer::Dispatch(double t, int is_expire) conn->RemoveTimer(this); (conn->*timer)(t); - - if ( conn->RefCnt() < 1 ) - reporter->InternalError("reference count inconsistency in ConnectionTimer::Dispatch"); } IMPLEMENT_SERIAL(ConnectionTimer, SER_CONNECTION_TIMER); @@ -68,7 +62,10 @@ bool ConnectionTimer::DoSerialize(SerialInfo* info) const else if ( timer == timer_func(&Connection::RemoveConnectionTimer) ) type = 4; else - reporter->InternalError("unknown function in ConnectionTimer::DoSerialize()"); + { + reporter->InternalWarning("unknown function in ConnectionTimer::DoSerialize()"); + return false; + } return conn->Serialize(info) && SERIALIZE(type) && SERIALIZE(do_expire); } @@ -180,7 +177,12 @@ Connection::Connection(NetSessions* s, HashKey* k, double t, const ConnID* id, Connection::~Connection() { if ( ! finished ) - reporter->InternalError("Done() not called before destruction of Connection"); + { + // TODO: not sure about this + reporter->InternalWarning( + "missing Connection::Done() before ~Connection"); + Done(); + } CancelTimers(); @@ -782,7 +784,15 @@ void Connection::Describe(ODesc* d) const break; case TRANSPORT_UNKNOWN: - reporter->InternalError("unknown transport in Connction::Describe()"); + d->Add("unknown"); + reporter->InternalWarning( + "unknown transport in Connction::Describe()"); + + break; + + default: + reporter->InternalError( + "unhandled transport type in Connection::Describe"); break; } diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index c1036accdd..17409a930b 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -546,7 +546,7 @@ Val* DNS_Mgr::LookupAddr(const IPAddr& addr) return LookupAddr(addr); default: - reporter->InternalError("bad mode in DNS_Mgr::LookupHost"); + reporter->InternalError("bad mode in DNS_Mgr::LookupAddr"); return 0; } } @@ -833,7 +833,10 @@ void DNS_Mgr::CompareMappings(DNS_Mapping* prev_dm, DNS_Mapping* new_dm) ListVal* new_a = new_dm->Addrs(); if ( ! prev_a || ! new_a ) - reporter->InternalError("confused in DNS_Mgr::CompareMappings"); + { + reporter->InternalWarning("confused in DNS_Mgr::CompareMappings"); + return; + } ListVal* prev_delta = AddrListDelta(prev_a, new_a); ListVal* new_delta = AddrListDelta(new_a, prev_a); diff --git a/src/Desc.cc b/src/Desc.cc index 40b6fcc3d1..2a44a65fdf 100644 --- a/src/Desc.cc +++ b/src/Desc.cc @@ -65,14 +65,20 @@ void ODesc::PushIndent() void ODesc::PopIndent() { if ( --indent_level < 0 ) - reporter->InternalError("ODesc::PopIndent underflow"); + { + indent_level = 0; + reporter->InternalWarning("ODesc::PopIndent underflow"); + } NL(); } void ODesc::PopIndentNoNL() { if ( --indent_level < 0 ) - reporter->InternalError("ODesc::PopIndent underflow"); + { + indent_level = 0; + reporter->InternalWarning("ODesc::PopIndent underflow"); + } } void ODesc::Add(const char* s, int do_indent) diff --git a/src/Event.cc b/src/Event.cc index d6bb0d8e58..980180def6 100644 --- a/src/Event.cc +++ b/src/Event.cc @@ -91,7 +91,10 @@ void EventMgr::QueueEvent(Event* event) void EventMgr::Dispatch() { if ( ! head ) - reporter->InternalError("EventMgr underflow"); + { + reporter->InternalWarning("EventMgr::Dispatch underflow"); + return; + } Event* current = head; diff --git a/src/EventRegistry.cc b/src/EventRegistry.cc index 2da16de51d..2428198255 100644 --- a/src/EventRegistry.cc +++ b/src/EventRegistry.cc @@ -90,9 +90,14 @@ void EventRegistry::PrintDebug() void EventRegistry::SetErrorHandler(const char* name) { EventHandler* eh = Lookup(name); - if ( ! eh ) - reporter->InternalError("unknown event handler in SetErrorHandler()"); - eh->SetErrorHandler(); + if ( eh ) + { + eh->SetErrorHandler(); + return; + } + + reporter->InternalWarning( + "unknown event handler '%s' in SetErrorHandler()", name); } diff --git a/src/File.cc b/src/File.cc index 880fd254ef..d28d0ca569 100644 --- a/src/File.cc +++ b/src/File.cc @@ -281,8 +281,13 @@ FILE* BroFile::File() FILE* BroFile::BringIntoCache() { + char buf[256]; + if ( f ) - reporter->InternalError("BroFile non-nil non-open file"); + { + reporter->InternalWarning("BroFile non-nil non-open file"); + return 0; + } if ( num_files_in_cache >= max_files_in_cache ) PurgeCache(); @@ -296,22 +301,30 @@ FILE* BroFile::BringIntoCache() if ( ! f ) { - reporter->Error("can't open %s", name); + strerror_r(errno, buf, sizeof(buf)); + reporter->Error("can't open %s: %s", name, buf); f = fopen("/dev/null", "w"); - if ( ! f ) - reporter->InternalError("out of file descriptors"); + if ( f ) + { + okay_to_manage = 0; + return f; + } - okay_to_manage = 0; - return f; + strerror_r(errno, buf, sizeof(buf)); + reporter->InternalWarning("can't open /dev/null: %s", buf); + return 0; } RaiseOpenEvent(); UpdateFileSize(); if ( fseek(f, position, SEEK_SET) < 0 ) - reporter->Error("reopen seek failed"); + { + strerror_r(errno, buf, sizeof(buf)); + reporter->Error("reopen seek failed: %s", buf); + } InsertAtBeginning(); @@ -386,18 +399,30 @@ int BroFile::Close() void BroFile::Suspend() { if ( ! is_in_cache ) - reporter->InternalError("BroFile::Suspend() called for non-cached file"); + { + reporter->InternalWarning("BroFile::Suspend() called for non-cached file"); + return; + } + if ( ! is_open ) - reporter->InternalError("BroFile::Suspend() called for non-open file"); + { + reporter->InternalWarning("BroFile::Suspend() called for non-open file"); + return; + } Unlink(); if ( ! f ) - reporter->InternalError("BroFile::Suspend() called for nil file"); + { + reporter->InternalWarning("BroFile::Suspend() called for nil file"); + return; + } if ( (position = ftell(f)) < 0 ) { - reporter->Error("ftell failed"); + char buf[256]; + strerror_r(errno, buf, sizeof(buf)); + reporter->Error("ftell failed: %s", buf); position = 0; } @@ -407,10 +432,13 @@ void BroFile::Suspend() void BroFile::PurgeCache() { - if ( ! tail ) - reporter->InternalError("BroFile purge of empty cache"); + if ( tail ) + { + tail->Suspend(); + return; + } - tail->Suspend(); + reporter->InternalWarning("BroFile purge of empty cache"); } void BroFile::Unlink() diff --git a/src/Frag.cc b/src/Frag.cc index 4b9047d072..5cfff270c7 100644 --- a/src/Frag.cc +++ b/src/Frag.cc @@ -22,7 +22,7 @@ void FragTimer::Dispatch(double t, int /* is_expire */) if ( f ) f->Expire(t); else - reporter->InternalError("fragment timer dispatched w/o reassembler"); + reporter->InternalWarning("fragment timer dispatched w/o reassembler"); } FragReassembler::FragReassembler(NetSessions* arg_s, @@ -155,14 +155,33 @@ void FragReassembler::AddFragment(double t, const IP_Hdr* ip, const u_char* pkt) NewBlock(network_time, offset, len, pkt); } +void FragReassembler::Weird(const char* name) const + { + unsigned int version = ((const ip*)proto_hdr)->ip_v; + + if ( version == 4 ) + { + IP_Hdr hdr((const ip*)proto_hdr, false); + s->Weird(name, &hdr); + } + else if ( version == 6 ) + { + IP_Hdr hdr((const ip6_hdr*)proto_hdr, false, proto_hdr_len); + s->Weird(name, &hdr); + } + else + { + reporter->InternalWarning("Unexpected IP version in FragReassembler"); + reporter->Weird(name); + } + } + void FragReassembler::Overlap(const u_char* b1, const u_char* b2, int n) { - IP_Hdr proto_h(proto_hdr, false, proto_hdr_len); - if ( memcmp((const void*) b1, (const void*) b2, n) ) - s->Weird("fragment_inconsistency", &proto_h); + Weird("fragment_inconsistency"); else - s->Weird("fragment_overlap", &proto_h); + Weird("fragment_overlap"); } void FragReassembler::BlockInserted(DataBlock* /* start_block */) @@ -188,9 +207,7 @@ void FragReassembler::BlockInserted(DataBlock* /* start_block */) // beyond it, which is not contiguous. This // can happen for benign reasons when we're // intermingling parts of two fragmented packets. - - IP_Hdr proto_h(proto_hdr, false, proto_hdr_len); - s->Weird("fragment_size_inconsistency", &proto_h); + Weird("fragment_size_inconsistency"); // We decide to analyze the contiguous portion now. // Extend the fragment up through the end of what @@ -203,8 +220,7 @@ void FragReassembler::BlockInserted(DataBlock* /* start_block */) else if ( last_block->upper > frag_size ) { - IP_Hdr proto_h(proto_hdr, false, proto_hdr_len); - s->Weird("fragment_size_inconsistency", &proto_h); + Weird("fragment_size_inconsistency"); frag_size = last_block->upper; } @@ -238,36 +254,42 @@ void FragReassembler::BlockInserted(DataBlock* /* start_block */) break; if ( b->upper > n ) - reporter->InternalError("bad fragment reassembly"); + { + reporter->InternalWarning("bad fragment reassembly"); + DeleteTimer(); + Expire(network_time); + return; + } memcpy((void*) &pkt[b->seq], (const void*) b->block, b->upper - b->seq); } delete reassembled_pkt; + reassembled_pkt = 0; - if ( ((const struct ip*)pkt_start)->ip_v == 4 ) + unsigned int version = ((const struct ip*)pkt_start)->ip_v; + + if ( version == 4 ) { struct ip* reassem4 = (struct ip*) pkt_start; reassem4->ip_len = htons(frag_size + proto_hdr_len); reassembled_pkt = new IP_Hdr(reassem4, true); + DeleteTimer(); } - else if ( ((const struct ip*)pkt_start)->ip_v == 6 ) + else if ( version == 6 ) { struct ip6_hdr* reassem6 = (struct ip6_hdr*) pkt_start; reassem6->ip6_plen = htons(frag_size + proto_hdr_len - 40); const IPv6_Hdr_Chain* chain = new IPv6_Hdr_Chain(reassem6, next_proto, n); reassembled_pkt = new IP_Hdr(reassem6, true, n, chain); + DeleteTimer(); } else - { - reporter->InternalError("bad IP version in fragment reassembly"); - } - - - DeleteTimer(); + reporter->InternalWarning("bad IP version in fragment reassembly: %d", + version); } void FragReassembler::Expire(double t) diff --git a/src/Frag.h b/src/Frag.h index 86cf3a9dd4..0d2fbed5b3 100644 --- a/src/Frag.h +++ b/src/Frag.h @@ -35,6 +35,7 @@ public: protected: void BlockInserted(DataBlock* start_block); void Overlap(const u_char* b1, const u_char* b2, int n); + void Weird(const char* name) const; u_char* proto_hdr; IP_Hdr* reassembled_pkt; diff --git a/src/ID.cc b/src/ID.cc index a6e592146b..7a3940e8c0 100644 --- a/src/ID.cc +++ b/src/ID.cc @@ -440,7 +440,6 @@ ID* ID::Unserialize(UnserialInfo* info) default: reporter->InternalError("unknown type for UnserialInfo::id_policy"); - } } @@ -543,7 +542,7 @@ bool ID::DoUnserialize(UnserialInfo* info) } if ( installed_tmp && ! global_scope()->Remove(name) ) - reporter->InternalError("tmp id missing"); + reporter->InternalWarning("missing tmp ID in %s unserialization", name); return true; } diff --git a/src/IP.cc b/src/IP.cc index 78311cc2d2..46702f2546 100644 --- a/src/IP.cc +++ b/src/IP.cc @@ -433,7 +433,10 @@ void IPv6_Hdr_Chain::Init(const struct ip6_hdr* ip6, int total_len, const u_char* hdrs = (const u_char*) ip6; if ( total_len < (int)sizeof(struct ip6_hdr) ) - reporter->InternalError("IPv6_HdrChain::Init with truncated IP header"); + { + reporter->InternalWarning("truncated IP header in IPv6_HdrChain::Init"); + return; + } do { @@ -623,9 +626,11 @@ VectorVal* IPv6_Hdr_Chain::BuildVal() const break; #endif default: - reporter->InternalError("IPv6_Hdr_Chain bad header %d", type); - break; + reporter->InternalWarning("IPv6_Hdr_Chain bad header %d", type); + Unref(ext_hdr); + continue; } + rval->Assign(rval->Size(), ext_hdr); } diff --git a/src/IP.h b/src/IP.h index c3a74b4a01..fd7a6de266 100644 --- a/src/IP.h +++ b/src/IP.h @@ -176,7 +176,14 @@ public: * Returns whether the header chain indicates a fragmented packet. */ bool IsFragment() const - { return chain[chain.size()-1]->Type() == IPPROTO_FRAGMENT; } + { + if ( chain.empty() ) + { + reporter->InternalWarning("empty IPv6 header chain"); + return false; + } + return chain[chain.size()-1]->Type() == IPPROTO_FRAGMENT; + } /** * Returns pointer to fragment header structure if the chain contains one. @@ -216,9 +223,13 @@ public: #ifdef ENABLE_MOBILE_IPV6 if ( homeAddr ) return IPAddr(*homeAddr); - else #endif - return IPAddr(((const struct ip6_hdr*)(chain[0]->Data()))->ip6_src); + if ( chain.empty() ) + { + reporter->InternalWarning("empty IPv6 header chain"); + return IPAddr(); + } + return IPAddr(((const struct ip6_hdr*)(chain[0]->Data()))->ip6_src); } /** @@ -230,8 +241,12 @@ public: { if ( finalDst ) return IPAddr(*finalDst); - else - return IPAddr(((const struct ip6_hdr*)(chain[0]->Data()))->ip6_dst); + if ( chain.empty() ) + { + reporter->InternalWarning("empty IPv6 header chain"); + return IPAddr(); + } + return IPAddr(((const struct ip6_hdr*)(chain[0]->Data()))->ip6_dst); } /** @@ -305,32 +320,6 @@ protected: */ class IP_Hdr { public: - /** - * Attempts to construct the header from some blob of data based on IP - * version number. Caller must have already checked that the header - * is not truncated. - * @param p pointer to memory containing an IPv4 or IPv6 packet. - * @param arg_del whether to take ownership of \a p pointer's memory. - * @param len the length of data, in bytes, pointed to by \a p. - */ - IP_Hdr(const u_char* p, bool arg_del, int len) - : ip4(0), ip6(0), del(arg_del), ip6_hdrs(0) - { - if ( ((const struct ip*)p)->ip_v == 4 ) - ip4 = (const struct ip*)p; - else if ( ((const struct ip*)p)->ip_v == 6 ) - { - ip6 = (const struct ip6_hdr*)p; - ip6_hdrs = new IPv6_Hdr_Chain(ip6, len); - } - else - { - if ( arg_del ) - delete [] p; - reporter->InternalError("bad IP version in IP_Hdr ctor"); - } - } - /** * Construct the header wrapper from an IPv4 packet. Caller must have * already checked that the header is not truncated. @@ -365,15 +354,12 @@ public: */ ~IP_Hdr() { - if ( ip6 ) - delete ip6_hdrs; + delete ip6_hdrs; if ( del ) { - if ( ip4 ) - delete [] (struct ip*) ip4; - else - delete [] (struct ip6_hdr*) ip6; + delete [] (struct ip*) ip4; + delete [] (struct ip6_hdr*) ip6; } } @@ -472,8 +458,14 @@ public: * For IPv6 header chains, returns the type of the last header in the chain. */ uint8 LastHeader() const - { return ip4 ? IPPROTO_RAW : - ((*ip6_hdrs)[ip6_hdrs->Size()-1])->Type(); } + { + if ( ip4 ) + return IPPROTO_RAW; + size_t i = ip6_hdrs->Size(); + if ( i > 0 ) + return (*ip6_hdrs)[i-1]->Type(); + return IPPROTO_NONE; + } /** * Returns the protocol type of the IP packet's payload, usually an @@ -481,8 +473,14 @@ public: * header's Next Header value. */ unsigned char NextProto() const - { return ip4 ? ip4->ip_p : - ((*ip6_hdrs)[ip6_hdrs->Size()-1])->NextHdr(); } + { + if ( ip4 ) + return ip4->ip_p; + size_t i = ip6_hdrs->Size(); + if ( i > 0 ) + return (*ip6_hdrs)[i-1]->NextHdr(); + return IPPROTO_NONE; + } /** * Returns the IPv4 Time to Live or IPv6 Hop Limit field. diff --git a/src/OpaqueVal.cc b/src/OpaqueVal.cc index b7ccd770ce..ea0c73d297 100644 --- a/src/OpaqueVal.cc +++ b/src/OpaqueVal.cc @@ -36,7 +36,7 @@ bool HashVal::Feed(const void* data, size_t size) if ( valid ) return DoFeed(data, size); - reporter->InternalError("invalid opaque hash value"); + Error("attempt to update an invalid opaque hash value"); return false; } diff --git a/src/PrefixTable.cc b/src/PrefixTable.cc index 58a488a9f1..d31203c9d3 100644 --- a/src/PrefixTable.cc +++ b/src/PrefixTable.cc @@ -20,7 +20,10 @@ void* PrefixTable::Insert(const IPAddr& addr, int width, void* data) Deref_Prefix(prefix); if ( ! node ) - reporter->InternalError("Cannot create node in patricia tree"); + { + reporter->InternalWarning("Cannot create node in patricia tree"); + return 0; + } void* old = node->data; @@ -49,7 +52,7 @@ void* PrefixTable::Insert(const Val* value, void* data) break; default: - reporter->InternalError("Wrong index type for PrefixTable"); + reporter->InternalWarning("Wrong index type for PrefixTable"); return 0; } } @@ -83,8 +86,8 @@ void* PrefixTable::Lookup(const Val* value, bool exact) const break; default: - reporter->InternalError("Wrong index type %d for PrefixTable", - value->Type()->Tag()); + reporter->InternalWarning("Wrong index type %d for PrefixTable", + value->Type()->Tag()); return 0; } } @@ -122,7 +125,7 @@ void* PrefixTable::Remove(const Val* value) break; default: - reporter->InternalError("Wrong index type for PrefixTable"); + reporter->InternalWarning("Wrong index type for PrefixTable"); return 0; } } diff --git a/src/Reassem.cc b/src/Reassem.cc index c3c19ff0e6..df7e3568ee 100644 --- a/src/Reassem.cc +++ b/src/Reassem.cc @@ -21,8 +21,6 @@ DataBlock::DataBlock(const u_char* data, int size, int arg_seq, upper = seq + size; block = new u_char[size]; - if ( ! block ) - reporter->InternalError("out of memory"); memcpy((void*) block, (const void*) data, size); diff --git a/src/RemoteSerializer.cc b/src/RemoteSerializer.cc index 43a963ab21..bcf4b090b0 100644 --- a/src/RemoteSerializer.cc +++ b/src/RemoteSerializer.cc @@ -3584,8 +3584,6 @@ bool SocketComm::ProcessParentMessage() InternalError(fmt("unknown msg type %d", parent_msgtype)); return true; } - - InternalError("cannot be reached"); } case ARGS: @@ -3609,9 +3607,9 @@ bool SocketComm::ProcessParentMessage() default: InternalError("unknown msgstate"); + return false; } - InternalError("cannot be reached"); return false; } diff --git a/src/Reporter.cc b/src/Reporter.cc index 6bc2577c72..80907e5696 100644 --- a/src/Reporter.cc +++ b/src/Reporter.cc @@ -137,11 +137,27 @@ void Reporter::InternalError(const char* fmt, ...) abort(); } +void Reporter::InternalAnalyzerError(analyzer::Analyzer* a, const char* fmt, + ...) + { + if ( a ) + a->SetSkip(true); + + va_list ap; + va_start(ap, fmt); + // Always log to stderr. + // TODO: would be nice to also log a call stack. + DoLog("analyzer error", reporter_error, stderr, 0, 0, true, true, 0, fmt, + ap); + va_end(ap); + } + void Reporter::InternalWarning(const char* fmt, ...) { va_list ap; va_start(ap, fmt); FILE* out = warnings_to_stderr ? stderr : 0; + // TODO: would be nice to also log a call stack. DoLog("internal warning", reporter_warning, out, 0, 0, true, true, 0, fmt, ap); va_end(ap); diff --git a/src/Reporter.h b/src/Reporter.h index 6a205cfd6d..79eb21c1de 100644 --- a/src/Reporter.h +++ b/src/Reporter.h @@ -12,6 +12,7 @@ #include "EventHandler.h" #include "IPAddr.h" +namespace analyzer { class Analyzer; } class Connection; class Location; class Reporter; @@ -91,6 +92,10 @@ public: // dump after the message has been reported. void InternalError(const char* fmt, ...) FMT_ATTR; + // Reporter an internal analyzer error. That analyzer will not process + // any further input, but Bro otherwise continues normally. + void InternalAnalyzerError(analyzer::Analyzer* a, const char* fmt, ...); + // Toggle whether non-fatal messages should be reported through the // scripting layer rather on standard output. Fatal errors are always // reported via stderr. diff --git a/src/RuleMatcher.cc b/src/RuleMatcher.cc index ed33db6792..49c6460ea7 100644 --- a/src/RuleMatcher.cc +++ b/src/RuleMatcher.cc @@ -521,7 +521,7 @@ static inline bool compare(const maskedvalue_list& mvals, uint32 v, break; default: - reporter->InternalError("unknown comparison type"); + reporter->InternalError("unknown RuleHdrTest comparison type"); break; } return false; @@ -556,7 +556,7 @@ static inline bool compare(const vector& prefixes, const IPAddr& a, break; default: - reporter->InternalError("unknown comparison type"); + reporter->InternalError("unknown RuleHdrTest comparison type"); break; } return false; @@ -661,7 +661,7 @@ RuleEndpointState* RuleMatcher::InitEndpoint(analyzer::Analyzer* analyzer, break; default: - reporter->InternalError("unknown protocol"); + reporter->InternalError("unknown RuleHdrTest protocol type"); break; } diff --git a/src/Serializer.cc b/src/Serializer.cc index 3f0a66a408..156ad67f2e 100644 --- a/src/Serializer.cc +++ b/src/Serializer.cc @@ -419,7 +419,7 @@ bool Serializer::UnserializeCall(UnserialInfo* info) break; default: - reporter->InternalError("invalid function flavor"); + reporter->InternalError("unserialized call for invalid function flavor"); break; } diff --git a/src/Sessions.cc b/src/Sessions.cc index b844dc614e..acc306d277 100644 --- a/src/Sessions.cc +++ b/src/Sessions.cc @@ -781,7 +781,10 @@ int NetSessions::ParseIPPacket(int caplen, const u_char* const pkt, int proto, } else - reporter->InternalError("Bad IP protocol version in DoNextInnerPacket"); + { + reporter->InternalWarning("Bad IP protocol version in ParseIPPacket"); + return -1; + } if ( (uint32)caplen != inner->TotalLen() ) return (uint32)caplen < inner->TotalLen() ? -1 : 1; @@ -993,21 +996,21 @@ void NetSessions::Remove(Connection* c) switch ( c->ConnTransport() ) { case TRANSPORT_TCP: if ( ! tcp_conns.RemoveEntry(k) ) - reporter->InternalError("connection missing"); + reporter->InternalWarning("connection missing"); break; case TRANSPORT_UDP: if ( ! udp_conns.RemoveEntry(k) ) - reporter->InternalError("connection missing"); + reporter->InternalWarning("connection missing"); break; case TRANSPORT_ICMP: if ( ! icmp_conns.RemoveEntry(k) ) - reporter->InternalError("connection missing"); + reporter->InternalWarning("connection missing"); break; case TRANSPORT_UNKNOWN: - reporter->InternalError("unknown transport when removing connection"); + reporter->InternalWarning("unknown transport when removing connection"); break; } @@ -1018,13 +1021,18 @@ void NetSessions::Remove(Connection* c) void NetSessions::Remove(FragReassembler* f) { - if ( ! f ) return; - HashKey* k = f->Key(); - if ( ! k ) - reporter->InternalError("fragment block not in dictionary"); + if ( ! f ) + return; - if ( ! fragments.RemoveEntry(k) ) - reporter->InternalError("fragment block missing"); + HashKey* k = f->Key(); + + if ( k ) + { + if ( ! fragments.RemoveEntry(k) ) + reporter->InternalWarning("fragment reassembler not in dict"); + } + else + reporter->InternalWarning("missing fragment reassembler hash key"); Unref(f); } @@ -1055,7 +1063,9 @@ void NetSessions::Insert(Connection* c) break; default: - reporter->InternalError("unknown connection type"); + reporter->InternalWarning("unknown connection type"); + Unref(c); + return; } if ( old && old != c ) @@ -1147,8 +1157,8 @@ Connection* NetSessions::NewConn(HashKey* k, double t, const ConnID* id, tproto = TRANSPORT_ICMP; break; default: - reporter->InternalError("unknown transport protocol"); - break; + reporter->InternalWarning("unknown transport protocol"); + return 0; }; if ( tproto == TRANSPORT_TCP ) @@ -1181,7 +1191,13 @@ Connection* NetSessions::NewConn(HashKey* k, double t, const ConnID* id, Connection* conn = new Connection(this, k, t, id, flow_label, encapsulation); conn->SetTransport(tproto); - analyzer_mgr->BuildInitialAnalyzerTree(conn); + + if ( ! analyzer_mgr->BuildInitialAnalyzerTree(conn) ) + { + conn->Done(); + Unref(conn); + return 0; + } bool external = conn->IsExternal(); diff --git a/src/Stmt.cc b/src/Stmt.cc index d879c598d2..3571cad197 100644 --- a/src/Stmt.cc +++ b/src/Stmt.cc @@ -799,8 +799,9 @@ int SwitchStmt::FindCaseLabelMatch(const Val* v) const if ( ! hk ) { reporter->PushLocation(e->GetLocationInfo()); - reporter->InternalError("switch expression type mismatch (%s/%s)", + reporter->Error("switch expression type mismatch (%s/%s)", type_name(v->Type()->Tag()), type_name(e->Type()->Tag())); + return -1; } int* label_idx = case_label_map.Lookup(hk); diff --git a/src/Val.cc b/src/Val.cc index d791ff195d..450f3c1653 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -238,7 +238,6 @@ bool Val::DoSerialize(SerialInfo* info) const return false; } - reporter->InternalError("should not be reached"); return false; } @@ -316,7 +315,6 @@ bool Val::DoUnserialize(UnserialInfo* info) return false; } - reporter->InternalError("should not be reached"); return false; } @@ -519,8 +517,9 @@ void Val::ValDescribe(ODesc* d) const break; default: - // Don't call Internal(), that'll loop! - reporter->InternalError("Val description unavailable"); + reporter->InternalWarning("Val description unavailable"); + d->Add(""); + break; } } @@ -1893,7 +1892,7 @@ Val* TableVal::Delete(const Val* index) Val* va = v ? (v->Value() ? v->Value() : this->Ref()) : 0; if ( subnets && ! subnets->Remove(index) ) - reporter->InternalError("index not in prefix table"); + reporter->InternalWarning("index not in prefix table"); if ( LoggingAccess() ) { @@ -1935,7 +1934,7 @@ Val* TableVal::Delete(const HashKey* k) { Val* index = table_hash->RecoverVals(k); if ( ! subnets->Remove(index) ) - reporter->InternalError("index not in prefix table"); + reporter->InternalWarning("index not in prefix table"); Unref(index); } @@ -2195,7 +2194,7 @@ void TableVal::DoExpire(double t) { Val* index = RecoverIndex(k); if ( ! subnets->Remove(index) ) - reporter->InternalError("index not in prefix table"); + reporter->InternalWarning("index not in prefix table"); Unref(index); } @@ -3300,7 +3299,7 @@ int same_atomic_val(const Val* v1, const Val* v2) return v1->AsSubNet() == v2->AsSubNet(); default: - reporter->InternalError("same_atomic_val called for non-atomic value"); + reporter->InternalWarning("same_atomic_val called for non-atomic value"); return 0; } diff --git a/src/analyzer/Analyzer.h b/src/analyzer/Analyzer.h index b709e3dda0..f7ca07ca51 100644 --- a/src/analyzer/Analyzer.h +++ b/src/analyzer/Analyzer.h @@ -303,7 +303,7 @@ public: * Signals the analyzer to skip all further input processsing. The \a * Next*() methods check this flag and discard the input if its set. * - * @param do_skipe If true, further processing will be skipped. + * @param do_skip If true, further processing will be skipped. */ void SetSkip(bool do_skip) { skip = do_skip; } diff --git a/src/analyzer/Manager.cc b/src/analyzer/Manager.cc index 2359e4ec98..6268fde177 100644 --- a/src/analyzer/Manager.cc +++ b/src/analyzer/Manager.cc @@ -250,6 +250,9 @@ bool Manager::RegisterAnalyzerForPort(Tag tag, TransportProto proto, uint32 port { tag_set* l = LookupPort(proto, port, true); + if ( ! l ) + return false; + #ifdef DEBUG const char* name = GetComponentName(tag); DBG_LOG(DBG_ANALYZER, "Registering analyzer %s for port %" PRIu32 "/%d", name, port, proto); @@ -263,6 +266,9 @@ bool Manager::UnregisterAnalyzerForPort(Tag tag, TransportProto proto, uint32 po { tag_set* l = LookupPort(proto, port, true); + if ( ! l ) + return true; // still a "successful" unregistration + #ifdef DEBUG const char* name = GetComponentName(tag); DBG_LOG(DBG_ANALYZER, "Unregistering analyzer %s for port %" PRIu32 "/%d", name, port, proto); @@ -277,18 +283,27 @@ Analyzer* Manager::InstantiateAnalyzer(Tag tag, Connection* conn) Component* c = Lookup(tag); if ( ! c ) - reporter->InternalError("request to instantiate unknown analyzer"); + { + reporter->InternalWarning("request to instantiate unknown analyzer"); + return 0; + } if ( ! c->Enabled() ) return 0; if ( ! c->Factory() ) - reporter->InternalError("analyzer %s cannot be instantiated dynamically", GetComponentName(tag)); + { + reporter->InternalWarning("analyzer %s cannot be instantiated dynamically", GetComponentName(tag)); + return 0; + } Analyzer* a = c->Factory()(conn); if ( ! a ) - reporter->InternalError("analyzer instantiation failed"); + { + reporter->InternalWarning("analyzer instantiation failed"); + return 0; + } a->SetAnalyzerTag(tag); @@ -315,7 +330,8 @@ Manager::tag_set* Manager::LookupPort(TransportProto proto, uint32 port, bool ad break; default: - reporter->InternalError("unsupport transport protocol in analyzer::Manager::LookupPort"); + reporter->InternalWarning("unsupported transport protocol in analyzer::Manager::LookupPort"); + return 0; } analyzer_map_by_port::const_iterator i = m->find(port); @@ -338,7 +354,6 @@ Manager::tag_set* Manager::LookupPort(PortVal* val, bool add_if_not_found) bool Manager::BuildInitialAnalyzerTree(Connection* conn) { - Analyzer* analyzer = 0; tcp::TCP_Analyzer* tcp = 0; udp::UDP_Analyzer* udp = 0; icmp::ICMP_Analyzer* icmp = 0; @@ -374,14 +389,9 @@ bool Manager::BuildInitialAnalyzerTree(Connection* conn) } default: - reporter->InternalError("unknown protocol"); - } - - if ( ! root ) - { - DBG_ANALYZER(conn, "cannot build analyzer tree"); + reporter->InternalWarning("unknown protocol can't build analyzer tree"); return false; - } + } // Any scheduled analyzer? for ( tag_set::iterator i = expected.begin(); i != expected.end(); i++ ) diff --git a/src/analyzer/Manager.h b/src/analyzer/Manager.h index d151709eda..77b40a1d68 100644 --- a/src/analyzer/Manager.h +++ b/src/analyzer/Manager.h @@ -214,7 +214,8 @@ public: * * @return The new analyzer instance. Note that the analyzer will not * have been added to the connection's analyzer tree yet. Returns - * null if tag is invalid or the requested analyzer is disabled. + * null if tag is invalid, the requested analyzer is disabled, or the + * analyzer can't be instantiated. */ Analyzer* InstantiateAnalyzer(Tag tag, Connection* c); diff --git a/src/analyzer/protocol/http/HTTP.cc b/src/analyzer/protocol/http/HTTP.cc index 4bb2385c27..5544bac2f6 100644 --- a/src/analyzer/protocol/http/HTTP.cc +++ b/src/analyzer/protocol/http/HTTP.cc @@ -698,7 +698,11 @@ void HTTP_Message::SubmitData(int len, const char* buf) { if ( buf != (const char*) data_buffer->Bytes() + buffer_offset || buffer_offset + len > buffer_size ) - reporter->InternalError("buffer misalignment"); + { + reporter->InternalAnalyzerError(MyHTTP_Analyzer(), + "HTTP message buffer misalignment"); + return; + } buffer_offset += len; if ( buffer_offset >= buffer_size ) @@ -743,7 +747,9 @@ void HTTP_Message::SubmitEvent(int event_type, const char* detail) break; default: - reporter->InternalError("unrecognized HTTP message event"); + reporter->InternalAnalyzerError(MyHTTP_Analyzer(), + "unrecognized HTTP message event"); + return; } MyHTTP_Analyzer()->HTTP_Event(category, detail); @@ -962,7 +968,13 @@ void HTTP_Analyzer::DeliverStream(int len, const u_char* data, bool is_orig) switch ( request_state ) { case EXPECT_REQUEST_LINE: - if ( HTTP_RequestLine(line, end_of_line) ) + { + int res = HTTP_RequestLine(line, end_of_line); + + if ( res < 0 ) + return; + + else if ( res > 0 ) { ++num_requests; @@ -1001,6 +1013,7 @@ void HTTP_Analyzer::DeliverStream(int len, const u_char* data, bool is_orig) } } } + } break; case EXPECT_REQUEST_MESSAGE: @@ -1225,7 +1238,10 @@ int HTTP_Analyzer::HTTP_RequestLine(const char* line, const char* end_of_line) request_method = new StringVal(end_of_method - line, line); if ( ! ParseRequest(rest, end_of_line) ) - reporter->InternalError("HTTP ParseRequest failed"); + { + reporter->InternalAnalyzerError(this, "HTTP ParseRequest failed"); + return -1; + } Conn()->Match(Rule::HTTP_REQUEST, (const u_char*) unescaped_URI->AsString()->Bytes(), diff --git a/src/analyzer/protocol/icmp/ICMP.cc b/src/analyzer/protocol/icmp/ICMP.cc index 17b5b0fcc1..6aa535fe95 100644 --- a/src/analyzer/protocol/icmp/ICMP.cc +++ b/src/analyzer/protocol/icmp/ICMP.cc @@ -60,9 +60,9 @@ void ICMP_Analyzer::DeliverPacket(int len, const u_char* data, break; default: - reporter->InternalError("unexpected IP proto in ICMP analyzer: %d", - ip->NextProto()); - break; + reporter->InternalAnalyzerError(this, + "unexpected IP proto in ICMP analyzer: %d", ip->NextProto()); + return; } if ( chksum != 0xffff ) @@ -99,7 +99,11 @@ void ICMP_Analyzer::DeliverPacket(int len, const u_char* data, else if ( ip->NextProto() == IPPROTO_ICMPV6 ) NextICMP6(current_timestamp, icmpp, len, caplen, data, ip); else - reporter->InternalError("unexpected next protocol in ICMP::DeliverPacket()"); + { + reporter->InternalAnalyzerError(this, + "expected ICMP as IP packet's protocol, got %d", ip->NextProto()); + return; + } if ( caplen >= len ) diff --git a/src/analyzer/protocol/login/Login.cc b/src/analyzer/protocol/login/Login.cc index de2445d81f..f9e72db87d 100644 --- a/src/analyzer/protocol/login/Login.cc +++ b/src/analyzer/protocol/login/Login.cc @@ -117,7 +117,11 @@ void Login_Analyzer::NewLine(bool orig, char* line) } if ( state != LOGIN_STATE_CONFUSED ) - reporter->InternalError("bad state in Login_Analyzer::NewLine"); + { + reporter->InternalAnalyzerError(this, + "bad state in Login_Analyzer::NewLine"); + return; + } // When we're in "confused", we feed each user input line to // login_confused_text, but also scan the text in the @@ -550,6 +554,9 @@ int Login_Analyzer::IsTimeout(const char* line) const int Login_Analyzer::IsEmpty(const char* line) const { + if ( ! line ) + return true; + while ( *line && isspace(*line) ) ++line; @@ -571,10 +578,14 @@ void Login_Analyzer::AddUserText(const char* line) } } -char* Login_Analyzer::PeekUserText() const +char* Login_Analyzer::PeekUserText() { if ( num_user_text <= 0 ) - reporter->InternalError("underflow in Login_Analyzer::PeekUserText()"); + { + reporter->InternalAnalyzerError(this, + "underflow in Login_Analyzer::PeekUserText()"); + return 0; + } return user_text[user_text_first]; } @@ -583,6 +594,9 @@ char* Login_Analyzer::PopUserText() { char* s = PeekUserText(); + if ( ! s ) + return 0; + if ( ++user_text_first == MAX_USER_TEXT ) user_text_first = 0; @@ -594,8 +608,11 @@ char* Login_Analyzer::PopUserText() Val* Login_Analyzer::PopUserTextVal() { char* s = PopUserText(); - BroString* bs = new BroString(1, byte_vec(s), strlen(s)); - return new StringVal(bs); + + if ( s ) + return new StringVal(new BroString(1, byte_vec(s), strlen(s))); + else + return new StringVal(""); } int Login_Analyzer::MatchesTypeahead(const char* line) const diff --git a/src/analyzer/protocol/login/Login.h b/src/analyzer/protocol/login/Login.h index 2178bdff1a..25d1b24005 100644 --- a/src/analyzer/protocol/login/Login.h +++ b/src/analyzer/protocol/login/Login.h @@ -55,8 +55,8 @@ protected: int IsEmpty(const char* line) const; void AddUserText(const char* line); // complains on overflow - char* PeekUserText() const; // internal error on underflow - char* PopUserText(); // internal error on underflow + char* PeekUserText(); // internal warning on underflow + char* PopUserText(); // internal warning on underflow Val* PopUserTextVal(); int MatchesTypeahead(const char* line) const; diff --git a/src/analyzer/protocol/login/NVT.cc b/src/analyzer/protocol/login/NVT.cc index e1a3ae05ec..e365306a56 100644 --- a/src/analyzer/protocol/login/NVT.cc +++ b/src/analyzer/protocol/login/NVT.cc @@ -39,8 +39,13 @@ TelnetOption::TelnetOption(NVT_Analyzer* arg_endp, unsigned int arg_code) void TelnetOption::RecvOption(unsigned int type) { TelnetOption* peer = endp->FindPeerOption(code); + if ( ! peer ) - reporter->InternalError("option peer missing in TelnetOption::RecvOption"); + { + reporter->InternalAnalyzerError(endp, + "option peer missing in TelnetOption::RecvOption"); + return; + } // WILL/WONT/DO/DONT are messages we've *received* from our peer. switch ( type ) { @@ -85,7 +90,9 @@ void TelnetOption::RecvOption(unsigned int type) break; default: - reporter->InternalError("bad option type in TelnetOption::RecvOption"); + reporter->InternalAnalyzerError(endp, + "bad option type in TelnetOption::RecvOption"); + return; } } @@ -165,7 +172,11 @@ void TelnetEncryptOption::RecvSubOption(u_char* data, int len) (TelnetEncryptOption*) endp->FindPeerOption(code); if ( ! peer ) - reporter->InternalError("option peer missing in TelnetEncryptOption::RecvSubOption"); + { + reporter->InternalAnalyzerError(endp, + "option peer missing in TelnetEncryptOption::RecvSubOption"); + return; + } if ( peer->DidRequest() || peer->DoingEncryption() || peer->Endpoint()->AuthenticationHasBeenAccepted() ) @@ -201,7 +212,10 @@ void TelnetAuthenticateOption::RecvSubOption(u_char* data, int len) (TelnetAuthenticateOption*) endp->FindPeerOption(code); if ( ! peer ) - reporter->InternalError("option peer missing in TelnetAuthenticateOption::RecvSubOption"); + { + reporter->InternalAnalyzerError(endp, + "option peer missing in TelnetAuthenticateOption::RecvSubOption"); + } if ( ! peer->DidRequestAuthentication() ) InconsistentOption(0); diff --git a/src/analyzer/protocol/login/RSH.cc b/src/analyzer/protocol/login/RSH.cc index 8aebb89116..44fc9f1e5f 100644 --- a/src/analyzer/protocol/login/RSH.cc +++ b/src/analyzer/protocol/login/RSH.cc @@ -131,7 +131,8 @@ void Contents_Rsh_Analyzer::DoDeliver(int len, const u_char* data) break; default: - reporter->InternalError("bad state in Contents_Rsh_Analyzer::DoDeliver"); + reporter->InternalAnalyzerError(this, + "bad state in Contents_Rsh_Analyzer::DoDeliver"); break; } } @@ -186,7 +187,10 @@ void Rsh_Analyzer::DeliverStream(int len, const u_char* data, bool orig) void Rsh_Analyzer::ClientUserName(const char* s) { if ( client_name ) - reporter->InternalError("multiple rsh client names"); + { + reporter->InternalAnalyzerError(this, "multiple rsh client names"); + return; + } client_name = new StringVal(s); } @@ -194,7 +198,11 @@ void Rsh_Analyzer::ClientUserName(const char* s) void Rsh_Analyzer::ServerUserName(const char* s) { if ( username ) - reporter->InternalError("multiple rsh initial client names"); + { + reporter->InternalAnalyzerError(this, + "multiple rsh initial client names"); + return; + } username = new StringVal(s); } diff --git a/src/analyzer/protocol/login/Rlogin.cc b/src/analyzer/protocol/login/Rlogin.cc index 9a31a47aa1..c418047239 100644 --- a/src/analyzer/protocol/login/Rlogin.cc +++ b/src/analyzer/protocol/login/Rlogin.cc @@ -193,7 +193,8 @@ void Contents_Rlogin_Analyzer::DoDeliver(int len, const u_char* data) break; default: - reporter->InternalError("bad state in Contents_Rlogin_Analyzer::DoDeliver"); + reporter->InternalAnalyzerError(this, + "bad state in Contents_Rlogin_Analyzer::DoDeliver"); break; } } @@ -224,7 +225,10 @@ Rlogin_Analyzer::Rlogin_Analyzer(Connection* conn) void Rlogin_Analyzer::ClientUserName(const char* s) { if ( client_name ) - reporter->InternalError("multiple rlogin client names"); + { + reporter->InternalAnalyzerError(this, "multiple rlogin client names"); + return; + } client_name = new StringVal(s); } diff --git a/src/analyzer/protocol/mime/MIME.cc b/src/analyzer/protocol/mime/MIME.cc index c66a0b9be7..8f3df63250 100644 --- a/src/analyzer/protocol/mime/MIME.cc +++ b/src/analyzer/protocol/mime/MIME.cc @@ -557,7 +557,12 @@ void MIME_Entity::init() MIME_Entity::~MIME_Entity() { if ( ! end_of_data ) - reporter->InternalError("EndOfData must be called before delete a MIME_Entity"); + { + // TODO: not sure about this + reporter->InternalWarning( + "missing MIME_Entity::EndOfData() before ~MIME_Entity"); + EndOfData(); + } delete current_header_line; Unref(content_type_str); @@ -1088,8 +1093,6 @@ void MIME_Entity::DecodeBase64(int len, const char* data) rlen = 128; char* prbuf = rbuf; int decoded = base64_decoder->Decode(len, data, &rlen, &prbuf); - if ( prbuf != rbuf ) - reporter->InternalError("buffer pointer modified in base64 decoding"); DataOctets(rlen, rbuf); len -= decoded; data += decoded; } @@ -1098,7 +1101,10 @@ void MIME_Entity::DecodeBase64(int len, const char* data) void MIME_Entity::StartDecodeBase64() { if ( base64_decoder ) - reporter->InternalError("previous Base64 decoder not released!"); + { + reporter->InternalWarning("previous MIME Base64 decoder not released"); + delete base64_decoder; + } base64_decoder = new Base64Converter(message->GetAnalyzer()); } @@ -1114,8 +1120,6 @@ void MIME_Entity::FinishDecodeBase64() if ( base64_decoder->Done(&rlen, &prbuf) ) { // some remaining data - if ( prbuf != rbuf ) - reporter->InternalError("buffer pointer modified in base64 decoding"); if ( rlen > 0 ) DataOctets(rlen, rbuf); } @@ -1390,7 +1394,11 @@ void MIME_Mail::SubmitAllHeaders(MIME_HeaderList& hlist) void MIME_Mail::SubmitData(int len, const char* buf) { if ( buf != (char*) data_buffer->Bytes() + buffer_start ) - reporter->InternalError("buffer misalignment"); + { + reporter->InternalAnalyzerError(GetAnalyzer(), + "MIME buffer misalignment"); + return; + } if ( compute_content_hash ) { @@ -1483,7 +1491,9 @@ void MIME_Mail::SubmitEvent(int event_type, const char* detail) break; default: - reporter->InternalError("unrecognized MIME_Mail event"); + reporter->InternalAnalyzerError(GetAnalyzer(), + "unrecognized MIME_Mail event"); + return; } if ( mime_event ) diff --git a/src/analyzer/protocol/mime/MIME.h b/src/analyzer/protocol/mime/MIME.h index 8d83609cc1..e337d24f13 100644 --- a/src/analyzer/protocol/mime/MIME.h +++ b/src/analyzer/protocol/mime/MIME.h @@ -193,7 +193,8 @@ public: virtual ~MIME_Message() { if ( ! finished ) - reporter->InternalError("Done() must be called before destruction"); + reporter->InternalAnalyzerError(analyzer, + "missing MIME_Message::Done() call"); } virtual void Done() { finished = 1; } diff --git a/src/analyzer/protocol/pop3/POP3.cc b/src/analyzer/protocol/pop3/POP3.cc index 52f9eb8445..8164b927a8 100644 --- a/src/analyzer/protocol/pop3/POP3.cc +++ b/src/analyzer/protocol/pop3/POP3.cc @@ -209,7 +209,9 @@ void POP3_Analyzer::ProcessRequest(int length, const char* line) break; default: - reporter->InternalError("unexpected authorization state"); + reporter->InternalAnalyzerError(this, + "unexpected POP3 authorization state"); + return; } delete decoded; @@ -565,7 +567,8 @@ void POP3_Analyzer::ProcessClientCmd() break; default: - reporter->InternalError("command not known"); + reporter->InternalAnalyzerError(this, "unknown POP3 command"); + return; } } diff --git a/src/analyzer/protocol/rpc/RPC.cc b/src/analyzer/protocol/rpc/RPC.cc index fd76bf551b..1c4cc46249 100644 --- a/src/analyzer/protocol/rpc/RPC.cc +++ b/src/analyzer/protocol/rpc/RPC.cc @@ -265,7 +265,10 @@ int RPC_Interpreter::DeliverRPC(const u_char* buf, int n, int rpclen, } else if ( n < 0 ) - reporter->InternalError("RPC underflow"); + { + reporter->InternalAnalyzerError(analyzer, "RPC underflow"); + return 0; + } return 1; } @@ -474,8 +477,12 @@ bool Contents_RPC::CheckResync(int& len, const u_char*& data, bool orig) } if ( resync_toskip != 0 ) + { // Should never happen. - reporter->InternalError("RPC resync: skipping over data failed"); + reporter->InternalAnalyzerError(this, + "RPC resync: skipping over data failed"); + return false; + } // Now lets see whether data points to the beginning of a RPC // frame. If the resync processs is successful, we should be @@ -625,7 +632,11 @@ void Contents_RPC::DeliverStream(int len, const u_char* data, bool orig) marker_buf.Init(4,4); if ( ! dummy_p ) - reporter->InternalError("inconsistent RPC record marker extraction"); + { + reporter->InternalAnalyzerError(this, + "inconsistent RPC record marker extraction"); + return; + } last_frag = (marker & 0x80000000) != 0; marker &= 0x7fffffff; diff --git a/src/analyzer/protocol/smb/SMB.cc b/src/analyzer/protocol/smb/SMB.cc index 4393626217..12b3fc4287 100644 --- a/src/analyzer/protocol/smb/SMB.cc +++ b/src/analyzer/protocol/smb/SMB.cc @@ -738,7 +738,9 @@ int SMB_Session::ParseTransaction(int is_orig, int cmd, break; default: - reporter->InternalError("command mismatch for ParseTransaction"); + reporter->InternalAnalyzerError(analyzer, + "command mismatch for SMB_Session::ParseTransaction"); + return 0; } int ret; diff --git a/src/analyzer/protocol/tcp/ContentLine.cc b/src/analyzer/protocol/tcp/ContentLine.cc index 2a810c5dd1..44051ee1d1 100644 --- a/src/analyzer/protocol/tcp/ContentLine.cc +++ b/src/analyzer/protocol/tcp/ContentLine.cc @@ -102,9 +102,6 @@ void ContentLine_Analyzer::DeliverStream(int len, const u_char* data, delete [] buf; buf = tmp; - - if ( ! buf ) - reporter->InternalError("out of memory delivering endpoint line"); } DoDeliver(len, data); @@ -126,7 +123,11 @@ void ContentLine_Analyzer::EndpointEOF(bool is_orig) void ContentLine_Analyzer::SetPlainDelivery(int64_t length) { if ( length < 0 ) - reporter->InternalError("negative length for plain delivery"); + { + reporter->InternalAnalyzerError(this, + "negative length for plain delivery"); + return; + } plain_delivery_length = length; } diff --git a/src/analyzer/protocol/tcp/TCP.cc b/src/analyzer/protocol/tcp/TCP.cc index 0c06d7fbd7..ca7fe073e8 100644 --- a/src/analyzer/protocol/tcp/TCP.cc +++ b/src/analyzer/protocol/tcp/TCP.cc @@ -1580,7 +1580,8 @@ BroFile* TCP_Analyzer::GetContentsFile(unsigned int direction) const default: break; } - reporter->InternalError("inconsistency in TCP_Analyzer::GetContentsFile"); + reporter->Error("bad direction %u in TCP_Analyzer::GetContentsFile", + direction); return 0; } diff --git a/src/analyzer/protocol/tcp/TCP_Endpoint.cc b/src/analyzer/protocol/tcp/TCP_Endpoint.cc index 144d4598dd..d596234021 100644 --- a/src/analyzer/protocol/tcp/TCP_Endpoint.cc +++ b/src/analyzer/protocol/tcp/TCP_Endpoint.cc @@ -34,10 +34,8 @@ TCP_Endpoint::TCP_Endpoint(TCP_Analyzer* arg_analyzer, int arg_is_orig) hist_last_SYN = hist_last_FIN = hist_last_RST = 0; - src_addr = is_orig ? tcp_analyzer->Conn()->RespAddr() : - tcp_analyzer->Conn()->OrigAddr(); - dst_addr = is_orig ? tcp_analyzer->Conn()->OrigAddr() : - tcp_analyzer->Conn()->RespAddr(); + src_addr = is_orig ? Conn()->RespAddr() : Conn()->OrigAddr(); + dst_addr = is_orig ? Conn()->OrigAddr() : Conn()->RespAddr(); checksum_base = ones_complement_checksum(src_addr, 0); checksum_base = ones_complement_checksum(dst_addr, checksum_base); @@ -54,6 +52,11 @@ TCP_Endpoint::~TCP_Endpoint() Unref(contents_file); } +Connection* TCP_Endpoint::Conn() const + { + return tcp_analyzer->Conn(); + } + void TCP_Endpoint::Done() { if ( contents_processor ) @@ -143,7 +146,7 @@ void TCP_Endpoint::SetState(EndpointState new_state) // handshake. if ( ! is_handshake(new_state) ) if ( is_handshake(state) && is_handshake(peer->state) ) - tcp_analyzer->Conn()->SetInactivityTimeout(tcp_inactivity_timeout); + Conn()->SetInactivityTimeout(tcp_inactivity_timeout); prev_state = state; state = new_state; @@ -207,8 +210,20 @@ int TCP_Endpoint::DataSent(double t, int seq, int len, int caplen, FILE* f = contents_file->Seek(seq - contents_start_seq); if ( fwrite(data, 1, len, f) < unsigned(len) ) - // ### this should really generate an event - reporter->InternalError("contents write failed"); + { + char buf[256]; + strerror_r(errno, buf, sizeof(buf)); + reporter->Error("TCP contents write failed: %s", buf); + + if ( contents_file_write_failure ) + { + val_list* vl = new val_list(); + vl->append(Conn()->BuildConnVal()); + vl->append(new Val(IsOrig(), TYPE_BOOL)); + vl->append(new StringVal(buf)); + tcp_analyzer->ConnectionEvent(contents_file_write_failure, vl); + } + } } return status; @@ -241,7 +256,7 @@ int TCP_Endpoint::CheckHistory(uint32 mask, char code) code = tolower(code); } - return tcp_analyzer->Conn()->CheckHistory(mask, code); + return Conn()->CheckHistory(mask, code); } void TCP_Endpoint::AddHistory(char code) @@ -249,6 +264,6 @@ void TCP_Endpoint::AddHistory(char code) if ( ! IsOrig() ) code = tolower(code); - tcp_analyzer->Conn()->AddHistory(code); + Conn()->AddHistory(code); } diff --git a/src/analyzer/protocol/tcp/TCP_Reassembler.cc b/src/analyzer/protocol/tcp/TCP_Reassembler.cc index 949be9f599..a1e20dc0e6 100644 --- a/src/analyzer/protocol/tcp/TCP_Reassembler.cc +++ b/src/analyzer/protocol/tcp/TCP_Reassembler.cc @@ -174,7 +174,8 @@ void TCP_Reassembler::Undelivered(int up_to_seq) } if ( seq_delta(up_to_seq, last_reassem_seq) <= 0 ) - // This should never happen. + // This should never happen. (Reassembler::TrimToSeq has the only call + // to this method and only if this condition is not true). reporter->InternalError("Calling Undelivered for data that has already been delivered (or has already been marked as undelivered"); if ( last_reassem_seq == 1 && @@ -322,17 +323,36 @@ void TCP_Reassembler::RecordToSeq(int start_seq, int stop_seq, BroFile* f) void TCP_Reassembler::RecordBlock(DataBlock* b, BroFile* f) { - unsigned int len = b->Size(); - if ( ! f->Write((const char*) b->block, len) ) - // ### this should really generate an event - reporter->InternalError("contents write failed"); + if ( f->Write((const char*) b->block, b->Size()) ) + return; + + reporter->Error("TCP_Reassembler contents write failed"); + + if ( contents_file_write_failure ) + { + val_list* vl = new val_list(); + vl->append(Endpoint()->Conn()->BuildConnVal()); + vl->append(new Val(IsOrig(), TYPE_BOOL)); + vl->append(new StringVal("TCP reassembler content write failure")); + tcp_analyzer->ConnectionEvent(contents_file_write_failure, vl); + } } void TCP_Reassembler::RecordGap(int start_seq, int upper_seq, BroFile* f) { - if ( ! f->Write(fmt("\n<>\n", seq_delta(upper_seq, start_seq))) ) - // ### this should really generate an event - reporter->InternalError("contents gap write failed"); + if ( f->Write(fmt("\n<>\n", seq_delta(upper_seq, start_seq))) ) + return; + + reporter->Error("TCP_Reassembler contents gap write failed"); + + if ( contents_file_write_failure ) + { + val_list* vl = new val_list(); + vl->append(Endpoint()->Conn()->BuildConnVal()); + vl->append(new Val(IsOrig(), TYPE_BOOL)); + vl->append(new StringVal("TCP reassembler gap write failure")); + tcp_analyzer->ConnectionEvent(contents_file_write_failure, vl); + } } void TCP_Reassembler::BlockInserted(DataBlock* start_block) diff --git a/src/analyzer/protocol/tcp/events.bif b/src/analyzer/protocol/tcp/events.bif index af61783ac4..216691dea1 100644 --- a/src/analyzer/protocol/tcp/events.bif +++ b/src/analyzer/protocol/tcp/events.bif @@ -287,3 +287,13 @@ event tcp_contents%(c: connection, is_orig: bool, seq: count, contents: string%) ## TODO. event tcp_rexmit%(c: connection, is_orig: bool, seq: count, len: count, data_in_flight: count, window: count%); +## Generated when failing to write contents of a TCP stream to a file. +## +## c: The connection whose contents are being recorded. +## +## is_orig: Which side of the connection encountered a failure to write. +## +## msg: A reason or description for the failure. +## +## .. bro:see:: set_contents_file get_contents_file +event contents_file_write_failure%(c: connection, is_orig: bool, msg: string%); diff --git a/src/analyzer/protocol/tcp/functions.bif b/src/analyzer/protocol/tcp/functions.bif index ff812b80ee..f5b0033ae8 100644 --- a/src/analyzer/protocol/tcp/functions.bif +++ b/src/analyzer/protocol/tcp/functions.bif @@ -114,7 +114,7 @@ function get_gap_summary%(%): gap_info ## missing data; this can happen, e.g., due to an ## :bro:id:`ack_above_hole` event. ## -## .. bro:see:: get_contents_file set_record_packets +## .. bro:see:: get_contents_file set_record_packets contents_file_write_failure function set_contents_file%(cid: conn_id, direction: count, f: file%): bool %{ Connection* c = sessions->FindConnection(cid); @@ -137,7 +137,7 @@ function set_contents_file%(cid: conn_id, direction: count, f: file%): bool ## but there is no contents file for *direction*, then the function ## generates an error and returns a file handle to ``stderr``. ## -## .. bro:see:: set_contents_file set_record_packets +## .. bro:see:: set_contents_file set_record_packets contents_file_write_failure function get_contents_file%(cid: conn_id, direction: count%): file %{ Connection* c = sessions->FindConnection(cid); diff --git a/src/file_analysis/Manager.cc b/src/file_analysis/Manager.cc index 8dfb220381..0337dbb098 100644 --- a/src/file_analysis/Manager.cc +++ b/src/file_analysis/Manager.cc @@ -362,12 +362,19 @@ Analyzer* Manager::InstantiateAnalyzer(Tag tag, RecordVal* args, File* f) const Component* c = Lookup(tag); if ( ! c ) - reporter->InternalError("cannot instantiate unknown file analyzer: %s", - tag.AsString().c_str()); + { + reporter->InternalWarning( + "unknown file analyzer instantiation request: %s", + tag.AsString().c_str()); + return 0; + } if ( ! c->Factory() ) - reporter->InternalError("file analyzer %s cannot be instantiated " + { + reporter->InternalWarning("file analyzer %s cannot be instantiated " "dynamically", c->CanonicalName()); + return 0; + } return c->Factory()(args, f); } diff --git a/src/input/Manager.cc b/src/input/Manager.cc index cd02db06af..1f0201393f 100644 --- a/src/input/Manager.cc +++ b/src/input/Manager.cc @@ -42,6 +42,13 @@ ReaderDefinition input_readers[] = { { BifEnum::Input::READER_DEFAULT, "None", 0, (ReaderBackend* (*)(ReaderFrontend* frontend))0 } }; +static void delete_value_ptr_array(Value** vals, int num_fields) + { + for ( int i = 0; i < num_fields; ++i ) + delete vals[i]; + delete [] vals; + } + /** * InputHashes are used as Dictionaries to store the value and index hashes * for all lines currently stored in a table. Index hash is stored as @@ -330,7 +337,9 @@ bool Manager::CreateStream(Stream* info, RecordVal* description) break; default: - reporter->InternalError("unknown reader mode"); + reporter->InternalWarning("unknown input reader mode"); + Unref(mode); + return false; } Unref(mode); @@ -1014,7 +1023,8 @@ void Manager::SendEntry(ReaderFrontend* reader, Value* *vals) Stream *i = FindStream(reader); if ( i == 0 ) { - reporter->InternalError("Unknown reader in SendEntry"); + reporter->InternalWarning("Unknown reader %s in SendEntry", + reader->Name()); return; } @@ -1041,10 +1051,7 @@ void Manager::SendEntry(ReaderFrontend* reader, Value* *vals) else assert(false); - for ( int i = 0; i < readFields; i++ ) - delete vals[i]; - - delete [] vals; + delete_value_ptr_array(vals, readFields); } int Manager::SendEntryTable(Stream* i, const Value* const *vals) @@ -1242,7 +1249,8 @@ void Manager::EndCurrentSend(ReaderFrontend* reader) if ( i == 0 ) { - reporter->InternalError("Unknown reader in EndCurrentSend"); + reporter->InternalWarning("Unknown reader %s in EndCurrentSend", + reader->Name()); return; } @@ -1352,7 +1360,8 @@ void Manager::SendEndOfData(ReaderFrontend* reader) if ( i == 0 ) { - reporter->InternalError("Unknown reader in SendEndOfData"); + reporter->InternalWarning("Unknown reader %s in SendEndOfData", + reader->Name()); return; } @@ -1378,7 +1387,7 @@ void Manager::Put(ReaderFrontend* reader, Value* *vals) Stream *i = FindStream(reader); if ( i == 0 ) { - reporter->InternalError("Unknown reader in Put"); + reporter->InternalWarning("Unknown reader %s in Put", reader->Name()); return; } @@ -1410,10 +1419,7 @@ void Manager::Put(ReaderFrontend* reader, Value* *vals) else assert(false); - for ( int i = 0; i < readFields; i++ ) - delete vals[i]; - - delete [] vals; + delete_value_ptr_array(vals, readFields); } int Manager::SendEventStreamEvent(Stream* i, EnumVal* type, const Value* const *vals) @@ -1585,7 +1591,8 @@ void Manager::Clear(ReaderFrontend* reader) Stream *i = FindStream(reader); if ( i == 0 ) { - reporter->InternalError("Unknown reader in Clear"); + reporter->InternalWarning("Unknown reader %s in Clear", + reader->Name()); return; } @@ -1606,7 +1613,7 @@ bool Manager::Delete(ReaderFrontend* reader, Value* *vals) Stream *i = FindStream(reader); if ( i == 0 ) { - reporter->InternalError("Unknown reader in Delete"); + reporter->InternalWarning("Unknown reader %s in Delete", reader->Name()); return false; } @@ -1686,11 +1693,7 @@ bool Manager::Delete(ReaderFrontend* reader, Value* *vals) return false; } - for ( int i = 0; i < readVals; i++ ) - delete vals[i]; - - delete [] vals; - + delete_value_ptr_array(vals, readVals); return success; } @@ -1722,6 +1725,7 @@ bool Manager::SendEvent(const string& name, const int num_vals, Value* *vals) if ( handler == 0 ) { reporter->Error("Event %s not found", name.c_str()); + delete_value_ptr_array(vals, num_vals); return false; } @@ -1735,6 +1739,7 @@ bool Manager::SendEvent(const string& name, const int num_vals, Value* *vals) if ( num_vals != num_event_vals ) { reporter->Error("Wrong number of values for event %s", name.c_str()); + delete_value_ptr_array(vals, num_vals); return false; } @@ -1744,11 +1749,7 @@ bool Manager::SendEvent(const string& name, const int num_vals, Value* *vals) mgr.QueueEvent(handler, vl, SOURCE_LOCAL); - for ( int i = 0; i < num_vals; i++ ) - delete vals[i]; - - delete [] vals; - + delete_value_ptr_array(vals, num_vals); return true; } @@ -1794,12 +1795,6 @@ RecordVal* Manager::ListValToRecordVal(ListVal* list, RecordType *request_type, { assert(position != 0 ); // we need the pointer to point to data; - if ( request_type->Tag() != TYPE_RECORD ) - { - reporter->InternalError("ListValToRecordVal called on non-record-value."); - return 0; - } - RecordVal* rec = new RecordVal(request_type->AsRecordType()); assert(list != 0); @@ -1830,12 +1825,6 @@ RecordVal* Manager::ValueToRecordVal(const Value* const *vals, { assert(position != 0); // we need the pointer to point to data. - if ( request_type->Tag() != TYPE_RECORD ) - { - reporter->InternalError("ValueToRecordVal called on non-record-value."); - return 0; - } - RecordVal* rec = new RecordVal(request_type->AsRecordType()); for ( int i = 0; i < request_type->NumFields(); i++ ) { diff --git a/src/input/readers/Binary.cc b/src/input/readers/Binary.cc index e49e6b7407..96a9028f7b 100644 --- a/src/input/readers/Binary.cc +++ b/src/input/readers/Binary.cc @@ -53,8 +53,8 @@ bool Binary::CloseInput() { if ( ! in || ! in->is_open() ) { - InternalError(Fmt("Trying to close closed file for stream %s", - fname.c_str())); + InternalWarning(Fmt("Trying to close closed file for stream %s", + fname.c_str())); return false; } diff --git a/src/input/readers/Raw.cc b/src/input/readers/Raw.cc index de0a264ec4..1c02339748 100644 --- a/src/input/readers/Raw.cc +++ b/src/input/readers/Raw.cc @@ -299,7 +299,8 @@ bool Raw::CloseInput() { if ( file == 0 ) { - InternalError(Fmt("Trying to close closed file for stream %s", fname.c_str())); + InternalWarning(Fmt("Trying to close closed file for stream %s", + fname.c_str())); return false; } #ifdef DEBUG @@ -492,9 +493,6 @@ int64_t Raw::GetLine(FILE* arg_file) Error(Fmt("Reader encountered unexpected error code %d", errno)); return -3; } - - InternalError("Internal control flow execution error in raw reader"); - assert(false); } // write to the stdin of the child process diff --git a/src/plugin/Component.cc b/src/plugin/Component.cc index 074f00c264..ee18d55cdf 100644 --- a/src/plugin/Component.cc +++ b/src/plugin/Component.cc @@ -44,7 +44,9 @@ void Component::Describe(ODesc* d) const break; default: - reporter->InternalError("unknown component type in plugin::Component::Describe"); + reporter->InternalWarning("unknown component type in plugin::Component::Describe"); + d->Add(""); + break; } d->Add("]"); diff --git a/src/plugin/ComponentManager.h b/src/plugin/ComponentManager.h index 16f9d80743..ccc076db28 100644 --- a/src/plugin/ComponentManager.h +++ b/src/plugin/ComponentManager.h @@ -169,11 +169,12 @@ const char* ComponentManager::GetComponentName(T tag) const C* c = Lookup(tag); - if ( ! c ) - reporter->InternalError("request for name of unknown component tag %s", - tag.AsString().c_str()); + if ( c ) + return c->CanonicalName(); - return c->CanonicalName(); + reporter->InternalWarning("requested name of unknown component tag %s", + tag.AsString().c_str()); + return error; } template diff --git a/src/threading/SerialTypes.cc b/src/threading/SerialTypes.cc index c0e26ccb32..6c9ddb8f30 100644 --- a/src/threading/SerialTypes.cc +++ b/src/threading/SerialTypes.cc @@ -294,7 +294,9 @@ bool Value::Read(SerializationFormat* fmt) } default: - reporter->InternalError("unsupported type %s in Value::Write", type_name(type)); + reporter->InternalError("unsupported type %s in Value::Read", + type_name(type)); + return false; } return false; @@ -398,7 +400,9 @@ bool Value::Write(SerializationFormat* fmt) const } default: - reporter->InternalError("unsupported type %s in Value::REad", type_name(type)); + reporter->InternalError("unsupported type %s in Value::Write", + type_name(type)); + return false; } return false; diff --git a/src/util.cc b/src/util.cc index a2a21c7bc6..bcc9b0bb27 100644 --- a/src/util.cc +++ b/src/util.cc @@ -345,9 +345,9 @@ unsigned char encode_hex(int h) '9', 'A', 'B', 'C', 'D', 'E', 'F' }; - if ( h < 0 || h >= 16 ) + if ( h < 0 || h > 15 ) { - reporter->InternalError("illegal value for encode_hex: %d", h); + reporter->InternalWarning("illegal value for encode_hex: %d", h); return 'X'; } From d6855dc4eb5baaf3b57178faa10efff8ccd44923 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 11 Oct 2013 14:47:25 -0700 Subject: [PATCH 02/11] Pass over the InternalError() changes. --- aux/broctl | 2 +- src/Conn.cc | 19 ++++++++----------- src/Desc.cc | 11 +++-------- src/Event.cc | 5 +---- src/File.cc | 22 +++++----------------- src/Frag.cc | 2 ++ src/IP.h | 8 ++++++++ src/Reassem.cc | 1 - src/RemoteSerializer.cc | 2 +- src/Reporter.h | 2 +- src/analyzer/protocol/mime/MIME.cc | 6 +----- src/analyzer/protocol/tcp/TCP.cc | 2 +- src/input/Manager.cc | 1 + src/threading/SerialTypes.cc | 2 -- 14 files changed, 33 insertions(+), 52 deletions(-) diff --git a/aux/broctl b/aux/broctl index 7ca51fc46c..81fa2d664a 160000 --- a/aux/broctl +++ b/aux/broctl @@ -1 +1 @@ -Subproject commit 7ca51fc46c5c3dd4f3d803e5c617c2e35129fb05 +Subproject commit 81fa2d664a7ef7306a03928484b10611fbe893b8 diff --git a/src/Conn.cc b/src/Conn.cc index d0a932e373..0098d297e0 100644 --- a/src/Conn.cc +++ b/src/Conn.cc @@ -27,6 +27,9 @@ void ConnectionTimer::Init(Connection* arg_conn, timer_func arg_timer, ConnectionTimer::~ConnectionTimer() { + if ( conn->RefCnt() < 1 ) + reporter->InternalError("reference count inconsistency in ~ConnectionTimer"); + conn->RemoveTimer(this); Unref(conn); } @@ -41,6 +44,9 @@ void ConnectionTimer::Dispatch(double t, int is_expire) conn->RemoveTimer(this); (conn->*timer)(t); + + if ( conn->RefCnt() < 1 ) + reporter->InternalError("reference count inconsistency in ConnectionTimer::Dispatch"); } IMPLEMENT_SERIAL(ConnectionTimer, SER_CONNECTION_TIMER); @@ -62,10 +68,7 @@ bool ConnectionTimer::DoSerialize(SerialInfo* info) const else if ( timer == timer_func(&Connection::RemoveConnectionTimer) ) type = 4; else - { - reporter->InternalWarning("unknown function in ConnectionTimer::DoSerialize()"); - return false; - } + reporter->InternalError("unknown function in ConnectionTimer::DoSerialize()"); return conn->Serialize(info) && SERIALIZE(type) && SERIALIZE(do_expire); } @@ -177,12 +180,7 @@ Connection::Connection(NetSessions* s, HashKey* k, double t, const ConnID* id, Connection::~Connection() { if ( ! finished ) - { - // TODO: not sure about this - reporter->InternalWarning( - "missing Connection::Done() before ~Connection"); - Done(); - } + reporter->InternalError("Done() not called before destruction of Connection"); CancelTimers(); @@ -793,7 +791,6 @@ void Connection::Describe(ODesc* d) const default: reporter->InternalError( "unhandled transport type in Connection::Describe"); - break; } d->SP(); diff --git a/src/Desc.cc b/src/Desc.cc index 2a44a65fdf..62c6130f40 100644 --- a/src/Desc.cc +++ b/src/Desc.cc @@ -65,20 +65,15 @@ void ODesc::PushIndent() void ODesc::PopIndent() { if ( --indent_level < 0 ) - { - indent_level = 0; - reporter->InternalWarning("ODesc::PopIndent underflow"); - } + reporter->InternalError("ODesc::PopIndent underflow"); + NL(); } void ODesc::PopIndentNoNL() { if ( --indent_level < 0 ) - { - indent_level = 0; - reporter->InternalWarning("ODesc::PopIndent underflow"); - } + reporter->InternalError("ODesc::PopIndent underflow"); } void ODesc::Add(const char* s, int do_indent) diff --git a/src/Event.cc b/src/Event.cc index 980180def6..0fb76d10bc 100644 --- a/src/Event.cc +++ b/src/Event.cc @@ -91,10 +91,7 @@ void EventMgr::QueueEvent(Event* event) void EventMgr::Dispatch() { if ( ! head ) - { - reporter->InternalWarning("EventMgr::Dispatch underflow"); - return; - } + reporter->InternalError("EventMgr::Dispatch underflow"); Event* current = head; diff --git a/src/File.cc b/src/File.cc index d28d0ca569..bf6a7e7f51 100644 --- a/src/File.cc +++ b/src/File.cc @@ -284,10 +284,7 @@ FILE* BroFile::BringIntoCache() char buf[256]; if ( f ) - { - reporter->InternalWarning("BroFile non-nil non-open file"); - return 0; - } + reporter->InternalError("BroFile non-nil non-open file"); if ( num_files_in_cache >= max_files_in_cache ) PurgeCache(); @@ -313,7 +310,7 @@ FILE* BroFile::BringIntoCache() } strerror_r(errno, buf, sizeof(buf)); - reporter->InternalWarning("can't open /dev/null: %s", buf); + reporter->Error("can't open /dev/null: %s", buf); return 0; } @@ -399,24 +396,15 @@ int BroFile::Close() void BroFile::Suspend() { if ( ! is_in_cache ) - { - reporter->InternalWarning("BroFile::Suspend() called for non-cached file"); - return; - } + reporter->InternalError("BroFile::Suspend() called for non-cached file"); if ( ! is_open ) - { - reporter->InternalWarning("BroFile::Suspend() called for non-open file"); - return; - } + reporter->InternalError("BroFile::Suspend() called for non-open file"); Unlink(); if ( ! f ) - { - reporter->InternalWarning("BroFile::Suspend() called for nil file"); - return; - } + reporter->InternalError("BroFile::Suspend() called for nil file"); if ( (position = ftell(f)) < 0 ) { diff --git a/src/Frag.cc b/src/Frag.cc index 5cfff270c7..199af78ca9 100644 --- a/src/Frag.cc +++ b/src/Frag.cc @@ -164,11 +164,13 @@ void FragReassembler::Weird(const char* name) const IP_Hdr hdr((const ip*)proto_hdr, false); s->Weird(name, &hdr); } + else if ( version == 6 ) { IP_Hdr hdr((const ip6_hdr*)proto_hdr, false, proto_hdr_len); s->Weird(name, &hdr); } + else { reporter->InternalWarning("Unexpected IP version in FragReassembler"); diff --git a/src/IP.h b/src/IP.h index fd7a6de266..fb936df1bc 100644 --- a/src/IP.h +++ b/src/IP.h @@ -182,6 +182,7 @@ public: reporter->InternalWarning("empty IPv6 header chain"); return false; } + return chain[chain.size()-1]->Type() == IPPROTO_FRAGMENT; } @@ -229,6 +230,7 @@ public: reporter->InternalWarning("empty IPv6 header chain"); return IPAddr(); } + return IPAddr(((const struct ip6_hdr*)(chain[0]->Data()))->ip6_src); } @@ -241,11 +243,13 @@ public: { if ( finalDst ) return IPAddr(*finalDst); + if ( chain.empty() ) { reporter->InternalWarning("empty IPv6 header chain"); return IPAddr(); } + return IPAddr(((const struct ip6_hdr*)(chain[0]->Data()))->ip6_dst); } @@ -461,9 +465,11 @@ public: { if ( ip4 ) return IPPROTO_RAW; + size_t i = ip6_hdrs->Size(); if ( i > 0 ) return (*ip6_hdrs)[i-1]->Type(); + return IPPROTO_NONE; } @@ -476,9 +482,11 @@ public: { if ( ip4 ) return ip4->ip_p; + size_t i = ip6_hdrs->Size(); if ( i > 0 ) return (*ip6_hdrs)[i-1]->NextHdr(); + return IPPROTO_NONE; } diff --git a/src/Reassem.cc b/src/Reassem.cc index df7e3568ee..19beaa0a16 100644 --- a/src/Reassem.cc +++ b/src/Reassem.cc @@ -19,7 +19,6 @@ DataBlock::DataBlock(const u_char* data, int size, int arg_seq, { seq = arg_seq; upper = seq + size; - block = new u_char[size]; memcpy((void*) block, (const void*) data, size); diff --git a/src/RemoteSerializer.cc b/src/RemoteSerializer.cc index bcf4b090b0..7d9c6163a0 100644 --- a/src/RemoteSerializer.cc +++ b/src/RemoteSerializer.cc @@ -3607,9 +3607,9 @@ bool SocketComm::ProcessParentMessage() default: InternalError("unknown msgstate"); - return false; } + // Cannot be reached. return false; } diff --git a/src/Reporter.h b/src/Reporter.h index 79eb21c1de..82e6cb575f 100644 --- a/src/Reporter.h +++ b/src/Reporter.h @@ -92,7 +92,7 @@ public: // dump after the message has been reported. void InternalError(const char* fmt, ...) FMT_ATTR; - // Reporter an internal analyzer error. That analyzer will not process + // Report an internal analyzer error. That analyzer will not process // any further input, but Bro otherwise continues normally. void InternalAnalyzerError(analyzer::Analyzer* a, const char* fmt, ...); diff --git a/src/analyzer/protocol/mime/MIME.cc b/src/analyzer/protocol/mime/MIME.cc index 8f3df63250..2128453b4c 100644 --- a/src/analyzer/protocol/mime/MIME.cc +++ b/src/analyzer/protocol/mime/MIME.cc @@ -557,12 +557,8 @@ void MIME_Entity::init() MIME_Entity::~MIME_Entity() { if ( ! end_of_data ) - { - // TODO: not sure about this - reporter->InternalWarning( + reporter->AnalyzerError(message ? message->GetAnalyzer() : 0, "missing MIME_Entity::EndOfData() before ~MIME_Entity"); - EndOfData(); - } delete current_header_line; Unref(content_type_str); diff --git a/src/analyzer/protocol/tcp/TCP.cc b/src/analyzer/protocol/tcp/TCP.cc index ca7fe073e8..cbf983b70a 100644 --- a/src/analyzer/protocol/tcp/TCP.cc +++ b/src/analyzer/protocol/tcp/TCP.cc @@ -1580,7 +1580,7 @@ BroFile* TCP_Analyzer::GetContentsFile(unsigned int direction) const default: break; } - reporter->Error("bad direction %u in TCP_Analyzer::GetContentsFile", + reporter->InternalWarning("bad direction %u in TCP_Analyzer::GetContentsFile", direction); return 0; } diff --git a/src/input/Manager.cc b/src/input/Manager.cc index 1f0201393f..a05eb59e99 100644 --- a/src/input/Manager.cc +++ b/src/input/Manager.cc @@ -46,6 +46,7 @@ static void delete_value_ptr_array(Value** vals, int num_fields) { for ( int i = 0; i < num_fields; ++i ) delete vals[i]; + delete [] vals; } diff --git a/src/threading/SerialTypes.cc b/src/threading/SerialTypes.cc index 6c9ddb8f30..a4f76a9b2d 100644 --- a/src/threading/SerialTypes.cc +++ b/src/threading/SerialTypes.cc @@ -296,7 +296,6 @@ bool Value::Read(SerializationFormat* fmt) default: reporter->InternalError("unsupported type %s in Value::Read", type_name(type)); - return false; } return false; @@ -402,7 +401,6 @@ bool Value::Write(SerializationFormat* fmt) const default: reporter->InternalError("unsupported type %s in Value::Write", type_name(type)); - return false; } return false; From 8e18f9d59e1f848f51cd8f58bf27756e3028173c Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 11 Oct 2013 14:49:56 -0700 Subject: [PATCH 03/11] Renaming InternalAnalyzerError to AnalyzerError. This is to avoid confusion when we abort and when not: InternalError() aborts; AnalyzerError() does not. --- src/Reporter.cc | 2 +- src/Reporter.h | 4 ++-- src/analyzer/protocol/http/HTTP.cc | 6 +++--- src/analyzer/protocol/icmp/ICMP.cc | 4 ++-- src/analyzer/protocol/login/Login.cc | 4 ++-- src/analyzer/protocol/login/NVT.cc | 8 ++++---- src/analyzer/protocol/login/RSH.cc | 6 +++--- src/analyzer/protocol/login/Rlogin.cc | 4 ++-- src/analyzer/protocol/mime/MIME.cc | 4 ++-- src/analyzer/protocol/mime/MIME.h | 2 +- src/analyzer/protocol/pop3/POP3.cc | 4 ++-- src/analyzer/protocol/rpc/RPC.cc | 6 +++--- src/analyzer/protocol/smb/SMB.cc | 2 +- src/analyzer/protocol/tcp/ContentLine.cc | 2 +- 14 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/Reporter.cc b/src/Reporter.cc index 80907e5696..9002633b10 100644 --- a/src/Reporter.cc +++ b/src/Reporter.cc @@ -137,7 +137,7 @@ void Reporter::InternalError(const char* fmt, ...) abort(); } -void Reporter::InternalAnalyzerError(analyzer::Analyzer* a, const char* fmt, +void Reporter::AnalyzerError(analyzer::Analyzer* a, const char* fmt, ...) { if ( a ) diff --git a/src/Reporter.h b/src/Reporter.h index 82e6cb575f..e477ad8934 100644 --- a/src/Reporter.h +++ b/src/Reporter.h @@ -92,9 +92,9 @@ public: // dump after the message has been reported. void InternalError(const char* fmt, ...) FMT_ATTR; - // Report an internal analyzer error. That analyzer will not process + // Report an analyzer error. That analyzer will be set to not process // any further input, but Bro otherwise continues normally. - void InternalAnalyzerError(analyzer::Analyzer* a, const char* fmt, ...); + void AnalyzerError(analyzer::Analyzer* a, const char* fmt, ...); // Toggle whether non-fatal messages should be reported through the // scripting layer rather on standard output. Fatal errors are always diff --git a/src/analyzer/protocol/http/HTTP.cc b/src/analyzer/protocol/http/HTTP.cc index 5544bac2f6..ffdcad226f 100644 --- a/src/analyzer/protocol/http/HTTP.cc +++ b/src/analyzer/protocol/http/HTTP.cc @@ -699,7 +699,7 @@ void HTTP_Message::SubmitData(int len, const char* buf) if ( buf != (const char*) data_buffer->Bytes() + buffer_offset || buffer_offset + len > buffer_size ) { - reporter->InternalAnalyzerError(MyHTTP_Analyzer(), + reporter->AnalyzerError(MyHTTP_Analyzer(), "HTTP message buffer misalignment"); return; } @@ -747,7 +747,7 @@ void HTTP_Message::SubmitEvent(int event_type, const char* detail) break; default: - reporter->InternalAnalyzerError(MyHTTP_Analyzer(), + reporter->AnalyzerError(MyHTTP_Analyzer(), "unrecognized HTTP message event"); return; } @@ -1239,7 +1239,7 @@ int HTTP_Analyzer::HTTP_RequestLine(const char* line, const char* end_of_line) if ( ! ParseRequest(rest, end_of_line) ) { - reporter->InternalAnalyzerError(this, "HTTP ParseRequest failed"); + reporter->AnalyzerError(this, "HTTP ParseRequest failed"); return -1; } diff --git a/src/analyzer/protocol/icmp/ICMP.cc b/src/analyzer/protocol/icmp/ICMP.cc index 6aa535fe95..510d3b3a80 100644 --- a/src/analyzer/protocol/icmp/ICMP.cc +++ b/src/analyzer/protocol/icmp/ICMP.cc @@ -60,7 +60,7 @@ void ICMP_Analyzer::DeliverPacket(int len, const u_char* data, break; default: - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "unexpected IP proto in ICMP analyzer: %d", ip->NextProto()); return; } @@ -100,7 +100,7 @@ void ICMP_Analyzer::DeliverPacket(int len, const u_char* data, NextICMP6(current_timestamp, icmpp, len, caplen, data, ip); else { - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "expected ICMP as IP packet's protocol, got %d", ip->NextProto()); return; } diff --git a/src/analyzer/protocol/login/Login.cc b/src/analyzer/protocol/login/Login.cc index f9e72db87d..8dcb7dba55 100644 --- a/src/analyzer/protocol/login/Login.cc +++ b/src/analyzer/protocol/login/Login.cc @@ -118,7 +118,7 @@ void Login_Analyzer::NewLine(bool orig, char* line) if ( state != LOGIN_STATE_CONFUSED ) { - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "bad state in Login_Analyzer::NewLine"); return; } @@ -582,7 +582,7 @@ char* Login_Analyzer::PeekUserText() { if ( num_user_text <= 0 ) { - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "underflow in Login_Analyzer::PeekUserText()"); return 0; } diff --git a/src/analyzer/protocol/login/NVT.cc b/src/analyzer/protocol/login/NVT.cc index e365306a56..7cb99b1950 100644 --- a/src/analyzer/protocol/login/NVT.cc +++ b/src/analyzer/protocol/login/NVT.cc @@ -42,7 +42,7 @@ void TelnetOption::RecvOption(unsigned int type) if ( ! peer ) { - reporter->InternalAnalyzerError(endp, + reporter->AnalyzerError(endp, "option peer missing in TelnetOption::RecvOption"); return; } @@ -90,7 +90,7 @@ void TelnetOption::RecvOption(unsigned int type) break; default: - reporter->InternalAnalyzerError(endp, + reporter->AnalyzerError(endp, "bad option type in TelnetOption::RecvOption"); return; } @@ -173,7 +173,7 @@ void TelnetEncryptOption::RecvSubOption(u_char* data, int len) if ( ! peer ) { - reporter->InternalAnalyzerError(endp, + reporter->AnalyzerError(endp, "option peer missing in TelnetEncryptOption::RecvSubOption"); return; } @@ -213,7 +213,7 @@ void TelnetAuthenticateOption::RecvSubOption(u_char* data, int len) if ( ! peer ) { - reporter->InternalAnalyzerError(endp, + reporter->AnalyzerError(endp, "option peer missing in TelnetAuthenticateOption::RecvSubOption"); } diff --git a/src/analyzer/protocol/login/RSH.cc b/src/analyzer/protocol/login/RSH.cc index 44fc9f1e5f..f768f4bdc2 100644 --- a/src/analyzer/protocol/login/RSH.cc +++ b/src/analyzer/protocol/login/RSH.cc @@ -131,7 +131,7 @@ void Contents_Rsh_Analyzer::DoDeliver(int len, const u_char* data) break; default: - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "bad state in Contents_Rsh_Analyzer::DoDeliver"); break; } @@ -188,7 +188,7 @@ void Rsh_Analyzer::ClientUserName(const char* s) { if ( client_name ) { - reporter->InternalAnalyzerError(this, "multiple rsh client names"); + reporter->AnalyzerError(this, "multiple rsh client names"); return; } @@ -199,7 +199,7 @@ void Rsh_Analyzer::ServerUserName(const char* s) { if ( username ) { - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "multiple rsh initial client names"); return; } diff --git a/src/analyzer/protocol/login/Rlogin.cc b/src/analyzer/protocol/login/Rlogin.cc index c418047239..d90c9be123 100644 --- a/src/analyzer/protocol/login/Rlogin.cc +++ b/src/analyzer/protocol/login/Rlogin.cc @@ -193,7 +193,7 @@ void Contents_Rlogin_Analyzer::DoDeliver(int len, const u_char* data) break; default: - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "bad state in Contents_Rlogin_Analyzer::DoDeliver"); break; } @@ -226,7 +226,7 @@ void Rlogin_Analyzer::ClientUserName(const char* s) { if ( client_name ) { - reporter->InternalAnalyzerError(this, "multiple rlogin client names"); + reporter->AnalyzerError(this, "multiple rlogin client names"); return; } diff --git a/src/analyzer/protocol/mime/MIME.cc b/src/analyzer/protocol/mime/MIME.cc index 2128453b4c..f4e7d3981f 100644 --- a/src/analyzer/protocol/mime/MIME.cc +++ b/src/analyzer/protocol/mime/MIME.cc @@ -1391,7 +1391,7 @@ void MIME_Mail::SubmitData(int len, const char* buf) { if ( buf != (char*) data_buffer->Bytes() + buffer_start ) { - reporter->InternalAnalyzerError(GetAnalyzer(), + reporter->AnalyzerError(GetAnalyzer(), "MIME buffer misalignment"); return; } @@ -1487,7 +1487,7 @@ void MIME_Mail::SubmitEvent(int event_type, const char* detail) break; default: - reporter->InternalAnalyzerError(GetAnalyzer(), + reporter->AnalyzerError(GetAnalyzer(), "unrecognized MIME_Mail event"); return; } diff --git a/src/analyzer/protocol/mime/MIME.h b/src/analyzer/protocol/mime/MIME.h index e337d24f13..2b2f88105d 100644 --- a/src/analyzer/protocol/mime/MIME.h +++ b/src/analyzer/protocol/mime/MIME.h @@ -193,7 +193,7 @@ public: virtual ~MIME_Message() { if ( ! finished ) - reporter->InternalAnalyzerError(analyzer, + reporter->AnalyzerError(analyzer, "missing MIME_Message::Done() call"); } diff --git a/src/analyzer/protocol/pop3/POP3.cc b/src/analyzer/protocol/pop3/POP3.cc index 8164b927a8..7768e2599f 100644 --- a/src/analyzer/protocol/pop3/POP3.cc +++ b/src/analyzer/protocol/pop3/POP3.cc @@ -209,7 +209,7 @@ void POP3_Analyzer::ProcessRequest(int length, const char* line) break; default: - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "unexpected POP3 authorization state"); return; } @@ -567,7 +567,7 @@ void POP3_Analyzer::ProcessClientCmd() break; default: - reporter->InternalAnalyzerError(this, "unknown POP3 command"); + reporter->AnalyzerError(this, "unknown POP3 command"); return; } } diff --git a/src/analyzer/protocol/rpc/RPC.cc b/src/analyzer/protocol/rpc/RPC.cc index 1c4cc46249..1ffc7dd26e 100644 --- a/src/analyzer/protocol/rpc/RPC.cc +++ b/src/analyzer/protocol/rpc/RPC.cc @@ -266,7 +266,7 @@ int RPC_Interpreter::DeliverRPC(const u_char* buf, int n, int rpclen, else if ( n < 0 ) { - reporter->InternalAnalyzerError(analyzer, "RPC underflow"); + reporter->AnalyzerError(analyzer, "RPC underflow"); return 0; } @@ -479,7 +479,7 @@ bool Contents_RPC::CheckResync(int& len, const u_char*& data, bool orig) if ( resync_toskip != 0 ) { // Should never happen. - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "RPC resync: skipping over data failed"); return false; } @@ -633,7 +633,7 @@ void Contents_RPC::DeliverStream(int len, const u_char* data, bool orig) if ( ! dummy_p ) { - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "inconsistent RPC record marker extraction"); return; } diff --git a/src/analyzer/protocol/smb/SMB.cc b/src/analyzer/protocol/smb/SMB.cc index 12b3fc4287..8a5665515b 100644 --- a/src/analyzer/protocol/smb/SMB.cc +++ b/src/analyzer/protocol/smb/SMB.cc @@ -738,7 +738,7 @@ int SMB_Session::ParseTransaction(int is_orig, int cmd, break; default: - reporter->InternalAnalyzerError(analyzer, + reporter->AnalyzerError(analyzer, "command mismatch for SMB_Session::ParseTransaction"); return 0; } diff --git a/src/analyzer/protocol/tcp/ContentLine.cc b/src/analyzer/protocol/tcp/ContentLine.cc index 44051ee1d1..63a3c07f00 100644 --- a/src/analyzer/protocol/tcp/ContentLine.cc +++ b/src/analyzer/protocol/tcp/ContentLine.cc @@ -124,7 +124,7 @@ void ContentLine_Analyzer::SetPlainDelivery(int64_t length) { if ( length < 0 ) { - reporter->InternalAnalyzerError(this, + reporter->AnalyzerError(this, "negative length for plain delivery"); return; } From fdb6d190b8f20cb442e64d18a406af670ce842f1 Mon Sep 17 00:00:00 2001 From: Daniel Thayer Date: Sun, 13 Oct 2013 20:31:48 -0500 Subject: [PATCH 04/11] Add check for curl command to active-http.test Added a check if the curl command is available when running the active-http.test so that it fails more quickly and with a clear error message if it's not available. --- testing/btest/scripts/base/utils/active-http.test | 1 + 1 file changed, 1 insertion(+) diff --git a/testing/btest/scripts/base/utils/active-http.test b/testing/btest/scripts/base/utils/active-http.test index f547e7dd15..622e8ca46c 100644 --- a/testing/btest/scripts/base/utils/active-http.test +++ b/testing/btest/scripts/base/utils/active-http.test @@ -1,5 +1,6 @@ # @TEST-REQUIRES: which python # +# @TEST-EXEC: which curl # @TEST-EXEC: btest-bg-run httpd python $SCRIPTS/httpd.py --max 1 # @TEST-EXEC: sleep 3 # @TEST-EXEC: btest-bg-run bro bro -b %INPUT From 2a87a70884c1b369b6c454353453c9c4ca940baf Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Mon, 14 Oct 2013 09:19:19 -0700 Subject: [PATCH 05/11] Reverting one of the my internal errors tweaks. --- src/analyzer/protocol/tcp/TCP.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/analyzer/protocol/tcp/TCP.cc b/src/analyzer/protocol/tcp/TCP.cc index cbf983b70a..aefc5a1808 100644 --- a/src/analyzer/protocol/tcp/TCP.cc +++ b/src/analyzer/protocol/tcp/TCP.cc @@ -1580,7 +1580,8 @@ BroFile* TCP_Analyzer::GetContentsFile(unsigned int direction) const default: break; } - reporter->InternalWarning("bad direction %u in TCP_Analyzer::GetContentsFile", + + reporter->Error("bad direction %u in TCP_Analyzer::GetContentsFile", direction); return 0; } From bd26dd910685c768fd4a2d3b1dfb8368014da6f5 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Mon, 14 Oct 2013 09:23:55 -0700 Subject: [PATCH 06/11] Updating submodule(s). [nomail] --- cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmake b/cmake index c966aecf2b..d902e23fd1 160000 --- a/cmake +++ b/cmake @@ -1 +1 @@ -Subproject commit c966aecf2bc83f7bbfdd1ac716c6627dd95cb2ec +Subproject commit d902e23fd14624eb9caf0b4a0e693014bf5bd684 From 12ac0a44c41ae4efef955d9aa7ea492b52608af8 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Mon, 14 Oct 2013 09:24:56 -0700 Subject: [PATCH 07/11] Updating submodule(s). [nomail] --- aux/binpac | 2 +- aux/bro-aux | 2 +- aux/broccoli | 2 +- aux/broctl | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/aux/binpac b/aux/binpac index 877d92eaaf..923994715b 160000 --- a/aux/binpac +++ b/aux/binpac @@ -1 +1 @@ -Subproject commit 877d92eaafb474aa8f34e5961a726644b4968303 +Subproject commit 923994715b34bf3292e402bbe00c00ff77556490 diff --git a/aux/bro-aux b/aux/bro-aux index b3f1fda060..1496e0319f 160000 --- a/aux/bro-aux +++ b/aux/bro-aux @@ -1 +1 @@ -Subproject commit b3f1fda060492ff4766c27535f49178739fbb926 +Subproject commit 1496e0319f6fa12bb39362ab0947c82e1d6c669b diff --git a/aux/broccoli b/aux/broccoli index 39ae475906..2fe7408ed9 160000 --- a/aux/broccoli +++ b/aux/broccoli @@ -1 +1 @@ -Subproject commit 39ae47590617083533640d5d03b2454d6959a8a7 +Subproject commit 2fe7408ed96b673f7d8bb273abd03d4786facfd2 diff --git a/aux/broctl b/aux/broctl index 81fa2d664a..893d88e492 160000 --- a/aux/broctl +++ b/aux/broctl @@ -1 +1 @@ -Subproject commit 81fa2d664a7ef7306a03928484b10611fbe893b8 +Subproject commit 893d88e492731b1b5adf123891273f496b5b1cce From 50784c64bc3764b6f6eb34f363eb814536e3c7b3 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Mon, 14 Oct 2013 09:51:07 -0700 Subject: [PATCH 08/11] Updating submodule(s). [nomail] --- aux/broctl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aux/broctl b/aux/broctl index 893d88e492..2ccc76eee5 160000 --- a/aux/broctl +++ b/aux/broctl @@ -1 +1 @@ -Subproject commit 893d88e492731b1b5adf123891273f496b5b1cce +Subproject commit 2ccc76eee5565b7142e10f9b625a05e9932f459f From 38ae7c98b498677dc3e4d10fb8cdcce5d5aa30d9 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 14 Oct 2013 14:02:39 -0500 Subject: [PATCH 09/11] Fix misc. issues reported by Coverity. Some uninitialized values, a possible null pointer dereference, and time-of-check-time-of-use on reading random seed file. --- src/FlowSrc.cc | 3 +++ src/PacketSort.cc | 3 +++ src/PersistenceSerializer.h | 3 ++- src/PktSrc.cc | 1 + src/PolicyFile.cc | 1 + src/RemoteSerializer.cc | 3 +++ src/analyzer/protocol/netflow/netflow-analyzer.pac | 5 ++++- src/input/Manager.cc | 4 +++- src/input/readers/Raw.cc | 3 ++- src/util.cc | 8 -------- 10 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/FlowSrc.cc b/src/FlowSrc.cc index 7a79e9063b..32aa4c4e3a 100644 --- a/src/FlowSrc.cc +++ b/src/FlowSrc.cc @@ -14,8 +14,11 @@ FlowSrc::FlowSrc() { // TODO: v9. + selectable_fd = -1; idle = false; data = 0; + pdu_len = -1; + exporter_ip = 0; current_timestamp = next_timestamp = 0.0; netflow_analyzer = new binpac::NetFlow::NetFlow_Analyzer(); } diff --git a/src/PacketSort.cc b/src/PacketSort.cc index a7e2b04572..429d8e2720 100644 --- a/src/PacketSort.cc +++ b/src/PacketSort.cc @@ -17,6 +17,9 @@ PacketSortElement::PacketSortElement(PktSrc* arg_src, is_tcp = 0; ip_hdr = 0; + tcp_flags = 0; + endp = 0; + payload_length = 0; key = 0; // Now check if it is a "parsable" TCP packet. diff --git a/src/PersistenceSerializer.h b/src/PersistenceSerializer.h index dcd712bf84..7274e60569 100644 --- a/src/PersistenceSerializer.h +++ b/src/PersistenceSerializer.h @@ -113,7 +113,8 @@ protected: conns = 0; conn_cookie = 0; peer = SOURCE_LOCAL; - }; + filename = 0; + } Type type; SerialInfo info; diff --git a/src/PktSrc.cc b/src/PktSrc.cc index f318405920..9d6bce6fe9 100644 --- a/src/PktSrc.cc +++ b/src/PktSrc.cc @@ -648,6 +648,7 @@ PktDumper::PktDumper(const char* arg_filename, bool arg_append) is_error = false; append = arg_append; dumper = 0; + open_time = 0.0; // We need a pcap_t with a reasonable link-layer type. We try to get it // from the packet sources. If not available, we fall back to Ethernet. diff --git a/src/PolicyFile.cc b/src/PolicyFile.cc index 7ed0b02d75..5d0082c6a9 100644 --- a/src/PolicyFile.cc +++ b/src/PolicyFile.cc @@ -86,6 +86,7 @@ bool LoadPolicyFileText(const char* policy_filename) char buf[256]; strerror_r(errno, buf, sizeof(buf)); reporter->Error("fstat failed on %s: %s", policy_filename, buf); + fclose(f); return false; } diff --git a/src/RemoteSerializer.cc b/src/RemoteSerializer.cc index 7d9c6163a0..c8cf03667b 100644 --- a/src/RemoteSerializer.cc +++ b/src/RemoteSerializer.cc @@ -3321,6 +3321,9 @@ SocketComm::SocketComm() id_counter = 10000; parent_peer = 0; parent_msgstate = TYPE; + parent_id = RemoteSerializer::PEER_NONE; + parent_msgtype = 0; + parent_args = 0; shutting_conns_down = false; terminating = false; killing = false; diff --git a/src/analyzer/protocol/netflow/netflow-analyzer.pac b/src/analyzer/protocol/netflow/netflow-analyzer.pac index 666de076c8..439215dbc1 100644 --- a/src/analyzer/protocol/netflow/netflow-analyzer.pac +++ b/src/analyzer/protocol/netflow/netflow-analyzer.pac @@ -31,8 +31,11 @@ flow NetFlow_Flow { internal_type("nf_v5_record")->AsRecordType(); nfheader_id_type = internal_type("nfheader_id")->AsRecordType(); - pdu_id = 0; identifier = NULL; + exporter_ip = 0; + uptime = 0; + export_time = 0; + pdu_id = 0; %} # %cleanup does not only put the cleanup code into the destructor, diff --git a/src/input/Manager.cc b/src/input/Manager.cc index a05eb59e99..ee1e1ef522 100644 --- a/src/input/Manager.cc +++ b/src/input/Manager.cc @@ -1518,7 +1518,6 @@ int Manager::PutTable(Stream* i, const Value* const *vals) EnumVal* ev; int startpos = 0; Val* predidx = ValueToRecordVal(vals, stream->itype, &startpos); - Ref(valval); if ( updated ) ev = new EnumVal(BifEnum::Input::EVENT_CHANGED, @@ -1529,7 +1528,10 @@ int Manager::PutTable(Stream* i, const Value* const *vals) bool result; if ( stream->num_val_fields > 0 ) // we have values + { + Ref(valval); result = CallPred(stream->pred, 3, ev, predidx, valval); + } else // no values result = CallPred(stream->pred, 2, ev, predidx); diff --git a/src/input/readers/Raw.cc b/src/input/readers/Raw.cc index 1c02339748..d795430ba3 100644 --- a/src/input/readers/Raw.cc +++ b/src/input/readers/Raw.cc @@ -289,7 +289,8 @@ bool Raw::OpenInput() return false; } - fcntl(fileno(file), F_SETFD, FD_CLOEXEC); + if ( ! SetFDFlags(fileno(file), F_SETFD, FD_CLOEXEC) ) + Warning(Fmt("Init: cannot set close-on-exec for %s", fname.c_str())); } return true; diff --git a/src/util.cc b/src/util.cc index bcc9b0bb27..dd232a83fa 100644 --- a/src/util.cc +++ b/src/util.cc @@ -652,16 +652,8 @@ void hmac_md5(size_t size, const unsigned char* bytes, unsigned char digest[16]) static bool read_random_seeds(const char* read_file, uint32* seed, uint32* buf, int bufsiz) { - struct stat st; FILE* f = 0; - if ( stat(read_file, &st) < 0 ) - { - reporter->Warning("Seed file '%s' does not exist: %s", - read_file, strerror(errno)); - return false; - } - if ( ! (f = fopen(read_file, "r")) ) { reporter->Warning("Could not open seed file '%s': %s", From 1d23f055bab092978729f3637fdd3997c25d4bba Mon Sep 17 00:00:00 2001 From: Daniel Thayer Date: Mon, 14 Oct 2013 15:05:06 -0500 Subject: [PATCH 10/11] Add check for sqlite3 command to tests that require it --- testing/btest/scripts/base/frameworks/input/sqlite/basic.bro | 2 ++ testing/btest/scripts/base/frameworks/input/sqlite/error.bro | 2 ++ testing/btest/scripts/base/frameworks/input/sqlite/port.bro | 2 ++ testing/btest/scripts/base/frameworks/input/sqlite/types.bro | 2 ++ testing/btest/scripts/base/frameworks/logging/sqlite/error.bro | 1 + testing/btest/scripts/base/frameworks/logging/sqlite/types.bro | 1 + .../btest/scripts/base/frameworks/logging/sqlite/wikipedia.bro | 1 + 7 files changed, 11 insertions(+) diff --git a/testing/btest/scripts/base/frameworks/input/sqlite/basic.bro b/testing/btest/scripts/base/frameworks/input/sqlite/basic.bro index 03bc91beb7..aa3a75ae4b 100644 --- a/testing/btest/scripts/base/frameworks/input/sqlite/basic.bro +++ b/testing/btest/scripts/base/frameworks/input/sqlite/basic.bro @@ -1,6 +1,8 @@ # # @TEST-GROUP: sqlite # +# @TEST-REQUIRES: which sqlite3 +# # @TEST-EXEC: cat conn.sql | sqlite3 conn.sqlite # @TEST-EXEC: btest-bg-run bro bro -b %INPUT # @TEST-EXEC: btest-bg-wait 10 diff --git a/testing/btest/scripts/base/frameworks/input/sqlite/error.bro b/testing/btest/scripts/base/frameworks/input/sqlite/error.bro index 53ac1e0863..c6712df99a 100644 --- a/testing/btest/scripts/base/frameworks/input/sqlite/error.bro +++ b/testing/btest/scripts/base/frameworks/input/sqlite/error.bro @@ -1,3 +1,5 @@ +# @TEST-REQUIRES: which sqlite3 +# # @TEST-EXEC: cat ssh.sql | sqlite3 ssh.sqlite # # @TEST-GROUP: sqlite diff --git a/testing/btest/scripts/base/frameworks/input/sqlite/port.bro b/testing/btest/scripts/base/frameworks/input/sqlite/port.bro index 049ad2a386..c2949b5b3e 100644 --- a/testing/btest/scripts/base/frameworks/input/sqlite/port.bro +++ b/testing/btest/scripts/base/frameworks/input/sqlite/port.bro @@ -1,6 +1,8 @@ # # @TEST-GROUP: sqlite # +# @TEST-REQUIRES: which sqlite3 +# # @TEST-EXEC: cat port.sql | sqlite3 port.sqlite # @TEST-EXEC: btest-bg-run bro bro -b %INPUT # @TEST-EXEC: btest-bg-wait 10 diff --git a/testing/btest/scripts/base/frameworks/input/sqlite/types.bro b/testing/btest/scripts/base/frameworks/input/sqlite/types.bro index 4e60de3a96..ddcbefa67f 100644 --- a/testing/btest/scripts/base/frameworks/input/sqlite/types.bro +++ b/testing/btest/scripts/base/frameworks/input/sqlite/types.bro @@ -1,3 +1,5 @@ +# @TEST-REQUIRES: which sqlite3 +# # @TEST-EXEC: cat ssh.sql | sqlite3 ssh.sqlite # # @TEST-GROUP: sqlite diff --git a/testing/btest/scripts/base/frameworks/logging/sqlite/error.bro b/testing/btest/scripts/base/frameworks/logging/sqlite/error.bro index 27193250a4..2e5d22f188 100644 --- a/testing/btest/scripts/base/frameworks/logging/sqlite/error.bro +++ b/testing/btest/scripts/base/frameworks/logging/sqlite/error.bro @@ -1,4 +1,5 @@ # +# @TEST-REQUIRES: which sqlite3 # @TEST-REQUIRES: has-writer SQLite # @TEST-GROUP: sqlite # diff --git a/testing/btest/scripts/base/frameworks/logging/sqlite/types.bro b/testing/btest/scripts/base/frameworks/logging/sqlite/types.bro index a6a1c04b02..7c896a7192 100644 --- a/testing/btest/scripts/base/frameworks/logging/sqlite/types.bro +++ b/testing/btest/scripts/base/frameworks/logging/sqlite/types.bro @@ -1,4 +1,5 @@ # +# @TEST-REQUIRES: which sqlite3 # @TEST-REQUIRES: has-writer SQLite # @TEST-GROUP: sqlite # diff --git a/testing/btest/scripts/base/frameworks/logging/sqlite/wikipedia.bro b/testing/btest/scripts/base/frameworks/logging/sqlite/wikipedia.bro index 0e40c60008..b48520440a 100644 --- a/testing/btest/scripts/base/frameworks/logging/sqlite/wikipedia.bro +++ b/testing/btest/scripts/base/frameworks/logging/sqlite/wikipedia.bro @@ -1,4 +1,5 @@ # +# @TEST-REQUIRES: which sqlite3 # @TEST-REQUIRES: has-writer SQLite # @TEST-GROUP: sqlite # From cf6d7ba5ae2088efb0c8b05063f6ca76c7092684 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 14 Oct 2013 15:37:45 -0500 Subject: [PATCH 11/11] Fix more Coverity-reported issues (introduced by internal error audit). Mem leaks and a null pointer deref (not actually expected to occur unless already in one of the the odd states that warrants an internal warning/error). Also dead code. --- src/Frag.cc | 3 +++ src/analyzer/protocol/login/NVT.cc | 1 + src/analyzer/protocol/pop3/POP3.cc | 1 + src/analyzer/protocol/smb/SMB.cc | 20 +++++--------------- 4 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/Frag.cc b/src/Frag.cc index 199af78ca9..b1efb41594 100644 --- a/src/Frag.cc +++ b/src/Frag.cc @@ -290,8 +290,11 @@ void FragReassembler::BlockInserted(DataBlock* /* start_block */) } else + { reporter->InternalWarning("bad IP version in fragment reassembly: %d", version); + delete [] pkt_start; + } } void FragReassembler::Expire(double t) diff --git a/src/analyzer/protocol/login/NVT.cc b/src/analyzer/protocol/login/NVT.cc index 7cb99b1950..462cd42177 100644 --- a/src/analyzer/protocol/login/NVT.cc +++ b/src/analyzer/protocol/login/NVT.cc @@ -215,6 +215,7 @@ void TelnetAuthenticateOption::RecvSubOption(u_char* data, int len) { reporter->AnalyzerError(endp, "option peer missing in TelnetAuthenticateOption::RecvSubOption"); + return; } if ( ! peer->DidRequestAuthentication() ) diff --git a/src/analyzer/protocol/pop3/POP3.cc b/src/analyzer/protocol/pop3/POP3.cc index 7768e2599f..388a055ee2 100644 --- a/src/analyzer/protocol/pop3/POP3.cc +++ b/src/analyzer/protocol/pop3/POP3.cc @@ -211,6 +211,7 @@ void POP3_Analyzer::ProcessRequest(int length, const char* line) default: reporter->AnalyzerError(this, "unexpected POP3 authorization state"); + delete decoded; return; } diff --git a/src/analyzer/protocol/smb/SMB.cc b/src/analyzer/protocol/smb/SMB.cc index 8a5665515b..9d388a0886 100644 --- a/src/analyzer/protocol/smb/SMB.cc +++ b/src/analyzer/protocol/smb/SMB.cc @@ -743,23 +743,13 @@ int SMB_Session::ParseTransaction(int is_orig, int cmd, return 0; } - int ret; - if ( is_orig ) - { - if ( cmd == SMB_COM_TRANSACTION || cmd == SMB_COM_TRANSACTION2 ) - ret = ParseTransactionRequest(cmd, hdr, body); + if ( ! is_orig ) + return ParseTransactionResponse(cmd, hdr, body); - else if ( cmd == SMB_COM_TRANSACTION_SECONDARY || - cmd == SMB_COM_TRANSACTION2_SECONDARY ) - ret = ParseTransactionSecondaryRequest(cmd, hdr, body); + if ( cmd == SMB_COM_TRANSACTION || cmd == SMB_COM_TRANSACTION2 ) + return ParseTransactionRequest(cmd, hdr, body); - else - ret = 0; - } - else - ret = ParseTransactionResponse(cmd, hdr, body); - - return ret; + return ParseTransactionSecondaryRequest(cmd, hdr, body); } int SMB_Session::ParseTransactionRequest(int cmd,