Replace nb_dns library with C-Ares

.gitmodules

@@ -52,3 +52,6 @@
 [submodule "auxil/gen-zam"]
 	path = auxil/gen-zam
 	url = https://github.com/zeek/gen-zam
+[submodule "auxil/c-ares"]
+	path = auxil/c-ares
+	url = https://github.com/c-ares/c-ares

CMakeLists.txt

@@ -480,6 +480,7 @@ include(CheckNameserCompat)
 include(GetArchitecture)
 include(RequireCXX17)
 include(FindKqueue)
+include(FindCAres)
 
 if ( (OPENSSL_VERSION VERSION_EQUAL "1.1.0") OR (OPENSSL_VERSION VERSION_GREATER "1.1.0") )
   set(ZEEK_HAVE_OPENSSL_1_1 true CACHE INTERNAL "" FORCE)

auxil/c-ares

@@ -0,0 +1 @@
+Subproject commit 2aa086f822aad5017a6f2061ef656f237a62d0ed

cmake

@@ -1 +1 @@
-Subproject commit 649c319f88e2966931892d55adb2ee50f278662b
+Subproject commit 0fa8b3a20ca6d4e32b56cdeee693cb46eec41f5e

src/DNS_Mgr.cc

@@ -4,31 +4,29 @@
 
 #include "zeek/zeek-config.h"
 
-#include <sys/socket.h>
-#include <sys/types.h>
-#ifdef TIME_WITH_SYS_TIME
-#include <sys/time.h>
-#include <time.h>
-#else
-#ifdef HAVE_SYS_TIME_H
-#include <sys/time.h>
-#else
-#include <time.h>
-#endif
-#endif
-
 #include <errno.h>
 #include <netinet/in.h>
+#include <stdlib.h>
+#include <sys/select.h>
+#include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
-#ifdef HAVE_MEMORY_H
-#include <memory.h>
-#endif
-#include <stdlib.h>
 #include <algorithm>
 #include <vector>
 
+#ifdef TIME_WITH_SYS_TIME
+#include <sys/time.h>
+#include <time.h>
+#elif defined(HAVE_SYS_TIME_H)
+#include <sys/time.h>
+#else
+#include <time.h>
+#endif
+
+#include <ares.h>
+#include <ares_dns.h>
+
 #include "zeek/3rdparty/doctest.h"
 #include "zeek/DNS_Mapping.h"
 #include "zeek/Event.h"
@@ -43,65 +41,116 @@
 #include "zeek/ZeekString.h"
 #include "zeek/iosource/Manager.h"
 
-extern "C"
+// Number of seconds we'll wait for a reply.
-	{
+constexpr int DNS_TIMEOUT = 5;
-	extern int select(int, fd_set*, fd_set*, fd_set*, struct timeval*);
-
-#include <netdb.h>
-
-#include "zeek/3rdparty/nb_dns.h"
-	}
-
-using namespace std;
 
 namespace zeek::detail
 	{
 
+static void hostbyaddr_callback(void* arg, int status, int timeouts, struct hostent* hostent)
+	{
+	printf("host callback\n");
+	// TODO: implement this
+	// TODO: figure out how to get TTL info here
+	}
+
+static void addrinfo_callback(void* arg, int status, int timeouts, struct ares_addrinfo* result)
+	{
+	printf("addrinfo callback\n");
+
+	if ( status != ARES_SUCCESS )
+		{
+		// TODO: error or something here, or just give up on it?
+		ares_freeaddrinfo(result);
+		return;
+		}
+
+	// TODO: the existing code doesn't handle hostname aliases at all. Should we?
+	// TODO: handle IPv6 mode
+
+	std::vector<in_addr*> addrs;
+	for ( ares_addrinfo_node* entry = result->nodes; entry != NULL; entry = entry->ai_next )
+		addrs.push_back(&reinterpret_cast<sockaddr_in*>(entry->ai_addr)->sin_addr);
+
+	// Push a null on the end so the addr list has a final point during later parsing.
+	addrs.push_back(NULL);
+
+	struct hostent he;
+	he.h_name = util::copy_string(result->name);
+	he.h_aliases = NULL;
+	he.h_addrtype = AF_INET;
+	he.h_length = sizeof(in_addr);
+	he.h_addr_list = reinterpret_cast<char**>(addrs.data());
+
+	auto req = reinterpret_cast<DNS_Mgr_Request*>(arg);
+	dns_mgr->AddResult(req, &he, result->nodes[0].ai_ttl);
+
+	delete[] he.h_name;
+
+	ares_freeaddrinfo(result);
+	}
+
+static void ares_sock_cb(void* data, int s, int read, int write)
+	{
+	printf("Change state fd %d read:%d write:%d\n", s, read, write);
+	if ( read == 1 )
+		iosource_mgr->RegisterFd(s, reinterpret_cast<DNS_Mgr*>(data));
+	else
+		iosource_mgr->UnregisterFd(s, reinterpret_cast<DNS_Mgr*>(data));
+	}
+
 class DNS_Mgr_Request
 	{
 public:
 	DNS_Mgr_Request(const char* h, int af, bool is_txt)
-		: host(util::copy_string(h)), fam(af), qtype(is_txt ? 16 : 0), addr(), request_pending()
+		: host(util::copy_string(h)), fam(af), qtype(is_txt ? 16 : 0), addr()
 		{
 		}
 
-	DNS_Mgr_Request(const IPAddr& a) : host(), fam(), qtype(), addr(a), request_pending() { }
+	DNS_Mgr_Request(const IPAddr& a) : addr(a) { }
 
 	~DNS_Mgr_Request() { delete[] host; }
 
 	// Returns nil if this was an address request.
 	const char* ReqHost() const { return host; }
 	const IPAddr& ReqAddr() const { return addr; }
+	int Family() const { return fam; }
 	bool ReqIsTxt() const { return qtype == 16; }
 
-	int MakeRequest(nb_dns_info* nb_dns);
+	void MakeRequest(ares_channel channel);
-	int RequestPending() const { return request_pending; }
+
-	void RequestDone() { request_pending = 0; }
+	bool RequestPending() const { return request_pending; }
+	void RequestDone() { request_pending = false; }
 
 protected:
-	char* host; // if non-nil, this is a host request
+	char* host = nullptr; // if non-nil, this is a host request
-	int fam; // address family query type for host requests
+	int fam = 0; // address family query type for host requests
-	int qtype; // Query type
+	int qtype = 0; // Query type
 	IPAddr addr;
-	int request_pending;
+	bool request_pending = false;
 	};
 
-int DNS_Mgr_Request::MakeRequest(nb_dns_info* nb_dns)
+void DNS_Mgr_Request::MakeRequest(ares_channel channel)
 	{
-	if ( ! nb_dns )
+	request_pending = true;
-		return 0;
 
-	request_pending = 1;
+	// TODO: TXT requests?
+	// TODO: could this use ares_create_query/ares_query instead of the
+	// ares_get* methods to make it more generic? I think we might need
+	// to do that for TXT requests.
 
-	char err[NB_DNS_ERRSIZE];
 	if ( host )
-		return nb_dns_host_request2(nb_dns, host, fam, qtype, (void*)this, err) >= 0;
+		{
+		ares_addrinfo_hints hints = {ARES_AI_CANONNAME, fam, 0, 0};
+		ares_getaddrinfo(channel, host, NULL, &hints, addrinfo_callback, this);
+		}
 	else
 		{
 		const uint32_t* bytes;
 		int len = addr.GetBytes(&bytes);
-		return nb_dns_addr_request2(nb_dns, (char*)bytes, len == 1 ? AF_INET : AF_INET6,
+
-		                            (void*)this, err) >= 0;
+		ares_gethostbyaddr(channel, bytes, len, addr.GetFamily() == IPv4 ? AF_INET : AF_INET6,
+		                   hostbyaddr_callback, this);
 		}
 	}
 
@@ -111,22 +160,22 @@ DNS_Mgr::DNS_Mgr(DNS_MgrMode arg_mode)
 
 	mode = arg_mode;
 
-	cache_name = dir = nullptr;
-
 	asyncs_pending = 0;
 	num_requests = 0;
 	successful = 0;
 	failed = 0;
-	nb_dns = nullptr;
+	ipv6_resolver = false;
+
+	ares_library_init(ARES_LIB_INIT_ALL);
 	}
 
 DNS_Mgr::~DNS_Mgr()
 	{
-	if ( nb_dns )
+	Flush();
-		nb_dns_finish(nb_dns);
 
-	delete[] cache_name;
+	ares_cancel(channel);
-	delete[] dir;
+	ares_destroy(channel);
+	ares_library_cleanup();
 	}
 
 void DNS_Mgr::InitSource()
