[mdns] fixes of mDNS subscriptions (#2148)

This commit covers following things:
- Previously we didn't handle the removal of addresses properly when
  resolving a host/service. This causes an issue that when there's an
  ongoing browse of a specific service type (`_trel._udp`, for
  example), the callback of `OnServiceResolved` will also give stale
  addresses of the resolved host.
- In mDNSResponder implementation, we used a flag
  `kDNSServiceFlagsTimeout` which prevents from doing an ongoing
  service instance resolution. I removed this flag so that it does an
  ongoing resolution. The new behavior aligns better with the
  `Mdns::Publisher`'s API definition.
- In mDNSResponder implementation, don't remove the service instance
  resolution after an address is resolved. This allows the
  implementation to return multiple addresses.
- Added a unit test for mDNS subscribers.

src/mdns/mdns.cpp

@@ -197,6 +197,21 @@ void Publisher::OnServiceResolved(std::string aType, DiscoveredInstanceInfo aIns
                 aInstanceInfo.mRemoved ? "remove" : "add", aInstanceInfo.mName.c_str(), aInstanceInfo.mHostName.c_str(),
                 aInstanceInfo.mAddresses.size());
 
+    if (!aInstanceInfo.mRemoved)
+    {
+        std::string addressesString;
+
+        for (const auto &address : aInstanceInfo.mAddresses)
+        {
+            addressesString += address.ToString() + ",";
+        }
+        if (addressesString.size())
+        {
+            addressesString.pop_back();
+        }
+        otbrLogInfo("addresses: [ %s ]", addressesString.c_str());
+    }
+
     DnsUtils::CheckServiceNameSanity(aType);
 
     assert(aInstanceInfo.mNetifIndex > 0);
@@ -619,5 +634,20 @@ void Publisher::UpdateHostResolutionEmaLatency(const std::string &aHostName, otb
     }
 }
 
+void Publisher::AddAddress(AddressList &aAddressList, const Ip6Address &aAddress)
+{
+    aAddressList.push_back(aAddress);
+}
+
+void Publisher::RemoveAddress(AddressList &aAddressList, const Ip6Address &aAddress)
+{
+    auto it = std::find(aAddressList.begin(), aAddressList.end(), aAddress);
+
+    if (it != aAddressList.end())
+    {
+        aAddressList.erase(it);
+    }
+}
+
 } // namespace Mdns
 } // namespace otbr

src/mdns/mdns.hpp

@@ -136,6 +136,9 @@ class Publisher : private NonCopyable
         uint16_t    mWeight   = 0;       ///< Service weight.
         TxtData     mTxtData;            ///< TXT RDATA bytes.
         uint32_t    mTtl = 0;            ///< Service TTL.
