From 22368821e989c310d8f1627a124df2c7063e914f Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk <66371704+kkasperczyk-no@users.noreply.github.com> Date: Wed, 26 Apr 2023 20:42:15 +0200 Subject: [PATCH] [dnssd] Fixed OT DNS API usage for Thread platform (#26199) * [dnssd] Fixed OT DNS API usage for Thread platform Introduced several fixes to the Thread platform DNS implementation: * Added checking if memory allocation for DnsResult was successful and dispatching dedicated methods to inform upper layer in case of memory allocation failure * Added checking if DNS response for DNS resolve includes AAAA record. In case it doesn't the additional DNS query to obtain IPv6 address will be sent. * Added checking if DNS response for DNS browse includes SRV, TXT and AAAA records. In case it doesn't the additional DNS queries to obtain SRV + TXT, and AAAA records will be sent. * Addressed review comments * Fixed error handling and potential memory leaks * Moved handling resolve after browse from Thread platform to Discovery_ImplPlatform. * Addressed second code review * Fixed string copying by adding the exact size of data to copy instead of relying on the max buffer size. --- src/lib/dnssd/Discovery_ImplPlatform.cpp | 4 +- ...nericThreadStackManagerImpl_OpenThread.cpp | 199 ++++++++++++++---- ...GenericThreadStackManagerImpl_OpenThread.h | 11 +- 3 files changed, 168 insertions(+), 46 deletions(-) diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index bffd7e8bff9872..675e7ac238106b 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -172,7 +172,9 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser { proxy->Retain(); // For some platforms browsed services are already resolved, so verify if resolve is really needed or call resolve callback - if (!services[i].mAddress.HasValue()) + + // Check if SRV, TXT and AAAA records were received in DNS responses + if (strlen(services[i].mHostName) == 0 || services[i].mTextEntrySize == 0 || !services[i].mAddress.HasValue()) { ChipDnssdResolve(&services[i], services[i].mInterface, HandleNodeResolve, context); } diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp index 603924e71bbc34..3142fd102ba670 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp @@ -2498,31 +2498,20 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_SetSrpDnsCallba template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::FromOtDnsResponseToMdnsData( otDnsServiceInfo & serviceInfo, const char * serviceType, chip::Dnssd::DnssdService & mdnsService, - DnsServiceTxtEntries & serviceTxtEntries) + DnsServiceTxtEntries & serviceTxtEntries, otError error) { char protocol[chip::Dnssd::kDnssdProtocolTextMaxSize + 1]; - if (strchr(serviceInfo.mHostNameBuffer, '.') == nullptr) - return CHIP_ERROR_INVALID_ARGUMENT; - - // Extract from the .. the part. - size_t substringSize = strchr(serviceInfo.mHostNameBuffer, '.') - serviceInfo.mHostNameBuffer; - if (substringSize >= ArraySize(mdnsService.mHostName)) - { - return CHIP_ERROR_INVALID_ARGUMENT; - } - Platform::CopyString(mdnsService.mHostName, serviceInfo.mHostNameBuffer); - if (strchr(serviceType, '.') == nullptr) return CHIP_ERROR_INVALID_ARGUMENT; // Extract from the ... the part. - substringSize = strchr(serviceType, '.') - serviceType; + size_t substringSize = strchr(serviceType, '.') - serviceType; if (substringSize >= ArraySize(mdnsService.mType)) { return CHIP_ERROR_INVALID_ARGUMENT; } - Platform::CopyString(mdnsService.mType, serviceType); + Platform::CopyString(mdnsService.mType, substringSize + 1, serviceType); // Extract from the ... the part. const char * protocolSubstringStart = serviceType + substringSize + 1; @@ -2535,7 +2524,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::FromOtDnsRespons { return CHIP_ERROR_INVALID_ARGUMENT; } - Platform::CopyString(protocol, protocolSubstringStart); + Platform::CopyString(protocol, substringSize + 1, protocolSubstringStart); if (strncmp(protocol, "_udp", chip::Dnssd::kDnssdProtocolTextMaxSize) == 0) { @@ -2549,37 +2538,97 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::FromOtDnsRespons { mdnsService.mProtocol = chip::Dnssd::DnssdServiceProtocol::kDnssdProtocolUnknown; } - mdnsService.mPort = serviceInfo.mPort; - mdnsService.mInterface = Inet::InterfaceId::Null(); - mdnsService.mAddressType = Inet::IPAddressType::kIPv6; - mdnsService.mAddress = chip::Optional(ToIPAddress(serviceInfo.mHostAddress)); - otDnsTxtEntryIterator iterator; - otDnsInitTxtEntryIterator(&iterator, serviceInfo.mTxtData, serviceInfo.mTxtDataSize); + // Check if SRV record was included in DNS response. + if (error != OT_ERROR_NOT_FOUND) + { + if (strchr(serviceInfo.mHostNameBuffer, '.') == nullptr) + return CHIP_ERROR_INVALID_ARGUMENT; - otDnsTxtEntry txtEntry; - FixedBufferAllocator alloc(serviceTxtEntries.mBuffer); + // Extract from the .. the part. + substringSize = strchr(serviceInfo.mHostNameBuffer, '.') - serviceInfo.mHostNameBuffer; + if (substringSize >= ArraySize(mdnsService.mHostName)) + { + return CHIP_ERROR_INVALID_ARGUMENT; + } + Platform::CopyString(mdnsService.mHostName, substringSize + 1, serviceInfo.mHostNameBuffer); - uint8_t entryIndex = 0; - while ((otDnsGetNextTxtEntry(&iterator, &txtEntry) == OT_ERROR_NONE) && entryIndex < kMaxDnsServiceTxtEntriesNumber) - { - if (txtEntry.mKey == nullptr || txtEntry.mValue == nullptr) - continue; + mdnsService.mPort = serviceInfo.mPort; + } - serviceTxtEntries.mTxtEntries[entryIndex].mKey = alloc.Clone(txtEntry.mKey); - serviceTxtEntries.mTxtEntries[entryIndex].mData = alloc.Clone(txtEntry.mValue, txtEntry.mValueLength); - serviceTxtEntries.mTxtEntries[entryIndex].mDataSize = txtEntry.mValueLength; - entryIndex++; + mdnsService.mInterface = Inet::InterfaceId::Null(); + + // Check if AAAA record was included in DNS response. + + if (!otIp6IsAddressUnspecified(&serviceInfo.mHostAddress)) + { + mdnsService.mAddressType = Inet::IPAddressType::kIPv6; + mdnsService.mAddress = MakeOptional(ToIPAddress(serviceInfo.mHostAddress)); } - ReturnErrorCodeIf(alloc.AnyAllocFailed(), CHIP_ERROR_BUFFER_TOO_SMALL); + // Check if TXT record was included in DNS response. + if (serviceInfo.mTxtDataSize != 0) + { + otDnsTxtEntryIterator iterator; + otDnsInitTxtEntryIterator(&iterator, serviceInfo.mTxtData, serviceInfo.mTxtDataSize); + + otDnsTxtEntry txtEntry; + FixedBufferAllocator alloc(serviceTxtEntries.mBuffer); - mdnsService.mTextEntries = serviceTxtEntries.mTxtEntries; - mdnsService.mTextEntrySize = entryIndex; + uint8_t entryIndex = 0; + while ((otDnsGetNextTxtEntry(&iterator, &txtEntry) == OT_ERROR_NONE) && entryIndex < kMaxDnsServiceTxtEntriesNumber) + { + if (txtEntry.mKey == nullptr || txtEntry.mValue == nullptr) + continue; + + serviceTxtEntries.mTxtEntries[entryIndex].mKey = alloc.Clone(txtEntry.mKey); + serviceTxtEntries.mTxtEntries[entryIndex].mData = alloc.Clone(txtEntry.mValue, txtEntry.mValueLength); + serviceTxtEntries.mTxtEntries[entryIndex].mDataSize = txtEntry.mValueLength; + entryIndex++; + } + + ReturnErrorCodeIf(alloc.AnyAllocFailed(), CHIP_ERROR_BUFFER_TOO_SMALL); + + mdnsService.mTextEntries = serviceTxtEntries.mTxtEntries; + mdnsService.mTextEntrySize = entryIndex; + } return CHIP_NO_ERROR; } +template +CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::ResolveAddress(intptr_t context, otDnsAddressCallback callback) +{ + DnsResult * dnsResult = reinterpret_cast(context); + + ThreadStackMgrImpl().LockThreadStack(); + + char fullHostName[chip::Dnssd::kHostNameMaxLength + 1 + SrpClient::kDefaultDomainNameSize + 1]; + snprintf(fullHostName, sizeof(fullHostName), "%s.%s", dnsResult->mMdnsService.mHostName, SrpClient::kDefaultDomainName); + + CHIP_ERROR error = MapOpenThreadError(otDnsClientResolveAddress(ThreadStackMgrImpl().OTInstance(), fullHostName, callback, + reinterpret_cast(dnsResult), NULL)); + + ThreadStackMgrImpl().UnlockThreadStack(); + + return error; +} + +template +void GenericThreadStackManagerImpl_OpenThread::DispatchAddressResolve(intptr_t context) +{ + CHIP_ERROR error = ResolveAddress(context, OnDnsAddressResolveResult); + + // In case of address resolve failure, fill the error code field and dispatch method to end resolve process. + if (error != CHIP_NO_ERROR) + { + DnsResult * dnsResult = reinterpret_cast(context); + dnsResult->error = error; + + DeviceLayer::PlatformMgr().ScheduleWork(DispatchResolve, reinterpret_cast(dnsResult)); + } +} + template void GenericThreadStackManagerImpl_OpenThread::DispatchResolve(intptr_t context) { @@ -2596,6 +2645,13 @@ void GenericThreadStackManagerImpl_OpenThread::DispatchResolve(intptr Platform::Delete(dnsResult); } +template +void GenericThreadStackManagerImpl_OpenThread::DispatchResolveNoMemory(intptr_t context) +{ + Span ipAddrs; + ThreadStackMgrImpl().mDnsResolveCallback(reinterpret_cast(context), nullptr, ipAddrs, CHIP_ERROR_NO_MEMORY); +} + template void GenericThreadStackManagerImpl_OpenThread::DispatchBrowseEmpty(intptr_t context) { @@ -2612,6 +2668,12 @@ void GenericThreadStackManagerImpl_OpenThread::DispatchBrowse(intptr_ Platform::Delete(dnsResult); } +template +void GenericThreadStackManagerImpl_OpenThread::DispatchBrowseNoMemory(intptr_t context) +{ + ThreadStackMgrImpl().mDnsBrowseCallback(reinterpret_cast(context), nullptr, 0, true, CHIP_ERROR_NO_MEMORY); +} + template void GenericThreadStackManagerImpl_OpenThread::OnDnsBrowseResult(otError aError, const otDnsBrowseResponse * aResponse, void * aContext) @@ -2647,12 +2709,16 @@ void GenericThreadStackManagerImpl_OpenThread::OnDnsBrowseResult(otEr serviceInfo.mTxtData = txtBuffer; serviceInfo.mTxtDataSize = sizeof(txtBuffer); - error = MapOpenThreadError(otDnsBrowseResponseGetServiceInfo(aResponse, serviceName, &serviceInfo)); + otError err = otDnsBrowseResponseGetServiceInfo(aResponse, serviceName, &serviceInfo); + error = MapOpenThreadError(err); - VerifyOrExit(error == CHIP_NO_ERROR, ); + VerifyOrExit(err == OT_ERROR_NOT_FOUND || err == OT_ERROR_NONE, ); DnsResult * dnsResult = Platform::New(aContext, CHIP_NO_ERROR); - error = FromOtDnsResponseToMdnsData(serviceInfo, type, dnsResult->mMdnsService, dnsResult->mServiceTxtEntry); + + VerifyOrExit(dnsResult != nullptr, error = CHIP_ERROR_NO_MEMORY); + + error = FromOtDnsResponseToMdnsData(serviceInfo, type, dnsResult->mMdnsService, dnsResult->mServiceTxtEntry, err); if (CHIP_NO_ERROR == error) { // Invoke callback for every service one by one instead of for the whole @@ -2672,7 +2738,15 @@ void GenericThreadStackManagerImpl_OpenThread::OnDnsBrowseResult(otEr exit: // Invoke callback to notify about end-of-browse or failure DnsResult * dnsResult = Platform::New(aContext, error); - DeviceLayer::PlatformMgr().ScheduleWork(DispatchBrowseEmpty, reinterpret_cast(dnsResult)); + + if (dnsResult == nullptr) + { + DeviceLayer::PlatformMgr().ScheduleWork(DispatchBrowseNoMemory, reinterpret_cast(aContext)); + } + else + { + DeviceLayer::PlatformMgr().ScheduleWork(DispatchBrowseEmpty, reinterpret_cast(dnsResult)); + } } template @@ -2701,12 +2775,36 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_DnsBrowse(const return error; } +template +void GenericThreadStackManagerImpl_OpenThread::OnDnsAddressResolveResult(otError aError, + const otDnsAddressResponse * aResponse, + void * aContext) +{ + CHIP_ERROR error; + DnsResult * dnsResult = reinterpret_cast(aContext); + otIp6Address address; + + error = MapOpenThreadError(otDnsAddressResponseGetAddress(aResponse, 0, &address, nullptr)); + if (error == CHIP_NO_ERROR) + { + dnsResult->mMdnsService.mAddress = MakeOptional(ToIPAddress(address)); + } + + dnsResult->error = error; + + DeviceLayer::PlatformMgr().ScheduleWork(DispatchResolve, reinterpret_cast(dnsResult)); +} + template void GenericThreadStackManagerImpl_OpenThread::OnDnsResolveResult(otError aError, const otDnsServiceResponse * aResponse, void * aContext) { CHIP_ERROR error; + otError otErr; DnsResult * dnsResult = Platform::New(aContext, MapOpenThreadError(aError)); + + VerifyOrExit(dnsResult != nullptr, error = CHIP_ERROR_NO_MEMORY); + // type buffer size is kDnssdTypeAndProtocolMaxSize + . + kMaxDomainNameSize + . + termination character char type[Dnssd::kDnssdTypeAndProtocolMaxSize + SrpClient::kMaxDomainNameSize + 3]; // hostname buffer size is kHostNameMaxLength + . + kMaxDomainNameSize + . + termination character @@ -2718,7 +2816,7 @@ void GenericThreadStackManagerImpl_OpenThread::OnDnsResolveResult(otE if (ThreadStackMgrImpl().mDnsResolveCallback == nullptr) { - ChipLogError(DeviceLayer, "Invalid dns browse callback"); + ChipLogError(DeviceLayer, "Invalid dns resolve callback"); return; } @@ -2734,16 +2832,31 @@ void GenericThreadStackManagerImpl_OpenThread::OnDnsResolveResult(otE serviceInfo.mTxtData = txtBuffer; serviceInfo.mTxtDataSize = sizeof(txtBuffer); - error = MapOpenThreadError(otDnsServiceResponseGetServiceInfo(aResponse, &serviceInfo)); + otErr = otDnsServiceResponseGetServiceInfo(aResponse, &serviceInfo); + error = MapOpenThreadError(otErr); VerifyOrExit(error == CHIP_NO_ERROR, ); - error = FromOtDnsResponseToMdnsData(serviceInfo, type, dnsResult->mMdnsService, dnsResult->mServiceTxtEntry); + error = FromOtDnsResponseToMdnsData(serviceInfo, type, dnsResult->mMdnsService, dnsResult->mServiceTxtEntry, otErr); exit: + if (dnsResult == nullptr) + { + DeviceLayer::PlatformMgr().ScheduleWork(DispatchResolveNoMemory, reinterpret_cast(aContext)); + return; + } dnsResult->error = error; - DeviceLayer::PlatformMgr().ScheduleWork(DispatchResolve, reinterpret_cast(dnsResult)); + + // If IPv6 address in unspecified (AAAA record not present), send additional DNS query to obtain IPv6 address. + if (otIp6IsAddressUnspecified(&serviceInfo.mHostAddress)) + { + DeviceLayer::PlatformMgr().ScheduleWork(DispatchAddressResolve, reinterpret_cast(dnsResult)); + } + else + { + DeviceLayer::PlatformMgr().ScheduleWork(DispatchResolve, reinterpret_cast(dnsResult)); + } } template diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h index 413f00649ab593..2397b5e143828d 100755 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h @@ -136,8 +136,11 @@ class GenericThreadStackManagerImpl_OpenThread CHIP_ERROR _DnsBrowse(const char * aServiceName, DnsBrowseCallback aCallback, void * aContext); CHIP_ERROR _DnsResolve(const char * aServiceName, const char * aInstanceName, DnsResolveCallback aCallback, void * aContext); static void DispatchResolve(intptr_t context); + static void DispatchResolveNoMemory(intptr_t context); + static void DispatchAddressResolve(intptr_t context); static void DispatchBrowseEmpty(intptr_t context); static void DispatchBrowse(intptr_t context); + static void DispatchBrowseNoMemory(intptr_t context); #endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT #endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT @@ -261,9 +264,13 @@ class GenericThreadStackManagerImpl_OpenThread static void OnDnsBrowseResult(otError aError, const otDnsBrowseResponse * aResponse, void * aContext); static void OnDnsResolveResult(otError aError, const otDnsServiceResponse * aResponse, void * aContext); + static void OnDnsAddressResolveResult(otError aError, const otDnsAddressResponse * aResponse, void * aContext); + + static CHIP_ERROR ResolveAddress(intptr_t context, otDnsAddressCallback callback); + static CHIP_ERROR FromOtDnsResponseToMdnsData(otDnsServiceInfo & serviceInfo, const char * serviceType, - chip::Dnssd::DnssdService & mdnsService, - DnsServiceTxtEntries & serviceTxtEntries); + chip::Dnssd::DnssdService & mdnsService, DnsServiceTxtEntries & serviceTxtEntries, + otError error); #endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT #endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT