From 679ce519dd0d6d3919990abdee7109a5eeb99aa0 Mon Sep 17 00:00:00 2001 From: itytophile <45497473+itytophile@users.noreply.github.com> Date: Fri, 23 Dec 2022 02:13:51 +0100 Subject: [PATCH] Fix DNSLookup memory leak (#422) * Fix memory leak with shared_ptr and -fsanitize=address * Replace addrinfo* by shared_ptr * fsanitize=address only on Linux * Add USE_WS Linux CI test * Remove fsanitize from the cmake files * Remove USE_WS in linux test suite --- ixwebsocket/IXDNSLookup.cpp | 24 ++++++++++-------------- ixwebsocket/IXDNSLookup.h | 17 ++++++++--------- ixwebsocket/IXSocketConnect.cpp | 5 ++--- test/IXDNSLookupTest.cpp | 10 +++------- ws/ws.cpp | 3 +-- 5 files changed, 24 insertions(+), 35 deletions(-) diff --git a/ixwebsocket/IXDNSLookup.cpp b/ixwebsocket/IXDNSLookup.cpp index 9a2874f1..b0283af7 100644 --- a/ixwebsocket/IXDNSLookup.cpp +++ b/ixwebsocket/IXDNSLookup.cpp @@ -23,6 +23,7 @@ #include #include #include +#include // mingw build quirks #if defined(_WIN32) && defined(__GNUC__) @@ -44,7 +45,7 @@ namespace ix ; } - struct addrinfo* DNSLookup::getAddrInfo(const std::string& hostname, + DNSLookup::AddrInfoPtr DNSLookup::getAddrInfo(const std::string& hostname, int port, std::string& errMsg) { @@ -63,10 +64,10 @@ namespace ix errMsg = gai_strerror(getaddrinfo_result); res = nullptr; } - return res; + return AddrInfoPtr{ res, freeaddrinfo }; } - struct addrinfo* DNSLookup::resolve(std::string& errMsg, + DNSLookup::AddrInfoPtr DNSLookup::resolve(std::string& errMsg, const CancellationRequest& isCancellationRequested, bool cancellable) { @@ -74,12 +75,7 @@ namespace ix : resolveUnCancellable(errMsg, isCancellationRequested); } - void DNSLookup::release(struct addrinfo* addr) - { - freeaddrinfo(addr); - } - - struct addrinfo* DNSLookup::resolveUnCancellable( + DNSLookup::AddrInfoPtr DNSLookup::resolveUnCancellable( std::string& errMsg, const CancellationRequest& isCancellationRequested) { errMsg = "no error"; @@ -94,7 +90,7 @@ namespace ix return getAddrInfo(_hostname, _port, errMsg); } - struct addrinfo* DNSLookup::resolveCancellable( + DNSLookup::AddrInfoPtr DNSLookup::resolveCancellable( std::string& errMsg, const CancellationRequest& isCancellationRequested) { errMsg = "no error"; @@ -157,7 +153,7 @@ namespace ix // gone, so we use temporary variables (res) or we pass in by copy everything that // getAddrInfo needs to work. std::string errMsg; - struct addrinfo* res = getAddrInfo(hostname, port, errMsg); + auto res = getAddrInfo(hostname, port, errMsg); if (auto lock = self.lock()) { @@ -181,13 +177,13 @@ namespace ix return _errMsg; } - void DNSLookup::setRes(struct addrinfo* addr) + void DNSLookup::setRes(DNSLookup::AddrInfoPtr addr) { std::lock_guard lock(_resMutex); - _res = addr; + _res = std::move(addr); } - struct addrinfo* DNSLookup::getRes() + DNSLookup::AddrInfoPtr DNSLookup::getRes() { std::lock_guard lock(_resMutex); return _res; diff --git a/ixwebsocket/IXDNSLookup.h b/ixwebsocket/IXDNSLookup.h index fcdd103d..d5a8ab52 100644 --- a/ixwebsocket/IXDNSLookup.h +++ b/ixwebsocket/IXDNSLookup.h @@ -24,22 +24,21 @@ namespace ix class DNSLookup : public std::enable_shared_from_this { public: + using AddrInfoPtr = std::shared_ptr; DNSLookup(const std::string& hostname, int port, int64_t wait = DNSLookup::kDefaultWait); ~DNSLookup() = default; - struct addrinfo* resolve(std::string& errMsg, + AddrInfoPtr resolve(std::string& errMsg, const CancellationRequest& isCancellationRequested, bool cancellable = true); - void release(struct addrinfo* addr); - private: - struct addrinfo* resolveCancellable(std::string& errMsg, + AddrInfoPtr resolveCancellable(std::string& errMsg, const CancellationRequest& isCancellationRequested); - struct addrinfo* resolveUnCancellable(std::string& errMsg, + AddrInfoPtr resolveUnCancellable(std::string& errMsg, const CancellationRequest& isCancellationRequested); - static struct addrinfo* getAddrInfo(const std::string& hostname, + AddrInfoPtr getAddrInfo(const std::string& hostname, int port, std::string& errMsg); @@ -48,15 +47,15 @@ namespace ix void setErrMsg(const std::string& errMsg); const std::string& getErrMsg(); - void setRes(struct addrinfo* addr); - struct addrinfo* getRes(); + void setRes(AddrInfoPtr addr); + AddrInfoPtr getRes(); std::string _hostname; int _port; int64_t _wait; const static int64_t kDefaultWait; - struct addrinfo* _res; + AddrInfoPtr _res; std::mutex _resMutex; std::string _errMsg; diff --git a/ixwebsocket/IXSocketConnect.cpp b/ixwebsocket/IXSocketConnect.cpp index 59ed76d4..6c9a6d5b 100644 --- a/ixwebsocket/IXSocketConnect.cpp +++ b/ixwebsocket/IXSocketConnect.cpp @@ -102,7 +102,7 @@ namespace ix // First do DNS resolution // auto dnsLookup = std::make_shared(hostname, port); - struct addrinfo* res = dnsLookup->resolve(errMsg, isCancellationRequested); + auto res = dnsLookup->resolve(errMsg, isCancellationRequested); if (res == nullptr) { return -1; @@ -112,7 +112,7 @@ namespace ix // iterate through the records to find a working peer struct addrinfo* address; - for (address = res; address != nullptr; address = address->ai_next) + for (address = res.get(); address != nullptr; address = address->ai_next) { // // Second try to connect to the remote host @@ -124,7 +124,6 @@ namespace ix } } - freeaddrinfo(res); return sockfd; } diff --git a/test/IXDNSLookupTest.cpp b/test/IXDNSLookupTest.cpp index a2405042..6902b3f8 100644 --- a/test/IXDNSLookupTest.cpp +++ b/test/IXDNSLookupTest.cpp @@ -19,13 +19,9 @@ TEST_CASE("dns", "[net]") auto dnsLookup = std::make_shared("www.google.com", 80); std::string errMsg; - struct addrinfo* res; - - res = dnsLookup->resolve(errMsg, [] { return false; }); + auto res = dnsLookup->resolve(errMsg, [] { return false; }); std::cerr << "Error message: " << errMsg << std::endl; REQUIRE(res != nullptr); - - dnsLookup->release(res); } SECTION("Test resolving a non-existing hostname") @@ -33,7 +29,7 @@ TEST_CASE("dns", "[net]") auto dnsLookup = std::make_shared("wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww", 80); std::string errMsg; - struct addrinfo* res = dnsLookup->resolve(errMsg, [] { return false; }); + auto res = dnsLookup->resolve(errMsg, [] { return false; }); std::cerr << "Error message: " << errMsg << std::endl; REQUIRE(res == nullptr); } @@ -44,7 +40,7 @@ TEST_CASE("dns", "[net]") std::string errMsg; // The callback returning true means we are requesting cancellation - struct addrinfo* res = dnsLookup->resolve(errMsg, [] { return true; }); + auto res = dnsLookup->resolve(errMsg, [] { return true; }); std::cerr << "Error message: " << errMsg << std::endl; REQUIRE(res == nullptr); } diff --git a/ws/ws.cpp b/ws/ws.cpp index a513e9bb..4d64be4c 100644 --- a/ws/ws.cpp +++ b/ws/ws.cpp @@ -892,9 +892,8 @@ namespace ix auto dnsLookup = std::make_shared(hostname, 80); std::string errMsg; - struct addrinfo* res; - res = dnsLookup->resolve(errMsg, [] { return false; }); + auto res = dnsLookup->resolve(errMsg, [] { return false; }); auto addr = res->ai_addr;