From ee27e71a81601f72841ff3b4a31fe3506a03cd8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Mon, 26 Jul 2021 17:42:59 +0200 Subject: [PATCH] [OpenThread] Optimize memory usage of DNS client (#8614) In the worst-case scenario, the DNS client must correctly handle a commissionable node service which contains a TXT entry with key "PI" and a pairing instruction up to 128 characters. Change the method of allocating memory for the resolved/discovered services to allow such long TXT entries yet not to consume too much memory on stack. --- src/lib/mdns/Advertiser.h | 2 ++ src/lib/support/SafeString.h | 23 ++++++++++++++ src/lib/support/tests/TestSafeString.cpp | 9 +++++- ...nericThreadStackManagerImpl_OpenThread.cpp | 31 ++++++++++--------- ...GenericThreadStackManagerImpl_OpenThread.h | 19 +++++++----- 5 files changed, 60 insertions(+), 24 deletions(-) diff --git a/src/lib/mdns/Advertiser.h b/src/lib/mdns/Advertiser.h index 7efc4cb9c49063..4438f7cd103c99 100644 --- a/src/lib/mdns/Advertiser.h +++ b/src/lib/mdns/Advertiser.h @@ -107,6 +107,7 @@ class OperationalAdvertisingParameters : public BaseAdvertisingParams(aArgs)...)); } +/** + * A function that determines, at compile time, the total length (not + * counting the null terminator) of a list of literal C strings. Suitable for + * determining sizes of buffers that might need to hold all of the given + * strings. + * + * Do NOT pass things that are not string literals to this function. + * + * Use like: + * constexpr size_t totalLen = TotalStringLength("abc", "defhij", "something"); + */ +constexpr size_t TotalStringLength() +{ + return 0; +} + +template +constexpr size_t TotalStringLength(const char (&)[FirstLength], RestOfTypes &&... aArgs) +{ + // Subtract 1 because we are not counting the null-terminator. + return FirstLength - 1 + TotalStringLength(std::forward(aArgs)...); +} + } // namespace chip diff --git a/src/lib/support/tests/TestSafeString.cpp b/src/lib/support/tests/TestSafeString.cpp index d0d03ef82bc449..d68dd20528126a 100644 --- a/src/lib/support/tests/TestSafeString.cpp +++ b/src/lib/support/tests/TestSafeString.cpp @@ -41,11 +41,18 @@ static void TestMaxStringLength(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, MaxStringLength("") == 0); } +static void TestTotalStringLength(nlTestSuite * inSuite, void * inContext) +{ + NL_TEST_ASSERT(inSuite, TotalStringLength("") == 0); + NL_TEST_ASSERT(inSuite, TotalStringLength("a") == 1); + NL_TEST_ASSERT(inSuite, TotalStringLength("def", "bc", "a") == 6); +} + #define NL_TEST_DEF_FN(fn) NL_TEST_DEF("Test " #fn, fn) /** * Test Suite. It lists all the test functions. */ -static const nlTest sTests[] = { NL_TEST_DEF_FN(TestMaxStringLength), NL_TEST_SENTINEL() }; +static const nlTest sTests[] = { NL_TEST_DEF_FN(TestMaxStringLength), NL_TEST_DEF_FN(TestTotalStringLength), NL_TEST_SENTINEL() }; int TestSafeString(void) { diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp index 135600adf4dfc9..400458bcdd8cc1 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp @@ -1325,22 +1325,22 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::FromOtDnsRespons otDnsInitTxtEntryIterator(&iterator, serviceInfo.mTxtData, serviceInfo.mTxtDataSize); otDnsTxtEntry txtEntry; + FixedBufferAllocator alloc(serviceTxtEntries.mBuffer); uint8_t entryIndex = 0; while ((otDnsGetNextTxtEntry(&iterator, &txtEntry) == OT_ERROR_NONE) && entryIndex < kMaxDnsServiceTxtEntriesNumber) { - if (txtEntry.mKey && strlen(txtEntry.mKey) <= kMaxDnsServiceTxtKeySize && txtEntry.mValue && - txtEntry.mValueLength <= kMaxDnsServiceTxtValueSize) - { - strcpy(serviceTxtEntries.mTxtKeyBuffers[entryIndex], txtEntry.mKey); - serviceTxtEntries.mTxtEntries[entryIndex].mKey = serviceTxtEntries.mTxtKeyBuffers[entryIndex]; - serviceTxtEntries.mTxtEntries[entryIndex].mDataSize = txtEntry.mValueLength; - memcpy(serviceTxtEntries.mTxtValueBuffers[entryIndex], txtEntry.mValue, txtEntry.mValueLength); - serviceTxtEntries.mTxtEntries[entryIndex].mData = serviceTxtEntries.mTxtValueBuffers[entryIndex]; - entryIndex++; - } + 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; @@ -1357,9 +1357,9 @@ void GenericThreadStackManagerImpl_OpenThread::OnDnsBrowseResult(otEr char type[chip::Mdns::kMdnsTypeAndProtocolMaxSize + SrpClient::kMaxDomainNameSize + 3]; // hostname buffer size is kMdnsHostNameMaxSize + . + kMaxDomainNameSize + . + termination character char hostname[chip::Mdns::kMdnsHostNameMaxSize + SrpClient::kMaxDomainNameSize + 3]; - - uint8_t txtBuffer[kMaxDnsServiceTxtEntriesNumber * - (kMaxDnsServiceTxtKeySize + 1 + kMaxDnsServiceTxtValueSize + sizeof(chip::Mdns::TextEntry))]; + // secure space for the raw TXT data in the worst-case scenario relevant for Matter: + // each entry consists of txt_entry_size (1B) + txt_entry_key + "=" + txt_entry_data + uint8_t txtBuffer[kMaxDnsServiceTxtEntriesNumber + kTotalDnsServiceTxtBufferSize]; otDnsServiceInfo serviceInfo; uint16_t index = 0; bool wasAnythingBrowsed; @@ -1448,8 +1448,9 @@ void GenericThreadStackManagerImpl_OpenThread::OnDnsResolveResult(otE char type[chip::Mdns::kMdnsTypeAndProtocolMaxSize + SrpClient::kMaxDomainNameSize + 3]; // hostname buffer size is kMdnsHostNameMaxSize + . + kMaxDomainNameSize + . + termination character char hostname[chip::Mdns::kMdnsHostNameMaxSize + SrpClient::kMaxDomainNameSize + 3]; - uint8_t txtBuffer[kMaxDnsServiceTxtEntriesNumber * - (kMaxDnsServiceTxtKeySize + 1 + kMaxDnsServiceTxtValueSize + sizeof(chip::Mdns::TextEntry))]; + // secure space for the raw TXT data in the worst-case scenario relevant for Matter: + // each entry consists of txt_entry_size (1B) + txt_entry_key + "=" + txt_entry_data + uint8_t txtBuffer[kMaxDnsServiceTxtEntriesNumber + kTotalDnsServiceTxtBufferSize]; otDnsServiceInfo serviceInfo; if (ThreadStackMgrImpl().mDnsResolveCallback == nullptr) diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h index 6aa930220a0ae9..9792d3c914a222 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h @@ -182,25 +182,28 @@ 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 = 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; + static constexpr uint8_t kMaxDnsServiceTxtEntriesNumber = + std::max(Mdns::CommissionAdvertisingParameters::kTxtMaxNumber, Mdns::OperationalAdvertisingParameters::kTxtMaxNumber); + static constexpr size_t kTotalDnsServiceTxtValueSize = std::max(Mdns::CommissionAdvertisingParameters::kTxtTotalValueSize, + Mdns::OperationalAdvertisingParameters::kTxtTotalValueSize); + static constexpr size_t kTotalDnsServiceTxtKeySize = + std::max(Mdns::CommissionAdvertisingParameters::kTxtTotalKeySize, Mdns::OperationalAdvertisingParameters::kTxtTotalKeySize); #else // Thread only supports operational discovery. static constexpr uint8_t kMaxDnsServiceTxtEntriesNumber = Mdns::OperationalAdvertisingParameters::kTxtMaxNumber; - static constexpr uint8_t kMaxDnsServiceTxtValueSize = Mdns::OperationalAdvertisingParameters::kTxtMaxValueSize; - static constexpr uint8_t kMaxDnsServiceTxtKeySize = Mdns::OperationalAdvertisingParameters::kTxtMaxKeySize; + static constexpr size_t kTotalDnsServiceTxtValueSize = Mdns::OperationalAdvertisingParameters::kTxtTotalValueSize; + static constexpr size_t kTotalDnsServiceTxtKeySize = Mdns::OperationalAdvertisingParameters::kTxtTotalKeySize; #endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD_COMMISSIONABLE_DISCOVERY + static constexpr size_t kTotalDnsServiceTxtBufferSize = + kTotalDnsServiceTxtKeySize + kMaxDnsServiceTxtEntriesNumber + kTotalDnsServiceTxtValueSize; DnsBrowseCallback mDnsBrowseCallback; DnsResolveCallback mDnsResolveCallback; struct DnsServiceTxtEntries { + uint8_t mBuffer[kTotalDnsServiceTxtBufferSize]; chip::Mdns::TextEntry mTxtEntries[kMaxDnsServiceTxtEntriesNumber]; - uint8_t mTxtValueBuffers[kMaxDnsServiceTxtEntriesNumber][kMaxDnsServiceTxtValueSize]; - char mTxtKeyBuffers[kMaxDnsServiceTxtEntriesNumber][kMaxDnsServiceTxtKeySize + 1]; }; struct DnsResult