Fixed DNS caching and made DNS cache empty results

(cherry picked from commit 438ae629e51b519d0d5f70531d0262be1b9fe2bc)

3 files changed, 102 insertions, 59 deletions

diff --git a/include/dns.h b/include/dns.h
index cbadc3916..653229b6f 100644
--- a/include/dns.h
+++ b/include/dns.h
@@ -33,6 +33,7 @@ enum QueryFlags
 
 enum DNSError
 {
+	DNS_ERROR_NONE,
 	DNS_ERROR_UNKNOWN,
 	DNS_ERROR_UNLOADED,
 	DNS_ERROR_TIMEOUT,
@@ -57,6 +58,8 @@ class CoreExport DNSRequest
 {
 	/* Timeout timer for this request */
 	DNSRequestTimeout *timeout;
+	/* Use result cache if available */
+	bool use_cache;
 
  public:
 	/* Request id */
@@ -73,9 +76,11 @@ class CoreExport DNSRequest
 
 	virtual ~DNSRequest();
 
+	void Process();
+
 	virtual void OnLookupComplete(const DNSRecord *r) = 0;
 
-	virtual void OnError(DNSError error, const Anope::string &message);
+	virtual void OnError(const DNSRecord *r);
 };
 
 /** A full packet sent to the nameserver, may contain multiple queries
@@ -116,6 +121,8 @@ struct DNSRecord
 	Anope::string result;
 	/* Type of query this was */
 	QueryType type;
+	/* Error, if there was one */
+	DNSError error;
 	/* Record class, should always be 1 */
 	unsigned short record_class;
 	/* Time to live */
@@ -123,7 +130,7 @@ struct DNSRecord
 	/* Record length */
 	unsigned short rdlength;
 
-	inline DNSRecord();
+	inline DNSRecord(const Anope::string &n);
 	/* When this record was created in our cache */
 	time_t created;
 };
diff --git a/modules/extra/m_dnsbl.cpp b/modules/extra/m_dnsbl.cpp
index f043cde1e..a9351d35e 100644
--- a/modules/extra/m_dnsbl.cpp
+++ b/modules/extra/m_dnsbl.cpp
@@ -146,11 +146,12 @@ class ModuleDNSBL : public Module
 			try
 			{
 				Anope::string dnsbl_host = user_ip.addr() + "." + b.name;
-				new DNSBLResolver(this, u, b, dnsbl_host, this->add_to_akill);
+				DNSBLResolver *res = new DNSBLResolver(this, u, b, dnsbl_host, this->add_to_akill);
+				res->Process();
 			}
 			catch (const SocketException &ex)
 			{
-				Log() << "Resolver: " << ex.GetReason();
+				Log() << "m_dnsbl: " << ex.GetReason();
 			}
 		}
 
diff --git a/src/dns.cpp b/src/dns.cpp
index 0ccd5d9dd..e587cc330 100644
--- a/src/dns.cpp
+++ b/src/dns.cpp
@@ -7,7 +7,7 @@ static inline unsigned short GetRandomID()
 	return rand();
 }
 
