From 37df562c0c554b69b270d91610fd9736cbd8298f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Sat, 17 Jul 2021 16:26:23 +0200 Subject: [PATCH] [dns-sd] Optimize memory usage of SRP client (#8370) * [dns-sd] Optimize memory usage of SRP client Allocate all SRP service data from a single flat buffer instead of using an array of arrays. That way, less memory is used when entries of the same kind differ in size. Also, don't duplicate TXT entry keys as they are constants. * Address code review comments * Add TODO * Restyled by clang-format Co-authored-by: Restyled.io --- src/lib/mdns/Advertiser.h | 33 ++++--- src/lib/mdns/Discovery_ImplPlatform.cpp | 4 +- src/lib/support/BUILD.gn | 2 + src/lib/support/FixedBufferAllocator.cpp | 52 ++++++++++ src/lib/support/FixedBufferAllocator.h | 96 +++++++++++++++++++ src/lib/support/tests/BUILD.gn | 1 + .../tests/TestFixedBufferAllocator.cpp | 79 +++++++++++++++ ...nericThreadStackManagerImpl_OpenThread.cpp | 62 ++++++------ ...GenericThreadStackManagerImpl_OpenThread.h | 56 ++++++----- 9 files changed, 313 insertions(+), 72 deletions(-) create mode 100644 src/lib/support/FixedBufferAllocator.cpp create mode 100644 src/lib/support/FixedBufferAllocator.h create mode 100644 src/lib/support/tests/TestFixedBufferAllocator.cpp diff --git a/src/lib/mdns/Advertiser.h b/src/lib/mdns/Advertiser.h index 8ac8adc6c4dfd9..7efc4cb9c49063 100644 --- a/src/lib/mdns/Advertiser.h +++ b/src/lib/mdns/Advertiser.h @@ -26,6 +26,7 @@ #include #include #include +#include namespace chip { namespace Mdns { @@ -34,6 +35,11 @@ static constexpr uint16_t kMdnsPort = 5353; // Need 8 bytes to fit a thread mac. static constexpr size_t kMaxMacSize = 8; +// Operational node TXT entries +static constexpr size_t kTxtRetryIntervalIdleMaxLength = 7; // [CRI] 0-3600000 +static constexpr size_t kTxtRetryIntervalActiveMaxLength = 7; // [CRA] 0-3600000 + +// Commissionable/commissioner node TXT entries static constexpr size_t kKeyDiscriminatorMaxLength = 5; static constexpr size_t kKeyVendorProductMaxLength = 11; static constexpr size_t kKeyAdditionalPairingMaxLength = 1; @@ -44,6 +50,7 @@ static constexpr size_t kKeyRotatingIdMaxLength = 100; static constexpr size_t kKeyPairingInstructionMaxLength = 128; static constexpr size_t kKeyPairingHintMaxLength = 10; +// Commissionable/commissioner node subtypes static constexpr size_t kSubTypeShortDiscriminatorMaxLength = 4; // _S
static constexpr size_t kSubTypeLongDiscriminatorMaxLength = 6; // _L static constexpr size_t kSubTypeVendorMaxLength = 7; // _V @@ -51,9 +58,8 @@ static constexpr size_t kSubTypeDeviceTypeMaxLength = 5; // _T static constexpr size_t kSubTypeCommissioningModeMaxLength = 3; // _C static constexpr size_t kSubTypeAdditionalPairingMaxLength = 3; // _A static constexpr size_t kSubTypeMaxNumber = 6; -static constexpr size_t kSubTypeMaxLength = - std::max({ kSubTypeShortDiscriminatorMaxLength, kSubTypeLongDiscriminatorMaxLength, kSubTypeVendorMaxLength, - kSubTypeDeviceTypeMaxLength, kSubTypeCommissioningModeMaxLength, kSubTypeAdditionalPairingMaxLength }); +static constexpr size_t kSubTypeTotalLength = kSubTypeShortDiscriminatorMaxLength + kSubTypeLongDiscriminatorMaxLength + + kSubTypeVendorMaxLength + kSubTypeDeviceTypeMaxLength + kSubTypeCommissioningModeMaxLength + kSubTypeAdditionalPairingMaxLength; enum class CommssionAdvertiseMode : uint8_t { @@ -98,10 +104,10 @@ class BaseAdvertisingParams class OperationalAdvertisingParameters : public BaseAdvertisingParams { public: - // Amount of mDNS text entries required for this advertising type - static constexpr uint8_t kNumAdvertisingTxtEntries = 2; - static constexpr uint8_t kTxtMaxKeySize = 3 + 1; // "CRI"/"CRA" as possible keys - static constexpr uint8_t kTxtMaxValueSize = 7 + 1; // Max for text representation of the 32-bit MRP intervals + static constexpr uint8_t kTxtMaxNumber = 2; + static constexpr uint8_t kTxtMaxKeySize = MaxStringLength("CRI", "CRA"); // possible keys + static constexpr uint8_t kTxtMaxValueSize = std::max({ kTxtRetryIntervalIdleMaxLength, kTxtRetryIntervalActiveMaxLength }); + static constexpr size_t kTxtTotalValueSize = kTxtRetryIntervalIdleMaxLength + kTxtRetryIntervalActiveMaxLength; OperationalAdvertisingParameters & SetPeerId(const PeerId & peerId) { @@ -131,10 +137,15 @@ class OperationalAdvertisingParameters : public BaseAdvertisingParams { public: - // Amount of mDNS text entries required for this advertising type - static constexpr uint8_t kNumAdvertisingTxtEntries = 8; // Min 1 - Max 8 - static constexpr uint8_t kTxtMaxKeySize = 2 + 1; // "D"/"VP"/"CM"/"DT"/"DN"/"RI"/"PI"/"PH" as possible keys - static constexpr uint8_t kTxtMaxValueSize = 128; // Max from PI - Pairing Instruction + static constexpr uint8_t kTxtMaxNumber = 9; + static constexpr uint8_t kTxtMaxKeySize = MaxStringLength("D", "VP", "CM", "DT", "DN", "RI", "PI", "PH"); // possible keys + static constexpr uint8_t kTxtMaxValueSize = + std::max({ kKeyDiscriminatorMaxLength, kKeyVendorProductMaxLength, kKeyAdditionalPairingMaxLength, + kKeyCommissioningModeMaxLength, kKeyDeviceTypeMaxLength, kKeyDeviceNameMaxLength, kKeyRotatingIdMaxLength, + kKeyPairingInstructionMaxLength, kKeyPairingHintMaxLength }); + static constexpr size_t kTxtTotalValueSize = kKeyDiscriminatorMaxLength + kKeyVendorProductMaxLength + + kKeyAdditionalPairingMaxLength + kKeyCommissioningModeMaxLength + kKeyDeviceTypeMaxLength + kKeyDeviceNameMaxLength + + kKeyRotatingIdMaxLength + kKeyPairingInstructionMaxLength + kKeyPairingHintMaxLength; CommissionAdvertisingParameters & SetShortDiscriminator(uint8_t discriminator) { diff --git a/src/lib/mdns/Discovery_ImplPlatform.cpp b/src/lib/mdns/Discovery_ImplPlatform.cpp index 7db89127dd3880..56813493e8cdc0 100644 --- a/src/lib/mdns/Discovery_ImplPlatform.cpp +++ b/src/lib/mdns/Discovery_ImplPlatform.cpp @@ -133,7 +133,7 @@ CHIP_ERROR DiscoveryImplPlatform::Advertise(const CommissionAdvertisingParameter char pairingHintBuf[kKeyPairingHintMaxLength + 1]; char pairingInstrBuf[kKeyPairingInstructionMaxLength + 1]; // size of textEntries array should be count of Bufs above - TextEntry textEntries[9]; + TextEntry textEntries[CommissionAdvertisingParameters::kTxtMaxNumber]; size_t textEntrySize = 0; // add null-character to the subtypes char shortDiscriminatorSubtype[kSubTypeShortDiscriminatorMaxLength + 1]; @@ -345,7 +345,7 @@ CHIP_ERROR DiscoveryImplPlatform::Advertise(const OperationalAdvertisingParamete constexpr uint8_t kMaxMRPRetryBufferSize = 7 + 1; char mrpRetryIntervalIdleBuf[kMaxMRPRetryBufferSize]; char mrpRetryIntervalActiveBuf[kMaxMRPRetryBufferSize]; - TextEntry mrpRetryIntervalEntries[OperationalAdvertisingParameters::kNumAdvertisingTxtEntries]; + TextEntry mrpRetryIntervalEntries[OperationalAdvertisingParameters::kTxtMaxNumber]; size_t textEntrySize = 0; uint32_t mrpRetryIntervalIdle, mrpRetryIntervalActive; int writtenCharactersNumber; diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index 560a067abac847..c93295bca7dd36 100644 --- a/src/lib/support/BUILD.gn +++ b/src/lib/support/BUILD.gn @@ -75,6 +75,8 @@ static_library("support") { "ErrorStr.h", "FibonacciUtils.cpp", "FibonacciUtils.h", + "FixedBufferAllocator.cpp", + "FixedBufferAllocator.h", "LifetimePersistedCounter.cpp", "LifetimePersistedCounter.h", "PersistedCounter.cpp", diff --git a/src/lib/support/FixedBufferAllocator.cpp b/src/lib/support/FixedBufferAllocator.cpp new file mode 100644 index 00000000000000..f4e94dd3dc8eb8 --- /dev/null +++ b/src/lib/support/FixedBufferAllocator.cpp @@ -0,0 +1,52 @@ +/* + * + * Copyright (c) 2021 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "FixedBufferAllocator.h" + +#include + +namespace chip { +uint8_t * FixedBufferAllocator::Alloc(size_t count) +{ + if (mBegin + count > mEnd) + { + mAnyAllocFailed = true; + return nullptr; + } + + uint8_t * ptr = mBegin; + mBegin += count; + return ptr; +} + +uint8_t * FixedBufferAllocator::Clone(const void * data, size_t dataLen) +{ + uint8_t * ptr = Alloc(dataLen); + + if (ptr != nullptr) + { + memcpy(ptr, data, dataLen); + } + + return ptr; +} + +char * FixedBufferAllocator::Clone(const char * str) +{ + return reinterpret_cast(Clone(str, strlen(str) + 1)); +} +} // namespace chip diff --git a/src/lib/support/FixedBufferAllocator.h b/src/lib/support/FixedBufferAllocator.h new file mode 100644 index 00000000000000..b3ebf98a9b2110 --- /dev/null +++ b/src/lib/support/FixedBufferAllocator.h @@ -0,0 +1,96 @@ +/* + * + * Copyright (c) 2021 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include + +namespace chip { +/** + * Memory allocator that uses a fixed-size buffer. + * + * This class allocates subsequent memory regions out of a fixed-size buffer. + * Deallocation of specific regions is unsupported and it is assumed that the entire + * buffer will be released at once. + */ +class FixedBufferAllocator +{ +public: + FixedBufferAllocator() = default; + FixedBufferAllocator(uint8_t * buffer, size_t capacity) { Init(buffer, capacity); } + + template + explicit FixedBufferAllocator(uint8_t (&buffer)[N]) + { + Init(buffer); + } + + void Init(uint8_t * buffer, size_t capacity) + { + mBegin = buffer; + mEnd = buffer + capacity; + mAnyAllocFailed = false; + } + + template + void Init(uint8_t (&buffer)[N]) + { + Init(buffer, N); + } + + /** + * Allocate a specified number of bytes. + * + * @param count Number of bytes to allocate. + * @return Pointer to the allocated memory region or nullptr on failure. + */ + uint8_t * Alloc(size_t count); + + /** + * Allocate memory for the specified data and copy the data into the allocated region. + * + * @param data Pointer to the data to be copied into the allocated memory region. + * @param dataLen Size of the data to be copied into the allocated memory region. + * @return Pointer to the allocated memory region or nullptr on failure. + */ + uint8_t * Clone(const void * data, size_t dataLen); + + /** + * Allocate memory for the specified string and copy the string, including + * the null-character, into the allocated region. + * + * @param str Pointer to the string to be copied into the allocated memory region. + * @return Pointer to the allocated memory region or nullptr on failure. + */ + char * Clone(const char * str); + + /** + * Returns whether any allocation has failed so far. + */ + bool AnyAllocFailed() const { return mAnyAllocFailed; } + +private: + FixedBufferAllocator(const FixedBufferAllocator &) = delete; + void operator=(const FixedBufferAllocator &) = delete; + + uint8_t * mBegin = nullptr; + uint8_t * mEnd = nullptr; + bool mAnyAllocFailed = false; +}; + +} // namespace chip diff --git a/src/lib/support/tests/BUILD.gn b/src/lib/support/tests/BUILD.gn index 9bba2b4784611a..dbd99c2c1e74d9 100644 --- a/src/lib/support/tests/BUILD.gn +++ b/src/lib/support/tests/BUILD.gn @@ -30,6 +30,7 @@ chip_test_suite("tests") { "TestCHIPCounter.cpp", "TestCHIPMem.cpp", "TestErrorStr.cpp", + "TestFixedBufferAllocator.cpp", "TestOwnerOf.cpp", "TestPool.cpp", "TestPrivateHeap.cpp", diff --git a/src/lib/support/tests/TestFixedBufferAllocator.cpp b/src/lib/support/tests/TestFixedBufferAllocator.cpp new file mode 100644 index 00000000000000..f7e3ea81d2a47b --- /dev/null +++ b/src/lib/support/tests/TestFixedBufferAllocator.cpp @@ -0,0 +1,79 @@ +/* + * + * Copyright (c) 2021 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include + +#include +#include + +using namespace chip; + +namespace { + +void TestClone(nlTestSuite * inSuite, void * inContext) +{ + uint8_t buffer[128]; + FixedBufferAllocator alloc(buffer); + + const char * kTestString = "Test string"; + const char * allocatedString = alloc.Clone(kTestString); + + NL_TEST_ASSERT(inSuite, allocatedString != nullptr); + NL_TEST_ASSERT(inSuite, allocatedString != kTestString); + NL_TEST_ASSERT(inSuite, strcmp(allocatedString, kTestString) == 0); + + const uint8_t kTestData[] = { 0xDE, 0xAD, 0xBE, 0xEF }; + const uint8_t * allocatedData = alloc.Clone(kTestData, sizeof(kTestData)); + + NL_TEST_ASSERT(inSuite, allocatedData != nullptr); + NL_TEST_ASSERT(inSuite, allocatedData != kTestData); + NL_TEST_ASSERT(inSuite, memcmp(allocatedData, kTestData, sizeof(kTestData)) == 0); +} + +void TestOutOfMemory(nlTestSuite * inSuite, void * inContext) +{ + uint8_t buffer[16]; + FixedBufferAllocator alloc(buffer); + + const char * kTestData = "0123456789abcdef"; + + // Allocating 16 bytes still works... + NL_TEST_ASSERT(inSuite, alloc.Clone(kTestData, 16) != nullptr); + NL_TEST_ASSERT(inSuite, !alloc.AnyAllocFailed()); + + // ...but cannot allocate even one more byte... + NL_TEST_ASSERT(inSuite, alloc.Clone(kTestData, 1) == nullptr); + NL_TEST_ASSERT(inSuite, alloc.AnyAllocFailed()); +} + +const nlTest sTests[] = { NL_TEST_DEF("Test successfull clone", TestClone), NL_TEST_DEF("Test out of memory", TestOutOfMemory), + NL_TEST_SENTINEL() }; + +} // namespace + +int TestFixedBufferAllocator() +{ + nlTestSuite theSuite = { "CHIP FixedBufferAllocator tests", &sTests[0], nullptr, nullptr }; + + // Run test suit againt one context. + nlTestRunner(&theSuite, nullptr); + return nlTestRunnerStats(&theSuite); +} + +CHIP_REGISTER_TEST_SUITE(TestFixedBufferAllocator) diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp index f315402631751b..135600adf4dfc9 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp @@ -49,6 +49,7 @@ #include #include #include +#include #include #include @@ -1075,6 +1076,13 @@ void GenericThreadStackManagerImpl_OpenThread::OnSrpClientStateChange } } +template +bool GenericThreadStackManagerImpl_OpenThread::SrpClient::Service::Matches(const char * aInstanceName, + const char * aName) const +{ + return IsUsed() && (strcmp(mService.mInstanceName, aInstanceName) == 0) && (strcmp(mService.mName, aName) == 0); +} + template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_AddSrpService(const char * aInstanceName, const char * aName, uint16_t aPort, @@ -1085,27 +1093,26 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_AddSrpService(c CHIP_ERROR error = CHIP_NO_ERROR; typename SrpClient::Service * srpService = nullptr; size_t entryId = 0; + FixedBufferAllocator alloc; Impl()->LockThreadStack(); VerifyOrExit(aInstanceName, error = CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrExit(strlen(aInstanceName) <= SrpClient::kMaxInstanceNameSize, error = CHIP_ERROR_INVALID_STRING_LENGTH); VerifyOrExit(aName, error = CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrExit(strlen(aName) <= SrpClient::kMaxNameSize, error = CHIP_ERROR_INVALID_STRING_LENGTH); // Try to find an empty slot in array for the new service and // remove the possible existing entry from anywhere in the list for (typename SrpClient::Service & service : mSrpClient.mServices) { // Remove possible existing entry - if ((strcmp(service.mInstanceName, aInstanceName) == 0) && (strcmp(service.mName, aName) == 0)) + if (service.Matches(aInstanceName, aName)) { SuccessOrExit(error = MapOpenThreadError(otSrpClientClearService(mOTInst, &service.mService))); // Clear memory immediately, as OnSrpClientNotification will not be called. memset(&service, 0, sizeof(service)); } - if ((srpService == nullptr) && (strcmp(service.mInstanceName, "") == 0)) + if ((srpService == nullptr) && !service.IsUsed()) { // Assign first empty slot found in array for a new service. srpService = &service; @@ -1115,30 +1122,23 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_AddSrpService(c } // Verify there is a slot found for the new service. - VerifyOrExit(srpService, error = MapOpenThreadError(OT_ERROR_NO_BUFS)); + VerifyOrExit(srpService != nullptr, error = CHIP_ERROR_BUFFER_TOO_SMALL); + alloc.Init(srpService->mServiceBuffer); otSrpClientSetLeaseInterval(mOTInst, aLeaseInterval); otSrpClientSetKeyLeaseInterval(mOTInst, aKeyLeaseInterval); - memcpy(srpService->mInstanceName, aInstanceName, strlen(aInstanceName) + 1); - srpService->mService.mInstanceName = srpService->mInstanceName; - - memcpy(srpService->mName, aName, strlen(aName) + 1); - srpService->mService.mName = srpService->mName; - - srpService->mService.mPort = aPort; + srpService->mService.mInstanceName = alloc.Clone(aInstanceName); + srpService->mService.mName = alloc.Clone(aName); + srpService->mService.mPort = aPort; #if OPENTHREAD_API_VERSION >= 132 - VerifyOrExit(aSubTypes.size() <= ArraySize(srpService->mSubTypeBuffers), error = CHIP_ERROR_BUFFER_TOO_SMALL); + VerifyOrExit(aSubTypes.size() < ArraySize(srpService->mSubTypes), error = CHIP_ERROR_BUFFER_TOO_SMALL); entryId = 0; for (const char * subType : aSubTypes) { - auto & destBuffer = srpService->mSubTypeBuffers[entryId]; - VerifyOrExit(strlen(subType) < ArraySize(destBuffer), error = CHIP_ERROR_BUFFER_TOO_SMALL); - strcpy(destBuffer, subType); - - srpService->mSubTypes[entryId++] = destBuffer; + srpService->mSubTypes[entryId++] = alloc.Clone(subType); } srpService->mSubTypes[entryId] = nullptr; @@ -1151,17 +1151,13 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_AddSrpService(c for (const chip::Mdns::TextEntry & entry : aTxtEntries) { - VerifyOrExit(strlen(entry.mKey) < SrpClient::kMaxTxtKeySize, error = CHIP_ERROR_BUFFER_TOO_SMALL); - strcpy(srpService->mTxtKeyBuffers[entryId], entry.mKey); - - VerifyOrExit(entry.mDataSize <= SrpClient::kMaxTxtValueSize, error = CHIP_ERROR_BUFFER_TOO_SMALL); - memcpy(srpService->mTxtValueBuffers[entryId], entry.mData, entry.mDataSize); - using OtTxtValueLength = decltype(srpService->mTxtEntries[entryId].mValueLength); - static_assert(SrpClient::kMaxTxtValueSize <= std::numeric_limits::max(), + static_assert(SrpClient::kServiceBufferSize <= std::numeric_limits::max(), "DNS TXT value length may not fit in otDnsTxtEntry structure"); - srpService->mTxtEntries[entryId].mKey = srpService->mTxtKeyBuffers[entryId]; - srpService->mTxtEntries[entryId].mValue = srpService->mTxtValueBuffers[entryId]; + + // TXT entry keys are constants, so they don't need to be cloned + srpService->mTxtEntries[entryId].mKey = entry.mKey; + srpService->mTxtEntries[entryId].mValue = alloc.Clone(entry.mData, entry.mDataSize); srpService->mTxtEntries[entryId].mValueLength = static_cast(entry.mDataSize); ++entryId; } @@ -1172,6 +1168,8 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_AddSrpService(c srpService->mService.mNumTxtEntries = static_cast(aTxtEntries.size()); srpService->mService.mTxtEntries = srpService->mTxtEntries; + VerifyOrExit(!alloc.AnyAllocFailed(), error = CHIP_ERROR_BUFFER_TOO_SMALL); + ChipLogProgress(DeviceLayer, "advertising srp service: %s.%s", srpService->mService.mInstanceName, srpService->mService.mName); error = MapOpenThreadError(otSrpClientAddService(mOTInst, &(srpService->mService))); @@ -1194,14 +1192,12 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_RemoveSrpServic Impl()->LockThreadStack(); VerifyOrExit(aInstanceName, error = CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrExit(strlen(aInstanceName) <= SrpClient::kMaxInstanceNameSize, error = CHIP_ERROR_INVALID_STRING_LENGTH); VerifyOrExit(aName, error = CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrExit(strlen(aName) <= SrpClient::kMaxNameSize, error = CHIP_ERROR_INVALID_STRING_LENGTH); // Check if service to remove exists. for (typename SrpClient::Service & service : mSrpClient.mServices) { - if ((strcmp(service.mInstanceName, aInstanceName) == 0) && (strcmp(service.mName, aName) == 0)) + if (service.Matches(aInstanceName, aName)) { srpService = &service; break; @@ -1333,7 +1329,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::FromOtDnsRespons uint8_t entryIndex = 0; while ((otDnsGetNextTxtEntry(&iterator, &txtEntry) == OT_ERROR_NONE) && entryIndex < kMaxDnsServiceTxtEntriesNumber) { - if (txtEntry.mKey && strlen(txtEntry.mKey) < kMaxDnsServiceTxtKeySize && txtEntry.mValue && + if (txtEntry.mKey && strlen(txtEntry.mKey) <= kMaxDnsServiceTxtKeySize && txtEntry.mValue && txtEntry.mValueLength <= kMaxDnsServiceTxtValueSize) { strcpy(serviceTxtEntries.mTxtKeyBuffers[entryIndex], txtEntry.mKey); @@ -1363,7 +1359,7 @@ void GenericThreadStackManagerImpl_OpenThread::OnDnsBrowseResult(otEr char hostname[chip::Mdns::kMdnsHostNameMaxSize + SrpClient::kMaxDomainNameSize + 3]; uint8_t txtBuffer[kMaxDnsServiceTxtEntriesNumber * - (kMaxDnsServiceTxtKeySize + kMaxDnsServiceTxtValueSize + sizeof(chip::Mdns::TextEntry))]; + (kMaxDnsServiceTxtKeySize + 1 + kMaxDnsServiceTxtValueSize + sizeof(chip::Mdns::TextEntry))]; otDnsServiceInfo serviceInfo; uint16_t index = 0; bool wasAnythingBrowsed; @@ -1453,7 +1449,7 @@ void GenericThreadStackManagerImpl_OpenThread::OnDnsResolveResult(otE // hostname buffer size is kMdnsHostNameMaxSize + . + kMaxDomainNameSize + . + termination character char hostname[chip::Mdns::kMdnsHostNameMaxSize + SrpClient::kMaxDomainNameSize + 3]; uint8_t txtBuffer[kMaxDnsServiceTxtEntriesNumber * - (kMaxDnsServiceTxtKeySize + kMaxDnsServiceTxtValueSize + sizeof(chip::Mdns::TextEntry))]; + (kMaxDnsServiceTxtKeySize + 1 + kMaxDnsServiceTxtValueSize + sizeof(chip::Mdns::TextEntry))]; otDnsServiceInfo serviceInfo; if (ThreadStackMgrImpl().mDnsResolveCallback == nullptr) diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h index 168a1ba295ec0f..6aa930220a0ae9 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h @@ -129,8 +129,6 @@ class GenericThreadStackManagerImpl_OpenThread struct SrpClient { static constexpr uint8_t kMaxServicesNumber = CHIP_DEVICE_CONFIG_THREAD_SRP_MAX_SERVICES; - static constexpr uint8_t kMaxInstanceNameSize = chip::Mdns::kMdnsInstanceNameMaxSize; - static constexpr uint8_t kMaxNameSize = chip::Mdns::kMdnsTypeAndProtocolMaxSize; static constexpr uint8_t kMaxHostNameSize = 16; static constexpr const char * kDefaultDomainName = "default.service.arpa"; static constexpr uint8_t kDefaultDomainNameSize = 20; @@ -138,29 +136,36 @@ class GenericThreadStackManagerImpl_OpenThread #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY // Thread supports both operational and commissionable discovery, so buffers sizes must be worst case. - static constexpr uint8_t kMaxTxtEntriesNumber = chip::Mdns::CommissionAdvertisingParameters::kNumAdvertisingTxtEntries; - static constexpr uint8_t kMaxTxtValueSize = chip::Mdns::CommissionAdvertisingParameters::kTxtMaxValueSize; - static constexpr uint8_t kMaxTxtKeySize = chip::Mdns::OperationalAdvertisingParameters::kTxtMaxKeySize; + static constexpr size_t kSubTypeMaxNumber = Mdns::kSubTypeMaxNumber; + static constexpr size_t kSubTypeTotalLength = Mdns::kSubTypeTotalLength; + static constexpr size_t kTxtMaxNumber = + std::max(Mdns::CommissionAdvertisingParameters::kTxtMaxNumber, Mdns::OperationalAdvertisingParameters::kTxtMaxNumber); + static constexpr size_t kTxtTotalValueLength = std::max(Mdns::CommissionAdvertisingParameters::kTxtTotalValueSize, + Mdns::OperationalAdvertisingParameters::kTxtTotalValueSize); #else // Thread only supports operational discovery. - static constexpr uint8_t kMaxTxtEntriesNumber = chip::Mdns::OperationalAdvertisingParameters::kNumAdvertisingTxtEntries; - static constexpr uint8_t kMaxTxtValueSize = chip::Mdns::OperationalAdvertisingParameters::kTxtMaxValueSize; - static constexpr uint8_t kMaxTxtKeySize = chip::Mdns::OperationalAdvertisingParameters::kTxtMaxKeySize; -#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY + static constexpr size_t kSubTypeMaxNumber = 0; + static constexpr size_t kSubTypeTotalLength = 0; + static constexpr size_t kTxtMaxNumber = Mdns::OperationalAdvertisingParameters::kTxtMaxNumber; + static constexpr size_t kTxtTotalValueLength = Mdns::OperationalAdvertisingParameters::kTxtTotalValueSize; +#endif + + static constexpr size_t kServiceBufferSize = Mdns::kMdnsInstanceNameMaxSize + 1 + // add null-terminator + Mdns::kMdnsTypeAndProtocolMaxSize + 1 + // add null-terminator + kSubTypeTotalLength + kSubTypeMaxNumber + // add null-terminator for each subtype + kTxtTotalValueLength; struct Service { otSrpClientService mService; - char mInstanceName[kMaxInstanceNameSize + 1]; - char mName[kMaxNameSize + 1]; + uint8_t mServiceBuffer[kServiceBufferSize]; #if OPENTHREAD_API_VERSION >= 132 - // TODO: use fixed buffer allocator to reduce the memory footprint from N*M to sum(M_i) - char mSubTypeBuffers[chip::Mdns::kSubTypeMaxNumber][chip::Mdns::kSubTypeMaxLength + 1]; - const char * mSubTypes[chip::Mdns::kSubTypeMaxNumber + 1]; // extra entry for nullptr at the end + const char * mSubTypes[kSubTypeMaxNumber + 1]; // extra entry for null terminator #endif - otDnsTxtEntry mTxtEntries[kMaxTxtEntriesNumber]; - uint8_t mTxtValueBuffers[kMaxTxtEntriesNumber][kMaxTxtValueSize]; - char mTxtKeyBuffers[kMaxTxtEntriesNumber][kMaxTxtKeySize]; + otDnsTxtEntry mTxtEntries[kTxtMaxNumber]; + + bool IsUsed() const { return mService.mInstanceName != nullptr; } + bool Matches(const char * aInstanceName, const char * aName) const; }; char mHostName[kMaxHostNameSize + 1]; @@ -177,16 +182,15 @@ class GenericThreadStackManagerImpl_OpenThread #if CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT #if CHIP_DEVICE_CONFIG_ENABLE_THREAD_COMMISSIONABLE_DISCOVERY // Thread supports both operational and commissionable discovery, so buffers sizes must be worst case. - static constexpr uint8_t kMaxDnsServiceTxtEntriesNumber = - chip::Mdns::CommissionAdvertisingParameters::kNumAdvertisingTxtEntries; - static constexpr uint8_t kMaxDnsServiceTxtValueSize = chip::Mdns::CommissionAdvertisingParameters::kTxtMaxValueSize; - static constexpr uint8_t kMaxDnsServiceTxtKeySize = chip::Mdns::OperationalAdvertisingParameters::kTxtMaxKeySize; + static constexpr uint8_t kMaxDnsServiceTxtEntriesNumber = Mdns::CommissionAdvertisingParameters::kTxtMaxNumber; + static constexpr uint8_t kMaxDnsServiceTxtValueSize = Mdns::CommissionAdvertisingParameters::kTxtMaxValueSize; + // TODO: Switch to max(commissionable TXT key size, operational TXT key size) after optimizing memory usage + static constexpr uint8_t kMaxDnsServiceTxtKeySize = Mdns::OperationalAdvertisingParameters::kTxtMaxKeySize; #else // Thread only supports operational discovery. - static constexpr uint8_t kMaxDnsServiceTxtEntriesNumber = - chip::Mdns::OperationalAdvertisingParameters::kNumAdvertisingTxtEntries; - static constexpr uint8_t kMaxDnsServiceTxtValueSize = chip::Mdns::OperationalAdvertisingParameters::kTxtMaxValueSize; - static constexpr uint8_t kMaxDnsServiceTxtKeySize = chip::Mdns::OperationalAdvertisingParameters::kTxtMaxKeySize; + static constexpr uint8_t kMaxDnsServiceTxtEntriesNumber = Mdns::OperationalAdvertisingParameters::kTxtMaxNumber; + static constexpr uint8_t kMaxDnsServiceTxtValueSize = Mdns::OperationalAdvertisingParameters::kTxtMaxValueSize; + static constexpr uint8_t kMaxDnsServiceTxtKeySize = Mdns::OperationalAdvertisingParameters::kTxtMaxKeySize; #endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD_COMMISSIONABLE_DISCOVERY DnsBrowseCallback mDnsBrowseCallback; @@ -196,7 +200,7 @@ class GenericThreadStackManagerImpl_OpenThread { chip::Mdns::TextEntry mTxtEntries[kMaxDnsServiceTxtEntriesNumber]; uint8_t mTxtValueBuffers[kMaxDnsServiceTxtEntriesNumber][kMaxDnsServiceTxtValueSize]; - char mTxtKeyBuffers[kMaxDnsServiceTxtEntriesNumber][kMaxDnsServiceTxtKeySize]; + char mTxtKeyBuffers[kMaxDnsServiceTxtEntriesNumber][kMaxDnsServiceTxtKeySize + 1]; }; struct DnsResult