From f2e5b70db4518f6cda1fb8914bfbc6f1fec0263f 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.cpp | 72 +++++++++---------- 4 files changed, 59 insertions(+), 49 deletions(-) diff --git a/src/app/DeferredAttributePersistenceProvider.cpp b/src/app/DeferredAttributePersistenceProvider.cpp index 37374e15284689..d4b8468fd6d752 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 & path, const ByteSpan & value) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index f3251dbbc4db8f..d9bc8f7e78f2fb 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -1049,8 +1049,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 0158ad4acd0d5d..70549b1c59cf1d 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.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp index 3142fd102ba670..fcfabefd1b0009 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp @@ -92,6 +92,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 @@ -2502,29 +2525,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) { @@ -2539,24 +2541,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.