-DNSRequest::DNSRequest(const Anope::string &addr, QueryType qt, bool cache, Module *c) : timeout(NULL), id(0), creator(c), address(addr), QT(qt)
+DNSRequest::DNSRequest(const Anope::string &addr, QueryType qt, bool cache, Module *c) : timeout(NULL), use_cache(cache), id(0), creator(c), address(addr), QT(qt)
 {
 	if (!DNSEngine)
 		DNSEngine = new DNSManager();
@@ -18,11 +18,33 @@ DNSRequest::DNSRequest(const Anope::string &addr, QueryType qt, bool cache, Modu
 	}
 	if (DNSEngine->packets.size() == 65535)
 		throw SocketException("DNS queue full");
-	
-	Log(LOG_DEBUG_2) << "Resolver: Processing request to lookup " << addr << ", of type " << qt;
+}
+
+DNSRequest::~DNSRequest()
+{
+	/* We never got a chance to fire off a query or create a timer */
+	if (!this->timeout)
+		return;
+	/* DNSRequest came back, delete the timeout */
+	if (!this->timeout->done)
+	{
+		delete this->timeout;
+	}
+	/* Timeout timed us out, delete us from the requests map */
+	else
+	{
+		/* We can leave the packet, if it comes back later we will drop it */
+		DNSEngine->requests.erase(this->id);
+	}
+}
+
+void DNSRequest::Process()
+{
+	Log(LOG_DEBUG_2) << "Resolver: Processing request to lookup " << this->address << ", of type " << this->QT;
 
-	if (cache && DNSEngine->CheckCache(this))
+	if (this->use_cache && DNSEngine->CheckCache(this))
 	{
+		Log(LOG_DEBUG_2) << "Resolver: Using cached result";
 		delete this;
 		return;
 	}
@@ -30,16 +52,18 @@ DNSRequest::DNSRequest(const Anope::string &addr, QueryType qt, bool cache, Modu
 	DNSPacket *p = new DNSPacket();	
 	p->flags = DNS_QUERYFLAGS_RD;
 
-	if (!p->AddQuestion(addr, qt))
+	if (!p->AddQuestion(this->address, this->QT))
 	{
-		Log() << "Resolver: Unable to lookup host " << addr << " of type " << qt << " - internal error";
+		Log() << "Resolver: Unable to lookup host " << this->address << " of type " << this->QT << " - internal error";
 		delete p;
 		delete this;
-		return;
+		throw SocketException("Unable to lookup host " + this->address + " of type " + stringify(this->QT) + " - internal error");
 	}
 
 	unsigned short packet_id;
-	while (DNSEngine->requests.count((packet_id = GetRandomID())));
+	do
+		packet_id = GetRandomID();
+	while (DNSEngine->requests.count(packet_id));
 
 	p->id = this->id = packet_id;
 	DNSEngine->requests[this->id] = this;
@@ -50,25 +74,7 @@ DNSRequest::DNSRequest(const Anope::string &addr, QueryType qt, bool cache, Modu
 	this->timeout = new DNSRequestTimeout(this, Config->DNSTimeout);
 }
 
-DNSRequest::~DNSRequest()
-{
-	/* We never got a chance to fire off a query or create a timer */
-	if (!this->timeout)
-		return;
-	/* DNSRequest came back, delete the timeout */
-	if (!this->timeout->done)
-	{
-		delete this->timeout;
-	}
-	/* Timeout timed us out, delete us from the requests map */
-	else
-	{
-		/* We can leave the packet, if it comes back later we will drop it */
-		DNSEngine->requests.erase(this->id);
-	}
-}
-
-void DNSRequest::OnError(DNSError error, const Anope::string &message)
+void DNSRequest::OnError(const DNSRecord *r)
 {
 }
 
@@ -207,9 +213,10 @@ inline void DNSPacket::FillBuffer(unsigned char *buffer)
 	memcpy(&buffer[12], this->payload, this->payload_count);
 }
 
-inline DNSRecord::DNSRecord()
+inline DNSRecord::DNSRecord(const Anope::string &n) : name(n)
 {
 	this->type = DNS_QUERY_NONE;
+	this->error = DNS_ERROR_NONE;
 	this->record_class = this->ttl = this->rdlength = 0;
 	this->created = Anope::CurTime;
 }
@@ -279,46 +286,59 @@ bool DNSSocket::ProcessRead()
 	if (!(recv_packet.flags & DNS_QUERYFLAGS_QR))
 	{
 		Log(LOG_DEBUG_2) << "Resolver: Received a non-answer";
-		request->OnError(DNS_ERROR_NOT_AN_ANSWER, "Received a non-answer");
+		DNSRecord rr(request->address);
+		rr.error = DNS_ERROR_NOT_AN_ANSWER;
+		request->OnError(&rr);
 	}
 	else if (recv_packet.flags & DNS_QUERYFLAGS_OPCODE)
 	{
 		Log(LOG_DEBUG_2) << "Resolver: Received a nonstandard query";
-		request->OnError(DNS_ERROR_NONSTANDARD_QUERY, "Received a nonstandard query");
+		DNSRecord rr(request->address);
+		rr.error = DNS_ERROR_NONSTANDARD_QUERY;
+		request->OnError(&rr);
 	}
 	else if (recv_packet.flags & DNS_QUERYFLAGS_RCODE)
 	{
+		DNSError error = DNS_ERROR_UNKNOWN;
+
 		switch (recv_packet.flags & DNS_QUERYFLAGS_RCODE)
 		{
 			case 1:
-				Log(LOG_DEBUG_2) << "Resolver: Format error";
-				request->OnError(DNS_ERROR_FORMAT_ERROR, "Format error");
+				Log(LOG_DEBUG_2) << "Resolver: format error";
+				error = DNS_ERROR_FORMAT_ERROR;
 				break;
 			case 2:
-				Log(LOG_DEBUG_2) << "Resolver: Server failure";
-				request->OnError(DNS_ERROR_SERVER_FAILURE, "Server failure");
+				Log(LOG_DEBUG_2) << "Resolver: server error";
+				error = DNS_ERROR_SERVER_FAILURE;
 				break;
 			case 3:
-				Log(LOG_DEBUG_2) << "Resolver: Domain name not found";
-				request->OnError(DNS_ERROR_DOMAIN_NOT_FOUND, "Domain not found");
+				Log(LOG_DEBUG_2) << "Resolver: domain not found";
+				error = DNS_ERROR_DOMAIN_NOT_FOUND;
 				break;
 			case 4:
-				Log(LOG_DEBUG_2) << "Resolver: Not Implemented - The name server does not support the requested kind of query.";
-				request->OnError(DNS_ERROR_NOT_IMPLEMENTED, "Not Implemented - The name server does not support the requested kind of query.");
+				Log(LOG_DEBUG_2) << "Resolver: not implemented";
+				error = DNS_ERROR_NOT_IMPLEMENTED;
 				break;
 			case 5:
-				Log(LOG_DEBUG_2) << "Resolver: Server refused to perform the specificed operation";
-				request->OnError(DNS_ERROR_REFUSED, "Server refused to perform the specified operation");
+				Log(LOG_DEBUG_2) << "Resolver: refused";
+				error = DNS_ERROR_REFUSED;
 				break;
 			default:
-				Log(LOG_DEBUG_2) << "Resolver: Unknown error";
-				request->OnError(DNS_ERROR_UNKNOWN, "Unknown error");
+				break;
 		}
+
+		DNSRecord *rr = new DNSRecord(request->address);
+		rr->ttl = 300;
+		rr->error = error;
+		request->OnError(rr);
+		DNSEngine->AddCache(rr);
 	}
 	else if (recv_packet.ancount < 1)
 	{
 		Log(LOG_DEBUG_2) << "Resolver: No resource records returned";
-		request->OnError(DNS_ERROR_NO_RECORDS, "No resource records returned");
+		DNSRecord rr(request->address);
+		rr.error = DNS_ERROR_NO_RECORDS;
+		request->OnError(&rr);
 	}
 	else
 	{
@@ -362,8 +382,7 @@ bool DNSSocket::ProcessRead()
 			else
 				packet_pos = packet_pos_ptr + 1;
 
-			DNSRecord *rr = new DNSRecord;
-			rr->name = name;
+			DNSRecord *rr = new DNSRecord(name);
 
 			Log(LOG_DEBUG_2) << "Resolver: Received answer for " << name;
 
@@ -390,7 +409,8 @@ bool DNSSocket::ProcessRead()
 					if (!inet_ntop(AF_INET, &ip, ipbuf, sizeof(ipbuf)))
 					{
 						Log(LOG_DEBUG_2) << "Resolver: Received an invalid IP for DNS_QUERY_A: " << Anope::LastError();
-						request->OnError(DNS_ERROR_FORMAT_ERROR, "Received an invalid IP from the nameserver: " + Anope::LastError());
+						rr->error = DNS_ERROR_FORMAT_ERROR;
+						request->OnError(rr);
 						delete rr;
 						rr = NULL;
 					}
@@ -409,7 +429,8 @@ bool DNSSocket::ProcessRead()
 					if (!inet_ntop(AF_INET6, &ip, ipbuf, sizeof(ipbuf)))
 					{
 						Log(LOG_DEBUG_2) << "Resolver: Received an invalid IP for DNS_QUERY_A: " << Anope::LastError();
-						request->OnError(DNS_ERROR_FORMAT_ERROR, "Received an invalid IP from the nameserver: " + Anope::LastError());
+						rr->error = DNS_ERROR_FORMAT_ERROR;
+						request->OnError(rr);
 						delete rr;
 						rr = NULL;
 					}
@@ -445,8 +466,9 @@ bool DNSSocket::ProcessRead()
 					break;
 				}
 				default:
+					rr->error = DNS_ERROR_INVALIDTYPE;
+					request->OnError(rr);
 					delete rr;
-					request->OnError(DNS_ERROR_INVALIDTYPE, "Invalid query type");
 					rr = NULL;
 			}
 
