Fix some text entry leaks on Darwin. (#21286)

We can get OnNewInterface twice for the same interface. When the happens we replace the old InterfaceInfo with a new one in the map. Because our cleanup was from ~ResolveContext, that meant we leaked any allocated memory the InterfaceInfo was holding on to. The fix is to make InterfaceInfo manage its own memory and make sure we transfer the ownership properly via move constructors when inserting into the map.
project-chip · Sep 19, 2023 · 5649017 · 5649017
1 parent d92d172
commit 5649017
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 19 deletions.
diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp
@@ -302,10 +302,7 @@ ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::
     protocol = GetProtocol(cbAddressType);
 }
 
-ResolveContext::~ResolveContext()
-{
-    RemoveInterfaces();
-}
+ResolveContext::~ResolveContext() {}
 
 void ResolveContext::DispatchFailure(DNSServiceErrorType err)
 {
@@ -377,8 +374,8 @@ bool ResolveContext::HasAddress()
 void ResolveContext::OnNewInterface(uint32_t interfaceId, const char * fullname, const char * hostnameWithDomain, uint16_t port,
                                     uint16_t txtLen, const unsigned char * txtRecord)
 {
-    ChipLogDetail(Discovery, "Mdns : %s hostname:%s fullname:%s interface: %" PRIu32, __func__, hostnameWithDomain, fullname,
-                  interfaceId);
+    ChipLogDetail(Discovery, "Mdns : %s hostname:%s fullname:%s interface: %" PRIu32 " port: %u TXT:\"%.*s\"", __func__,
+                  hostnameWithDomain, fullname, interfaceId, port, static_cast<int>(txtLen), txtRecord);
 
     InterfaceInfo interface;
     interface.service.mPort = ntohs(port);
@@ -406,29 +403,45 @@ void ResolveContext::OnNewInterface(uint32_t interfaceId, const char * fullname,
     // resolving.
     interface.fullyQualifiedDomainName = hostnameWithDomain;
 
-    interfaces.insert(std::pair<uint32_t, InterfaceInfo>(interfaceId, interface));
+    interfaces.insert(std::make_pair(interfaceId, std::move(interface)));
 }
 
 bool ResolveContext::HasInterface()
 {
     return interfaces.size();
 }
 
-void ResolveContext::RemoveInterfaces()
+InterfaceInfo::InterfaceInfo()
 {
-    for (auto & interface : interfaces)
+    service.mTextEntrySize = 0;
+    service.mTextEntries   = nullptr;
+}
+
+InterfaceInfo::InterfaceInfo(InterfaceInfo && other) :
+    service(std::move(other.service)), addresses(std::move(other.addresses)),
+    fullyQualifiedDomainName(std::move(other.fullyQualifiedDomainName))
+{
+    // Make sure we're not trying to free any state from the other DnssdService,
+    // since we took over ownership of its allocated bits.
+    other.service.mTextEntrySize = 0;
+    other.service.mTextEntries   = nullptr;
+}
+
+InterfaceInfo::~InterfaceInfo()
+{
+    if (service.mTextEntries == nullptr)
     {
-        size_t count = interface.second.service.mTextEntrySize;
-        for (size_t i = 0; i < count; i++)
-        {
-            const auto & textEntry = interface.second.service.mTextEntries[i];
-            free(const_cast<char *>(textEntry.mKey));
-            free(const_cast<uint8_t *>(textEntry.mData));
-        }
-        Platform::MemoryFree(const_cast<TextEntry *>(interface.second.service.mTextEntries));
+        return;
     }
 
-    interfaces.clear();
+    const size_t count = service.mTextEntrySize;
+    for (size_t i = 0; i < count; i++)
+    {
+        const auto & textEntry = service.mTextEntries[i];
+        free(const_cast<char *>(textEntry.mKey));
+        free(const_cast<uint8_t *>(textEntry.mData));
+    }
+    Platform::MemoryFree(const_cast<TextEntry *>(service.mTextEntries));
 }
 
 } // namespace Dnssd

diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h
@@ -117,6 +117,13 @@ struct BrowseContext : public GenericContext
 
 struct InterfaceInfo
 {
+    InterfaceInfo();
+    InterfaceInfo(InterfaceInfo && other);
+    // Copying is not safe, because DnssdService bits need to be
+    // copied/deallocated properly.
+    InterfaceInfo(const InterfaceInfo & other) = delete;
+    ~InterfaceInfo();
+
     DnssdService service;
     std::vector<Inet::IPAddress> addresses;
     std::string fullyQualifiedDomainName;
@@ -141,7 +148,6 @@ struct ResolveContext : public GenericContext
     void OnNewInterface(uint32_t interfaceId, const char * fullname, const char * hostname, uint16_t port, uint16_t txtLen,
                         const unsigned char * txtRecord);
     bool HasInterface();
-    void RemoveInterfaces();
 };
 
 } // namespace Dnssd