Analyzer: Do not add child analyzers when finished

Depending on an analyzer's implementation, its Done() method may
attempt to access analyzer or connection state when executing.
When this happens in the destructor of the parent analyzer during
the process of destructing a connection, this state may have been
deleted, resulting in use-after-free crashes or worse memory
corruption.

The following cases have been observed in the wild for when this happens.

  * PIA matching during Done() for undelivered TCP data enables a Spicy
    based analyzer which in turn attempts to raise an analyzer violation
    during Done()->EndOfData().

  * Spicy analyzers attaching new analyzers during their Done() processing
    which in turn attempt to use TCP() (to call FindChild()) during Done()
    while the analyzer tree / connection is being destructed.

The second scenario was previously found to happen in the HTTP analyzer
and fixed with 6ef9423f3c.

Plug these scenarios by short-circuiting AddChildAnalyzer() if the analyzer
or connection have finished or are being finished.

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;
 

src/analyzer/Analyzer.cc

@@ -113,19 +113,7 @@ void Analyzer::CtorInit(const zeek::Tag& arg_tag, Connection* arg_conn) {
 
 Analyzer::~Analyzer() {
     assert(finished);
-
+    assert(new_children.empty());
-    // 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;
 
     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;