+
+        void AddAddress(const Ip6Address &aAddress) { Publisher::AddAddress(mAddresses, aAddress); }
+        void RemoveAddress(const Ip6Address &aAddress) { Publisher::RemoveAddress(mAddresses, aAddress); }
     };
 
     /**
@@ -148,6 +151,9 @@ class Publisher : private NonCopyable
         AddressList mAddresses;      ///< IP6 addresses.
         uint32_t    mNetifIndex = 0; ///< Network interface.
         uint32_t    mTtl        = 0; ///< Host TTL.
+
+        void AddAddress(const Ip6Address &aAddress) { Publisher::AddAddress(mAddresses, aAddress); }
+        void RemoveAddress(const Ip6Address &aAddress) { Publisher::RemoveAddress(mAddresses, aAddress); }
     };
 
     /**
@@ -574,6 +580,9 @@ class Publisher : private NonCopyable
                                                    otbrError          aError);
     void UpdateHostResolutionEmaLatency(const std::string &aHostName, otbrError aError);
 
+    static void AddAddress(AddressList &aAddressList, const Ip6Address &aAddress);
+    static void RemoveAddress(AddressList &aAddressList, const Ip6Address &aAddress);
+
     ServiceRegistrationMap mServiceRegistrations;
     HostRegistrationMap    mHostRegistrations;
 

src/mdns/mdns_avahi.cpp

@@ -1238,6 +1238,7 @@ void PublisherAvahi::ServiceResolver::HandleResolveServiceResult(AvahiServiceRes
         {
             avahi_record_browser_free(mRecordBrowser);
             mRecordBrowser = nullptr;
+            mInstanceInfo.mAddresses.clear();
         }
         // NOTE: This `ServiceResolver` object may be freed in `OnServiceResolved`.
         mRecordBrowser = avahi_record_browser_new(mPublisherAvahi->mClient, aInterfaceIndex, AVAHI_PROTO_UNSPEC,
@@ -1299,7 +1300,7 @@ void PublisherAvahi::ServiceResolver::HandleResolveHostResult(AvahiRecordBrowser
             aName, aInterfaceIndex, aProtocol, aClazz, aType, aSize, static_cast<int>(aFlags),
             static_cast<int>(aEvent));
 
-    VerifyOrExit(aEvent == AVAHI_BROWSER_NEW);
+    VerifyOrExit(aEvent == AVAHI_BROWSER_NEW || aEvent == AVAHI_BROWSER_REMOVE);
     VerifyOrExit(aSize == OTBR_IP6_ADDRESS_SIZE || aSize == OTBR_IP4_ADDRESS_SIZE,
                  otbrLogErr("Unexpected address data length: %zu", aSize), avahiError = AVAHI_ERR_INVALID_ADDRESS);
     VerifyOrExit(aSize == OTBR_IP6_ADDRESS_SIZE, otbrLogInfo("IPv4 address ignored"),
@@ -1308,9 +1309,16 @@ void PublisherAvahi::ServiceResolver::HandleResolveHostResult(AvahiRecordBrowser
 
     VerifyOrExit(!address.IsLinkLocal() && !address.IsMulticast() && !address.IsLoopback() && !address.IsUnspecified(),
                  avahiError = AVAHI_ERR_INVALID_ADDRESS);
-    otbrLogInfo("Resolved host address: %s", address.ToString().c_str());
-
-    mInstanceInfo.mAddresses.push_back(std::move(address));
+    otbrLogInfo("Resolved host address: %s %s", aEvent == AVAHI_BROWSER_NEW ? "add" : "remove",
+                address.ToString().c_str());
+    if (aEvent == AVAHI_BROWSER_NEW)
+    {
+        mInstanceInfo.AddAddress(address);
+    }
+    else
+    {
+        mInstanceInfo.RemoveAddress(address);
+    }
     resolved = true;
 
 exit:
@@ -1423,7 +1431,7 @@ void PublisherAvahi::HostSubscription::HandleResolveResult(AvahiRecordBrowser
             aName, aInterfaceIndex, aProtocol, aClazz, aType, aSize, static_cast<int>(aFlags),
             static_cast<int>(aEvent));
 
-    VerifyOrExit(aEvent == AVAHI_BROWSER_NEW);
+    VerifyOrExit(aEvent == AVAHI_BROWSER_NEW || aEvent == AVAHI_BROWSER_REMOVE);
     VerifyOrExit(aSize == OTBR_IP6_ADDRESS_SIZE || aSize == OTBR_IP4_ADDRESS_SIZE,
                  otbrLogErr("Unexpected address data length: %zu", aSize), avahiError = AVAHI_ERR_INVALID_ADDRESS);
     VerifyOrExit(aSize == OTBR_IP6_ADDRESS_SIZE, otbrLogInfo("IPv4 address ignored"),
@@ -1432,10 +1440,18 @@ void PublisherAvahi::HostSubscription::HandleResolveResult(AvahiRecordBrowser
 
     VerifyOrExit(!address.IsLinkLocal() && !address.IsMulticast() && !address.IsLoopback() && !address.IsUnspecified(),
                  avahiError = AVAHI_ERR_INVALID_ADDRESS);
-    otbrLogInfo("Resolved host address: %s", address.ToString().c_str());
+    otbrLogInfo("Resolved host address: %s %s", aEvent == AVAHI_BROWSER_NEW ? "add" : "remove",
+                address.ToString().c_str());
 
     mHostInfo.mHostName = std::string(aName) + ".";
-    mHostInfo.mAddresses.push_back(std::move(address));
+    if (aEvent == AVAHI_BROWSER_NEW)
+    {
+        mHostInfo.AddAddress(address);
+    }
+    else
+    {
+        mHostInfo.RemoveAddress(address);
+    }
     mHostInfo.mNetifIndex = static_cast<uint32_t>(aInterfaceIndex);
     // TODO: Use a more proper TTL
     mHostInfo.mTtl = kDefaultTtl;

src/mdns/mdns_mdnssd.cpp

@@ -941,19 +941,6 @@ void PublisherMDnsSd::ServiceSubscription::Resolve(uint32_t           aInterface
     mResolvingInstances.back()->Resolve();
 }
 
-void PublisherMDnsSd::ServiceSubscription::RemoveInstanceResolution(
-    PublisherMDnsSd::ServiceInstanceResolution &aInstanceResolution)
-{
-    auto it = std::find_if(mResolvingInstances.begin(), mResolvingInstances.end(),
-                           [&aInstanceResolution](const std::unique_ptr<ServiceInstanceResolution> &aElem) {
-                               return &aInstanceResolution == aElem.get();
-                           });
-
-    assert(it != mResolvingInstances.end());
-
-    mResolvingInstances.erase(it);
-}
-
 void PublisherMDnsSd::ServiceSubscription::UpdateAll(MainloopContext &aMainloop) const
 {
     Update(aMainloop);
@@ -1056,7 +1043,7 @@ otbrError PublisherMDnsSd::ServiceInstanceResolution::GetAddrInfo(uint32_t aInte
 
     otbrLogInfo("DNSServiceGetAddrInfo %s inf %d", mInstanceInfo.mHostName.c_str(), aInterfaceIndex);
 
-    dnsError = DNSServiceGetAddrInfo(&mServiceRef, kDNSServiceFlagsTimeout, aInterfaceIndex,
+    dnsError = DNSServiceGetAddrInfo(&mServiceRef, /* flags */ 0, aInterfaceIndex,
                                      kDNSServiceProtocol_IPv6 | kDNSServiceProtocol_IPv4,
                                      mInstanceInfo.mHostName.c_str(), HandleGetAddrInfoResult, this);
 
