From a3b963ad4ee1e10ab79018c025ca7b4d1468faee Mon Sep 17 00:00:00 2001
From: Jon Siwek <jsiwek@illinois.edu>
Date: Mon, 16 Sep 2013 16:14:01 -0500
Subject: [PATCH] Refactor Analyzer::AddChildAnalyzer and usages.

Make feedback available regarding whether adding a child analyzer fails
because one of the same type already exists (so one can avoid invalid
pointer access of a delete'd analyzer).
---
 src/analyzer/Analyzer.cc                   | 11 +++++------
 src/analyzer/Analyzer.h                    |  9 ++++++---
 src/analyzer/protocol/gnutella/Gnutella.cc |  4 +---
 src/analyzer/protocol/socks/SOCKS.cc       | 10 +++++++---
 4 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/src/analyzer/Analyzer.cc b/src/analyzer/Analyzer.cc
index b8b739f3cb..7a71c2e0d4 100644
--- a/src/analyzer/Analyzer.cc
+++ b/src/analyzer/Analyzer.cc
@@ -378,13 +378,13 @@ void Analyzer::ForwardEndOfData(bool orig)
 	AppendNewChildren();
 	}
 
-void Analyzer::AddChildAnalyzer(Analyzer* analyzer, bool init)
+bool Analyzer::AddChildAnalyzer(Analyzer* analyzer, bool init)
 	{
 	if ( HasChildAnalyzer(analyzer->GetAnalyzerTag()) )
 		{
 		analyzer->Done();
 		delete analyzer;
-		return;
+		return false;
 		}
 
 	// We add new children to new_children first.  They are then
@@ -401,6 +401,7 @@ void Analyzer::AddChildAnalyzer(Analyzer* analyzer, bool init)
 
 	DBG_LOG(DBG_ANALYZER, "%s added child %s",
 			fmt_analyzer(this).c_str(), fmt_analyzer(analyzer).c_str());
+	return true;
 	}
 
 Analyzer* Analyzer::AddChildAnalyzer(Tag analyzer)
@@ -409,10 +410,8 @@ Analyzer* Analyzer::AddChildAnalyzer(Tag analyzer)
 		{
 		Analyzer* a = analyzer_mgr->InstantiateAnalyzer(analyzer, conn);
 
-		if ( a )
-			AddChildAnalyzer(a);
-
-		return a;
+		if ( a && AddChildAnalyzer(a) )
+			return a;
 		}
 
 	return 0;
diff --git a/src/analyzer/Analyzer.h b/src/analyzer/Analyzer.h
index 396d45d60e..b709e3dda0 100644
--- a/src/analyzer/Analyzer.h
+++ b/src/analyzer/Analyzer.h
@@ -353,9 +353,10 @@ public:
 	 * discarded.
 	 *
 	 * @param analyzer The ananlyzer to add. Takes ownership.
+	 * @return false if analyzer type was already a child, else true.
 	 */
-	void AddChildAnalyzer(Analyzer* analyzer)
-		{ AddChildAnalyzer(analyzer, true); }
+	bool AddChildAnalyzer(Analyzer* analyzer)
+		{ return AddChildAnalyzer(analyzer, true); }
 
 	/**
 	 * Adds a new child analyzer to the analyzer tree. If an analyzer of
@@ -363,6 +364,7 @@ public:
 	 * discarded.
 	 *
 	 * @param tag The type of analyzer to add.
+	 * @return the new analyzer instance that was added.
 	 */
 	Analyzer* AddChildAnalyzer(Tag tag);
 
@@ -600,8 +602,9 @@ protected:
 	 * @param analyzer The analyzer to add. Takes ownership.
 	 *
 	 * @param init If true, Init() will be calle.d
+	 * @return false if analyzer type was already a child, else true.
 	 */
-	void AddChildAnalyzer(Analyzer* analyzer, bool init);
+	bool AddChildAnalyzer(Analyzer* analyzer, bool init);
 
 	/**
 	 * Inits all child analyzers. This is an internal method.
diff --git a/src/analyzer/protocol/gnutella/Gnutella.cc b/src/analyzer/protocol/gnutella/Gnutella.cc
index ff71a55fc8..84a33381a0 100644
--- a/src/analyzer/protocol/gnutella/Gnutella.cc
+++ b/src/analyzer/protocol/gnutella/Gnutella.cc
@@ -137,10 +137,8 @@ int Gnutella_Analyzer::IsHTTP(string header)
 
 	analyzer::Analyzer* a = analyzer_mgr->InstantiateAnalyzer("HTTP", Conn());
 
-	if ( a )
+	if ( a && Parent()->AddChildAnalyzer(a) )
 		{
-		Parent()->AddChildAnalyzer(a);
-
 		if ( Parent()->IsAnalyzer("TCP") )
 			{
 			// Replay buffered data.
diff --git a/src/analyzer/protocol/socks/SOCKS.cc b/src/analyzer/protocol/socks/SOCKS.cc
index f9d81b8a16..76212d822b 100644
--- a/src/analyzer/protocol/socks/SOCKS.cc
+++ b/src/analyzer/protocol/socks/SOCKS.cc
@@ -62,9 +62,13 @@ void SOCKS_Analyzer::DeliverStream(int len, const u_char* data, bool orig)
 		if ( ! pia )
 			{
 			pia = new pia::PIA_TCP(Conn());
-			AddChildAnalyzer(pia);
-			pia->FirstPacket(true, 0);
-			pia->FirstPacket(false, 0);
+			if ( AddChildAnalyzer(pia) )
+				{
+				pia->FirstPacket(true, 0);
+				pia->FirstPacket(false, 0);
+				}
+			else
+				pia = 0;
 			}
 
 		ForwardStream(len, data, orig);