@@ -134,21 +183,36 @@ void DNS_Mgr::InitSource()
 	if ( did_init )
 		return;
 
+	ares_options options;
+	int optmask = 0;
+
+	options.flags = ARES_FLAG_STAYOPEN;
+	optmask |= ARES_OPT_FLAGS;
+
+	options.timeout = DNS_TIMEOUT;
+	optmask |= ARES_OPT_TIMEOUT;
+
+	options.sock_state_cb = ares_sock_cb;
+	options.sock_state_cb_data = this;
+	optmask |= ARES_OPT_SOCK_STATE_CB;
+
+	int status = ares_init_options(&channel, &options, optmask);
+	if ( status != ARES_SUCCESS )
+		reporter->FatalError("Failed to initialize c-ares for DNS resolution: %s",
+		                     ares_strerror(status));
+
 	// Note that Init() may be called by way of LookupHost() during the act of
 	// parsing a hostname literal (e.g. google.com), so we can't use a
 	// script-layer option to configure the DNS resolver as it may not be
 	// configured to the user's desired address at the time when we need to to
 	// the lookup.
 	auto dns_resolver = getenv("ZEEK_DNS_RESOLVER");
-	auto dns_resolver_addr = dns_resolver ? IPAddr(dns_resolver) : IPAddr();
+	if ( dns_resolver )
-	char err[NB_DNS_ERRSIZE];
-
-	if ( dns_resolver_addr == IPAddr() )
-		nb_dns = nb_dns_init(err);
-	else
 		{
-		// nb_dns expects a sockaddr, so copy the address out of the IPAddr
+		ares_addr_node servers;
-		// object into one so it can be passed.
+		servers.next = nullptr;
+
+		auto dns_resolver_addr = IPAddr(dns_resolver);
 		struct sockaddr_storage ss = {0};
 
 		if ( dns_resolver_addr.GetFamily() == IPv4 )
@@ -156,25 +220,21 @@ void DNS_Mgr::InitSource()
 			struct sockaddr_in* sa = (struct sockaddr_in*)&ss;
 			sa->sin_family = AF_INET;
 			dns_resolver_addr.CopyIPv4(&sa->sin_addr);
+
+			servers.family = AF_INET;
+			memcpy(&(servers.addr.addr4), &sa->sin_addr, sizeof(struct in_addr));
 			}
 		else
 			{
 			struct sockaddr_in6* sa = (struct sockaddr_in6*)&ss;
 			sa->sin6_family = AF_INET6;
 			dns_resolver_addr.CopyIPv6(&sa->sin6_addr);
+
+			servers.family = AF_INET6;
+			memcpy(&(servers.addr.addr6), &sa->sin6_addr, sizeof(ares_in6_addr));
 			}
 
-		nb_dns = nb_dns_init2(err, (struct sockaddr*)&ss);
+		ares_set_servers(channel, &servers);
-		}
-
-	if ( nb_dns )
-		{
-		if ( ! doctest::is_running_in_test && ! iosource_mgr->RegisterFd(nb_dns_fd(nb_dns), this) )
-			reporter->FatalError("Failed to register nb_dns file descriptor with iosource_mgr");
-		}
-	else
-		{
-		reporter->Warning("problem initializing NB-DNS: %s", err);
 		}
 
 	did_init = true;
@@ -186,7 +246,7 @@ void DNS_Mgr::InitPostScript()
 		{
 		dm_rec = id::find_type<RecordType>("dns_mapping");
 
-		// Registering will call Init()
+		// Registering will call InitSource(), which sets up all of the DNS library stuff
 		iosource_mgr->Register(this, true);
 		}
 	else
