From aceeac7c231ecb32cfd60e65d87d1b7a013d60b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Thu, 18 Jan 2024 20:59:36 +0100 Subject: [PATCH] [1.1] Cherry pick Thread DNS client and memory leak fixes (#31457) * [app] Fix DeferredAttributePersister memory leak (#31075) * [app] Fix DeferredAttributePerister memory leak ScopedMemoryBuffer's Release() method was used instead of Free(). Add CHECK_RETURN_VALUE annotation to the Release() method to prevent from making such a mistake in the future. Signed-off-by: Damian Krolik * Code review --------- Signed-off-by: Damian Krolik (cherry picked from commit 3e8aeeb7d6cb58811b7ec5be57f576aef7855e13) * [OpenThread] Harden DNS record parsing (#31227) OpenThread applications would crash upon receiving an empty DNS TXT record. The reason was that the code for copying OT DNS service info object into Matter DnssdService object would not initialize the TXT entry count in the latter object in such a case. In the reported case, the Matter stack was presented an empty TXT record because OpenThread's DNS client received a TXT record with TTL 0 and it discarded its contents. Nevertheless, the issue could be reproduced by publishing Matter service without TXT entries and kicking off DNS query. 1. Initialize the TXT entry and subtype count properly in all scenarios. 2. Do not even process the service info object if an error was returned by OpenThread before. 3. Extract some boilerplate to a separate function to improve readability. Signed-off-by: Damian Krolik (cherry picked from commit 76b6bb5bcc13c4de6aa67885f6c2328fc8b5998b) --- .../DeferredAttributePersistenceProvider.cpp | 2 +- src/darwin/Framework/CHIP/MTRBaseDevice.mm | 4 +- src/lib/support/ScopedBuffer.h | 30 +++++--- ...nericThreadStackManagerImpl_OpenThread.hpp | 72 +++++++++---------- 4 files changed, 59 insertions(+), 49 deletions(-) diff --git a/src/app/DeferredAttributePersistenceProvider.cpp b/src/app/DeferredAttributePersistenceProvider.cpp index 62d5c446fe2091..82652792806d5e 100644 --- a/src/app/DeferredAttributePersistenceProvider.cpp +++ b/src/app/DeferredAttributePersistenceProvider.cpp @@ -39,7 +39,7 @@ void DeferredAttribute::Flush(AttributePersistenceProvider & persister) { VerifyOrReturn(IsArmed()); persister.WriteValue(mPath, ByteSpan(mValue.Get(), mValue.AllocatedSize())); - mValue.Release(); + mValue.Free(); } CHIP_ERROR DeferredAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index d0ef32ec1b0bf9..e38d5e3dacfe2b 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -1141,8 +1141,8 @@ - (void)readAttributePaths:(NSArray * _Nullable)attri // callback->AdoptReadClient(std::move(readClient)); callback.release(); - attributePathParamsList.Release(); - eventPathParamsList.Release(); + IgnoreUnusedVariable(attributePathParamsList.Release()); + IgnoreUnusedVariable(eventPathParamsList.Release()); return err; }); std::move(*bridge).DispatchAction(self); diff --git a/src/lib/support/ScopedBuffer.h b/src/lib/support/ScopedBuffer.h index 258b7f262935fb..7a3c0aaca5902c 100644 --- a/src/lib/support/ScopedBuffer.h +++ b/src/lib/support/ScopedBuffer.h @@ -25,6 +25,7 @@ #pragma once #include +#include #include #include @@ -84,10 +85,11 @@ class ScopedMemoryBufferBase const void * Ptr() const { return mBuffer; } /** - * Releases the undelying buffer. Buffer stops being managed and will not be - * auto-freed. + * Releases the underlying buffer. + * + * The buffer stops being managed and will not be auto-freed. */ - void * Release() + CHECK_RETURN_VALUE void * Release() { void * buffer = mBuffer; mBuffer = nullptr; @@ -139,13 +141,18 @@ class ScopedMemoryBuffer : public Impl::ScopedMemoryBufferBase static_assert(std::is_trivially_destructible::value, "Destructors won't get run"); - inline T * Get() { return static_cast(Base::Ptr()); } - inline T & operator[](size_t index) { return Get()[index]; } + T * Get() { return static_cast(Base::Ptr()); } + T & operator[](size_t index) { return Get()[index]; } - inline const T * Get() const { return static_cast(Base::Ptr()); } - inline const T & operator[](size_t index) const { return Get()[index]; } + const T * Get() const { return static_cast(Base::Ptr()); } + const T & operator[](size_t index) const { return Get()[index]; } - inline T * Release() { return static_cast(Base::Release()); } + /** + * Releases the underlying buffer. + * + * The buffer stops being managed and will not be auto-freed. + */ + CHECK_RETURN_VALUE T * Release() { return static_cast(Base::Release()); } ScopedMemoryBuffer & Calloc(size_t elementCount) { @@ -222,7 +229,12 @@ class ScopedMemoryBufferWithSize : public ScopedMemoryBuffer ScopedMemoryBuffer::Free(); } - T * Release() + /** + * Releases the underlying buffer. + * + * The buffer stops being managed and will not be auto-freed. + */ + CHECK_RETURN_VALUE T * Release() { T * buffer = ScopedMemoryBuffer::Release(); mCount = 0; diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp index a75f791966ac41..21bb87668cefb2 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp @@ -94,6 +94,29 @@ void initNetworkCommissioningThreadDriver(void) #endif } +#if CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT +CHIP_ERROR ReadDomainNameComponent(const char *& in, char * out, size_t outSize) +{ + const char * dotPos = strchr(in, '.'); + VerifyOrReturnError(dotPos != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + + const size_t componentSize = static_cast(dotPos - in); + VerifyOrReturnError(componentSize < outSize, CHIP_ERROR_INVALID_ARGUMENT); + + memcpy(out, in, componentSize); + out[componentSize] = '\0'; + in += componentSize + 1; + + return CHIP_NO_ERROR; +} + +template +CHIP_ERROR ReadDomainNameComponent(const char *& in, char (&out)[N]) +{ + return ReadDomainNameComponent(in, out, N); +} +#endif + NetworkCommissioning::otScanResponseIterator mScanResponseIter; } // namespace @@ -2317,29 +2340,8 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::FromOtDnsRespons { char protocol[chip::Dnssd::kDnssdProtocolTextMaxSize + 1]; - if (strchr(serviceType, '.') == nullptr) - return CHIP_ERROR_INVALID_ARGUMENT; - - // Extract from the ... the part. - size_t substringSize = strchr(serviceType, '.') - serviceType; - if (substringSize >= ArraySize(mdnsService.mType)) - { - return CHIP_ERROR_INVALID_ARGUMENT; - } - Platform::CopyString(mdnsService.mType, substringSize + 1, serviceType); - - // Extract from the ... the part. - const char * protocolSubstringStart = serviceType + substringSize + 1; - - if (strchr(protocolSubstringStart, '.') == nullptr) - return CHIP_ERROR_INVALID_ARGUMENT; - - substringSize = strchr(protocolSubstringStart, '.') - protocolSubstringStart; - if (substringSize >= ArraySize(protocol)) - { - return CHIP_ERROR_INVALID_ARGUMENT; - } - Platform::CopyString(protocol, substringSize + 1, protocolSubstringStart); + ReturnErrorOnFailure(ReadDomainNameComponent(serviceType, mdnsService.mType)); + ReturnErrorOnFailure(ReadDomainNameComponent(serviceType, protocol)); if (strncmp(protocol, "_udp", chip::Dnssd::kDnssdProtocolTextMaxSize) == 0) { @@ -2354,24 +2356,20 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::FromOtDnsRespons mdnsService.mProtocol = chip::Dnssd::DnssdServiceProtocol::kDnssdProtocolUnknown; } + mdnsService.mInterface = Inet::InterfaceId::Null(); + mdnsService.mSubTypeSize = 0; + mdnsService.mTextEntrySize = 0; + // Check if SRV record was included in DNS response. - if (error != OT_ERROR_NOT_FOUND) + // If not, return partial information about the service and exit early. + if (error != OT_ERROR_NONE) { - if (strchr(serviceInfo.mHostNameBuffer, '.') == nullptr) - return CHIP_ERROR_INVALID_ARGUMENT; - - // 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); - - mdnsService.mPort = serviceInfo.mPort; + return CHIP_NO_ERROR; } - mdnsService.mInterface = Inet::InterfaceId::Null(); + const char * host = serviceInfo.mHostNameBuffer; + ReturnErrorOnFailure(ReadDomainNameComponent(host, mdnsService.mHostName)); + mdnsService.mPort = serviceInfo.mPort; // Check if AAAA record was included in DNS response.