Fix MDNS duplicate IP broadcasting: (#10571)

Sending 'by interfaces' sends replies in both IPv4 and IPv6 targets even though queries are received on a specific type. Since on dual stack we listen on both IPv4 and IPv6 this results in double packet sending every time. Fix logic error: udp->GetBoundInterface() is never set in the current code as we bind ip/port rather than interface.
project-chip · Oct 20, 2021 · 7c7e5c7 · 7c7e5c7
1 parent e15b52a
commit 7c7e5c7
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 18 deletions.
diff --git a/examples/all-clusters-app/esp32/main/main.cpp b/examples/all-clusters-app/esp32/main/main.cpp
@@ -330,6 +330,49 @@ class DeviceListModel : public ListScreen::Model
     }
 };
 
+class ActionListModel : public ListScreen::Model
+{
+    int GetItemCount() override { return static_cast<int>(mActions.size()); }
+    std::string GetItemText(int i) override { return mActions[i].title.c_str(); }
+    void ItemAction(int i) override
+    {
+        ESP_LOGI(TAG, "generic action %d", i);
+        mActions[i].action();
+    }
+
+protected:
+    void AddAction(const char * name, std::function<void(void)> action) { mActions.push_back(Action(name, action)); }
+
+private:
+    struct Action
+    {
+        std::string title;
+        std::function<void(void)> action;
+
+        Action(const char * t, std::function<void(void)> a) : title(t), action(a) {}
+    };
+
+    std::vector<Action> mActions;
+};
+
+class MdnsDebugListModel : public ActionListModel
+{
+public:
+    std::string GetTitle() override { return "mDNS Debug"; }
+
+    MdnsDebugListModel() { AddAction("(Re-)Init", std::bind(&MdnsDebugListModel::DoReinit, this)); }
+
+private:
+    void DoReinit()
+    {
+        CHIP_ERROR err = Dnssd::ServiceAdvertiser::Instance().Init(&DeviceLayer::InetLayer);
+        if (err != CHIP_NO_ERROR)
+        {
+            ESP_LOGE(TAG, "Error initializing: %s", err.AsString());
+        }
+    }
+};
+
 class SetupListModel : public ListScreen::Model
 {
 public:
@@ -696,10 +739,10 @@ extern "C" void app_main()
                        ESP_LOGI(TAG, "Opening device list");
                        ScreenManager::PushScreen(chip::Platform::New<ListScreen>(chip::Platform::New<DeviceListModel>()));
                    })
-            ->Item("Custom",
+            ->Item("mDNS Debug",
                    []() {
-                       ESP_LOGI(TAG, "Opening custom screen");
-                       ScreenManager::PushScreen(chip::Platform::New<CustomScreen>());
+                       ESP_LOGI(TAG, "Opening MDNS debug");
+                       ScreenManager::PushScreen(chip::Platform::New<ListScreen>(chip::Platform::New<MdnsDebugListModel>()));
                    })
             ->Item("QR Code",
                    [=]() {
@@ -722,6 +765,11 @@ extern "C" void app_main()
                        ESP_LOGI(TAG, "Opening Setup list");
                        ScreenManager::PushScreen(chip::Platform::New<ListScreen>(chip::Platform::New<SetupListModel>()));
                    })
+            ->Item("Custom",
+                   []() {
+                       ESP_LOGI(TAG, "Opening custom screen");
+                       ScreenManager::PushScreen(chip::Platform::New<CustomScreen>());
+                   })
             ->Item("More")
             ->Item("Items")
             ->Item("For")

diff --git a/src/lib/dnssd/minimal_mdns/ResponseSender.cpp b/src/lib/dnssd/minimal_mdns/ResponseSender.cpp
@@ -174,8 +174,8 @@ CHIP_ERROR ResponseSender::FlushReply()
         else
         {
             ChipLogDetail(Discovery, "Broadcasting mDns reply for query from %s", srcAddressString);
-            ReturnErrorOnFailure(
-                mServer->BroadcastSend(mResponseBuilder.ReleasePacket(), kMdnsStandardPort, mSendState.GetSourceInterfaceId()));
+            ReturnErrorOnFailure(mServer->BroadcastSend(mResponseBuilder.ReleasePacket(), kMdnsStandardPort,
+                                                        mSendState.GetSourceInterfaceId(), mSendState.GetSourceAddress().Type()));
         }
     }
 

diff --git a/src/lib/dnssd/minimal_mdns/Server.cpp b/src/lib/dnssd/minimal_mdns/Server.cpp
@@ -222,7 +222,8 @@ CHIP_ERROR ServerBase::DirectSend(chip::System::PacketBufferHandle && data, cons
     return CHIP_ERROR_NOT_CONNECTED;
 }
 