@@ -195,10 +255,10 @@ void DNS_Mgr::InitPostScript()
 		InitSource();
 		}
 
-	const char* cache_dir = dir ? dir : ".";
+	// Load the DNS cache from disk, if it exists.
-	cache_name = new char[strlen(cache_dir) + 64];
+	std::string cache_dir = dir.empty() ? dir : ".";
-	sprintf(cache_name, "%s/%s", cache_dir, ".zeek-dns-cache");
+	cache_name = util::fmt("%s/%s", cache_dir.c_str(), ".zeek-dns-cache");
-	LoadCache(fopen(cache_name, "r"));
+	LoadCache(cache_name);
 	}
 
 static TableValPtr fake_name_lookup_result(const char* name)
@@ -229,11 +289,11 @@ TableValPtr DNS_Mgr::LookupHost(const char* name)
 	if ( mode == DNS_FAKE )
 		return fake_name_lookup_result(name);
 
+	// This should have been run already from InitPostScript(), but just run it again just
+	// in case it hadn't.
 	InitSource();
 
-	if ( ! nb_dns )
+	// Check the cache before attempting to look up the name remotely.
-		return empty_addr_set();
-
 	if ( mode != DNS_PRIME )
 		{
 		HostMap::iterator it = host_mappings.find(name);
@@ -258,23 +318,45 @@ TableValPtr DNS_Mgr::LookupHost(const char* name)
 			}
 		}
 
-	// Not found, or priming.
+	// Not found, or priming. We use ares_getaddrinfo here because we want the TTL value
 	switch ( mode )
 		{
 		case DNS_PRIME:
-			requests.push_back(new DNS_Mgr_Request(name, AF_INET, false));
+			{
-			requests.push_back(new DNS_Mgr_Request(name, AF_INET6, false));
+			// TODO: not sure we need to do these split like this if we can pass AF_UNSPEC
+			// in the hints structure. Do we really need the two different request objects?
+			auto v4 = new DNS_Mgr_Request(name, AF_INET, false);
+			ares_addrinfo_hints v4_hints = {ARES_AI_CANONNAME, AF_INET, 0, 0};
+			ares_getaddrinfo(channel, name, NULL, &v4_hints, addrinfo_callback, v4);
+
+			// TODO: check if ipv6 support is needed if we use AF_UNSPEC above
+			// auto v6 = new DNS_Mgr_Request(name, AF_INET6, false);
+			// ares_addrinfo_hints v6_hints = { 0, AF_INET6, 0, 0 };
+			// ares_getaddrinfo(channel, name, NULL, &v6_hints, addrinfo_callback, v6);
+
 			return empty_addr_set();
+			}
 
 		case DNS_FORCE:
 			reporter->FatalError("can't find DNS entry for %s in cache", name);
 			return nullptr;
 
 		case DNS_DEFAULT:
-			requests.push_back(new DNS_Mgr_Request(name, AF_INET, false));
+			{
-			requests.push_back(new DNS_Mgr_Request(name, AF_INET6, false));
+			auto v4 = new DNS_Mgr_Request(name, AF_INET, false);
+			ares_addrinfo_hints v4_hints = {ARES_AI_CANONNAME, AF_INET, 0, 0};
+			ares_getaddrinfo(channel, name, NULL, &v4_hints, addrinfo_callback, v4);
+
+			// TODO: check if ipv6 support is needed if we use AF_UNSPEC above
+			// auto v6 = new DNS_Mgr_Request(name, AF_INET6, false);
+			// ares_addrinfo_hints v6_hints = { 0, AF_INET6, 0, 0 };
+			// ares_getaddrinfo(channel, name, NULL, &v6_hints, addrinfo_callback, v6);
+
 			Resolve();
+
+			// Call LookupHost() a second time to get the newly stored value out of the cache.
 			return LookupHost(name);
+			}
 
 		default:
 			reporter->InternalError("bad mode in DNS_Mgr::LookupHost");
@@ -287,11 +369,11 @@ StringValPtr DNS_Mgr::LookupAddr(const IPAddr& addr)
 	if ( mode == DNS_FAKE )
 		return make_intrusive<StringVal>(fake_addr_lookup_result(addr));
 
+	// This should have been run already from InitPostScript(), but just run it again just
+	// in case it hadn't.
 	InitSource();
 
-	if ( ! nb_dns )
+	// Check the cache before attempting to look up the name remotely.
-		return make_intrusive<StringVal>("<none>");
-
 	if ( mode != DNS_PRIME )
 		{
 		AddrMap::iterator it = addr_mappings.find(addr);
@@ -303,28 +385,41 @@ StringValPtr DNS_Mgr::LookupAddr(const IPAddr& addr)
 				return d->Host();
 			else
 				{
-				string s(addr);
+				std::string s(addr);
 				reporter->Warning("can't resolve IP address: %s", s.c_str());
 				return make_intrusive<StringVal>(s.c_str());
 				}
 			}
 		}
 
+	const uint32_t* bytes;
+	int len = addr.GetBytes(&bytes);
+
 	// Not found, or priming.
 	switch ( mode )
 		{
 		case DNS_PRIME:
-			requests.push_back(new DNS_Mgr_Request(addr));
+			{
+			auto req = new DNS_Mgr_Request(addr);
+			ares_gethostbyaddr(channel, bytes, len, addr.GetFamily() == IPv4 ? AF_INET : AF_INET6,
+			                   hostbyaddr_callback, req);
 			return make_intrusive<StringVal>("<none>");
+			}
 
 		case DNS_FORCE:
 			reporter->FatalError("can't find DNS entry for %s in cache", addr.AsString().c_str());
 			return nullptr;
 
 		case DNS_DEFAULT:
-			requests.push_back(new DNS_Mgr_Request(addr));
+			{
+			auto req = new DNS_Mgr_Request(addr);
+			ares_gethostbyaddr(channel, bytes, len, addr.GetFamily() == IPv4 ? AF_INET : AF_INET6,
+			                   hostbyaddr_callback, req);
 			Resolve();
+
+			// Call LookupAddr() a second time to get the newly stored value out of the cache.
 			return LookupAddr(addr);
+			}
 
 		default:
 			reporter->InternalError("bad mode in DNS_Mgr::LookupAddr");
@@ -332,109 +427,35 @@ StringValPtr DNS_Mgr::LookupAddr(const IPAddr& addr)
 		}
 	}
 
