Merge remote-tracking branch 'origin/topic/jsiwek/dns-mgr-fixes'

* origin/topic/jsiwek/dns-mgr-fixes:
  Fix timing out DNS lookups that were already resolved
  Remove an unhelpful/optimistic DNS_Mgr optimization
  Fix DNS_Mgr priority_queue usage
  Remove dead code from DNS_Mgr
  Improve DNS_Mgr I/O loop: prevent starvation due to busy Broker
  Fix a ref counnting bug in DNS_Mgr

CHANGES

@@ -1,4 +1,10 @@
 
+2.6-300 | 2019-05-15 09:00:57 -0700
+
+  * Fixes to DNS lookup, including ref-counting bugs, preventing starvation
+    of the DNS_Mgr in the I/O loop, dead code removal, and a fix that
+    prevents the timeout of already resolved DNS lookups (Jon Siwek, Corelight)
+
 2.6-292 | 2019-05-14 19:01:05 -0700
 
   * Fix maybe-uninitialized compiler warning (Jon Siwek, Corelight)

VERSION

@@ -1 +1 @@
-2.6-292
+2.6-300

src/DNS_Mgr.cc

@@ -289,10 +289,13 @@ ListVal* DNS_Mapping::Addrs()
 
 TableVal* DNS_Mapping::AddrsSet() {
 	ListVal* l = Addrs();
-	if ( l )
+
-		return l->ConvertToSet();
+	if ( ! l )
-	else
 		return empty_addr_set();
+
+	auto rval = l->ConvertToSet();
+	Unref(l);
+	return rval;
 	}
 
 StringVal* DNS_Mapping::Host()
@@ -389,6 +392,7 @@ DNS_Mgr::DNS_Mgr(DNS_MgrMode arg_mode)
 	successful = 0;
 	failed = 0;
 	nb_dns = nullptr;
+	next_timestamp = -1.0;
 	}
 
 DNS_Mgr::~DNS_Mgr()
@@ -1249,8 +1253,17 @@ void DNS_Mgr::GetFds(iosource::FD_Set* read, iosource::FD_Set* write,
 
 double DNS_Mgr::NextTimestamp(double* network_time)
 	{
-	// This is kind of cheating ...
+	if ( asyncs_timeouts.empty() )
-	return asyncs_timeouts.size() ? timer_mgr->Time() : -1.0;
+		// No pending requests.
+		return -1.0;
+
+	if ( next_timestamp < 0 )
+		// Store the timestamp to help prevent starvation by some other
+		// IOSource always trying to use the same timestamp
+		// (assuming network_time does actually increase).
+		next_timestamp = timer_mgr->Time();
+
+	return next_timestamp;
 	}
 
 void DNS_Mgr::CheckAsyncAddrRequest(const IPAddr& addr, bool timeout)
@@ -1356,7 +1369,7 @@ void DNS_Mgr::CheckAsyncHostRequest(const char* host, bool timeout)
 
 void DNS_Mgr::Flush()
 	{
-	DoProcess(false);
+	DoProcess();
 
 	HostMap::iterator it;
 	for ( it = host_mappings.begin(); it != host_mappings.end(); ++it )
@@ -1378,10 +1391,11 @@ void DNS_Mgr::Flush()
 
 void DNS_Mgr::Process()
 	{
-	DoProcess(false);
+	DoProcess();
+	next_timestamp = -1.0;
 	}
 
-void DNS_Mgr::DoProcess(bool flush)
+void DNS_Mgr::DoProcess()
 	{
 	if ( ! nb_dns )
 		return;
@@ -1390,23 +1404,23 @@ void DNS_Mgr::DoProcess(bool flush)
 		{
 		AsyncRequest* req = asyncs_timeouts.top();
 
-		if ( req->time + DNS_TIMEOUT > current_time() || flush )
+		if ( req->time + DNS_TIMEOUT > current_time() )
 			break;
 
+		if ( ! req->processed )
+			{
 			if ( req->IsAddrReq() )
 				CheckAsyncAddrRequest(req->host, true);
 			else if ( req->is_txt )
 				CheckAsyncTextRequest(req->name.c_str(), true);
 			else
 				CheckAsyncHostRequest(req->name.c_str(), true);
+			}
 
 		asyncs_timeouts.pop();
 		delete req;
 		}
 
-	if ( asyncs_addrs.size() == 0 && asyncs_names.size() == 0 && asyncs_texts.size() == 0 )
-		return;
-
 	if ( AnswerAvailable(0) <= 0 )
 		return;
 

src/DNS_Mgr.h

@@ -132,7 +132,7 @@ protected:
 	void CheckAsyncTextRequest(const char* host, bool timeout);
 
 	// Process outstanding requests.
-	void DoProcess(bool flush);
+	void DoProcess();
 
 	// IOSource interface.
 	void GetFds(iosource::FD_Set* read, iosource::FD_Set* write,
@@ -172,12 +172,13 @@ protected:
 
 	struct AsyncRequest {
 		double time;
+		bool is_txt;
+		bool processed;
 		IPAddr host;
 		string name;
-		bool is_txt;
 		CallbackList callbacks;
 
-		AsyncRequest() : time(0.0), is_txt(false) { }
+		AsyncRequest() : time(0.0), is_txt(false), processed(false) { }
 
 		bool IsAddrReq() const	{ return name.length() == 0; }
 
@@ -190,6 +191,7 @@ protected:
 				delete *i;
 				}
 			callbacks.clear();
+			processed = true;
 			}
 
 		void Resolved(TableVal* addrs)
@@ -201,6 +203,7 @@ protected:
 				delete *i;
 				}
 			callbacks.clear();
+			processed = true;
 			}
 
 		void Timeout()
@@ -212,6 +215,7 @@ protected:
 				delete *i;
 				}
 			callbacks.clear();
+			processed = true;
 			}
 
 	};
@@ -228,7 +232,14 @@ protected:
 	typedef list<AsyncRequest*> QueuedList;
 	QueuedList asyncs_queued;
 
-	typedef priority_queue<AsyncRequest*> TimeoutQueue;
+	struct AsyncRequestCompare {
+		bool operator()(const AsyncRequest* a, const AsyncRequest* b)
+			{
+			return a->time > b->time;
+			}
+	};
+
+	typedef priority_queue<AsyncRequest*, std::vector<AsyncRequest*>, AsyncRequestCompare> TimeoutQueue;
 	TimeoutQueue asyncs_timeouts;
 
 	int asyncs_pending;
@@ -236,6 +247,7 @@ protected:
 	unsigned long num_requests;
 	unsigned long successful;
 	unsigned long failed;
+	double next_timestamp;
 };
 
 extern DNS_Mgr* dns_mgr;