From a6288f87744703067fcf5f1b1958fd9f54c144eb Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 20 Oct 2021 10:41:31 -0400 Subject: [PATCH] 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. --- examples/all-clusters-app/esp32/main/main.cpp | 54 +++++++++++++++++-- src/lib/dnssd/minimal_mdns/ResponseSender.cpp | 4 +- src/lib/dnssd/minimal_mdns/Server.cpp | 27 ++++++---- src/lib/dnssd/minimal_mdns/Server.h | 5 +- 4 files changed, 72 insertions(+), 18 deletions(-) diff --git a/examples/all-clusters-app/esp32/main/main.cpp b/examples/all-clusters-app/esp32/main/main.cpp index 00b9dccb5053ee..9cd06508620954 100644 --- 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(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 action) { mActions.push_back(Action(name, action)); } + +private: + struct Action + { + std::string title; + std::function action; + + Action(const char * t, std::function a) : title(t), action(a) {} + }; + + std::vector 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(chip::Platform::New())); }) - ->Item("Custom", + ->Item("mDNS Debug", []() { - ESP_LOGI(TAG, "Opening custom screen"); - ScreenManager::PushScreen(chip::Platform::New()); + ESP_LOGI(TAG, "Opening MDNS debug"); + ScreenManager::PushScreen(chip::Platform::New(chip::Platform::New())); }) ->Item("QR Code", [=]() { @@ -722,6 +765,11 @@ extern "C" void app_main() ESP_LOGI(TAG, "Opening Setup list"); ScreenManager::PushScreen(chip::Platform::New(chip::Platform::New())); }) + ->Item("Custom", + []() { + ESP_LOGI(TAG, "Opening custom screen"); + ScreenManager::PushScreen(chip::Platform::New()); + }) ->Item("More") ->Item("Items") ->Item("For") diff --git a/src/lib/dnssd/minimal_mdns/ResponseSender.cpp b/src/lib/dnssd/minimal_mdns/ResponseSender.cpp index 5dc3cffa781465..89c1db82d089fb 100644 --- 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 index 1af62c85cb50d6..049a9c88916f4b 100644 --- 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 index 66eef610cb41bd..b8ca2bc1d71849 100644 --- 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) {