-void DNS_Mgr::Verify() { }
+constexpr int MAX_PENDING_REQUESTS = 20;
-
-#define MAX_PENDING_REQUESTS 20
 
 void DNS_Mgr::Resolve()
 	{
-	if ( ! nb_dns )
+	int nfds = 0;
-		return;
+	struct timeval *tvp, tv;
+	fd_set read_fds, write_fds;
 
-	int i;
+	tv.tv_sec = DNS_TIMEOUT;
+	tv.tv_usec = 0;
 
-	int first_req = 0;
+	for ( int i = 0; i < MAX_PENDING_REQUESTS; i++ )
-	int num_pending = min(requests.length(), MAX_PENDING_REQUESTS);
-	int last_req = num_pending - 1;
-
-	// Prime with the initial requests.
-	for ( i = first_req; i <= last_req; ++i )
-		requests[i]->MakeRequest(nb_dns);
-
-	// Start resolving.  Each time an answer comes in, we can issue a
-	// new request, if we have more.
-	while ( num_pending > 0 )
 		{
-		int status = AnswerAvailable(DNS_TIMEOUT);
+		FD_ZERO(&read_fds);
+		FD_ZERO(&write_fds);
+		nfds = ares_fds(channel, &read_fds, &write_fds);
+		if ( nfds == 0 )
+			break;
 
-		if ( status <= 0 )
+		tvp = ares_timeout(channel, &tv, &tv);
-			{
+		select(nfds, &read_fds, &write_fds, NULL, tvp);
-			// Error or timeout.  Process all pending requests as
+		ares_process(channel, &read_fds, &write_fds);
-			// unanswered and reprime.
-			for ( i = first_req; i <= last_req; ++i )
-				{
-				DNS_Mgr_Request* dr = requests[i];
-				if ( dr->RequestPending() )
-					{
-					AddResult(dr, nullptr);
-					dr->RequestDone();
 		}
 	}
 
-			first_req = last_req + 1;
-			num_pending = min(requests.length() - first_req, MAX_PENDING_REQUESTS);
-			last_req = first_req + num_pending - 1;
-
-			for ( i = first_req; i <= last_req; ++i )
-				requests[i]->MakeRequest(nb_dns);
-
-			continue;
-			}
-
-		char err[NB_DNS_ERRSIZE];
-		struct nb_dns_result r;
-		status = nb_dns_activity(nb_dns, &r, err);
-		if ( status < 0 )
-			reporter->Warning("NB-DNS error in DNS_Mgr::WaitForReplies (%s)", err);
-		else if ( status > 0 )
-			{
-			DNS_Mgr_Request* dr = (DNS_Mgr_Request*)r.cookie;
-			if ( dr->RequestPending() )
-				{
-				AddResult(dr, &r);
-				dr->RequestDone();
-				}
-
-			// Room for another, if we have it.
-			if ( last_req < requests.length() - 1 )
-				{
-				++last_req;
-				requests[last_req]->MakeRequest(nb_dns);
-				}
-			else
-				--num_pending;
-			}
-		}
-
-	// All done with the list of requests.
-	for ( i = requests.length() - 1; i >= 0; --i )
-		delete requests.remove_nth(i);
-	}
-
-bool DNS_Mgr::Save()
-	{
-	if ( ! cache_name )
-		return false;
-
-	FILE* f = fopen(cache_name, "w");
-
-	if ( ! f )
-		return false;
-
-	Save(f, host_mappings);
-	Save(f, addr_mappings);
-	// Save(f, text_mappings); // We don't save the TXT mappings (yet?).
-
-	fclose(f);
-
-	return true;
-	}
-
 void DNS_Mgr::Event(EventHandlerPtr e, DNS_Mapping* dm)
 	{
 	if ( ! e )
 		return;
-
 	event_mgr.Enqueue(e, BuildMappingVal(dm));
 	}
 
@@ -473,14 +494,11 @@ ValPtr DNS_Mgr::BuildMappingVal(DNS_Mapping* dm)
 	return r;
 	}
 
-void DNS_Mgr::AddResult(DNS_Mgr_Request* dr, struct nb_dns_result* r)
+void DNS_Mgr::AddResult(DNS_Mgr_Request* dr, struct hostent* h, uint32_t ttl)
 	{
-	struct hostent* h = (r && r->host_errno == 0) ? r->hostent : nullptr;
-	u_int32_t ttl = (r && r->host_errno == 0) ? r->ttl : 0;
-
 	DNS_Mapping* new_dm;
 	DNS_Mapping* prev_dm;
-	int keep_prev = 0;
+	bool keep_prev = false;
 
 	if ( dr->ReqHost() )
 		{
@@ -502,7 +520,7 @@ void DNS_Mgr::AddResult(DNS_Mgr_Request* dr, struct nb_dns_result* r)
 			if ( new_dm->Failed() && prev_dm && prev_dm->Valid() )
 				{
 				text_mappings[dr->ReqHost()] = prev_dm;
-				++keep_prev;
+				keep_prev = true;
 				}
 			}
 		else
@@ -537,7 +555,7 @@ void DNS_Mgr::AddResult(DNS_Mgr_Request* dr, struct nb_dns_result* r)
 				else
 					host_mappings[dr->ReqHost()].second = prev_dm;
 
-				++keep_prev;
+				keep_prev = true;
 				}
 			}
 		}
@@ -551,7 +569,7 @@ void DNS_Mgr::AddResult(DNS_Mgr_Request* dr, struct nb_dns_result* r)
 		if ( new_dm->Failed() && prev_dm && prev_dm->Valid() )
 			{
 			addr_mappings[dr->ReqAddr()] = prev_dm;
-			++keep_prev;
+			keep_prev = true;
 			}
 		}
 
@@ -644,11 +662,14 @@ void DNS_Mgr::DumpAddrList(FILE* f, ListVal* al)
 		}
 	}
 