@@ -1093,22 +1080,31 @@ void PublisherMDnsSd::ServiceInstanceResolution::HandleGetAddrInfoResult(DNSServ
     OTBR_UNUSED_VARIABLE(aInterfaceIndex);
 
     Ip6Address address;
+    bool       isAdd = (aFlags & kDNSServiceFlagsAdd) != 0;
 
     otbrLog(aErrorCode == kDNSServiceErr_NoError ? OTBR_LOG_INFO : OTBR_LOG_WARNING, OTBR_LOG_TAG,
             "DNSServiceGetAddrInfo reply: flags=%" PRIu32 ", host=%s, sa_family=%u, error=%" PRId32, aFlags, aHostName,
             static_cast<unsigned int>(aAddress->sa_family), aErrorCode);
 
     VerifyOrExit(aErrorCode == kDNSServiceErr_NoError);
-    VerifyOrExit((aFlags & kDNSServiceFlagsAdd) && aAddress->sa_family == AF_INET6);
+    VerifyOrExit(aAddress->sa_family == AF_INET6);
 
     address.CopyFrom(*reinterpret_cast<const struct sockaddr_in6 *>(aAddress));
     VerifyOrExit(!address.IsUnspecified() && !address.IsLinkLocal() && !address.IsMulticast() && !address.IsLoopback(),
                  otbrLogDebug("DNSServiceGetAddrInfo ignores address %s", address.ToString().c_str()));
 
-    mInstanceInfo.mAddresses.push_back(address);
-    mInstanceInfo.mTtl = aTtl;
+    otbrLogInfo("DNSServiceGetAddrInfo reply: %s address=%s, ttl=%" PRIu32, isAdd ? "add" : "remove",
+                address.ToString().c_str(), aTtl);
 
-    otbrLogInfo("DNSServiceGetAddrInfo reply: address=%s, ttl=%" PRIu32, address.ToString().c_str(), aTtl);
+    if (isAdd)
+    {
+        mInstanceInfo.AddAddress(address);
+    }
+    else
+    {
+        mInstanceInfo.RemoveAddress(address);
+    }
+    mInstanceInfo.mTtl = aTtl;
 
 exit:
     if (!mInstanceInfo.mAddresses.empty() || aErrorCode != kDNSServiceErr_NoError)
@@ -1123,10 +1119,6 @@ void PublisherMDnsSd::ServiceInstanceResolution::FinishResolution(void)
     std::string            serviceName  = mSubscription->mType;
     DiscoveredInstanceInfo instanceInfo = mInstanceInfo;
 
-    // NOTE: `RemoveInstanceResolution` will free this `ServiceInstanceResolution` object.
-    //       So, We can't access `mSubscription` after `RemoveInstanceResolution`.
-    subscription->RemoveInstanceResolution(*this);
-
     // NOTE: The `ServiceSubscription` object may be freed in `OnServiceResolved`.
     subscription->mPublisher.OnServiceResolved(serviceName, instanceInfo);
 }
