From d4682463a1d503c349ac95e275b11d67d402268c Mon Sep 17 00:00:00 2001 From: Mattes D Date: Sun, 18 Jan 2015 12:35:02 +0100 Subject: [PATCH] cNetwork: Fixed race conditions with lookups; proper shutdown. --- src/OSSupport/HostnameLookup.cpp | 25 +++++++-- src/OSSupport/HostnameLookup.h | 12 +++- src/OSSupport/IPLookup.cpp | 32 +++++++++-- src/OSSupport/IPLookup.h | 15 ++++- src/OSSupport/NetworkSingleton.cpp | 89 +++++++++++++++--------------- src/OSSupport/NetworkSingleton.h | 44 ++++++--------- tests/Network/Google.cpp | 1 + 7 files changed, 130 insertions(+), 88 deletions(-) diff --git a/src/OSSupport/HostnameLookup.cpp b/src/OSSupport/HostnameLookup.cpp index 9e35f7163..860e0d88f 100644 --- a/src/OSSupport/HostnameLookup.cpp +++ b/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(a_Callbacks); + cNetworkSingleton::Get().AddHostnameLookup(Lookup); + Lookup->Lookup(a_Hostname); + return true; } diff --git a/src/OSSupport/HostnameLookup.h b/src/OSSupport/HostnameLookup.h index 98f48b933..d69f24707 100644 --- a/src/OSSupport/HostnameLookup.h +++ b/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 cHostnameLookupPtr; typedef std::vector cHostnameLookupPtrs; diff --git a/src/OSSupport/IPLookup.cpp b/src/OSSupport/IPLookup.cpp index bbcfbfe40..a52b6480b 100644 --- a/src/OSSupport/IPLookup.cpp +++ b/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(sizeof(sa)); memset(&sa, 0, sizeof(sa)); - evutil_parse_sockaddr_port(a_IP.c_str(), reinterpret_cast(&sa), &salen); + if (evutil_parse_sockaddr_port(a_IP.c_str(), reinterpret_cast(&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(a_Callbacks); + cNetworkSingleton::Get().AddIPLookup(res); + return res->Lookup(a_IP); } diff --git a/src/OSSupport/IPLookup.h b/src/OSSupport/IPLookup.h index f39b955aa..af878cbf1 100644 --- a/src/OSSupport/IPLookup.h +++ b/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 cIPLookupPtr; typedef std::vector cIPLookupPtrs; diff --git a/src/OSSupport/NetworkSingleton.cpp b/src/OSSupport/NetworkSingleton.cpp index c9d9b1d81..8c38fb4eb 100644 --- a/src/OSSupport/NetworkSingleton.cpp +++ b/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(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(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); diff --git a/src/OSSupport/NetworkSingleton.h b/src/OSSupport/NetworkSingleton.h index 064e075fe..1d26fc8f4 100644 --- a/src/OSSupport/NetworkSingleton.h +++ b/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 cTCPLinkImplPtr; typedef std::vector cTCPLinkImplPtrs; +class cServerHandleImpl; typedef SharedPtr cServerHandleImplPtr; typedef std::vector cServerHandleImplPtrs; +class cHostnameLookup; typedef SharedPtr cHostnameLookupPtr; typedef std::vector cHostnameLookupPtrs; +class cIPLookup; typedef SharedPtr cIPLookupPtr; typedef std::vector cIPLookupPtrs; @@ -42,43 +43,29 @@ typedef std::vector 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); diff --git a/tests/Network/Google.cpp b/tests/Network/Google.cpp index de5f95e1f..8ee04f8ee 100644 --- a/tests/Network/Google.cpp +++ b/tests/Network/Google.cpp @@ -95,6 +95,7 @@ int main() evtFinish.Wait(); LOGD("Network test finished"); + return 0; }