-void DNS_Mgr::LoadCache(FILE* f)
+void DNS_Mgr::LoadCache(const std::string& path)
 	{
+	FILE* f = fopen(path.c_str(), "r");
+
 	if ( ! f )
 		return;
 
+	// Loop until we find a mapping that doesn't initialize correctly.
 	DNS_Mapping* m = new DNS_Mapping(f);
 	for ( ; ! m->NoMapping() && ! m->InitFailed(); m = new DNS_Mapping(f) )
 		{
@@ -677,6 +698,25 @@ void DNS_Mgr::LoadCache(FILE* f)
 	fclose(f);
 	}
 
+bool DNS_Mgr::Save()
+	{
+	if ( cache_name.empty() )
+		return false;
+
+	FILE* f = fopen(cache_name.c_str(), "w");
+
+	if ( ! f )
+		return false;
+
+	Save(f, host_mappings);
+	Save(f, addr_mappings);
+	// Save(f, text_mappings); // We don't save the TXT mappings (yet?).
+
+	fclose(f);
+
+	return true;
+	}
+
 void DNS_Mgr::Save(FILE* f, const AddrMap& m)
 	{
 	for ( AddrMap::const_iterator it = m.begin(); it != m.end(); ++it )
@@ -688,9 +728,7 @@ void DNS_Mgr::Save(FILE* f, const AddrMap& m)
 
 void DNS_Mgr::Save(FILE* f, const HostMap& m)
 	{
-	HostMap::const_iterator it;
+	for ( HostMap::const_iterator it = m.begin(); it != m.end(); ++it )
-
-	for ( it = m.begin(); it != m.end(); ++it )
 		{
 		if ( it->second.first )
 			it->second.first->Save(f);
@@ -721,7 +759,7 @@ const char* DNS_Mgr::LookupAddrInCache(const IPAddr& addr)
 	return d->names.empty() ? "<\?\?\?>" : d->names[0].c_str();
 	}
 
-TableValPtr DNS_Mgr::LookupNameInCache(const string& name)
+TableValPtr DNS_Mgr::LookupNameInCache(const std::string& name)
 	{
 	HostMap::iterator it = host_mappings.find(name);
 	if ( it == host_mappings.end() )
@@ -750,7 +788,7 @@ TableValPtr DNS_Mgr::LookupNameInCache(const string& name)
 	return tv6;
 	}
 
-const char* DNS_Mgr::LookupTextInCache(const string& name)
+const char* DNS_Mgr::LookupTextInCache(const std::string& name)
 	{
 	TextMap::iterator it = text_mappings.find(name);
 	if ( it == text_mappings.end() )
@@ -792,6 +830,8 @@ static void resolve_lookup_cb(DNS_Mgr::LookupCallback* callback, const char* res
 
 void DNS_Mgr::AsyncLookupAddr(const IPAddr& host, LookupCallback* callback)
 	{
+	// This should have been run already from InitPostScript(), but just run it again just
+	// in case it hadn't.
 	InitSource();
 
 	if ( mode == DNS_FAKE )
@@ -828,8 +868,10 @@ void DNS_Mgr::AsyncLookupAddr(const IPAddr& host, LookupCallback* callback)
 	IssueAsyncRequests();
 	}
 
-void DNS_Mgr::AsyncLookupName(const string& name, LookupCallback* callback)
+void DNS_Mgr::AsyncLookupName(const std::string& name, LookupCallback* callback)
 	{
+	// This should have been run already from InitPostScript(), but just run it again just
+	// in case it hadn't.
 	InitSource();
 
 	if ( mode == DNS_FAKE )
@@ -866,8 +908,10 @@ void DNS_Mgr::AsyncLookupName(const string& name, LookupCallback* callback)
 	IssueAsyncRequests();
 	}
 
-void DNS_Mgr::AsyncLookupNameText(const string& name, LookupCallback* callback)
+void DNS_Mgr::AsyncLookupNameText(const std::string& name, LookupCallback* callback)
 	{
+	// This should have been run already from InitPostScript(), but just run it again just
+	// in case it hadn't.
 	InitSource();
 
 	if ( mode == DNS_FAKE )
@@ -906,51 +950,35 @@ void DNS_Mgr::AsyncLookupNameText(const string& name, LookupCallback* callback)
 	IssueAsyncRequests();
 	}
 
-static bool DoRequest(nb_dns_info* nb_dns, DNS_Mgr_Request* dr)
-	{
-	if ( dr->MakeRequest(nb_dns) )
-		// dr stored in nb_dns cookie and deleted later when results available.
-		return true;
-
-	reporter->Warning("can't issue DNS request");
-	delete dr;
-	return false;
-	}
-
 void DNS_Mgr::IssueAsyncRequests()
 	{
-	while ( asyncs_queued.size() && asyncs_pending < MAX_PENDING_REQUESTS )
+	while ( ! asyncs_queued.empty() && asyncs_pending < MAX_PENDING_REQUESTS )
 		{
 		AsyncRequest* req = asyncs_queued.front();
 		asyncs_queued.pop_front();
 
 		++num_requests;
-
+		req->time = util::current_time();
-		bool success;
 
 		if ( req->IsAddrReq() )
-			success = DoRequest(nb_dns, new DNS_Mgr_Request(req->host));
+			{
+			auto* m_req = new DNS_Mgr_Request(req->host);
+			m_req->MakeRequest(channel);
+			}
 		else if ( req->is_txt )
-			success = DoRequest(nb_dns,
+			{
-			                    new DNS_Mgr_Request(req->name.c_str(), AF_INET, req->is_txt));
+			auto* m_req = new DNS_Mgr_Request(req->name.c_str(), AF_INET, req->is_txt);
+			m_req->MakeRequest(channel);
+			}
 		else
 			{
 			// If only one request type succeeds, don't consider it a failure.
-			success = DoRequest(nb_dns,
+			auto* m_req4 = new DNS_Mgr_Request(req->name.c_str(), AF_INET, req->is_txt);
-			                    new DNS_Mgr_Request(req->name.c_str(), AF_INET, req->is_txt));
+			m_req4->MakeRequest(channel);
-			success = DoRequest(nb_dns,
+			auto* m_req6 = new DNS_Mgr_Request(req->name.c_str(), AF_INET6, req->is_txt);
-			                    new DNS_Mgr_Request(req->name.c_str(), AF_INET6, req->is_txt)) ||
+			m_req6->MakeRequest(channel);
-			          success;
 			}
 
-		if ( ! success )
-			{
-			req->Timeout();
-			++failed;
-			continue;
-			}
-
-		req->time = util::current_time();
 		asyncs_timeouts.push(req);
 
 		++asyncs_pending;
@@ -1088,10 +1116,7 @@ double DNS_Mgr::GetNextTimeout()
 
 void DNS_Mgr::Process()
 	{
-	if ( ! nb_dns )
+	while ( ! asyncs_timeouts.empty() )
-		return;
-
-	while ( asyncs_timeouts.size() > 0 )
 		{
 		AsyncRequest* req = asyncs_timeouts.top();
 
@@ -1112,9 +1137,10 @@ void DNS_Mgr::Process()
 		delete req;
 		}
 
-	if ( AnswerAvailable(0) <= 0 )
+	Resolve();
-		return;
 
+	// TODO: what does the rest below do?
+	/*
 	char err[NB_DNS_ERRSIZE];
 	struct nb_dns_result r;
 
@@ -1150,50 +1176,12 @@ void DNS_Mgr::Process()
 
 	    delete dr;
 	    }
-	}
+	*/
-
-int DNS_Mgr::AnswerAvailable(int timeout)
-	{
-	if ( ! nb_dns )
-		return -1;
-
-	int fd = nb_dns_fd(nb_dns);
-	if ( fd < 0 )
-		{
-		reporter->Warning("nb_dns_fd() failed in DNS_Mgr::WaitForReplies");
-		return -1;
-		}
-
-	fd_set read_fds;
-
-	FD_ZERO(&read_fds);
-	FD_SET(fd, &read_fds);
-
-	struct timeval t;
-	t.tv_sec = timeout;
-	t.tv_usec = 0;
-
-	int status = select(fd + 1, &read_fds, 0, 0, &t);
-
-	if ( status < 0 )
-		{
-		if ( errno != EINTR )
-			reporter->Warning("problem with DNS select");
-
-		return -1;
-		}
-
-	if ( status > 1 )
-		{
-		reporter->Warning("strange return from DNS select");
-		return -1;
-		}
-
-	return status;
 	}
 
 void DNS_Mgr::GetStats(Stats* stats)
 	{
+	// TODO: can this use the telemetry framework?
 	stats->requests = num_requests;
 	stats->successful = successful;
 	stats->failed = failed;
@@ -1203,12 +1191,6 @@ void DNS_Mgr::GetStats(Stats* stats)
 	stats->cached_texts = text_mappings.size();
 	}
 
-void DNS_Mgr::Terminate()
-	{
-	if ( nb_dns )
-		iosource_mgr->UnregisterFd(nb_dns_fd(nb_dns), this);
-	}
-
 void DNS_Mgr::TestProcess()
 	{
 	// Only allow usage of this method when running unit tests.
@@ -1218,11 +1200,11 @@ void DNS_Mgr::TestProcess()
 
 void DNS_Mgr::AsyncRequest::Resolved(const char* name)
 	{
-	for ( CallbackList::iterator i = callbacks.begin(); i != callbacks.end(); ++i )
+	for ( const auto& cb : callbacks )
 		{
-		(*i)->Resolved(name);
+		cb->Resolved(name);
 		if ( ! doctest::is_running_in_test )
-			delete *i;
+			delete cb;
 		}
 
 	callbacks.clear();
@@ -1231,11 +1213,11 @@ void DNS_Mgr::AsyncRequest::Resolved(const char* name)
 
 void DNS_Mgr::AsyncRequest::Resolved(TableVal* addrs)
 	{
-	for ( CallbackList::iterator i = callbacks.begin(); i != callbacks.end(); ++i )
+	for ( const auto& cb : callbacks )
 		{
-		(*i)->Resolved(addrs);
+		cb->Resolved(addrs);
 		if ( ! doctest::is_running_in_test )
-			delete *i;
+			delete cb;
 		}
 
 	callbacks.clear();
@@ -1244,11 +1226,11 @@ void DNS_Mgr::AsyncRequest::Resolved(TableVal* addrs)
 
 void DNS_Mgr::AsyncRequest::Timeout()
 	{
-	for ( CallbackList::iterator i = callbacks.begin(); i != callbacks.end(); ++i )
+	for ( const auto& cb : callbacks )
 		{
-		(*i)->Timeout();
+		cb->Timeout();
 		if ( ! doctest::is_running_in_test )
-			delete *i;
+			delete cb;
 		}
 
 	callbacks.clear();
@@ -1257,6 +1239,8 @@ void DNS_Mgr::AsyncRequest::Timeout()
 
 TableValPtr DNS_Mgr::empty_addr_set()
 	{
+	// TODO: can this be returned statically as well? Does the result get used in a way
+	// that would modify the same value being returned repeatedly?
 	auto addr_t = base_type(TYPE_ADDR);
 	auto set_index = make_intrusive<TypeList>(addr_t);
 	set_index->Append(std::move(addr_t));
@@ -1333,7 +1317,6 @@ TEST_CASE("dns_mgr prime,save,load")
 	auto addr_result = mgr.LookupAddr(ones);
 	CHECK(strcmp(addr_result->CheckString(), "<none>") == 0);
 
-	mgr.Verify();
 	mgr.Resolve();
 
 	// Save off the resulting values from Resolve() into a file on disk
@@ -1373,7 +1356,6 @@ TEST_CASE("dns_mgr alternate server")
 	// DNS_Mgr mgr2(DNS_DEFAULT, true);
 	// mgr2.InitPostScript();
 	// result = mgr2.LookupAddr("1.1.1.1");
-	// mgr2.Verify();
 	// mgr2.Resolve();
 
 	// result = mgr2.LookupAddr("1.1.1.1");

src/DNS_Mgr.h

@@ -2,6 +2,7 @@
 
 #pragma once
 
+#include <ares.h>
 #include <list>
 #include <map>
 #include <queue>
@@ -15,9 +16,6 @@
 
 namespace zeek
 	{
-
-class EventHandler;
-class RecordType;
 class Val;
 class ListVal;
 class TableVal;
@@ -31,16 +29,9 @@ using StringValPtr = IntrusivePtr<StringVal>;
 
 	} // namespace zeek
 
-// Defined in nb_dns.h
-struct nb_dns_info;
-struct nb_dns_result;
-
 namespace zeek::detail
 	{
-
 class DNS_Mgr_Request;
-using DNS_mgr_request_list = PList<DNS_Mgr_Request>;
-
 class DNS_Mapping;
 
 enum DNS_MgrMode
@@ -51,49 +42,111 @@ enum DNS_MgrMode
 	DNS_FAKE, // don't look up names, just return dummy results
 	};
 
-// Number of seconds we'll wait for a reply.
-#define DNS_TIMEOUT 5
-
 class DNS_Mgr final : public iosource::IOSource
 	{
 public:
 	explicit DNS_Mgr(DNS_MgrMode mode);
 	~DNS_Mgr() override;
 
+	/**
+	 * Finalizes the manager initialization. This should be called only after all
+	 * of the scripts have been parsed at startup.
+	 */
 	void InitPostScript();
+
+	/**
+	 * Attempts to process one more round of requests and then flushes the
+	 * mapping caches.
+	 */
 	void Flush();
 
-	// Looks up the address or addresses of the given host, and returns
+	/**
-	// a set of addr.
+	 * Looks up the address(es) of a given host and returns a set of addr.
+	 * This is a synchronous method and will block until results are ready.
+	 *
+	 * @param host The host name to look up an address for.
+	 * @return A set of addresses.
+	 */
 	TableValPtr LookupHost(const char* host);
 
+	/**
+	 * Looks up the hostname of a given address. This is a synchronous method
+	 * and will block until results are ready.
+	 *
+	 * @param host The addr to lookup a hostname for.
+	 * @return The hostname.
+	 */
 	StringValPtr LookupAddr(const IPAddr& addr);
 
-	// Define the directory where to store the data.
+	/**
-	void SetDir(const char* arg_dir) { dir = util::copy_string(arg_dir); }
+	 * Sets the directory where to store DNS data when Save() is called.
+	 */
+	void SetDir(const char* arg_dir) { dir = arg_dir; }
 
-	void Verify();
+	/**
+	 * Waits for responses to become available or a timeout to occur,
+	 * and handles any responses.
+	 */
 	void Resolve();
+
+	/**
+	 * Saves the current name and address caches to disk.
+	 */
 	bool Save();
 
-	const char* LookupAddrInCache(const IPAddr& addr);
+	/**
-	TableValPtr LookupNameInCache(const std::string& name);
+	 * Base class for callback handling for asynchronous lookups.
-	const char* LookupTextInCache(const std::string& name);
+	 */
-
-	// Support for async lookups.
 	class LookupCallback
 		{
 	public:
-		LookupCallback() { }
+		virtual ~LookupCallback() = default;
-		virtual ~LookupCallback() { }
 
+		/**
+		 * Called when an address lookup finishes.
+		 *
+		 * @param name The resulting name from the lookup.
+		 */
 		virtual void Resolved(const char* name){};
+
+		/**
+		 * Called when a name lookup finishes.
+		 *
+		 * @param addrs A table of the resulting addresses from the lookup.
+		 */
 		virtual void Resolved(TableVal* addrs){};
+
+		/**
+		 * Called when a timeout request occurs.
+		 */
 		virtual void Timeout() = 0;
 		};
 
+	/**
+	 * Schedules an asynchronous request to lookup a hostname for an IP address.
+	 * This is the equivalent of an "A" or "AAAA" request, depending on if the
+	 * address is ipv4 or ipv6.
+	 *
+	 * @param host The address to lookup names for.
+	 * @param callback A callback object to call when the request completes.
+	 */
 	void AsyncLookupAddr(const IPAddr& host, LookupCallback* callback);
+
+	/**
+	 * Schedules an asynchronous request to lookup an address for a hostname.
+	 * This is the equivalent of a "PTR" request.
+	 *
+	 * @param host The hostname to look up addresses for.
+	 * @param callback A callback object to call when the request completes.
+	 */
 	void AsyncLookupName(const std::string& name, LookupCallback* callback);
+
+	/**
+	 * Schedules an asynchronous TXT request for a hostname.
+	 *
+	 * @param host The address to lookup names for.
+	 * @param callback A callback object to call when the request completes.
+	 */
 	void AsyncLookupNameText(const std::string& name, LookupCallback* callback);
 
 	struct Stats
@@ -107,10 +160,27 @@ public:
 		unsigned long cached_texts;
 		};
 
+	/**
+	 * Returns the current statistics for the DNS_Manager.
+	 *
+	 * @param stats A pointer to a stats object to return the data in.
+	 */
 	void GetStats(Stats* stats);
 
-	void Terminate();
+	/**
+	 * Adds a result from a request to the caches. This is public so that the
+	 * callback methods can call it from outside of the DNS_Mgr class.
+	 *
+	 * @param dr The request associated with the result.
+	 * @param h A hostent structure containing the actual result data.
+	 * @param ttl A ttl value contained in the response from the server.
+	 */
+	void AddResult(DNS_Mgr_Request* dr, struct hostent* h, uint32_t ttl);
 
+	/**
+	 * Returns an empty set of addresses, used in various error cases and during
+	 * cache priming.
+	 */
 	static TableValPtr empty_addr_set();
 
 	/**
@@ -123,13 +193,16 @@ protected:
 	friend class LookupCallback;
 	friend class DNS_Mgr_Request;
 
+	const char* LookupAddrInCache(const IPAddr& addr);
+	TableValPtr LookupNameInCache(const std::string& name);
+	const char* LookupTextInCache(const std::string& name);
+
 	void Event(EventHandlerPtr e, DNS_Mapping* dm);
 	void Event(EventHandlerPtr e, DNS_Mapping* dm, ListValPtr l1, ListValPtr l2);
 	void Event(EventHandlerPtr e, DNS_Mapping* old_dm, DNS_Mapping* new_dm);
 
 	ValPtr BuildMappingVal(DNS_Mapping* dm);
 
-	void AddResult(DNS_Mgr_Request* dr, struct nb_dns_result* r);
 	void CompareMappings(DNS_Mapping* prev_dm, DNS_Mapping* new_dm);
 	ListValPtr AddrListDelta(ListVal* al1, ListVal* al2);
 	void DumpAddrList(FILE* f, ListVal* al);
@@ -137,15 +210,10 @@ protected:
 	using HostMap = std::map<std::string, std::pair<DNS_Mapping*, DNS_Mapping*>>;
 	using AddrMap = std::map<IPAddr, DNS_Mapping*>;
 	using TextMap = std::map<std::string, DNS_Mapping*>;
-	void LoadCache(FILE* f);
+	void LoadCache(const std::string& path);
 	void Save(FILE* f, const AddrMap& m);
 	void Save(FILE* f, const HostMap& m);
 
-	// Selects on the fd to see if there is an answer available (timeout
-	// is secs). Returns 0 on timeout, -1 on EINTR or other error, and 1
-	// if answer is ready.
-	int AnswerAvailable(int timeout);
-
 	// Issue as many queued async requests as slots are available.
 	void IssueAsyncRequests();
 
@@ -167,29 +235,30 @@ protected:
 	AddrMap addr_mappings;
 	TextMap text_mappings;
 
+	using DNS_mgr_request_list = PList<DNS_Mgr_Request>;
 	DNS_mgr_request_list requests;
 
-	nb_dns_info* nb_dns;
+	std::string cache_name;
-	char* cache_name;
+	std::string dir; // directory in which cache_name resides
-	char* dir; // directory in which cache_name resides
 
-	bool did_init;
+	bool did_init = false;
-	int asyncs_pending;
+	int asyncs_pending = 0;
 
 	RecordTypePtr dm_rec;
 
+	ares_channel channel;
+	bool ipv6_resolver = false;
+
 	using CallbackList = std::list<LookupCallback*>;
 
 	struct AsyncRequest
 		{
-		double time;
+		double time = 0.0;
 		IPAddr host;
 		std::string name;
 		CallbackList callbacks;
-		bool is_txt;
+		bool is_txt = false;
-		bool processed;
+		bool processed = false;
-
-		AsyncRequest() : time(0.0), is_txt(false), processed(false) { }
 
 		bool IsAddrReq() const { return name.empty(); }
 

src/zeek-setup.cc

@@ -18,6 +18,7 @@
 #include "zeek/3rdparty/sqlite3.h"
 
 #define DOCTEST_CONFIG_IMPLEMENT
+
 #include "zeek/3rdparty/doctest.h"
 #include "zeek/Anon.h"
 #include "zeek/DFA.h"
@@ -330,7 +331,6 @@ static void terminate_zeek()
 	input_mgr->Terminate();
 	thread_mgr->Terminate();
 	broker_mgr->Terminate();
-	dns_mgr->Terminate();
 
 	event_mgr.Drain();
 
@@ -342,6 +342,7 @@ static void terminate_zeek()
 	delete packet_mgr;
 	delete analyzer_mgr;
 	delete file_mgr;
+	delete dns_mgr;
 	// broker_mgr, timer_mgr, and supervisor are deleted via iosource_mgr
 	delete iosource_mgr;
 	delete event_registry;
@@ -460,6 +461,8 @@ SetupResult setup(int argc, char** argv, Options* zopts)
 	if ( dns_type == DNS_DEFAULT && fake_dns() )
 		dns_type = DNS_FAKE;
 
+	dns_mgr = new DNS_Mgr(dns_type);
+
 	RETSIGTYPE (*oldhandler)(int);
 
 	zeek_script_prefixes = options.script_prefixes;
@@ -599,8 +602,6 @@ SetupResult setup(int argc, char** argv, Options* zopts)
 
 	push_scope(nullptr, nullptr);
 
-	dns_mgr = new DNS_Mgr(dns_type);
-
 	// It would nice if this were configurable.  This is similar to the
 	// chicken and the egg problem.  It would be configurable by parsing
 	// policy, but we can't parse policy without DNS resolution.
@@ -756,6 +757,9 @@ SetupResult setup(int argc, char** argv, Options* zopts)
 		file_mgr->InitPostScript();
 		dns_mgr->InitPostScript();
 
+		dns_mgr->LookupHost("www.apple.com");
+		//		dns_mgr->LookupAddr("17.253.144.10");
+
 #ifdef USE_PERFTOOLS_DEBUG
 		}
 #endif
@@ -859,9 +863,12 @@ SetupResult setup(int argc, char** argv, Options* zopts)
 		if ( (oldhandler = setsignal(SIGHUP, sig_handler)) != SIG_DFL )
 			(void)setsignal(SIGHUP, oldhandler);
 
+		// If we were priming the DNS cache (i.e. -P was passed as an argument), flush anything
+		// remaining to be resolved and save the cache to disk. We can just exit now because
+		// we've done everything we need to do. The run loop isn't started in this case, so
+		// nothing else should be happening.
 		if ( dns_type == DNS_PRIME )
 			{
-			dns_mgr->Verify();
 			dns_mgr->Resolve();
 
 			if ( ! dns_mgr->Save() )

src/zeek.bif

@@ -3657,7 +3657,7 @@ public:
 		}
 
 	// Overridden from zeek::detail::DNS_Mgr:Lookup:Callback.
-	virtual void Resolved(const char* name)
+	void Resolved(const char* name) override
 		{
 		zeek::Val* result = new zeek::StringVal(name);
 		trigger->Cache(call, result);
@@ -3665,14 +3665,14 @@ public:
 		trigger->Release();
 		}
 
-	virtual void Resolved(zeek::TableVal* addrs)
+	void Resolved(zeek::TableVal* addrs) override
 		{
 		// No Ref() for addrs.
 		trigger->Cache(call, addrs);
 		trigger->Release();
 		}
 
-	virtual void Timeout()
+	void Timeout() override
 		{
 		if ( lookup_name )
 			{