diff --git a/src/Conn.h b/src/Conn.h index 67f41dc68d..c1d16f54bd 100644 --- a/src/Conn.h +++ b/src/Conn.h @@ -195,6 +195,9 @@ public: bool PermitWeird(const char* name, uint64_t threshold, uint64_t rate, double duration); + // Returns true once Done() is called. + bool IsFinished() { return finished; } + private: friend class session::detail::Timer; diff --git a/src/analyzer/Analyzer.cc b/src/analyzer/Analyzer.cc index e8f2b65d28..5db65b44a2 100644 --- a/src/analyzer/Analyzer.cc +++ b/src/analyzer/Analyzer.cc @@ -113,19 +113,7 @@ void Analyzer::CtorInit(const zeek::Tag& arg_tag, Connection* arg_conn) { Analyzer::~Analyzer() { assert(finished); - - // Make sure any late entries into the analyzer tree are handled (e.g. - // from some Done() implementation). - LOOP_OVER_GIVEN_CHILDREN(i, new_children) { - if ( ! (*i)->finished ) - (*i)->Done(); - } - - // Deletion of new_children done in separate loop in case a Done() - // implementation tries to inspect analyzer tree w/ assumption that - // all analyzers are still valid. - LOOP_OVER_GIVEN_CHILDREN(i, new_children) - delete *i; + assert(new_children.empty()); LOOP_OVER_CHILDREN(i) delete *i; @@ -330,6 +318,30 @@ void Analyzer::ForwardEndOfData(bool orig) { bool Analyzer::AddChildAnalyzer(Analyzer* analyzer, bool init) { auto t = analyzer->GetAnalyzerTag(); + // Prevent attaching child analyzers to analyzer subtrees where + // either the parent has finished or is being removed. Further, + // don't attach analyzers when the connection has finished or is + // currently being finished (executing Done()). + // + // Scenarios in which analyzers have been observed that late in + // analyzer / connection lifetime are: + // + // * A DPD signature match on undelivered TCP data that is flushed + // during Connection::Done(). The PIA analyzer activates a new + // analyzer adding it to the TCP analyzer. + // + // * Analyzers flushing buffered state during Done(), resulting + // in new analyzers being created. + // + // Analyzers added during Done() are problematic as calling Done() + // within the parent's destructor isn't safe, so we prevent these + // situations. + if ( Removing() || IsFinished() || Conn()->IsFinished() ) { + analyzer->Done(); + delete analyzer; + return false; + } + if ( HasChildAnalyzer(t) || IsPreventedChildAnalyzer(t) ) { analyzer->Done(); delete analyzer;