@@ -489,7 +511,7 @@ bool DNSSocket::ProcessWrite()
 	return cont;
 }
 
-DNSManager::DNSManager() : Timer(3600, Anope::CurTime, true)
+DNSManager::DNSManager() : Timer(300, Anope::CurTime, true)
 {
 	this->sock = NULL;
 }
@@ -510,17 +532,22 @@ bool DNSManager::CheckCache(DNSRequest *request)
 	if (it != this->cache.end())
 	{
 		std::multimap<Anope::string, DNSRecord *>::iterator it_end = this->cache.upper_bound(request->address);
+		bool ret = false;
 		
 		for (; it != it_end; ++it)
 		{
 			DNSRecord *rec = it->second;
 			if (rec->created + rec->ttl >= Anope::CurTime)
 			{
-				request->OnLookupComplete(rec);
+				if (rec->error == DNS_ERROR_NONE)
+					request->OnLookupComplete(rec);
+				else
+					request->OnError(rec);
+				ret = true;
 			}
 		}
 		
-		return true;
+		return ret;
 	}
 
 	return false;
@@ -528,14 +555,18 @@ bool DNSManager::CheckCache(DNSRequest *request)
 
 void DNSManager::Tick(time_t now)
 {
-	for (std::multimap<Anope::string, DNSRecord *>::iterator it = this->cache.begin(), it_end = this->cache.end(); it != it_end; ++it)
+	Log(LOG_DEBUG_2) << "Resolver: Purging DNS cache";
+
+	for (std::multimap<Anope::string, DNSRecord *>::iterator it = this->cache.begin(), it_end = this->cache.end(); it != it_end;)
 	{
+		Anope::string host = it->first;
 		DNSRecord *req = it->second;
+		++it;
 
 		if (req->created + req->ttl < now)
 		{
+			this->cache.erase(host);
 			delete req;
-			this->cache.erase(it);
 		}
 	}
 }
@@ -550,7 +581,9 @@ void DNSManager::Cleanup(Module *mod)
 
 		if (req->creator && req->creator == mod)
 		{
-			req->OnError(DNS_ERROR_UNLOADED, "Module is being unloaded");
+			DNSRecord rr(req->address);
+			rr.error = DNS_ERROR_UNLOADED;
+			req->OnError(&rr);
 			delete req;
 			this->requests.erase(id);
 		}
@@ -569,7 +602,9 @@ DNSRequestTimeout::~DNSRequestTimeout()
 void DNSRequestTimeout::Tick(time_t)
 {
 	this->done = true;
-	this->request->OnError(DNS_ERROR_TIMEOUT, "Request timed out");
+	DNSRecord rr(this->request->address);
+	rr.error = DNS_ERROR_TIMEOUT;
+	this->request->OnError(&rr);
 	delete this->request;
 }