@@ -1170,25 +1162,34 @@ void PublisherMDnsSd::HostSubscription::HandleResolveResult(DNSServiceRef
     OTBR_UNUSED_VARIABLE(aServiceRef);
 
     Ip6Address address;
+    bool       isAdd = (aFlags & kDNSServiceFlagsAdd) != 0;
 
     otbrLog(aErrorCode == kDNSServiceErr_NoError ? OTBR_LOG_INFO : OTBR_LOG_WARNING, OTBR_LOG_TAG,
             "DNSServiceGetAddrInfo reply: flags=%" PRIu32 ", host=%s, sa_family=%u, error=%" PRId32, aFlags, aHostName,
             static_cast<unsigned int>(aAddress->sa_family), aErrorCode);
 
     VerifyOrExit(aErrorCode == kDNSServiceErr_NoError);
-    VerifyOrExit((aFlags & kDNSServiceFlagsAdd) && aAddress->sa_family == AF_INET6);
+    VerifyOrExit(aAddress->sa_family == AF_INET6);
 
     address.CopyFrom(*reinterpret_cast<const struct sockaddr_in6 *>(aAddress));
     VerifyOrExit(!address.IsLinkLocal(),
                  otbrLogDebug("DNSServiceGetAddrInfo ignore link-local address %s", address.ToString().c_str()));
 
-    mHostInfo.mHostName = aHostName;
-    mHostInfo.mAddresses.push_back(address);
+    otbrLogInfo("DNSServiceGetAddrInfo reply: %s address=%s, ttl=%" PRIu32, isAdd ? "add" : "remove",
+                address.ToString().c_str(), aTtl);
+
+    if (isAdd)
+    {
+        mHostInfo.AddAddress(address);
+    }
+    else
+    {
+        mHostInfo.RemoveAddress(address);
+    }
+    mHostInfo.mHostName   = aHostName;
     mHostInfo.mNetifIndex = aInterfaceIndex;
     mHostInfo.mTtl        = aTtl;
 
-    otbrLogInfo("DNSServiceGetAddrInfo reply: address=%s, ttl=%" PRIu32, address.ToString().c_str(), aTtl);
-
     // NOTE: This `HostSubscription` object may be freed in `OnHostResolved`.
     mPublisher.OnHostResolved(mHostName, mHostInfo);
 

src/mdns/mdns_mdnssd.hpp

@@ -256,7 +256,6 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher
                      const std::string &aInstanceName,
                      const std::string &aType,
                      const std::string &aDomain);
-        void RemoveInstanceResolution(ServiceInstanceResolution &aInstanceResolution);
         void UpdateAll(MainloopContext &aMainloop) const;
         void ProcessAll(const MainloopContext &aMainloop, std::vector<DNSServiceRef> &aReadyServices) const;
 

tests/mdns/CMakeLists.txt

@@ -87,3 +87,20 @@ set_tests_properties(
     PROPERTIES
         ENVIRONMENT "OTBR_MDNS=${OTBR_MDNS};OTBR_TEST_MDNS=$<TARGET_FILE:otbr-test-mdns>"
 )
+
+add_executable(otbr-test-mdns-subscribe
+    test_subscribe.cpp
+)
+
+target_link_libraries(otbr-test-mdns-subscribe PRIVATE
+    otbr-config
+    otbr-mdns
+    $<$<BOOL:${CPPUTEST_LIBRARY_DIRS}>:-L$<JOIN:${CPPUTEST_LIBRARY_DIRS}," -L">>
+    ${CPPUTEST_LIBRARIES}
+)
+
+
+add_test(
+    NAME mdns-subscribe
+    COMMAND otbr-test-mdns-subscribe
+)