cNetwork: Fixed race conditions with lookups; proper shutdown.

src/OSSupport/HostnameLookup.cpp

@@ -15,10 +15,22 @@
 ////////////////////////////////////////////////////////////////////////////////
 // cHostnameLookup:
 
-cHostnameLookup::cHostnameLookup(const AString & a_Hostname, cNetwork::cResolveNameCallbacksPtr a_Callbacks):
-	m_Callbacks(a_Callbacks),
-	m_Hostname(a_Hostname)
+cHostnameLookup::cHostnameLookup(cNetwork::cResolveNameCallbacksPtr a_Callbacks):
+	m_Callbacks(a_Callbacks)
 {
+}
+
+
+
+
+
+void cHostnameLookup::Lookup(const AString & a_Hostname)
+{
+	// Store the hostname for the callback:
+	m_Hostname = a_Hostname;
+
+	// Start the lookup:
+	// Note that we don't have to store the LibEvent lookup handle, LibEvent will free it on its own.
 	evutil_addrinfo hints;
 	memset(&hints, 0, sizeof(hints));
 	hints.ai_protocol = IPPROTO_TCP;
@@ -79,7 +91,7 @@ void cHostnameLookup::Callback(int a_ErrCode, evutil_addrinfo * a_Addr, void * a
 	// If only unsupported families were reported, call the Error handler:
 	if (!HasResolved)
 	{
-		Self->m_Callbacks->OnError(1);
+		Self->m_Callbacks->OnError(DNS_ERR_NODATA);
 	}
 	else
 	{
@@ -101,7 +113,10 @@ bool cNetwork::HostnameToIP(
 	cNetwork::cResolveNameCallbacksPtr a_Callbacks
 )
 {
-	return cNetworkSingleton::Get().HostnameToIP(a_Hostname, a_Callbacks);
+	auto Lookup = std::make_shared<cHostnameLookup>(a_Callbacks);
+	cNetworkSingleton::Get().AddHostnameLookup(Lookup);
+	Lookup->Lookup(a_Hostname);
+	return true;
 }
 
 

src/OSSupport/HostnameLookup.h

@@ -21,6 +21,15 @@
 /** Holds information about an in-progress Hostname-to-IP lookup. */
 class cHostnameLookup
 {
+public:
+	/** Creates the lookup object. Doesn't start the lookup yet. */
+	cHostnameLookup(cNetwork::cResolveNameCallbacksPtr a_Callbacks);
+
+	/** Starts the lookup. */
+	void Lookup(const AString & a_Hostname);
+
+protected:
+
 	/** The callbacks to call for resolved names / errors. */
 	cNetwork::cResolveNameCallbacksPtr m_Callbacks;
 
@@ -28,9 +37,6 @@ class cHostnameLookup
 	AString m_Hostname;
 
 	static void Callback(int a_ErrCode, struct evutil_addrinfo * a_Addr, void * a_Self);
-
-public:
-	cHostnameLookup(const AString & a_Hostname, cNetwork::cResolveNameCallbacksPtr a_Callbacks);
 };
 typedef SharedPtr<cHostnameLookup> cHostnameLookupPtr;
 typedef std::vector<cHostnameLookupPtr> cHostnameLookupPtrs;

src/OSSupport/IPLookup.cpp

@@ -15,14 +15,31 @@
 ////////////////////////////////////////////////////////////////////////////////
 // cIPLookup:
 
-cIPLookup::cIPLookup(const AString & a_IP, cNetwork::cResolveNameCallbacksPtr a_Callbacks):
-	m_Callbacks(a_Callbacks),
-	m_IP(a_IP)
+cIPLookup::cIPLookup(cNetwork::cResolveNameCallbacksPtr a_Callbacks):
+	m_Callbacks(a_Callbacks)
 {
+	ASSERT(a_Callbacks != nullptr);
+}
+
+
+
+
+
+bool cIPLookup::Lookup(const AString & a_IP)
+{
+	// Parse the IP address string into a sockaddr structure:
+	m_IP = a_IP;
 	sockaddr_storage sa;
 	int salen = static_cast<int>(sizeof(sa));
 	memset(&sa, 0, sizeof(sa));
-	evutil_parse_sockaddr_port(a_IP.c_str(), reinterpret_cast<sockaddr *>(&sa), &salen);
+	if (evutil_parse_sockaddr_port(a_IP.c_str(), reinterpret_cast<sockaddr *>(&sa), &salen) != 0)
+	{
+		LOGD("Failed to parse IP address \"%s\".", a_IP.c_str());
+		return false;
+	}
+
+	// Call the proper resolver based on the address family:
+	// Note that there's no need to store the evdns_request handle returned, LibEvent frees it on its own.
 	switch (sa.ss_family)
 	{
 		case AF_INET:
@@ -41,9 +58,10 @@ cIPLookup::cIPLookup(const AString & a_IP, cNetwork::cResolveNameCallbacksPtr a_
 		{
 			LOGWARNING("%s: Unknown address family: %d", __FUNCTION__, sa.ss_family);
 			ASSERT(!"Unknown address family");
-			break;
+			return false;
 		}
 	}  // switch (address family)
+	return true;
 }
 
 
@@ -83,7 +101,9 @@ bool cNetwork::IPToHostName(
 	cNetwork::cResolveNameCallbacksPtr a_Callbacks
 )
 {
-	return cNetworkSingleton::Get().IPToHostName(a_IP, a_Callbacks);
+	auto res = std::make_shared<cIPLookup>(a_Callbacks);
+	cNetworkSingleton::Get().AddIPLookup(res);
+	return res->Lookup(a_IP);
 }
 
 

src/OSSupport/IPLookup.h

@@ -20,16 +20,25 @@
 /** Holds information about an in-progress IP-to-Hostname lookup. */
 class cIPLookup
 {
+public:
+	/** Creates the lookup object. Doesn't start the lookup yet. */
+	cIPLookup(cNetwork::cResolveNameCallbacksPtr a_Callbacks);
+
+	/** Starts the lookup.
+	Returns true if lookup started successfully, false on failure (invalid IP format etc.) */
+	bool Lookup(const AString & a_IP);
+
+protected:
+
 	/** The callbacks to call for resolved names / errors. */
 	cNetwork::cResolveNameCallbacksPtr m_Callbacks;
 
 	/** The IP that was queried (needed for the callbacks). */
 	AString m_IP;
 
-	static void Callback(int a_Result, char a_Type, int a_Count, int a_Ttl, void * a_Addresses, void * a_Self);
 
-public:
-	cIPLookup(const AString & a_IP, cNetwork::cResolveNameCallbacksPtr a_Callbacks);
+	/** Callback that is called by LibEvent when there's an event for the request. */
+	static void Callback(int a_Result, char a_Type, int a_Count, int a_Ttl, void * a_Addresses, void * a_Self);
 };
 typedef SharedPtr<cIPLookup> cIPLookupPtr;
 typedef std::vector<cIPLookupPtr> cIPLookupPtrs;

src/OSSupport/NetworkSingleton.cpp

@@ -70,6 +70,30 @@ cNetworkSingleton::cNetworkSingleton(void)
 
 
 
+cNetworkSingleton::~cNetworkSingleton()
+{
+	// Wait for the LibEvent event loop to terminate:
+	event_base_loopbreak(m_EventBase);
+	m_EventLoopTerminated.Wait();
+
+	// Remove all objects:
+	{
+		cCSLock Lock(m_CS);
+		m_Connections.clear();
+		m_Servers.clear();
+		m_HostnameLookups.clear();
+		m_IPLookups.clear();
+	}
+
+	// Free the underlying LibEvent objects:
+	evdns_base_free(m_DNSBase, true);
+	event_base_free(m_EventBase);
+}
+
+
+
+
+
 cNetworkSingleton & cNetworkSingleton::Get(void)
 {
 	static cNetworkSingleton Instance;
@@ -80,50 +104,6 @@ cNetworkSingleton & cNetworkSingleton::Get(void)
 
 
 
-bool cNetworkSingleton::HostnameToIP(
-	const AString & a_Hostname,
-	cNetwork::cResolveNameCallbacksPtr a_Callbacks
-)
-{
-	try
-	{
-		// TODO: This has a race condition with possible memory leak:
-		// If a lookup finishes immediately, the constructor calls the removal before this addition
-		cCSLock Lock(m_CS);
-		m_HostnameLookups.push_back(std::make_shared<cHostnameLookup>(a_Hostname, a_Callbacks));
-	}
-	catch (...)
-	{
-		return false;
-	}
-	return true;
-}
-
-
-
-
-bool cNetworkSingleton::IPToHostName(
-	const AString & a_IP,
-	cNetwork::cResolveNameCallbacksPtr a_Callbacks
-)
-{
-	try
-	{
-		// TODO: This has a race condition with possible memory leak:
-		// If a lookup finishes immediately, the constructor calls the removal before this addition
-		cCSLock Lock(m_CS);
-		m_IPLookups.push_back(std::make_shared<cIPLookup>(a_IP, a_Callbacks));
-	}
-	catch (...)
-	{
-		return false;
-	}
-	return true;
-}
-
-
-
-
 void cNetworkSingleton::LogCallback(int a_Severity, const char * a_Msg)
 {
 	switch (a_Severity)
@@ -147,6 +127,17 @@ void cNetworkSingleton::LogCallback(int a_Severity, const char * a_Msg)
 void cNetworkSingleton::RunEventLoop(cNetworkSingleton * a_Self)
 {
 	event_base_loop(a_Self->m_EventBase, EVLOOP_NO_EXIT_ON_EMPTY);
+	a_Self->m_EventLoopTerminated.Set();
+}
+
+
+
+
+
+void cNetworkSingleton::AddHostnameLookup(cHostnameLookupPtr a_HostnameLookup)
+{
+	cCSLock Lock(m_CS);
+	m_HostnameLookups.push_back(a_HostnameLookup);
 }
 
 
@@ -170,6 +161,16 @@ void cNetworkSingleton::RemoveHostnameLookup(const cHostnameLookup * a_HostnameL
 
 
 
+void cNetworkSingleton::AddIPLookup(cIPLookupPtr a_IPLookup)
+{
+	cCSLock Lock(m_CS);
+	m_IPLookups.push_back(a_IPLookup);
+}
+
+
+
+
+
 void cNetworkSingleton::RemoveIPLookup(const cIPLookup * a_IPLookup)
 {
 	cCSLock Lock(m_CS);

src/OSSupport/NetworkSingleton.h

@@ -14,6 +14,7 @@
 
 #include "Network.h"
 #include "CriticalSection.h"
+#include "Event.h"
 
 
 
@@ -22,16 +23,16 @@
 // fwd:
 struct event_base;
 struct evdns_base;
-class cServerHandleImpl;
 class cTCPLinkImpl;
-class cHostnameLookup;
-class cIPLookup;
 typedef SharedPtr<cTCPLinkImpl> cTCPLinkImplPtr;
 typedef std::vector<cTCPLinkImplPtr> cTCPLinkImplPtrs;
+class cServerHandleImpl;
 typedef SharedPtr<cServerHandleImpl> cServerHandleImplPtr;
 typedef std::vector<cServerHandleImplPtr> cServerHandleImplPtrs;
+class cHostnameLookup;
 typedef SharedPtr<cHostnameLookup> cHostnameLookupPtr;
 typedef std::vector<cHostnameLookupPtr> cHostnameLookupPtrs;
+class cIPLookup;
 typedef SharedPtr<cIPLookup> cIPLookupPtr;
 typedef std::vector<cIPLookupPtr> cIPLookupPtrs;
 
@@ -42,43 +43,29 @@ typedef std::vector<cIPLookupPtr> cIPLookupPtrs;
 class cNetworkSingleton
 {
 public:
+	~cNetworkSingleton();
+
 	/** Returns the singleton instance of this class */
 	static cNetworkSingleton & Get(void);
 
-
-	// The following functions are implementations for the cNetwork class
-
-	/** Queues a DNS query to resolve the specified hostname to IP address.
-	Calls one of the callbacks when the resolving succeeds, or when it fails.
-	Returns true if queueing was successful, false if not.
-	Note that the return value doesn't report the success of the actual lookup; the lookup happens asynchronously on the background.
-	TODO: Move this out into a separate file with cHostnameLookup. */
-	bool HostnameToIP(
-		const AString & a_Hostname,
-		cNetwork::cResolveNameCallbacksPtr a_Callbacks
-	);
-
-
-	/** Queues a DNS query to resolve the specified IP address to a hostname.
-	Calls one of the callbacks when the resolving succeeds, or when it fails.
-	Returns true if queueing was successful, false if not.
-	Note that the return value doesn't report the success of the actual lookup; the lookup happens asynchronously on the background.
-	TODO: Move this out into a separate file with cIPLookup. */
-	bool IPToHostName(
-		const AString & a_IP,
-		cNetwork::cResolveNameCallbacksPtr a_Callbacks
-	);
-
 	/** Returns the main LibEvent handle for event registering. */
 	event_base * GetEventBase(void) { return m_EventBase; }
 
 	/** Returns the LibEvent handle for DNS lookups. */
 	evdns_base * GetDNSBase(void) { return m_DNSBase; }
 
+	/** Adds the specified hostname lookup to m_HostnameLookups.
+	Used by the underlying lookup implementation when a new lookup is initiated. */
+	void AddHostnameLookup(cHostnameLookupPtr a_HostnameLookup);
+
 	/** Removes the specified hostname lookup from m_HostnameLookups.
 	Used by the underlying lookup implementation when the lookup is finished. */
 	void RemoveHostnameLookup(const cHostnameLookup * a_HostnameLookup);
 
+	/** Adds the specified IP lookup to M_IPLookups.
+	Used by the underlying lookup implementation when a new lookup is initiated. */
+	void AddIPLookup(cIPLookupPtr a_IPLookup);
+
 	/** Removes the specified IP lookup from m_IPLookups.
 	Used by the underlying lookup implementation when the lookup is finished. */
 	void RemoveIPLookup(const cIPLookup * a_IPLookup);
@@ -123,6 +110,9 @@ protected:
 	/** Mutex protecting all containers against multithreaded access. */
 	cCriticalSection m_CS;
 
+	/** Event that gets signalled when the event loop terminates. */
+	cEvent m_EventLoopTerminated;
+
 
 	/** Initializes the LibEvent internals. */
 	cNetworkSingleton(void);

tests/Network/Google.cpp

@@ -95,6 +95,7 @@ int main()
 	
 	evtFinish.Wait();
 	LOGD("Network test finished");
+	return 0;
 }