-CHIP_ERROR ServerBase::BroadcastSend(chip::System::PacketBufferHandle && data, uint16_t port, chip::Inet::InterfaceId interface)
+CHIP_ERROR ServerBase::BroadcastSend(chip::System::PacketBufferHandle && data, uint16_t port, chip::Inet::InterfaceId interface,
+                                     chip::Inet::IPAddressType addressType)
 {
     for (size_t i = 0; i < mEndpointCount; i++)
     {
@@ -233,7 +234,12 @@ CHIP_ERROR ServerBase::BroadcastSend(chip::System::PacketBufferHandle && data, u
             continue;
         }
 
-        if ((info->udp->GetBoundInterface() != interface) && (info->udp->GetBoundInterface() != INET_NULL_INTERFACEID))
+        if ((info->interfaceId != interface) && (info->interfaceId != INET_NULL_INTERFACEID))
+        {
+            continue;
+        }
+
+        if ((addressType != chip::Inet::kIPAddressType_Any) && (info->addressType != addressType))
         {
             continue;
         }
@@ -242,17 +248,17 @@ CHIP_ERROR ServerBase::BroadcastSend(chip::System::PacketBufferHandle && data, u
 
         /// The same packet needs to be sent over potentially multiple interfaces.
         /// LWIP does not like having a pbuf sent over serparate interfaces, hence we create a copy
+        /// for sending via `CloneData`
+        ///
         /// TODO: this wastes one copy of the data and that could be optimized away
-        chip::System::PacketBufferHandle copy = data.CloneData();
-
         if (info->addressType == chip::Inet::kIPAddressType_IPv6)
         {
-            err = info->udp->SendTo(mIpv6BroadcastAddress, port, std::move(copy), info->udp->GetBoundInterface());
+            err = info->udp->SendTo(mIpv6BroadcastAddress, port, data.CloneData(), info->udp->GetBoundInterface());
         }
 #if INET_CONFIG_ENABLE_IPV4
         else if (info->addressType == chip::Inet::kIPAddressType_IPv4)
         {
-            err = info->udp->SendTo(mIpv4BroadcastAddress, port, std::move(copy), info->udp->GetBoundInterface());
+            err = info->udp->SendTo(mIpv4BroadcastAddress, port, data.CloneData(), info->udp->GetBoundInterface());
         }
 #endif
         else
@@ -295,17 +301,17 @@ CHIP_ERROR ServerBase::BroadcastSend(chip::System::PacketBufferHandle && data, u
 
         /// The same packet needs to be sent over potentially multiple interfaces.
         /// LWIP does not like having a pbuf sent over serparate interfaces, hence we create a copy
+        /// for sending via `CloneData`
+        ///
         /// TODO: this wastes one copy of the data and that could be optimized away
-        chip::System::PacketBufferHandle copy = data.CloneData();
-
         if (info->addressType == chip::Inet::kIPAddressType_IPv6)
         {
-            err = info->udp->SendTo(mIpv6BroadcastAddress, port, std::move(copy), info->udp->GetBoundInterface());
+            err = info->udp->SendTo(mIpv6BroadcastAddress, port, data.CloneData(), info->udp->GetBoundInterface());
         }
 #if INET_CONFIG_ENABLE_IPV4
         else if (info->addressType == chip::Inet::kIPAddressType_IPv4)
         {
-            err = info->udp->SendTo(mIpv4BroadcastAddress, port, std::move(copy), info->udp->GetBoundInterface());
+            err = info->udp->SendTo(mIpv4BroadcastAddress, port, data.CloneData(), info->udp->GetBoundInterface());
         }
 #endif
         else
@@ -318,7 +324,6 @@ CHIP_ERROR ServerBase::BroadcastSend(chip::System::PacketBufferHandle && data, u
         if (err == CHIP_NO_ERROR)
         {
             hadSuccesfulSend = true;
-            ChipLogProgress(Discovery, "mDNS broadcast success");
         }
         else
         {

diff --git a/src/lib/dnssd/minimal_mdns/Server.h b/src/lib/dnssd/minimal_mdns/Server.h
@@ -113,8 +113,9 @@ class ServerBase
     /// Send a specific packet broadcast to all interfaces
     virtual CHIP_ERROR BroadcastSend(chip::System::PacketBufferHandle && data, uint16_t port);
 
-    /// Send a specific packet broadcast to a specific interface
-    virtual CHIP_ERROR BroadcastSend(chip::System::PacketBufferHandle && data, uint16_t port, chip::Inet::InterfaceId interface);
+    /// Send a specific packet broadcast to a specific interface using a specific address type
+    virtual CHIP_ERROR BroadcastSend(chip::System::PacketBufferHandle && data, uint16_t port, chip::Inet::InterfaceId interface,
+                                     chip::Inet::IPAddressType addressType);
 
     ServerBase & SetDelegate(ServerDelegate * d)
     {