Merge remote-tracking branch 'origin/topic/robin/file-analysis-fixes'

* origin/topic/robin/file-analysis-fixes:
  Adding test with command line that used to trigger a crash.
  Cleaning up a couple of comments.
  Fix delay in disabling file analyzers.
  Fix file analyzer memory management.

The merge changes around functionality a bit again - instead of having
a list of done analyzers, analyzers are simply set to skipping when they
are removed, and cleaned up later on destruction of the AnalyzerSet.

BIT-1782 #merged

CHANGES

@@ -1,4 +1,11 @@
 
+2.5-39 | 2017-02-01 14:03:08 -0800
+
+  * Fix file analyzer memory management, and a delay in disabling file analyzers.
+    File analyzers are no longer deleted immediately; this is delayed until
+    a file opject is destroyed. Furthermore, no data is sent to file analyzers
+    anymore after they have been disabled.
+
 2.5-33 | 2017-02-01 10:07:47 -0500
 
   * New file types sigs. (Keith Lehigh)

VERSION

@@ -1 +1 @@
-2.5-33
+2.5-39

src/file_analysis/Analyzer.h

@@ -123,6 +123,21 @@ public:
 	void SetGotStreamDelivery()
 		{ got_stream_delivery = true; }
 
+	/**
+	 * Signals that the analyzer is to skip all further input
+	 * processsing. This won't have an immediate effect internally, but
+	 * the flag can be queried through Skipping().
+	 *
+	 * @param do_skip If true, further processing will be skipped.
+	 */
+	void SetSkip(bool do_skip)		{ skip = do_skip; }
+
+	/**
+	 * Returns true if the analyzer has been told to skip processing all
+	 * further input.
+	 */
+	bool Skipping() const			{ return skip; }
+
 protected:
 
 	/**
@@ -136,7 +151,8 @@ protected:
 	    : tag(arg_tag),
 	      args(arg_args->Ref()->AsRecordVal()),
 	      file(arg_file),
-	      got_stream_delivery(false)
+	      got_stream_delivery(false),
+	      skip(false)
 		{
 		id = ++id_counter;
 		}
@@ -154,7 +170,8 @@ protected:
 	    : tag(),
 	      args(arg_args->Ref()->AsRecordVal()),
 	      file(arg_file),
-	      got_stream_delivery(false)
+	      got_stream_delivery(false),
+	      skip(false)
 		{
 		id = ++id_counter;
 		}
@@ -166,6 +183,7 @@ private:
 	RecordVal* args;	/**< \c AnalyzerArgs val gives tunable analyzer params. */
 	File* file;	/**< The file to which the analyzer is attached. */
 	bool got_stream_delivery;
+	bool skip;
 
 	static ID id_counter;
 };

src/file_analysis/AnalyzerSet.cc

@@ -129,8 +129,11 @@ bool AnalyzerSet::Remove(file_analysis::Tag tag, HashKey* key)
 	        file->GetID().c_str(),
 	        file_mgr->GetComponentName(tag).c_str());
 
-	a->Done();
+
-	delete a;
+	// 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);
 
 	return true;
 	}

src/file_analysis/File.cc

@@ -391,9 +391,15 @@ void File::DeliverStream(const u_char* data, uint64 len)
 			// Catch this analyzer up with the BOF buffer.
 			for ( int i = 0; i < num_bof_chunks_behind; ++i )
 				{
-				if ( ! a->DeliverStream(bof_buffer.chunks[i]->Bytes(),
+				if ( ! a->Skipping() )
-				                        bof_buffer.chunks[i]->Len()) )
+					{
-					analyzers.QueueRemove(a->Tag(), a->Args());
+					if ( ! a->DeliverStream(bof_buffer.chunks[i]->Bytes(),
+								bof_buffer.chunks[i]->Len()) )
+						{
+						a->SetSkip(true);
+						analyzers.QueueRemove(a->Tag(), a->Args());
+						}
+					}
 
 				bytes_delivered += bof_buffer.chunks[i]->Len();
 				}
@@ -403,8 +409,14 @@ void File::DeliverStream(const u_char* data, uint64 len)
 			// Analyzer should be fully caught up to stream_offset now.
 			}
 
-		if ( ! a->DeliverStream(data, len) )
+		if ( ! a->Skipping() )
-			analyzers.QueueRemove(a->Tag(), a->Args());
+			{
+			if ( ! a->DeliverStream(data, len) )
+				{
+				a->SetSkip(true);
+				analyzers.QueueRemove(a->Tag(), a->Args());
+				}
+			}
 		}
 
 	stream_offset += len;
@@ -468,9 +480,13 @@ void File::DeliverChunk(const u_char* data, uint64 len, uint64 offset)
 	while ( (a = analyzers.NextEntry(c)) )
 		{
 		DBG_LOG(DBG_FILE_ANALYSIS, "chunk delivery to analyzer %s", file_mgr->GetComponentName(a->Tag()).c_str());
-		if ( ! a->DeliverChunk(data, len, offset) )
+		if ( ! a->Skipping() )
 			{
-			analyzers.QueueRemove(a->Tag(), a->Args());
+			if ( ! a->DeliverChunk(data, len, offset) )
+				{
+				a->SetSkip(true);
+				analyzers.QueueRemove(a->Tag(), a->Args());
+				}
 			}
 		}
 

testing/btest/Baseline/scripts.base.frameworks.file-analysis.byteranges/files.log

@@ -0,0 +1,10 @@
+#separator \x09
+#set_separator	,
+#empty_field	(empty)
+#unset_field	-
+#path	files
+#open	2017-01-31-22-51-55
+#fields	ts	fuid	tx_hosts	rx_hosts	conn_uids	source	depth	analyzers	mime_type	filename	duration	local_orig	is_orig	seen_bytes	total_bytes	missing_bytes	overflow_bytes	timedout	parent_fuid	md5	sha1	sha256	extracted	extracted_cutoff	extracted_size
+#types	time	string	set[addr]	set[addr]	set[string]	string	count	set[string]	string	string	interval	bool	bool	count	count	count	count	bool	string	string	string	string	string	bool	count
+1258867934.558264	F2xow8TIkvHG4Zz41	198.189.255.75	192.168.1.105	CHhAvVGS1DHFjwGM9	HTTP	0	EXTRACT	-	-	0.046240	-	F	54229	605292323	4244449	0	T	-	-	-	-	extract-1258867934.558264-HTTP-F2xow8TIkvHG4Zz41	T	4000
+#close	2017-01-31-22-51-55

testing/btest/scripts/base/frameworks/file-analysis/byteranges.bro

@@ -0,0 +1,6 @@
+# This used to crash the file reassemly code.
+#
+# @TEST-EXEC: bro -r $TRACES/http/byteranges.trace frameworks/files/extract-all-files FileExtract::default_limit=4000
+# 
+# @TEST-EXEC: btest-diff files.log
+