From 1de6cfc2e335ab965e65ea8955336e1afdcede90 Mon Sep 17 00:00:00 2001
From: Johanna Amann <johanna@icir.org>
Date: Sat, 4 Feb 2017 16:47:07 -0800
Subject: [PATCH] Fix memory leak in file analyzer.

This undoes the changes applied in merge 9db27a6d60dbc0bdc77cb64d4f770489455043cd
and goes back to the state in the branch as of the merge 5ab3b86.

Getting rid of the additional layer of removing analyzers and just
keeping them in the set introduced subtle differences in behavior since
a few calls were still passed along. Skipping all of these with SetSkip
introduced yet other subtle behavioral differences.
---
 src/file_analysis/AnalyzerSet.cc | 5 +++--
 src/file_analysis/File.cc        | 8 ++++++++
 src/file_analysis/File.h         | 6 ++++++
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/file_analysis/AnalyzerSet.cc b/src/file_analysis/AnalyzerSet.cc
index 5b7bdd875c..35968c9a02 100644
--- a/src/file_analysis/AnalyzerSet.cc
+++ b/src/file_analysis/AnalyzerSet.cc
@@ -129,11 +129,12 @@ bool AnalyzerSet::Remove(file_analysis::Tag tag, HashKey* key)
 	        file->GetID().c_str(),
 	        file_mgr->GetComponentName(tag).c_str());
 
+	a->Done();
 
 	// We don't delete the analyzer object right here because the remove
 	// operation may execute at a time when it can still be accessed.
-	// Instead we let disable it; it will be deleted together with the AnalyzerSet.
-	a->SetSkip(true);
+	// Instead we let the file know to delete the analyzer later.
+	file->DoneWithAnalyzer(a);
 
 	return true;
 	}
diff --git a/src/file_analysis/File.cc b/src/file_analysis/File.cc
index ff65eb0c32..46e67f7cd8 100644
--- a/src/file_analysis/File.cc
+++ b/src/file_analysis/File.cc
@@ -107,6 +107,9 @@ File::~File()
 	DBG_LOG(DBG_FILE_ANALYSIS, "[%s] Destroying File object", id.c_str());
 	Unref(val);
 	delete file_reassembler;
+
+	for ( auto a : done_analyzers )
+		delete a;
 	}
 
 void File::UpdateLastActivityTime()
@@ -494,6 +497,11 @@ void File::DeliverChunk(const u_char* data, uint64 len, uint64 offset)
 		EndOfFile();
 	}
 
+void File::DoneWithAnalyzer(Analyzer* analyzer)
+	{
+	done_analyzers.push_back(analyzer);
+	}
+
 void File::DataIn(const u_char* data, uint64 len, uint64 offset)
 	{
 	analyzers.DrainModifications();
diff --git a/src/file_analysis/File.h b/src/file_analysis/File.h
index 6ad90e986b..c799907a8f 100644
--- a/src/file_analysis/File.h
+++ b/src/file_analysis/File.h
@@ -119,6 +119,11 @@ public:
 	 */
 	bool RemoveAnalyzer(file_analysis::Tag tag, RecordVal* args);
 
+	/**
+	 * Signal that this analyzer can be deleted once it's safe to do so.
+	 */
+	void DoneWithAnalyzer(Analyzer* analyzer);
+
 	/**
 	 * Pass in non-sequential data and deliver to attached analyzers.
 	 * @param data pointer to start of a chunk of file data.
@@ -287,6 +292,7 @@ protected:
 	bool postpone_timeout;     /**< Whether postponing timeout is requested. */
 	bool done;                 /**< If this object is about to be deleted. */
 	AnalyzerSet analyzers;     /**< A set of attached file analyzers. */
+	std::list<Analyzer *> done_analyzers; /**< Analyzers we're done with, remembered here until they can be safely deleted. */
 
 	struct BOF_Buffer {
 		BOF_Buffer() : full(false), size(0) {}