From 1b6a96be72ec96383d53734caaf0790d1623be4d Mon Sep 17 00:00:00 2001 From: Kevin Schoedel <67607049+kpschoedel@users.noreply.github.com> Date: Wed, 10 Feb 2021 14:29:33 -0500 Subject: [PATCH] Fix and test heap allocation of PacketBuffers (#4632) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix and test heap allocation of PacketBuffers #### Problem CHIP has to option to use heap-allocated packet buffers (with `CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE == 0`) but this configuration has been neglected, and CHIP neither builds nor runs in this configuration. #### Summary of Changes Build fixes: - Default `CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE` to 15 for heap case. 15 is the default packet buffer pool size in SystemConfig.h. See also issue #4335 - Our definition of the CRMP retransmission table size does not really make sense Runtime fixes: - Ensure echo server has enough buffer space for the response (and added `CHECK_RETURN_VALUE` to `EnsureReservedSize()`). - Added room for MAC in echo requester. - Added room for MAC in `chipSendUnicast()`. - Added parameters to `CloneData()` to request header and trailing space. - Fixed CHIPMem initialization for some tests. - Fix tests that didn't allocate enough space. Build and code changes to assist memory troubleshooting (these should have no cost unless enabled): - Added a build flag `chip_config_memory_debug_checks` and corresponding preprocessor definition `CHIP_CONFIG_MEMORY_DEBUG_CHECKS` to enable extra memory checks, at a performance cost. In particular, new functions `Platform::MemoryDebugCheckPointer()` and `PacketBufferHandle::Check()` operate when this is enabled. - Added a build flag `chip_config_memory_debug_dmalloc` and corresponding preprocessor definition `CHIP_CONFIG_MEMORY_DEBUG_DMALLOC` to use the dmalloc debugging malloc library. - Clarified conditions for the four configurations of PacketBuffer allocation. - Replaced some PacketBuffer #define constants with scoped constexprs. - Renamed `CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC` to `CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE` to remove the ambiguity of what a ‘maximum allocation’ is. Build and automated test changes: - Made standalone builds default to heap allocation. - Run a set of unit tests with dmalloc. fixes #4395 - Heap packet buffer allocation is broken fixes #4390 - ReliableMessageProtocol assumes a buffer pool * review * review 2 * fix messaging shutdown * review 3 * fix tests Co-authored-by: Justin Wood --- .github/workflows/build.yaml | 4 +- config/android/CHIPProjectConfig.h | 4 +- config/ios/CHIPProjectConfig.h | 4 +- config/standalone/CHIPProjectConfig.h | 4 +- scripts/tests/gn_tests.sh | 7 + src/app/Command.cpp | 7 +- src/app/tests/TestMessageDef.cpp | 61 ++-- src/app/util/chip-message-send.cpp | 3 +- src/ble/BLEEndPoint.cpp | 2 +- src/ble/BtpEngine.cpp | 2 +- src/controller/CHIPDevice.cpp | 2 +- src/controller/java/CHIPProjectConfig.h | 4 +- .../GenericNetworkProvisioningServerImpl.cpp | 2 +- .../GenericSoftwareUpdateManagerImpl.cpp | 2 +- src/inet/IPEndPointBasis.cpp | 2 +- src/inet/TCPEndPoint.h | 4 +- src/inet/tests/TestInetEndPoint.cpp | 16 +- src/lib/core/BUILD.gn | 4 +- src/lib/core/CHIPConfig.h | 32 +- src/lib/core/core.gni | 8 +- src/lib/core/tests/TestCHIPTLV.cpp | 2 +- src/lib/support/BUILD.gn | 3 + src/lib/support/CHIPMem-Malloc.cpp | 32 +- src/lib/support/CHIPMem-SimpleAlloc.cpp | 11 +- src/lib/support/CHIPMem.h | 40 ++- src/messaging/ExchangeMgr.cpp | 4 +- src/messaging/ReliableMessageMgr.cpp | 11 +- src/messaging/ReliableMessageProtocolConfig.h | 6 +- src/messaging/tests/MessagingContext.cpp | 1 + src/messaging/tests/TestExchangeMgr.cpp | 12 +- .../tests/TestReliableMessageProtocol.cpp | 7 +- .../ESP32/NetworkProvisioningServerImpl.cpp | 2 +- src/platform/Linux/CHIPBluezHelper.cpp | 2 +- src/protocols/bdx/tests/TestBdxMessages.cpp | 26 +- .../bdx/tests/TestBdxTransferSession.cpp | 19 +- src/system/SystemConfig.h | 18 +- src/system/SystemPacketBuffer.cpp | 253 ++++++++-------- src/system/SystemPacketBuffer.h | 279 ++++++++++-------- src/system/TLVPacketBufferBackingStore.cpp | 2 +- src/system/tests/TestSystemPacketBuffer.cpp | 57 ++-- src/transport/SecureSessionMgr.h | 4 +- .../raw/tests/NetworkTestHelpers.cpp | 6 +- src/transport/tests/TestSecureSessionMgr.cpp | 1 - 43 files changed, 627 insertions(+), 345 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index ea8a6750d4dac0..b7f991686db932 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -1,4 +1,4 @@ -# Copyright (c) 2020 Project CHIP Authors +# Copyright (c) 2020-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. @@ -56,7 +56,7 @@ jobs: - name: Setup Build run: | case $BUILD_TYPE in - "main") GN_ARGS='';; + "main") GN_ARGS='chip_config_memory_debug_checks=true chip_config_memory_debug_dmalloc=true';; "clang") GN_ARGS='is_clang=true';; "mbedtls") GN_ARGS='chip_crypto="mbedtls"';; *) ;; diff --git a/config/android/CHIPProjectConfig.h b/config/android/CHIPProjectConfig.h index 1685b9c719b31f..d164cff3dcda26 100644 --- a/config/android/CHIPProjectConfig.h +++ b/config/android/CHIPProjectConfig.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * Copyright (c) 2016-2017 Nest Labs, Inc. * Copyright (c) 2019-2020 Google LLC. * All rights reserved. @@ -44,7 +44,7 @@ #define CHIP_CONFIG_LEGACY_KEY_EXPORT_DELEGATE 0 -#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC 300 +#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE 300 #define CHIP_CONFIG_ENABLE_FUNCT_ERROR_LOGGING 1 diff --git a/config/ios/CHIPProjectConfig.h b/config/ios/CHIPProjectConfig.h index 2761263ce6ff07..a46586c759ecfb 100644 --- a/config/ios/CHIPProjectConfig.h +++ b/config/ios/CHIPProjectConfig.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * Copyright (c) 2016-2017 Nest Labs, Inc. * Copyright (c) 2019-2020 Google LLC. * All rights reserved. @@ -44,7 +44,7 @@ #define CHIP_CONFIG_LEGACY_KEY_EXPORT_DELEGATE 0 -#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC 300 +#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE 300 #define CHIP_CONFIG_ENABLE_FUNCT_ERROR_LOGGING 1 diff --git a/config/standalone/CHIPProjectConfig.h b/config/standalone/CHIPProjectConfig.h index feb00ce7cf7e6d..1d5b2625cd481d 100644 --- a/config/standalone/CHIPProjectConfig.h +++ b/config/standalone/CHIPProjectConfig.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * Copyright (c) 2016-2017 Nest Labs, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -65,7 +65,7 @@ #define CHIP_CONFIG_LEGACY_KEY_EXPORT_DELEGATE 0 -#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC 300 +#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE 0 #define CHIP_CONFIG_ENABLE_FUNCT_ERROR_LOGGING 1 diff --git a/scripts/tests/gn_tests.sh b/scripts/tests/gn_tests.sh index 964a66cc922290..99b9a8913a0fd2 100755 --- a/scripts/tests/gn_tests.sh +++ b/scripts/tests/gn_tests.sh @@ -24,6 +24,13 @@ CHIP_ROOT="$(dirname "$0")/../.." source "$CHIP_ROOT/scripts/activate.sh" +dmalloc=$(gn --root="$CHIP_ROOT" args "$CHIP_ROOT/out/$BUILD_TYPE" --short --list=chip_config_memory_debug_dmalloc) +if [ "x$dmalloc" == "xchip_config_memory_debug_dmalloc = true" ]; then + eval "$(dmalloc -b -l DMALLOC_LOG -i 1 high)" + export G_SLICE + G_SLICE=always-malloc # required for dmalloc compatibility +fi + env set -x diff --git a/src/app/Command.cpp b/src/app/Command.cpp index 41e42518813431..ef9791e809c45b 100644 --- a/src/app/Command.cpp +++ b/src/app/Command.cpp @@ -58,7 +58,7 @@ CHIP_ERROR Command::Reset() if (mCommandMessageBuf.IsNull()) { // TODO: Calculate the packet buffer size - mCommandMessageBuf = System::PacketBufferHandle::New(System::kMaxPacketBufferSize); + mCommandMessageBuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); VerifyOrExit(!mCommandMessageBuf.IsNull(), err = CHIP_ERROR_NO_MEMORY); } @@ -145,7 +145,7 @@ void Command::Shutdown() chip::TLV::TLVWriter & Command::CreateCommandDataElementTLVWriter() { - mCommandDataBuf = chip::System::PacketBufferHandle::New(System::kMaxPacketBufferSize); + mCommandDataBuf = chip::System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); if (mCommandDataBuf.IsNull()) { ChipLogDetail(DataManagement, "Unable to allocate packet buffer"); @@ -275,7 +275,8 @@ CHIP_ERROR Command::FinalizeCommandsMessage() err = mCommandMessageWriter.Finalize(&mCommandMessageBuf); SuccessOrExit(err); - mCommandMessageBuf->EnsureReservedSize(CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE); + VerifyOrExit(mCommandMessageBuf->EnsureReservedSize(System::PacketBuffer::kDefaultHeaderReserve), + err = CHIP_ERROR_BUFFER_TOO_SMALL); exit: ChipLogFunctError(err); diff --git a/src/app/tests/TestMessageDef.cpp b/src/app/tests/TestMessageDef.cpp index 2425351174aa74..ff4a1350219c8a 100644 --- a/src/app/tests/TestMessageDef.cpp +++ b/src/app/tests/TestMessageDef.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -748,7 +749,7 @@ void AttributePathTest(nlTestSuite * apSuite, void * apContext) AttributePath::Builder attributePathBuilder; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); attributePathBuilder.Init(&writer); BuildAttributePath(apSuite, attributePathBuilder); chip::System::PacketBufferHandle buf; @@ -771,7 +772,7 @@ void AttributePathListTest(nlTestSuite * apSuite, void * apContext) chip::System::PacketBufferTLVReader reader; AttributePathList::Builder attributePathListBuilder; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); err = attributePathListBuilder.Init(&writer); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -796,7 +797,7 @@ void EventPathTest(nlTestSuite * apSuite, void * apContext) EventPath::Builder eventPathBuilder; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); eventPathBuilder.Init(&writer); BuildEventPath(apSuite, eventPathBuilder); chip::System::PacketBufferHandle buf; @@ -820,7 +821,7 @@ void EventPathListTest(nlTestSuite * apSuite, void * apContext) chip::System::PacketBufferTLVReader reader; EventPathList::Builder eventPathListBuilder; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); err = eventPathListBuilder.Init(&writer); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -844,7 +845,7 @@ void CommandPathTest(nlTestSuite * apSuite, void * apContext) chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; CommandPath::Builder commandPathBuilder; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); err = commandPathBuilder.Init(&writer); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -870,7 +871,7 @@ void EventDataElementTest(nlTestSuite * apSuite, void * apContext) EventDataElement::Parser eventDataElementParser; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); eventDataElementBuilder.Init(&writer); BuildEventDataElement(apSuite, eventDataElementBuilder); chip::System::PacketBufferHandle buf; @@ -893,7 +894,7 @@ void EventListTest(nlTestSuite * apSuite, void * apContext) chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; EventList::Builder eventListBuilder; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); eventListBuilder.Init(&writer); BuildEventList(apSuite, eventListBuilder); chip::System::PacketBufferHandle buf; @@ -915,7 +916,7 @@ void StatusElementTest(nlTestSuite * apSuite, void * apContext) StatusElement::Parser statusElementParser; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); statusElementBuilder.Init(&writer); BuildStatusElement(apSuite, statusElementBuilder); chip::System::PacketBufferHandle buf; @@ -939,7 +940,7 @@ void AttributeStatusElementTest(nlTestSuite * apSuite, void * apContext) AttributeStatusElement::Parser attributeStatusElementParser; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); attributeStatusElementBuilder.Init(&writer); BuildAttributeStatusElement(apSuite, attributeStatusElementBuilder); chip::System::PacketBufferHandle buf; @@ -961,7 +962,7 @@ void AttributeStatusListTest(nlTestSuite * apSuite, void * apContext) CHIP_ERROR err = CHIP_NO_ERROR; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); AttributeStatusList::Builder attributeStatusListBuilder; err = attributeStatusListBuilder.Init(&writer); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -985,7 +986,7 @@ void AttributeDataElementTest(nlTestSuite * apSuite, void * apContext) AttributeDataElement::Parser attributeDataElementParser; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); attributeDataElementBuilder.Init(&writer); BuildAttributeDataElement(apSuite, attributeDataElementBuilder); chip::System::PacketBufferHandle buf; @@ -1007,7 +1008,7 @@ void AttributeDataListTest(nlTestSuite * apSuite, void * apContext) CHIP_ERROR err = CHIP_NO_ERROR; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); AttributeDataList::Builder attributeDataListBuilder; attributeDataListBuilder.Init(&writer); BuildAttributeDataList(apSuite, attributeDataListBuilder); @@ -1028,7 +1029,7 @@ void AttributeDataVersionListTest(nlTestSuite * apSuite, void * apContext) CHIP_ERROR err = CHIP_NO_ERROR; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); AttributeDataVersionList::Builder attributeDataVersionListBuilder; attributeDataVersionListBuilder.Init(&writer); BuildAttributeDataVersionList(apSuite, attributeDataVersionListBuilder); @@ -1051,7 +1052,7 @@ void CommandDataElementTest(nlTestSuite * apSuite, void * apContext) CommandDataElement::Parser commandDataElementParser; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); commandDataElementBuilder.Init(&writer); BuildCommandDataElement(apSuite, commandDataElementBuilder); chip::System::PacketBufferHandle buf; @@ -1074,7 +1075,7 @@ void CommandListTest(nlTestSuite * apSuite, void * apContext) chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; CommandList::Builder commandListBuilder; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); commandListBuilder.Init(&writer); BuildCommandList(apSuite, commandListBuilder); chip::System::PacketBufferHandle buf; @@ -1094,7 +1095,7 @@ void ReportDataTest(nlTestSuite * apSuite, void * apContext) CHIP_ERROR err = CHIP_NO_ERROR; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); BuildReportData(apSuite, writer); chip::System::PacketBufferHandle buf; err = writer.Finalize(&buf); @@ -1113,7 +1114,7 @@ void InvokeCommandTest(nlTestSuite * apSuite, void * apContext) CHIP_ERROR err = CHIP_NO_ERROR; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); BuildInvokeCommand(apSuite, writer); chip::System::PacketBufferHandle buf; err = writer.Finalize(&buf); @@ -1132,7 +1133,7 @@ void ReadRequestTest(nlTestSuite * apSuite, void * apContext) CHIP_ERROR err = CHIP_NO_ERROR; chip::System::PacketBufferTLVWriter writer; chip::System::PacketBufferTLVReader reader; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); BuildReadRequest(apSuite, writer); chip::System::PacketBufferHandle buf; err = writer.Finalize(&buf); @@ -1176,6 +1177,26 @@ const nlTest sTests[] = // clang-format on } // namespace +/** + * Set up the test suite. + */ +static int TestSetup(void * inContext) +{ + CHIP_ERROR error = chip::Platform::MemoryInit(); + if (error != CHIP_NO_ERROR) + return FAILURE; + return SUCCESS; +} + +/** + * Tear down the test suite. + */ +static int TestTeardown(void * inContext) +{ + chip::Platform::MemoryShutdown(); + return SUCCESS; +} + int TestMessageDef() { // clang-format off @@ -1183,8 +1204,8 @@ int TestMessageDef() { "MessageDef", &sTests[0], - nullptr, - nullptr + TestSetup, + TestTeardown, }; // clang-format on diff --git a/src/app/util/chip-message-send.cpp b/src/app/util/chip-message-send.cpp index 72d6263f7f0c40..43b885000e9152 100644 --- a/src/app/util/chip-message-send.cpp +++ b/src/app/util/chip-message-send.cpp @@ -25,7 +25,8 @@ #include #include #include -#include // For SecureSessionMgr +#include +#include using namespace chip; diff --git a/src/ble/BLEEndPoint.cpp b/src/ble/BLEEndPoint.cpp index 1f1ff9596d2184..6abdb70877ffe8 100644 --- a/src/ble/BLEEndPoint.cpp +++ b/src/ble/BLEEndPoint.cpp @@ -115,7 +115,7 @@ BLE_ERROR BLEEndPoint::StartConnect() mState = kState_Connecting; // Build BLE transport protocol capabilities request. - buf = System::PacketBufferHandle::New(System::kMaxPacketBufferSize); + buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); VerifyOrExit(!buf.IsNull(), err = BLE_ERROR_NO_MEMORY); // Zero-initialize BLE transport capabilities request. diff --git a/src/ble/BtpEngine.cpp b/src/ble/BtpEngine.cpp index d9be4b956ee868..ef83b90a7133b7 100644 --- a/src/ble/BtpEngine.cpp +++ b/src/ble/BtpEngine.cpp @@ -318,7 +318,7 @@ BLE_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle dat data->ConsumeHead(startReader.OctetsRead()); // Create a new buffer for use as the Rx re-assembly area. - mRxBuf = System::PacketBufferHandle::New(System::kMaxPacketBufferSize); + mRxBuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); VerifyOrExit(!mRxBuf.IsNull(), err = BLE_ERROR_NO_MEMORY); diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index 49db43ceb7fc9a..4711af7a8b7870 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -254,7 +254,7 @@ CHIP_ERROR Device::OpenPairingWindow(uint32_t timeout, bool useToken, uint16_t d // Issue: https://github.com/project-chip/connectedhomeip/issues/4725 // Construct and send "open pairing window" message to the device - System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::kMaxPacketBufferSize); + System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); System::PacketBufferTLVWriter writer; writer.Init(std::move(buf)); diff --git a/src/controller/java/CHIPProjectConfig.h b/src/controller/java/CHIPProjectConfig.h index 43d4c201e19878..c610773af170b0 100644 --- a/src/controller/java/CHIPProjectConfig.h +++ b/src/controller/java/CHIPProjectConfig.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * Copyright (c) 2016-2017 Nest Labs, Inc. * Copyright (c) 2019-2020 Google LLC. * All rights reserved. @@ -43,7 +43,7 @@ #define CHIP_CONFIG_LEGACY_KEY_EXPORT_DELEGATE 0 -#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC 300 +#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE 300 #define CHIP_CONFIG_ENABLE_FUNCT_ERROR_LOGGING 1 diff --git a/src/include/platform/internal/GenericNetworkProvisioningServerImpl.cpp b/src/include/platform/internal/GenericNetworkProvisioningServerImpl.cpp index 9aaa0bdbca0340..9b85f798dcf661 100644 --- a/src/include/platform/internal/GenericNetworkProvisioningServerImpl.cpp +++ b/src/include/platform/internal/GenericNetworkProvisioningServerImpl.cpp @@ -494,7 +494,7 @@ CHIP_ERROR GenericNetworkProvisioningServerImpl::HandleGetNetworks(ui #endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD // Allocate a buffer to hold the response. - respBuf = PacketBufferHandle::New(kMaxPacketBufferSize); + respBuf = PacketBufferHandle::New(PacketBuffer::kMaxSize); VerifyOrExit(respBuf != NULL, err = CHIP_ERROR_NO_MEMORY); // Encode the GetNetworks response data. diff --git a/src/include/platform/internal/GenericSoftwareUpdateManagerImpl.cpp b/src/include/platform/internal/GenericSoftwareUpdateManagerImpl.cpp index 7c2c3f50057961..e1ce027fb32866 100644 --- a/src/include/platform/internal/GenericSoftwareUpdateManagerImpl.cpp +++ b/src/include/platform/internal/GenericSoftwareUpdateManagerImpl.cpp @@ -217,7 +217,7 @@ CHIP_ERROR GenericSoftwareUpdateManagerImpl::PrepareQuery(void) } // Allocate a buffer to hold the image query. - mImageQueryPacketBuffer = PacketBufferHandle::New(kMaxPacketBufferSize); + mImageQueryPacketBuffer = PacketBufferHandle::New(PacketBuffer::kMaxSize); VerifyOrExit(mImageQueryPacketBuffer != NULL, err = CHIP_ERROR_NO_MEMORY); err = imageQuery.pack(mImageQueryPacketBuffer); diff --git a/src/inet/IPEndPointBasis.cpp b/src/inet/IPEndPointBasis.cpp index 30ecb34e82060d..6a5aec63128144 100644 --- a/src/inet/IPEndPointBasis.cpp +++ b/src/inet/IPEndPointBasis.cpp @@ -1068,7 +1068,7 @@ void IPEndPointBasis::HandlePendingIO(uint16_t aPort) lPacketInfo.Clear(); lPacketInfo.DestPort = aPort; - lBuffer = System::PacketBufferHandle::New(System::kMaxPacketBufferSizeWithoutReserve, 0); + lBuffer = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSizeWithoutReserve, 0); if (!lBuffer.IsNull()) { diff --git a/src/inet/TCPEndPoint.h b/src/inet/TCPEndPoint.h index 0a9bb7b3bb59fa..14a4e1b5411c0f 100644 --- a/src/inet/TCPEndPoint.h +++ b/src/inet/TCPEndPoint.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * Copyright (c) 2013-2017 Nest Labs, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -562,7 +562,7 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis /** * Size of the largest TCP packet that can be received. */ - constexpr static size_t kMaxReceiveMessageSize = System::kMaxPacketBufferSizeWithoutReserve; + constexpr static size_t kMaxReceiveMessageSize = System::PacketBuffer::kMaxSizeWithoutReserve; private: static chip::System::ObjectPool sPool; diff --git a/src/inet/tests/TestInetEndPoint.cpp b/src/inet/tests/TestInetEndPoint.cpp index cb7fd16d88dff0..7ff2c0936a5b5e 100644 --- a/src/inet/tests/TestInetEndPoint.cpp +++ b/src/inet/tests/TestInetEndPoint.cpp @@ -39,6 +39,7 @@ #include #include +#include #include #include @@ -320,7 +321,7 @@ static void TestInetEndPointInternal(nlTestSuite * inSuite, void * inContext) #endif // INET_CONFIG_ENABLE_IPV4 UDPEndPoint * testUDPEP = nullptr; TCPEndPoint * testTCPEP1 = nullptr; - PacketBufferHandle buf = PacketBufferHandle::New(kMaxPacketBufferSize); + PacketBufferHandle buf = PacketBufferHandle::New(PacketBuffer::kMaxSize); bool didBind = false; bool didListen = false; @@ -440,7 +441,7 @@ static void TestInetEndPointInternal(nlTestSuite * inSuite, void * inContext) #if INET_CONFIG_ENABLE_IPV4 err = testUDPEP->Bind(kIPAddressType_IPv4, addr_v4, 3000, intId); NL_TEST_ASSERT(inSuite, err != INET_NO_ERROR); - buf = PacketBufferHandle::New(kMaxPacketBufferSize); + buf = PacketBufferHandle::New(PacketBuffer::kMaxSize); err = testUDPEP->SendTo(addr_v4, 3000, std::move(buf)); testUDPEP->Free(); #endif // INET_CONFIG_ENABLE_IPV4 @@ -448,7 +449,7 @@ static void TestInetEndPointInternal(nlTestSuite * inSuite, void * inContext) // TcpEndPoint special cases to cover the error branch err = testTCPEP1->GetPeerInfo(nullptr, nullptr); NL_TEST_ASSERT(inSuite, err == INET_ERROR_INCORRECT_STATE); - buf = PacketBufferHandle::New(kMaxPacketBufferSize); + buf = PacketBufferHandle::New(PacketBuffer::kMaxSize); err = testTCPEP1->Send(std::move(buf), false); NL_TEST_ASSERT(inSuite, err == INET_ERROR_INCORRECT_STATE); err = testTCPEP1->EnableKeepAlive(10, 100); @@ -542,16 +543,19 @@ static const nlTest sTests[] = { NL_TEST_DEF("InetEndPoint::PreTest", TestInetPr */ static int TestSetup(void * inContext) { - return (SUCCESS); + CHIP_ERROR error = chip::Platform::MemoryInit(); + if (error != CHIP_NO_ERROR) + return FAILURE; + return SUCCESS; } /** * Tear down the test suite. - * Free memory reserved at TestSetup. */ static int TestTeardown(void * inContext) { - return (SUCCESS); + chip::Platform::MemoryShutdown(); + return SUCCESS; } #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS diff --git a/src/lib/core/BUILD.gn b/src/lib/core/BUILD.gn index faa86297f9f019..b48b8a67f48808 100644 --- a/src/lib/core/BUILD.gn +++ b/src/lib/core/BUILD.gn @@ -1,4 +1,4 @@ -# Copyright (c) 2020 Project CHIP Authors +# Copyright (c) 2020-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. @@ -66,6 +66,8 @@ buildconfig_header("chip_buildconfig") { "HAVE_NEW=false", "CHIP_CONFIG_MEMORY_MGMT_SIMPLE=${chip_config_memory_management_simple}", "CHIP_CONFIG_MEMORY_MGMT_PLATFORM=${chip_config_memory_management_platform}", + "CHIP_CONFIG_MEMORY_DEBUG_CHECKS=${chip_config_memory_debug_checks}", + "CHIP_CONFIG_MEMORY_DEBUG_DMALLOC=${chip_config_memory_debug_dmalloc}", "CHIP_CONFIG_PROVIDE_OBSOLESCENT_INTERFACES=false", ] } diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h index 8380825e91fa6c..385d95a6e0f424 100644 --- a/src/lib/core/CHIPConfig.h +++ b/src/lib/core/CHIPConfig.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * Copyright (c) 2019 Google LLC. * Copyright (c) 2013-2018 Nest Labs, Inc. * @@ -550,6 +550,36 @@ #define CHIP_CONFIG_SIMPLE_ALLOCATOR_USE_SMALL_BUFFERS 0 #endif // CHIP_CONFIG_SIMPLE_ALLOCATOR_USE_SMALL_BUFFERS +/** + * @def CHIP_CONFIG_MEMORY_DEBUG_CHECKS + * + * @brief + * Enable (1) or disable (0) building with additional code + * for memory-related checks. + */ +#ifndef CHIP_CONFIG_MEMORY_DEBUG_CHECKS +#define CHIP_CONFIG_MEMORY_DEBUG_CHECKS 0 +#endif // CHIP_CONFIG_MEMORY_DEBUG_CHECKS + +/** + * @def CHIP_CONFIG_MEMORY_DEBUG_DMALLOC + * + * @brief + * Enable (1) or disable (0) malloc memory allocator support + * for dmalloc, an open-source debug malloc library. When enabled, + * additional checks and logging of allocations may be performed, + * with some performance cost. + * + * @note This configuration is most relevant when + * #CHIP_CONFIG_MEMORY_MGMT_MALLOC is set, but may also + * affect other configurations where application or platform + * code uses the malloc() family. + * + */ +#ifndef CHIP_CONFIG_MEMORY_DEBUG_DMALLOC +#define CHIP_CONFIG_MEMORY_DEBUG_DMALLOC 0 +#endif // CHIP_CONFIG_MEMORY_DEBUG_DMALLOC + /** * @name chip Security Manager Time-Consuming Crypto Alerts. * diff --git a/src/lib/core/core.gni b/src/lib/core/core.gni index deecc5075e87bc..b7fd5edb792793 100644 --- a/src/lib/core/core.gni +++ b/src/lib/core/core.gni @@ -1,4 +1,4 @@ -# Copyright (c) 2020 Project CHIP Authors +# Copyright (c) 2020-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. @@ -41,6 +41,12 @@ declare_args() { # Memory management style: malloc, simple, platform. chip_config_memory_management = "malloc" + + # Memory management debug option: enable additional checks. + chip_config_memory_debug_checks = false + + # Memory management debug option: use dmalloc + chip_config_memory_debug_dmalloc = false } if (chip_target_style == "") { diff --git a/src/lib/core/tests/TestCHIPTLV.cpp b/src/lib/core/tests/TestCHIPTLV.cpp index 199497922df88a..e3ab5f92c0af86 100644 --- a/src/lib/core/tests/TestCHIPTLV.cpp +++ b/src/lib/core/tests/TestCHIPTLV.cpp @@ -2471,7 +2471,7 @@ void CheckBufferOverflow(nlTestSuite * inSuite, void * inContext) ReadEncoding1(inSuite, reader); - buf = System::PacketBufferHandle::New(System::kMaxPacketBufferSizeWithoutReserve, 0); + buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSizeWithoutReserve, 0); } } diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index 78ef1d837077d6..3aa67f3f141f8b 100644 --- a/src/lib/support/BUILD.gn +++ b/src/lib/support/BUILD.gn @@ -133,4 +133,7 @@ static_library("support") { if (chip_logging_style == "android") { libs += [ "log" ] } + if (chip_config_memory_debug_dmalloc) { + libs += [ "dmallocthcxx" ] + } } diff --git a/src/lib/support/CHIPMem-Malloc.cpp b/src/lib/support/CHIPMem-Malloc.cpp index 5e520e05e8fee1..2d8b6b9af72a4f 100644 --- a/src/lib/support/CHIPMem-Malloc.cpp +++ b/src/lib/support/CHIPMem-Malloc.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -33,6 +33,11 @@ #include #endif +#if CHIP_CONFIG_MEMORY_DEBUG_DMALLOC +#include +#include +#endif // CHIP_CONFIG_MEMORY_DEBUG_DMALLOC + #if CHIP_CONFIG_MEMORY_MGMT_MALLOC namespace chip { @@ -41,6 +46,7 @@ namespace Platform { #ifdef NDEBUG #define VERIFY_INITIALIZED() +#define VERIFY_POINTER(p) #else @@ -57,6 +63,15 @@ static void VerifyInitialized(const char * func) } } +#define VERIFY_POINTER(p) \ + do \ + if (((p) != nullptr) && (MemoryDebugCheckPointer((p)) == false)) \ + { \ + fprintf(stderr, "ABORT: chip::Platform::%s() found corruption on %p\n", __func__, (p)); \ + abort(); \ + } \ + while (0) + #endif CHIP_ERROR MemoryAllocatorInit(void * buf, size_t bufSize) @@ -80,6 +95,9 @@ void MemoryAllocatorShutdown() abort(); } #endif +#if CHIP_CONFIG_MEMORY_DEBUG_DMALLOC + dmalloc_shutdown(); +#endif // CHIP_CONFIG_MEMORY_DEBUG_DMALLOC } void * MemoryAlloc(size_t size) @@ -103,15 +121,27 @@ void * MemoryCalloc(size_t num, size_t size) void * MemoryRealloc(void * p, size_t size) { VERIFY_INITIALIZED(); + VERIFY_POINTER(p); return realloc(p, size); } void MemoryFree(void * p) { VERIFY_INITIALIZED(); + VERIFY_POINTER(p); free(p); } +bool MemoryInternalCheckPointer(const void * p, size_t min_size) +{ +#if CHIP_CONFIG_MEMORY_DEBUG_DMALLOC + return CanCastTo(min_size) && (p != nullptr) && + (dmalloc_verify_pnt(__FILE__, __LINE__, __func__, p, 1, static_cast(min_size)) == MALLOC_VERIFY_NOERROR); +#else // CHIP_CONFIG_MEMORY_DEBUG_DMALLOC + return (p != nullptr); +#endif // CHIP_CONFIG_MEMORY_DEBUG_DMALLOC +} + } // namespace Platform } // namespace chip diff --git a/src/lib/support/CHIPMem-SimpleAlloc.cpp b/src/lib/support/CHIPMem-SimpleAlloc.cpp index fdc887a5eb232a..4099e74e92a5fa 100644 --- a/src/lib/support/CHIPMem-SimpleAlloc.cpp +++ b/src/lib/support/CHIPMem-SimpleAlloc.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * Copyright (c) 2019-2020 Google LLC. * Copyright (c) 2018 Nest Labs, Inc. * All rights reserved. @@ -408,7 +408,7 @@ void * MemoryAlloc(size_t size, bool isLongTermAlloc) { if (sMemBufs[blockBufferIndex] == NULL) { - sMemBufs[blockBufferIndex] = PacketBuffer::NewWithAvailableSize(0, kMinBufferSize).Release(); + sMemBufs[blockBufferIndex] = PacketBufferHandle::New(kMinBufferSize, 0).UnsafeRelease(); if (sMemBufs[blockBufferIndex] == NULL) return NULL; } @@ -537,6 +537,13 @@ void MemoryFree(void * p) } } +bool MemoryInternalCheckPointer(const void * p, size_t min_size) +{ + // TODO: check that \a p is actually an allocated pointer, + // by factoring the allocation-finding out of MemoryFree(). + return p != nullptr; +} + } // namespace Platform } // namespace chip diff --git a/src/lib/support/CHIPMem.h b/src/lib/support/CHIPMem.h index d1ad2a26eababf..af49fc046be7c3 100644 --- a/src/lib/support/CHIPMem.h +++ b/src/lib/support/CHIPMem.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -182,5 +182,43 @@ inline void Delete(T * p) MemoryFree(p); } +// See MemoryDebugCheckPointer(). +extern bool MemoryInternalCheckPointer(const void * p, size_t min_size); + +/** + * In debug builds, test the validity of a pointer obtained from a chip::Platform memory allocation. + * + * @param[in] p Pointer to a memory block previously allocated with MemoryAlloc, MemoryCalloc, + * MemoryRealloc, or New, and not freed. + * @param[in] min_size Gives a size that the allocated block is expected to be able to hold. + * + * @e Unless configured with #CHIP_CONFIG_MEMORY_DEBUG_CHECKS, this function returns `true` without performing + * any check, inlined with the expectation that the compiler can remove any associated failure code. + * + * With #CHIP_CONFIG_MEMORY_DEBUG_CHECKS enabled: + * + * This function is guaranteed to return `false` if \a p is `nullptr`. The function returns `true` if \a p is a valid + * pointer to an allocation *and* the implementation memory manager is in a fully functioning state. + * + * @note For non-null \a p, the function *may* return `true` even if the pointer is invalid. That is, a particular + * implementation or configuration is not guaranteed to catch any particular faulty state. + * @note For non-null \a p, the function return value *may* be incorrect if the memory manager is in a faulty state + * (e.g. corrupt heap), even if the faulty state does not directly involve \a p. + * @note For non-null \a p, the function *may* abort the program rather than return at all if the memory manager is in + * a faulty state, even if \a p is valid. + * @note For a non-null \a p, checking *may* be slow. + * + * + * @return An implementation- and configuration-defined estimate of whether \a p is a valid allocated pointer. + */ +inline bool MemoryDebugCheckPointer(const void * p, size_t min_size = 0) +{ +#if CHIP_CONFIG_MEMORY_DEBUG_CHECKS + return MemoryInternalCheckPointer(p, min_size); +#else // CHIP_CONFIG_MEMORY_DEBUG_CHECKS + return true; +#endif // CHIP_CONFIG_MEMORY_DEBUG_CHECKS +} + } // namespace Platform } // namespace chip diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index 005eeb6188c27e..76bd6dca62d234 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-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. @@ -89,6 +89,8 @@ CHIP_ERROR ExchangeManager::Init(SecureSessionMgr * sessionMgr) CHIP_ERROR ExchangeManager::Shutdown() { + mReliableMessageMgr.Shutdown(); + if (mSessionMgr != nullptr) { mSessionMgr->SetDelegate(nullptr); diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 65ddada1acd072..2c3854b5427bf1 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -56,11 +56,11 @@ void ReliableMessageMgr::Init(chip::System::Layer * systemLayer, SecureSessionMg void ReliableMessageMgr::Shutdown() { + StopTimer(); + mSystemLayer = nullptr; mSessionMgr = nullptr; - StopTimer(); - // Clear the retransmit table for (RetransTableEntry & rEntry : mRetransTable) { @@ -398,8 +398,9 @@ void ReliableMessageMgr::ClearRetransTable(RetransTableEntry & rEntry) // Clear all other fields rEntry = RetransTableEntry(); - // Schedule next physical wakeup - StartTimer(); + // Schedule next physical wakeup, unless shutting down + if (mSystemLayer) + StartTimer(); } } diff --git a/src/messaging/ReliableMessageProtocolConfig.h b/src/messaging/ReliableMessageProtocolConfig.h index 16b72f0a3c8f6e..96ba381809212c 100644 --- a/src/messaging/ReliableMessageProtocolConfig.h +++ b/src/messaging/ReliableMessageProtocolConfig.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * Copyright (c) 2017 Nest Labs, Inc. * All rights reserved. * @@ -86,8 +86,10 @@ namespace Messaging { #ifndef CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE #ifdef PBUF_POOL_SIZE #define CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE (PBUF_POOL_SIZE) +#elif CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE != 0 +#define CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE (CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE) #else -#define CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE (CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC) +#define CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE 15 #endif // PBUF_POOL_SIZE #endif // CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp index 10eb827b4b9761..a389111032ab0b 100644 --- a/src/messaging/tests/MessagingContext.cpp +++ b/src/messaging/tests/MessagingContext.cpp @@ -48,6 +48,7 @@ CHIP_ERROR MessagingContext::Init(nlTestSuite * suite, TransportMgrBase * transp // Shutdown all layers, finalize operations CHIP_ERROR MessagingContext::Shutdown() { + mExchangeManager.Shutdown(); return IOContext::Shutdown(); } diff --git a/src/messaging/tests/TestExchangeMgr.cpp b/src/messaging/tests/TestExchangeMgr.cpp index 920e10896e0bdb..5cfaa789517f56 100644 --- a/src/messaging/tests/TestExchangeMgr.cpp +++ b/src/messaging/tests/TestExchangeMgr.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -146,12 +147,12 @@ void CheckExchangeMessages(nlTestSuite * inSuite, void * inContext) // send a malicious packet // TODO: https://github.com/project-chip/connectedhomeip/issues/4635 - // ec1->SendMessage(0x0001, 0x0002, System::PacketBufferHandle::New(System::kMaxPacketBufferSize), + // ec1->SendMessage(0x0001, 0x0002, System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize), // SendFlags(Messaging::SendMessageFlags::kNone)); NL_TEST_ASSERT(inSuite, !mockUnsolicitedAppDelegate.IsOnMessageReceivedCalled); // send a good packet - ec1->SendMessage(0x0001, 0x0001, System::PacketBufferHandle::New(System::kMaxPacketBufferSize), + ec1->SendMessage(0x0001, 0x0001, System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize), SendFlags(Messaging::SendMessageFlags::kNone)); NL_TEST_ASSERT(inSuite, mockUnsolicitedAppDelegate.IsOnMessageReceivedCalled); } @@ -190,7 +191,11 @@ nlTestSuite sSuite = */ int Initialize(void * aContext) { - CHIP_ERROR err = gTransportMgr.Init("LOOPBACK"); + CHIP_ERROR err = chip::Platform::MemoryInit(); + if (err != CHIP_NO_ERROR) + return FAILURE; + + err = gTransportMgr.Init("LOOPBACK"); if (err != CHIP_NO_ERROR) return FAILURE; @@ -204,6 +209,7 @@ int Initialize(void * aContext) int Finalize(void * aContext) { CHIP_ERROR err = reinterpret_cast(aContext)->Shutdown(); + chip::Platform::MemoryShutdown(); return (err == CHIP_NO_ERROR) ? SUCCESS : FAILURE; } diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index aedc8f2ae98174..713aeefc7ee175 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -247,7 +247,11 @@ nlTestSuite sSuite = */ int Initialize(void * aContext) { - CHIP_ERROR err = gTransportMgr.Init("LOOPBACK"); + CHIP_ERROR err = chip::Platform::MemoryInit(); + if (err != CHIP_NO_ERROR) + return FAILURE; + + err = gTransportMgr.Init("LOOPBACK"); if (err != CHIP_NO_ERROR) return FAILURE; @@ -261,6 +265,7 @@ int Initialize(void * aContext) int Finalize(void * aContext) { CHIP_ERROR err = reinterpret_cast(aContext)->Shutdown(); + chip::Platform::MemoryShutdown(); return (err == CHIP_NO_ERROR) ? SUCCESS : FAILURE; } diff --git a/src/platform/ESP32/NetworkProvisioningServerImpl.cpp b/src/platform/ESP32/NetworkProvisioningServerImpl.cpp index 794b7b45cab6b4..fefc169b53579f 100644 --- a/src/platform/ESP32/NetworkProvisioningServerImpl.cpp +++ b/src/platform/ESP32/NetworkProvisioningServerImpl.cpp @@ -271,7 +271,7 @@ void NetworkProvisioningServerImpl::HandleScanDone() // Allocate a packet buffer to hold the encoded scan results. PacketBufferHandle respBuf = - System::PacketBufferHandle::New(kMaxPacketBufferSize - 1, CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE + 1); + System::PacketBufferHandle::New(PacketBuffer::kMaxSize - 1, System::PacketBuffer::kDefaultHeaderReserve + 1); VerifyOrExit(!respBuf.IsNull(), err = CHIP_ERROR_NO_MEMORY); // Encode the list of scan results into the response buffer. If the encoded size of all diff --git a/src/platform/Linux/CHIPBluezHelper.cpp b/src/platform/Linux/CHIPBluezHelper.cpp index 5098b9f9fa2fde..dd0cdcbcdc4f5e 100644 --- a/src/platform/Linux/CHIPBluezHelper.cpp +++ b/src/platform/Linux/CHIPBluezHelper.cpp @@ -1279,7 +1279,7 @@ static void UpdateAdditionalDataCharacteristic(BluezGattCharacteristic1 * charac TLVWriter innerWriter; chip::System::PacketBufferHandle bufferHandle; - writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + writer.Init(chip::System::PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); err = writer.OpenContainer(AnonymousTag, kTLVType_Structure, innerWriter); SuccessOrExit(err); diff --git a/src/protocols/bdx/tests/TestBdxMessages.cpp b/src/protocols/bdx/tests/TestBdxMessages.cpp index d8df11ba23fc2e..8f8e7d9e58bebf 100644 --- a/src/protocols/bdx/tests/TestBdxMessages.cpp +++ b/src/protocols/bdx/tests/TestBdxMessages.cpp @@ -2,6 +2,8 @@ #include +#include +#include #include #include @@ -138,13 +140,33 @@ static const nlTest sTests[] = }; // clang-format on +/** + * Set up the test suite. + */ +static int TestSetup(void * inContext) +{ + CHIP_ERROR error = chip::Platform::MemoryInit(); + if (error != CHIP_NO_ERROR) + return FAILURE; + return SUCCESS; +} + +/** + * Tear down the test suite. + */ +static int TestTeardown(void * inContext) +{ + chip::Platform::MemoryShutdown(); + return SUCCESS; +} + // clang-format off static nlTestSuite sSuite = { "Test-CHIP-BdxMessages", &sTests[0], - nullptr, - nullptr + TestSetup, + TestTeardown, }; // clang-format on diff --git a/src/protocols/bdx/tests/TestBdxTransferSession.cpp b/src/protocols/bdx/tests/TestBdxTransferSession.cpp index 4e2c2d3247f143..da2130310dcaf1 100644 --- a/src/protocols/bdx/tests/TestBdxTransferSession.cpp +++ b/src/protocols/bdx/tests/TestBdxTransferSession.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -749,13 +750,27 @@ static const nlTest sTests[] = }; // clang-format on +int TestBdxTransferSession_Setup(void * inContext) +{ + CHIP_ERROR error = chip::Platform::MemoryInit(); + if (error != CHIP_NO_ERROR) + return FAILURE; + return SUCCESS; +} + +int TestBdxTransferSession_Teardown(void * inContext) +{ + chip::Platform::MemoryShutdown(); + return SUCCESS; +} + // clang-format off static nlTestSuite sSuite = { "Test-CHIP-TransferSession", &sTests[0], - nullptr, - nullptr + TestBdxTransferSession_Setup, + TestBdxTransferSession_Teardown }; // clang-format on diff --git a/src/system/SystemConfig.h b/src/system/SystemConfig.h index 5c0fa17b070193..464e8bd9f21bf9 100644 --- a/src/system/SystemConfig.h +++ b/src/system/SystemConfig.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2021 Project CHIP Authors * Copyright (c) 2016-2018 Nest Labs, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -135,9 +135,9 @@ #define CHIP_SYSTEM_CONFIG_FREERTOS_LOCKING INET_CONFIG_FREERTOS_LOCKING #endif // !defined(CHIP_SYSTEM_CONFIG_FREERTOS_LOCKING) && defined(INET_CONFIG_FREERTOS_LOCKING) -#if !defined(CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC) && defined(INET_CONFIG_NUM_BUFS) -#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC INET_CONFIG_NUM_BUFS -#endif // !defined(CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC) && defined(INET_CONFIG_NUM_BUFS) +#if !defined(CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE) && defined(INET_CONFIG_NUM_BUFS) +#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE INET_CONFIG_NUM_BUFS +#endif // !defined(CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE) && defined(INET_CONFIG_NUM_BUFS) #if !defined(CHIP_SYSTEM_CONFIG_NUM_TIMERS) && defined(INET_CONFIG_NUM_TIMERS) #define CHIP_SYSTEM_CONFIG_NUM_TIMERS INET_CONFIG_NUM_TIMERS @@ -324,16 +324,16 @@ #endif /* CHIP_SYSTEM_HEADER_RESERVE_SIZE */ /** - * @def CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC + * @def CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE * * @brief * This is the total number of packet buffers for the BSD sockets configuration. * * This may be set to zero (0) to enable unbounded dynamic allocation using malloc. */ -#ifndef CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC -#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC 15 -#endif /* CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC */ +#ifndef CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE +#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE 15 +#endif /* CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE */ /** * @def CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX @@ -367,7 +367,7 @@ * * The size of PacketBuffer structure does not need to be included in this value. */ -#if CHIP_SYSTEM_CONFIG_USE_LWIP +#if CHIP_SYSTEM_CONFIG_USE_LWIP && !defined(DOXYGEN) #ifdef CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX #error "CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX cannot be defined on an LwIP-based platform." #endif /* CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX */ diff --git a/src/system/SystemPacketBuffer.cpp b/src/system/SystemPacketBuffer.cpp index 1490ef7bf1870e..99f6b0378136ee 100644 --- a/src/system/SystemPacketBuffer.cpp +++ b/src/system/SystemPacketBuffer.cpp @@ -53,22 +53,21 @@ #if CHIP_SYSTEM_CONFIG_USE_LWIP #include #include -#else // !CHIP_SYSTEM_CONFIG_USE_LWIP -#if CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0 +#endif // CHIP_SYSTEM_CONFIG_USE_LWIP + +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP #include #endif -#endif // !CHIP_SYSTEM_CONFIG_USE_LWIP namespace chip { namespace System { +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_POOL // -// Pool allocation for PacketBuffer objects (toll-free bridged with LwIP pbuf allocator if CHIP_SYSTEM_CONFIG_USE_LWIP) +// Pool allocation for PacketBuffer objects. // -#if !CHIP_SYSTEM_CONFIG_USE_LWIP -#if CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC -static BufferPoolElement sBufferPool[CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC]; +PacketBuffer::BufferPoolElement PacketBuffer::sBufferPool[CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE]; PacketBuffer * PacketBuffer::sFreeList = PacketBuffer::BuildFreeList(); @@ -87,7 +86,96 @@ static Mutex sBufferPoolMutex; } while (0) #endif // !CHIP_SYSTEM_CONFIG_NO_LOCKING -#endif // CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC +PacketBuffer * PacketBuffer::BuildFreeList() +{ + pbuf * lHead = nullptr; + + for (int i = 0; i < CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE; i++) + { + pbuf * lCursor = &sBufferPool[i].Header; + lCursor->next = lHead; + lCursor->ref = 0; + lHead = lCursor; + } + + Mutex::Init(sBufferPoolMutex); + + return static_cast(lHead); +} + +#elif CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP +// +// Heap allocation for PacketBuffer objects. +// + +#if CHIP_CONFIG_MEMORY_DEBUG_CHECKS +void PacketBuffer::InternalCheck(const PacketBuffer * buffer) +{ + if (buffer) + { + VerifyOrDieWithMsg(::chip::Platform::MemoryDebugCheckPointer(buffer, buffer->alloc_size + kStructureSize), chipSystemLayer, + "invalid packet buffer pointer"); + VerifyOrDieWithMsg(buffer->alloc_size >= buffer->ReservedSize() + buffer->len, chipSystemLayer, + "packet buffer overflow %u < %u+%u", buffer->alloc_size, buffer->ReservedSize(), buffer->len); + } +} +#endif // CHIP_CONFIG_MEMORY_DEBUG_CHECKS + +// Number of unused bytes below which \c RightSize() won't bother reallocating. +constexpr uint16_t kRightSizingThreshold = 16; + +void PacketBufferHandle::InternalRightSize() +{ + // Require a single buffer with no other references. + if ((mBuffer == nullptr) || (mBuffer->next != nullptr) || (mBuffer->ref != 1)) + { + return; + } + + // Reallocate only if enough space will be saved. + uint8_t * const start = reinterpret_cast(mBuffer) + PacketBuffer::kStructureSize; + uint8_t * const payload = reinterpret_cast(mBuffer->payload); + const uint16_t usedSize = static_cast(payload - start + mBuffer->len); + if (usedSize + kRightSizingThreshold > mBuffer->alloc_size) + { + return; + } + + const size_t blockSize = usedSize + PacketBuffer::kStructureSize; + PacketBuffer * newBuffer = reinterpret_cast(chip::Platform::MemoryAlloc(blockSize)); + if (newBuffer == nullptr) + { + ChipLogError(chipSystemLayer, "PacketBuffer: pool EMPTY."); + return; + } + + uint8_t * const newStart = reinterpret_cast(newBuffer) + PacketBuffer::kStructureSize; + newBuffer->next = nullptr; + newBuffer->payload = newStart + (payload - start); + newBuffer->tot_len = mBuffer->tot_len; + newBuffer->len = mBuffer->len; + newBuffer->ref = 1; + newBuffer->alloc_size = static_cast(usedSize); + memcpy(reinterpret_cast(newBuffer) + PacketBuffer::kStructureSize, start, usedSize); + + PacketBuffer::Free(mBuffer); + mBuffer = newBuffer; +} + +#elif CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_CUSTOM + +void PacketBufferHandle::InternalRightSize() +{ + PacketBuffer * lNewPacket = static_cast(pbuf_rightsize((struct pbuf *) mBuffer, -1)); + if (lNewPacket != mBuffer) + { + mBuffer = lNewPacket; + SYSTEM_STATS_UPDATE_LWIP_PBUF_COUNTS(); + ChipLogProgress(chipSystemLayer, "PacketBuffer: RightSize Copied"); + } +} + +#endif // CHIP_SYSTEM_PACKETBUFFER_STORE #ifndef LOCK_BUF_POOL #define LOCK_BUF_POOL() \ @@ -103,11 +191,9 @@ static Mutex sBufferPoolMutex; } while (0) #endif // !defined(UNLOCK_BUF_POOL) -#endif // !CHIP_SYSTEM_CONFIG_USE_LWIP - void PacketBuffer::SetStart(uint8_t * aNewStart) { - uint8_t * const kStart = reinterpret_cast(this) + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; + uint8_t * const kStart = reinterpret_cast(this) + kStructureSize; uint8_t * const kEnd = kStart + this->AllocSize(); if (aNewStart < kStart) @@ -136,8 +222,13 @@ void PacketBuffer::SetDataLength(uint16_t aNewLen, PacketBuffer * aChainHead) this->len = aNewLen; this->tot_len = static_cast(this->tot_len + lDelta); + // SetDataLength is often called after a client finished writing to the buffer, + // so it's a good time to check for possible corruption. + Check(this); + while (aChainHead != nullptr && aChainHead != this) { + Check(aChainHead); aChainHead->tot_len = static_cast(aChainHead->tot_len + lDelta); aChainHead = static_cast(aChainHead->next); } @@ -145,7 +236,7 @@ void PacketBuffer::SetDataLength(uint16_t aNewLen, PacketBuffer * aChainHead) uint16_t PacketBuffer::MaxDataLength() const { - const uint8_t * const kStart = reinterpret_cast(this) + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; + const uint8_t * const kStart = reinterpret_cast(this) + kStructureSize; const ptrdiff_t kDelta = static_cast(this->payload) - kStart; return static_cast(this->AllocSize() - kDelta); } @@ -160,7 +251,7 @@ uint16_t PacketBuffer::ReservedSize() const // Cast to size_t is safe because this->payload always points to "after" // this. const size_t kDelta = static_cast(static_cast(this->payload) - reinterpret_cast(this)); - return static_cast(kDelta - CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE); + return static_cast(kDelta - kStructureSize); } void PacketBuffer::AddToEnd(PacketBufferHandle && aPacketHandle) @@ -189,7 +280,7 @@ void PacketBuffer::AddToEnd(PacketBufferHandle && aPacketHandle) void PacketBuffer::CompactHead() { - uint8_t * const kStart = reinterpret_cast(this) + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; + uint8_t * const kStart = reinterpret_cast(this) + kStructureSize; if (this->payload != kStart) { @@ -321,32 +412,33 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese // Adding three 16-bit-int sized numbers together will never overflow // assuming int is at least 32 bits. static_assert(INT_MAX >= INT32_MAX, "int is not big enough"); - static_assert(CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE < UINT16_MAX, "Check for overflow more carefully"); + static_assert(PacketBuffer::kStructureSize == sizeof(PacketBuffer), "PacketBuffer size mismatch"); + static_assert(PacketBuffer::kStructureSize < UINT16_MAX, "Check for overflow more carefully"); static_assert(SIZE_MAX >= INT_MAX, "Our additions might not fit in size_t"); - static_assert(CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX <= UINT16_MAX, "PacketBuffer may have size not fitting uint16_t"); + static_assert(PacketBuffer::kMaxSizeWithoutReserve <= UINT16_MAX, "PacketBuffer may have size not fitting uint16_t"); // When `aAvailableSize` fits in uint16_t (as tested below) and size_t is at least 32 bits (as asserted above), // these additions will not overflow. const size_t lAllocSize = aReservedSize + aAvailableSize; - const size_t lBlockSize = CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE + lAllocSize; + const size_t lBlockSize = PacketBuffer::kStructureSize + lAllocSize; PacketBuffer * lPacket; CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_PacketBufferNew, return PacketBufferHandle()); - if (aAvailableSize > UINT16_MAX || lAllocSize > CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX || lBlockSize > UINT16_MAX) + if (aAvailableSize > UINT16_MAX || lAllocSize > PacketBuffer::kMaxSizeWithoutReserve || lBlockSize > UINT16_MAX) { ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large."); return PacketBufferHandle(); } -#if CHIP_SYSTEM_CONFIG_USE_LWIP +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_POOL || \ + CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_CUSTOM lPacket = static_cast(pbuf_alloc(PBUF_RAW, static_cast(lBlockSize), PBUF_POOL)); SYSTEM_STATS_UPDATE_LWIP_PBUF_COUNTS(); -#else // !CHIP_SYSTEM_CONFIG_USE_LWIP -#if CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC +#elif CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_POOL static_cast(lBlockSize); @@ -361,13 +453,14 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese UNLOCK_BUF_POOL(); -#else // !CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC +#elif CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP lPacket = reinterpret_cast(chip::Platform::MemoryAlloc(lBlockSize)); SYSTEM_STATS_INCREMENT(chip::System::Stats::kSystemLayer_NumPacketBufs); -#endif // !CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC -#endif // !CHIP_SYSTEM_CONFIG_USE_LWIP +#else +#error "Unimplemented CHIP_SYSTEM_PACKETBUFFER_STORE case" +#endif // CHIP_SYSTEM_PACKETBUFFER_STORE if (lPacket == nullptr) { @@ -375,13 +468,13 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese return PacketBufferHandle(); } - lPacket->payload = reinterpret_cast(lPacket) + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE + aReservedSize; + lPacket->payload = reinterpret_cast(lPacket) + PacketBuffer::kStructureSize + aReservedSize; lPacket->len = lPacket->tot_len = 0; lPacket->next = nullptr; lPacket->ref = 1; -#if CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0 +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP lPacket->alloc_size = static_cast(lAllocSize); -#endif // CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0 +#endif return PacketBufferHandle(lPacket); } @@ -416,7 +509,8 @@ PacketBufferHandle PacketBufferHandle::NewWithData(const void * aData, size_t aD */ void PacketBuffer::Free(PacketBuffer * aPacket) { -#if CHIP_SYSTEM_CONFIG_USE_LWIP +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_POOL || \ + CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_CUSTOM if (aPacket != nullptr) { @@ -425,7 +519,8 @@ void PacketBuffer::Free(PacketBuffer * aPacket) SYSTEM_STATS_UPDATE_LWIP_PBUF_COUNTS(); } -#else // !CHIP_SYSTEM_CONFIG_USE_LWIP +#elif CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_POOL || \ + CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP LOCK_BUF_POOL(); @@ -439,13 +534,16 @@ void PacketBuffer::Free(PacketBuffer * aPacket) if (aPacket->ref == 0) { SYSTEM_STATS_DECREMENT(chip::System::Stats::kSystemLayer_NumPacketBufs); +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP + ::chip::Platform::MemoryDebugCheckPointer(aPacket, aPacket->alloc_size + kStructureSize); +#endif aPacket->Clear(); -#if CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_POOL aPacket->next = sFreeList; sFreeList = aPacket; -#else // !CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC +#elif CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP chip::Platform::MemoryFree(aPacket); -#endif // !CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC +#endif // CHIP_SYSTEM_PACKETBUFFER_STORE aPacket = lNextPacket; } else @@ -456,7 +554,9 @@ void PacketBuffer::Free(PacketBuffer * aPacket) UNLOCK_BUF_POOL(); -#endif // !CHIP_SYSTEM_CONFIG_USE_LWIP +#else +#error "Unimplemented CHIP_SYSTEM_PACKETBUFFER_STORE case" +#endif // CHIP_SYSTEM_PACKETBUFFER_STORE } /** @@ -468,9 +568,9 @@ void PacketBuffer::Clear() { tot_len = 0; len = 0; -#if CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0 +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP alloc_size = 0; -#endif // CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0 +#endif } /** @@ -491,27 +591,6 @@ PacketBuffer * PacketBuffer::FreeHead(PacketBuffer * aPacket) return lNextPacket; } -#if !CHIP_SYSTEM_CONFIG_USE_LWIP && CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC - -PacketBuffer * PacketBuffer::BuildFreeList() -{ - PacketBuffer * lHead = nullptr; - - for (int i = 0; i < CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC; i++) - { - PacketBuffer * lCursor = &sBufferPool[i].Header; - lCursor->next = lHead; - lCursor->ref = 0; - lHead = lCursor; - } - - Mutex::Init(sBufferPoolMutex); - - return lHead; -} - -#endif // !CHIP_SYSTEM_CONFIG_USE_LWIP && CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC - PacketBufferHandle PacketBufferHandle::PopHead() { PacketBuffer * head = mBuffer; @@ -526,74 +605,16 @@ PacketBufferHandle PacketBufferHandle::PopHead() return PacketBufferHandle(head); } -PacketBufferHandle PacketBufferHandle::CloneData() +PacketBufferHandle PacketBufferHandle::CloneData(uint16_t aAdditionalSize, uint16_t aReservedSize) { if (!mBuffer->Next().IsNull()) { // We do not clone an entire chain. return PacketBufferHandle(); } - return NewWithData(mBuffer->Start(), mBuffer->DataLength()); -} - -#if CHIP_SYSTEM_CONFIG_USE_LWIP && LWIP_PBUF_FROM_CUSTOM_POOLS - -void PacketBufferHandle::RightSizeForLwIPCustomPools() -{ - PacketBuffer * lNewPacket = static_cast(pbuf_rightsize((struct pbuf *) mBuffer, -1)); - if (lNewPacket != mBuffer) - { - mBuffer = lNewPacket; - SYSTEM_STATS_UPDATE_LWIP_PBUF_COUNTS(); - ChipLogProgress(chipSystemLayer, "PacketBuffer: RightSize Copied"); - } + return NewWithData(mBuffer->Start(), mBuffer->DataLength(), aAdditionalSize, aReservedSize); } -#elif CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0 - -// Number of unused bytes below which \c RightSizeForMemoryAlloc() won't bother reallocating. -constexpr uint16_t kRightSizingThreshold = 16; - -void PacketBufferHandle::RightSizeForMemoryAlloc() -{ - // Require a single buffer with no other references. - if ((mBuffer == nullptr) || (mBuffer->next != nullptr) || (mBuffer->ref != 1)) - { - return; - } - - // Reallocate only if enough space will be saved. - uint8_t * const start = reinterpret_cast(mBuffer) + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; - uint8_t * const payload = reinterpret_cast(mBuffer->payload); - const size_t usedSize = static_cast(payload - start + mBuffer->len); - if (usedSize + kRightSizingThreshold > mBuffer->alloc_size) - { - return; - } - - const uint16_t blockSize = usedSize + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; - PacketBuffer * newBuffer = reinterpret_cast(chip::Platform::MemoryAlloc(blockSize)); - if (newBuffer == nullptr) - { - ChipLogError(chipSystemLayer, "PacketBuffer: pool EMPTY."); - return; - } - - uint8_t * const newStart = reinterpret_cast(newBuffer) + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; - newBuffer->next = nullptr; - newBuffer->payload = newStart + (payload - start); - newBuffer->tot_len = mBuffer->tot_len; - newBuffer->len = mBuffer->len; - newBuffer->ref = 1; - newBuffer->alloc_size = static_cast(usedSize); - memcpy(reinterpret_cast(newBuffer) + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE, start, usedSize); - - chip::Platform::MemoryFree(mBuffer); - mBuffer = newBuffer; -} - -#endif - } // namespace System namespace Encoding { diff --git a/src/system/SystemPacketBuffer.h b/src/system/SystemPacketBuffer.h index f4863f9e32a949..2c07ea03e9e767 100644 --- a/src/system/SystemPacketBuffer.h +++ b/src/system/SystemPacketBuffer.h @@ -45,6 +45,44 @@ class PacketBufferTest; +/* + * Preprocessor definitions related to packet buffer memory configuration. + * These are not part of the public PacketBuffer interface; they are present + * here so that certain public functions can have definitions optimized for + * particular configurations. + */ + +#undef CHIP_SYSTEM_PACKETBUFFER_STORE // One of the following constants: +#define CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_POOL 1 // Default lwIP allocation +#define CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_CUSTOM 2 // Custom lwIP allocation +#define CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_POOL 3 // Internal fixed pool +#define CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP 4 // Platform::MemoryAlloc + +#undef CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE // True if RightSize() has a nontrivial implementation +#undef CHIP_SYSTEM_PACKETBUFFER_HAS_CHECK // True if Check() has a nontrivial implementation + +#if CHIP_SYSTEM_CONFIG_USE_LWIP +#if LWIP_PBUF_FROM_CUSTOM_POOLS +#define CHIP_SYSTEM_PACKETBUFFER_STORE CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_CUSTOM +#define CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE 1 +#define CHIP_SYSTEM_PACKETBUFFER_HAS_CHECK 0 +#else +#define CHIP_SYSTEM_PACKETBUFFER_STORE CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_POOL +#define CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE 0 +#define CHIP_SYSTEM_PACKETBUFFER_HAS_CHECK 0 +#endif +#else +#if CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE +#define CHIP_SYSTEM_PACKETBUFFER_STORE CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_POOL +#define CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE 0 +#define CHIP_SYSTEM_PACKETBUFFER_HAS_CHECK 0 +#else +#define CHIP_SYSTEM_PACKETBUFFER_STORE CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP +#define CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE 1 +#define CHIP_SYSTEM_PACKETBUFFER_HAS_CHECK CHIP_CONFIG_MEMORY_DEBUG_CHECKS +#endif +#endif + namespace chip { namespace System { @@ -58,9 +96,9 @@ struct pbuf uint16_t tot_len; uint16_t len; uint16_t ref; -#if CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0 +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP uint16_t alloc_size; -#endif // CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0 +#endif }; #endif // !CHIP_SYSTEM_CONFIG_USE_LWIP @@ -84,8 +122,7 @@ struct pbuf * * New objects of PacketBuffer class are initialized at the beginning of an allocation of memory obtained from the underlying * environment, e.g. from LwIP pbuf target pools, from the standard C library heap, from an internal buffer pool. In the - * simple case, the size of the data buffer is #CHIP_SYSTEM_PACKETBUFFER_SIZE. A composer is provided that permits usage of - * data buffers of other sizes. + * simple pool case, the size of the data buffer is PacketBuffer::kBlockSize. * * PacketBuffer objects may be chained to accomodate larger payloads. Chaining, however, is not transparent, and users of the * class must explicitly decide to support chaining. Examples of classes written with chaining support are as follows: @@ -96,7 +133,35 @@ struct pbuf */ class DLL_EXPORT PacketBuffer : private pbuf { +private: + // The effective size of the packet buffer structure. +#if CHIP_SYSTEM_CONFIG_USE_LWIP + static constexpr uint16_t kStructureSize = LWIP_MEM_ALIGN_SIZE(sizeof(struct ::pbuf)); +#else // CHIP_SYSTEM_CONFIG_USE_LWIP + static constexpr uint16_t kStructureSize = CHIP_SYSTEM_ALIGN_SIZE(sizeof(::chip::System::pbuf), 4u); +#endif // CHIP_SYSTEM_CONFIG_USE_LWIP + public: + /** + * The maximum size buffer an application can allocate with no protocol header reserve. + */ +#if CHIP_SYSTEM_CONFIG_USE_LWIP + static constexpr uint16_t kMaxSizeWithoutReserve = (LWIP_MEM_ALIGN_SIZE(PBUF_POOL_BUFSIZE) - PacketBuffer::kStructureSize); +#else + static constexpr uint16_t kMaxSizeWithoutReserve = CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX; +#endif + + /** + * The number of bytes to reserve in a network packet buffer to contain all the possible protocol encapsulation headers + * before the application data. + */ + static constexpr uint16_t kDefaultHeaderReserve = CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE; + + /** + * The maximum size buffer an application can allocate with the default protocol header reserve. + */ + static constexpr uint16_t kMaxSize = kMaxSizeWithoutReserve - kDefaultHeaderReserve; + /** * Return the size of the allocation including the reserved and payload data spaces but not including space * allocated for the PacketBuffer structure. @@ -105,7 +170,23 @@ class DLL_EXPORT PacketBuffer : private pbuf * * @return size of the allocation */ - uint16_t AllocSize() const; + uint16_t AllocSize() const + { +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_POOL || \ + CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_POOL + return kMaxSizeWithoutReserve; +#elif CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_HEAP + return this->alloc_size; +#elif CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_CUSTOM + // Temporary workaround for custom pbufs by assuming size to be PBUF_POOL_BUFSIZE + if (this->flags & PBUF_FLAG_IS_CUSTOM) + return LWIP_MEM_ALIGN_SIZE(PBUF_POOL_BUFSIZE) - kStructureSize; + else + return LWIP_MEM_ALIGN_SIZE(memp_sizes[this->pool]) - kStructureSize; +#else +#error "Unimplemented CHIP_SYSTEM_PACKETBUFFER_STORE case" +#endif // CHIP_SYSTEM_PACKETBUFFER_STORE + } /** * Get a pointer to the start of data in a buffer. @@ -228,7 +309,7 @@ class DLL_EXPORT PacketBuffer : private pbuf * * @return \c true if the requested reserved size is available, \c false if there's not enough room in the buffer. */ - bool EnsureReservedSize(uint16_t aReservedSize); + CHECK_RETURN_VALUE bool EnsureReservedSize(uint16_t aReservedSize); /** * Align the buffer payload on the specified bytes boundary. @@ -257,12 +338,46 @@ class DLL_EXPORT PacketBuffer : private pbuf */ CHECK_RETURN_VALUE PacketBufferHandle Last(); + /** + * Perform an implementation-defined check on the validity of a PacketBuffer pointer. + * + * Unless enabled by #CHIP_CONFIG_MEMORY_DEBUG_CHECKS == 1, this function does nothing. + * + * When enabled, it performs an implementation- and configuration-defined check on + * the validity of the packet buffer. It MAY log an error and/or abort the program + * if the packet buffer or the implementation-defined memory management system is in + * a faulty state. (Some configurations may not actually perform any check.) + * + * @note A null pointer is not considered faulty. + * + * @param[in] buffer - the packet buffer to check. + */ + static void Check(const PacketBuffer * buffer) + { +#if CHIP_SYSTEM_PACKETBUFFER_HAS_CHECK + InternalCheck(buffer); +#endif + } + private: -#if !CHIP_SYSTEM_CONFIG_USE_LWIP && CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC - static PacketBuffer * sFreeList; + // Memory required for a maximum-size PacketBuffer. + static constexpr uint16_t kBlockSize = PacketBuffer::kStructureSize + PacketBuffer::kMaxSizeWithoutReserve; + // Note: this condition includes DOXYGEN to work around a Doxygen error. DOXYGEN is never defined in any actual build. +#if CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_POOL || defined(DOXYGEN) + typedef union + { + pbuf Header; + uint8_t Block[PacketBuffer::kBlockSize]; + } BufferPoolElement; + static BufferPoolElement sBufferPool[CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE]; + static PacketBuffer * sFreeList; static PacketBuffer * BuildFreeList(); -#endif // !CHIP_SYSTEM_CONFIG_USE_LWIP && CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC +#endif // CHIP_SYSTEM_PACKETBUFFER_STORE == CHIP_SYSTEM_PACKETBUFFER_STORE_CHIP_POOL || defined(DOXYGEN) + +#if CHIP_SYSTEM_PACKETBUFFER_HAS_CHECK + static void InternalCheck(const PacketBuffer * buffer); +#endif void AddRef(); static void Free(PacketBuffer * aPacket); @@ -277,101 +392,7 @@ class DLL_EXPORT PacketBuffer : private pbuf friend class ::PacketBufferTest; }; -} // namespace System -} // namespace chip - -// Sizing definitions - -/** - * @def CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE - * - * The effective size of the packet buffer structure. - * - * TODO: This is an implementation details that does not need to be public and should be moved to the source file. - */ - -#if CHIP_SYSTEM_CONFIG_USE_LWIP -#define CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE LWIP_MEM_ALIGN_SIZE(sizeof(struct ::pbuf)) -#else // CHIP_SYSTEM_CONFIG_USE_LWIP -#define CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE CHIP_SYSTEM_ALIGN_SIZE(sizeof(::chip::System::PacketBuffer), 4u) -#endif // CHIP_SYSTEM_CONFIG_USE_LWIP - -/** - * @def CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX - * - * See SystemConfig.h for full description. This is defined in here specifically for LwIP platform to preserve backwards - * compatibility. - * - * TODO: This is an implementation details that does not need to be public and should be moved to the source file. - * - */ -#if CHIP_SYSTEM_CONFIG_USE_LWIP -#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX (LWIP_MEM_ALIGN_SIZE(PBUF_POOL_BUFSIZE) - CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE) -#endif // CHIP_SYSTEM_CONFIG_USE_LWIP - -/** - * @def CHIP_SYSTEM_PACKETBUFFER_SIZE - * - * The memory footprint of a PacketBuffer object, computed from max capacity size and the size of the packet buffer structure. - * - * TODO: This is an implementation details that does not need to be public and should be moved to the source file. - */ - -#define CHIP_SYSTEM_PACKETBUFFER_SIZE (CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE) - -namespace chip { -namespace System { - -/** - * The maximum size buffer an application can allocate with the default reserve, i.e. \c PacketBufferHandle::New(size). - */ -constexpr uint16_t kMaxPacketBufferSize = CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX - CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE; - -/** - * The maximum size buffer an application can allocate with no reserve, i.e. \c PacketBufferHandle::New(size, 0). - */ -constexpr uint16_t kMaxPacketBufferSizeWithoutReserve = CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX; - -// -// Pool allocation for PacketBuffer objects (toll-free bridged with LwIP pbuf allocator if CHIP_SYSTEM_CONFIG_USE_LWIP) -// -#if !CHIP_SYSTEM_CONFIG_USE_LWIP && CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC - -typedef union -{ - PacketBuffer Header; - uint8_t Block[CHIP_SYSTEM_PACKETBUFFER_SIZE]; -} BufferPoolElement; - -#endif // !CHIP_SYSTEM_CONFIG_USE_LWIP && CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC - -inline uint16_t PacketBuffer::AllocSize() const -{ -#if CHIP_SYSTEM_CONFIG_USE_LWIP -#if LWIP_PBUF_FROM_CUSTOM_POOLS - // Temporary workaround for custom pbufs by assuming size to be PBUF_POOL_BUFSIZE - if (this->flags & PBUF_FLAG_IS_CUSTOM) - return LWIP_MEM_ALIGN_SIZE(PBUF_POOL_BUFSIZE) - CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; - else - return LWIP_MEM_ALIGN_SIZE(memp_sizes[this->pool]) - CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; -#else // !LWIP_PBUF_FROM_CUSTOM_POOLS - return LWIP_MEM_ALIGN_SIZE(PBUF_POOL_BUFSIZE) - CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; -#endif // !LWIP_PBUF_FROM_CUSTOM_POOLS -#else // !CHIP_SYSTEM_CONFIG_USE_LWIP -#if CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0 - return this->alloc_size; -#else // CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC != 0 - extern BufferPoolElement gDummyBufferPoolElement; - return sizeof(gDummyBufferPoolElement.Block) - CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; -#endif // CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC != 0 -#endif // !CHIP_SYSTEM_CONFIG_USE_LWIP -} - -} // namespace System -} // namespace chip - -namespace chip { -namespace System { +static_assert(sizeof(pbuf) == sizeof(PacketBuffer), "PacketBuffer must not have additional members"); /** * @class PacketBufferHandle @@ -495,8 +516,8 @@ class DLL_EXPORT PacketBufferHandle */ void RightSize() { -#if CHIP_SYSTEM_CONFIG_USE_LWIP && LWIP_PBUF_FROM_CUSTOM_POOLS - RightSizeForLwIPCustomPools(); +#if CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE + InternalRightSize(); #endif } @@ -535,6 +556,7 @@ class DLL_EXPORT PacketBufferHandle */ CHECK_RETURN_VALUE PacketBuffer * UnsafeRelease() && { + PacketBuffer::Check(mBuffer); PacketBuffer * buffer = mBuffer; mBuffer = nullptr; return buffer; @@ -550,7 +572,7 @@ class DLL_EXPORT PacketBufferHandle * provides a pointer to the start of this space. * * Fails and returns \c nullptr if no memory is available, or if the size requested is too large. - * When the sum of \a aAvailableSize and \a aReservedSize is no greater than \c kMaxPacketBufferSizeWithoutReserve, + * When the sum of \a aAvailableSize and \a aReservedSize is no greater than \c PacketBuffer::kMaxSizeWithoutReserve, * that is guaranteed not to be too large. * * On success, it is guaranteed that \c AvailableDataSize() is no less than \a aAvailableSize. @@ -560,7 +582,7 @@ class DLL_EXPORT PacketBufferHandle * * @return On success, a PacketBufferHandle to the allocated buffer. On fail, \c nullptr. */ - static PacketBufferHandle New(size_t aAvailableSize, uint16_t aReservedSize = CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE); + static PacketBufferHandle New(size_t aAvailableSize, uint16_t aReservedSize = PacketBuffer::kDefaultHeaderReserve); /** * Allocates a packet buffer with initial contents. @@ -573,20 +595,49 @@ class DLL_EXPORT PacketBufferHandle * @return On success, a PacketBufferHandle to the allocated buffer. On fail, \c nullptr. */ static PacketBufferHandle NewWithData(const void * aData, size_t aDataSize, uint16_t aAdditionalSize = 0, - uint16_t aReservedSize = CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE); + uint16_t aReservedSize = PacketBuffer::kDefaultHeaderReserve); /** * Creates a copy of the data in this packet. * * Does NOT support chained buffers. * + * @param[in] aAdditionalSize Size of additional application data space after the initial contents. + * @param[in] aReservedSize Number of octets to reserve for protocol headers. + * * @returns empty handle on allocation failure. */ - PacketBufferHandle CloneData(); + PacketBufferHandle CloneData(uint16_t aAdditionalSize = 0, uint16_t aReservedSize = PacketBuffer::kDefaultHeaderReserve); + + /** + * Perform an implementation-defined check on the validity of a PacketBufferHandle. + * + * Unless enabled by #CHIP_CONFIG_MEMORY_DEBUG_CHECKS == 1, this function does nothing. + * + * When enabled, it performs an implementation- and configuration-defined check on + * the validity of the packet buffer. It MAY log an error and/or abort the program + * if the packet buffer or the implementation-defined memory management system is in + * a faulty state. (Some configurations may not actually perform any check.) + * + * @note A null handle is not considered faulty. + * + * @param[in] aPacket - the packet buffer handle to check. + */ + void Check() const + { +#if CHIP_SYSTEM_PACKETBUFFER_HAS_CHECK + PacketBuffer::Check(mBuffer); +#endif + } protected: #if CHIP_SYSTEM_CONFIG_USE_LWIP - static struct pbuf * GetLwIPpbuf(const PacketBufferHandle & handle) { return static_cast(handle.mBuffer); } + // For use via LwIPPacketBufferView only. + static struct pbuf * GetLwIPpbuf(const PacketBufferHandle & handle) + { + PacketBuffer::Check(handle.mBuffer); + return static_cast(handle.mBuffer); + } #endif // CHIP_SYSTEM_CONFIG_USE_LWIP private: @@ -609,10 +660,8 @@ class DLL_EXPORT PacketBufferHandle bool operator==(const PacketBufferHandle & aOther) { return mBuffer == aOther.mBuffer; } -#if CHIP_SYSTEM_CONFIG_USE_LWIP && LWIP_PBUF_FROM_CUSTOM_POOLS - void RightSizeForLwIPCustomPools(); -#elif !CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC - void RightSizeForMemoryAlloc(); +#if CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE + void InternalRightSize(); #endif PacketBuffer * mBuffer; diff --git a/src/system/TLVPacketBufferBackingStore.cpp b/src/system/TLVPacketBufferBackingStore.cpp index 80998a3c97a589..effa547453ee1e 100644 --- a/src/system/TLVPacketBufferBackingStore.cpp +++ b/src/system/TLVPacketBufferBackingStore.cpp @@ -91,7 +91,7 @@ CHIP_ERROR TLVPacketBufferBackingStore::GetNewBuffer(chip::TLV::TLVWriter & writ mCurrentBuffer.Advance(); if (mCurrentBuffer.IsNull()) { - mCurrentBuffer = PacketBufferHandle::New(System::kMaxPacketBufferSizeWithoutReserve, 0); + mCurrentBuffer = PacketBufferHandle::New(System::PacketBuffer::kMaxSizeWithoutReserve, 0); if (mCurrentBuffer.IsNull()) { return CHIP_ERROR_NO_MEMORY; diff --git a/src/system/tests/TestSystemPacketBuffer.cpp b/src/system/tests/TestSystemPacketBuffer.cpp index 5bdf68d8817137..74f64a8e4d885f 100644 --- a/src/system/tests/TestSystemPacketBuffer.cpp +++ b/src/system/tests/TestSystemPacketBuffer.cpp @@ -34,6 +34,7 @@ #include #include +#include #include #include #include @@ -129,6 +130,8 @@ class PacketBufferTest } static void PrintHandle(const char * tag, const PacketBufferHandle & handle) { PrintHandle(tag, handle.mBuffer); } + static constexpr uint16_t kBlockSize = PacketBuffer::kBlockSize; + private: struct BufferConfiguration { @@ -178,8 +181,8 @@ class PacketBufferTest std::vector handles; }; -const uint16_t sTestReservedSizes[] = { 0, 10, 128, 1536, CHIP_SYSTEM_PACKETBUFFER_SIZE }; -const uint16_t sTestLengths[] = { 0, 1, 10, 128, CHIP_SYSTEM_PACKETBUFFER_SIZE, UINT16_MAX }; +const uint16_t sTestReservedSizes[] = { 0, 10, 128, 1536, PacketBufferTest::kBlockSize }; +const uint16_t sTestLengths[] = { 0, 1, 10, 128, PacketBufferTest::kBlockSize, UINT16_MAX }; PacketBufferTest::TestContext sContext = { sTestReservedSizes, @@ -200,6 +203,7 @@ PacketBufferTest::PacketBufferTest(TestContext * context) : mContext(context) int PacketBufferTest::TestSetup(void * inContext) { + chip::Platform::MemoryInit(); TestContext * const theContext = reinterpret_cast(inContext); theContext->test = new PacketBufferTest(theContext); if (theContext->test == nullptr) @@ -262,7 +266,7 @@ void PacketBufferTest::PrepareTestBuffer(BufferConfiguration * config, int flags { if (config->handle.IsNull()) { - config->handle = PacketBufferHandle::New(chip::System::kMaxPacketBufferSizeWithoutReserve, 0); + config->handle = PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSizeWithoutReserve, 0); if (config->handle.IsNull()) { printf("NewPacketBuffer: Failed to allocate packet buffer (%zu retained): %s\n", handles.size(), strerror(errno)); @@ -279,11 +283,11 @@ void PacketBufferTest::PrepareTestBuffer(BufferConfiguration * config, int flags exit(EXIT_FAILURE); } - const size_t lInitialSize = CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE + config->reserved_size; - const size_t lAllocSize = CHIP_SYSTEM_PACKETBUFFER_SIZE; + const size_t lInitialSize = PacketBuffer::kStructureSize + config->reserved_size; + const size_t lAllocSize = kBlockSize; uint8_t * const raw = reinterpret_cast(config->handle.Get()); - memset(raw + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE, 0, lAllocSize - CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE); + memset(raw + PacketBuffer::kStructureSize, 0, lAllocSize - PacketBuffer::kStructureSize); config->start_buffer = raw; config->end_buffer = raw + lAllocSize; @@ -342,7 +346,7 @@ bool PacketBufferTest::ResetHandles() * * Description: For every buffer-configuration from inContext, create a buffer's instance * using the New() method. Then, verify that when the size of the reserved space - * passed to New() is greater than #CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX, + * passed to New() is greater than PacketBuffer::kMaxSizeWithoutReserve, * the method returns nullptr. Otherwise, check for correctness of initializing * the new buffer's internal state. */ @@ -356,7 +360,7 @@ void PacketBufferTest::CheckNew(nlTestSuite * inSuite, void * inContext) { const PacketBufferHandle buffer = PacketBufferHandle::New(0, config.reserved_size); - if (config.reserved_size > CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX) + if (config.reserved_size > PacketBuffer::kMaxSizeWithoutReserve) { NL_TEST_ASSERT(inSuite, buffer.IsNull()); continue; @@ -376,6 +380,7 @@ void PacketBufferTest::CheckNew(nlTestSuite * inSuite, void * inContext) } } +#if CHIP_SYSTEM_CONFIG_USE_LWIP || CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE != 0 // Use the rest of the buffer space std::vector allocate_all_the_things; for (;;) @@ -388,6 +393,7 @@ void PacketBufferTest::CheckNew(nlTestSuite * inSuite, void * inContext) // Hold on to the buffer, to use up all the buffer space. allocate_all_the_things.push_back(std::move(buffer)); } +#endif // CHIP_SYSTEM_CONFIG_USE_LWIP || CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE != 0 } /** @@ -423,7 +429,7 @@ void PacketBufferTest::CheckSetStart(nlTestSuite * inSuite, void * inContext) PacketBufferTest * const test = theContext->test; NL_TEST_ASSERT(inSuite, test->mContext == theContext); - static constexpr ptrdiff_t sSizePacketBuffer = CHIP_SYSTEM_PACKETBUFFER_SIZE; + static constexpr ptrdiff_t sSizePacketBuffer = kBlockSize; for (auto & config : test->configurations) { @@ -448,10 +454,10 @@ void PacketBufferTest::CheckSetStart(nlTestSuite * inSuite, void * inContext) config.handle->SetStart(test_start); - if (verify_start < config.start_buffer + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE) + if (verify_start < config.start_buffer + PacketBuffer::kStructureSize) { // Set start before valid payload beginning. - verify_start = config.start_buffer + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE; + verify_start = config.start_buffer + PacketBuffer::kStructureSize; } if (verify_start > config.end_buffer) @@ -835,7 +841,7 @@ void PacketBufferTest::CheckCompactHead(nlTestSuite * inSuite, void * inContext) config.handle->CompactHead(); - NL_TEST_ASSERT(inSuite, config.handle->payload == (config.start_buffer + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE)); + NL_TEST_ASSERT(inSuite, config.handle->payload == (config.start_buffer + PacketBuffer::kStructureSize)); NL_TEST_ASSERT(inSuite, config.handle->tot_len == data_length); } @@ -885,8 +891,7 @@ void PacketBufferTest::CheckCompactHead(nlTestSuite * inSuite, void * inContext) config_1.handle->CompactHead(); - NL_TEST_ASSERT(inSuite, - config_1.handle->payload == (config_1.start_buffer + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE)); + NL_TEST_ASSERT(inSuite, config_1.handle->payload == (config_1.start_buffer + PacketBuffer::kStructureSize)); if (config_1.handle->tot_len > config_1.handle->MaxDataLength()) { @@ -1091,9 +1096,9 @@ void PacketBufferTest::CheckEnsureReservedSize(nlTestSuite * inSuite, void * inC const uint16_t kAllocSize = config.handle->AllocSize(); uint16_t reserved_size = config.reserved_size; - if (CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE + config.reserved_size > kAllocSize) + if (PacketBuffer::kStructureSize + config.reserved_size > kAllocSize) { - reserved_size = static_cast(kAllocSize - CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE); + reserved_size = static_cast(kAllocSize - PacketBuffer::kStructureSize); } if (length <= reserved_size) @@ -1102,7 +1107,7 @@ void PacketBufferTest::CheckEnsureReservedSize(nlTestSuite * inSuite, void * inC continue; } - if ((length + config.init_len) > (kAllocSize - CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE)) + if ((length + config.init_len) > (kAllocSize - PacketBuffer::kStructureSize)) { NL_TEST_ASSERT(inSuite, config.handle->EnsureReservedSize(length) == false); continue; @@ -1301,8 +1306,8 @@ void PacketBufferTest::CheckFree(nlTestSuite * inSuite, void * inContext) // start with various buffer ref counts for (size_t r = 0; r < kRefs; r++) { - config_1.handle = PacketBufferHandle::New(chip::System::kMaxPacketBufferSizeWithoutReserve, 0); - config_2.handle = PacketBufferHandle::New(chip::System::kMaxPacketBufferSizeWithoutReserve, 0); + config_1.handle = PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSizeWithoutReserve, 0); + config_2.handle = PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSizeWithoutReserve, 0); NL_TEST_ASSERT(inSuite, !config_1.handle.IsNull()); NL_TEST_ASSERT(inSuite, !config_2.handle.IsNull()); @@ -1453,7 +1458,7 @@ void PacketBufferTest::CheckHandleConstruct(nlTestSuite * inSuite, void * inCont PacketBufferHandle handle_2(nullptr); NL_TEST_ASSERT(inSuite, handle_2.IsNull()); - PacketBufferHandle handle_3(PacketBufferHandle::New(chip::System::kMaxPacketBufferSize)); + PacketBufferHandle handle_3(PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSize)); NL_TEST_ASSERT(inSuite, !handle_3.IsNull()); // Private constructor. @@ -1672,7 +1677,7 @@ void PacketBufferTest::CheckHandleRightSize(nlTestSuite * inSuite, void * inCont NL_TEST_ASSERT(inSuite, test->mContext == theContext); const char kPayload[] = "Joy!"; - PacketBufferHandle handle = PacketBufferHandle::New(chip::System::kMaxPacketBufferSizeWithoutReserve, 0); + PacketBufferHandle handle = PacketBufferHandle::New(chip::System::PacketBuffer::kMaxSizeWithoutReserve, 0); PacketBuffer * buffer = handle.mBuffer; memcpy(handle->Start(), kPayload, sizeof kPayload); @@ -1686,20 +1691,20 @@ void PacketBufferTest::CheckHandleRightSize(nlTestSuite * inSuite, void * inCont NL_TEST_ASSERT(inSuite, handle.mBuffer == buffer); } -#if (CHIP_SYSTEM_CONFIG_USE_LWIP && LWIP_PBUF_FROM_CUSTOM_POOLS) || (CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0) +#if CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE handle.RightSize(); NL_TEST_ASSERT(inSuite, handle.mBuffer != buffer); - NL_TEST_ASSERT(inSuite, handle->DataSize() == sizeof kPayload); + NL_TEST_ASSERT(inSuite, handle->DataLength() == sizeof kPayload); NL_TEST_ASSERT(inSuite, memcmp(handle->Start(), kPayload, sizeof kPayload) == 0); -#else // !((CHIP_SYSTEM_CONFIG_USE_LWIP && LWIP_PBUF_FROM_CUSTOM_POOLS) || (CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0)) +#else // CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE // For this configuration, RightSize() does nothing. handle.RightSize(); NL_TEST_ASSERT(inSuite, handle.mBuffer == buffer); -#endif // !((CHIP_SYSTEM_CONFIG_USE_LWIP && LWIP_PBUF_FROM_CUSTOM_POOLS) || (CHIP_SYSTEM_CONFIG_PACKETBUFFER_MAXALLOC == 0)) +#endif // CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE } void PacketBufferTest::CheckPacketBufferWriter(nlTestSuite * inSuite, void * inContext) @@ -1716,7 +1721,9 @@ void PacketBufferTest::CheckPacketBufferWriter(nlTestSuite * inSuite, void * inC NL_TEST_ASSERT(inSuite, !nay.IsNull()); yay.Put(kPayload); + yay.Put('\0'); nay.Put(kPayload); + nay.Put('\0'); NL_TEST_ASSERT(inSuite, yay.Fit()); NL_TEST_ASSERT(inSuite, !nay.Fit()); diff --git a/src/transport/SecureSessionMgr.h b/src/transport/SecureSessionMgr.h index 6858d09ea08a8e..adeee6c20ed988 100644 --- a/src/transport/SecureSessionMgr.h +++ b/src/transport/SecureSessionMgr.h @@ -318,8 +318,8 @@ constexpr uint16_t kMaxFooterSize = kMaxTagLen; */ inline System::PacketBufferHandle New(size_t aAvailableSize) { - static_assert(CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX > kMaxFooterSize, "inadequate capacity"); - if (aAvailableSize > CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX - kMaxFooterSize) + static_assert(System::PacketBuffer::kMaxSize > kMaxFooterSize, "inadequate capacity"); + if (aAvailableSize > System::PacketBuffer::kMaxSize - kMaxFooterSize) { return System::PacketBufferHandle(); } diff --git a/src/transport/raw/tests/NetworkTestHelpers.cpp b/src/transport/raw/tests/NetworkTestHelpers.cpp index d3902bba3a9547..281fab2cf64572 100644 --- a/src/transport/raw/tests/NetworkTestHelpers.cpp +++ b/src/transport/raw/tests/NetworkTestHelpers.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-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. @@ -19,6 +19,7 @@ #include +#include #include #include @@ -27,7 +28,7 @@ namespace Test { CHIP_ERROR IOContext::Init(nlTestSuite * suite) { - CHIP_ERROR err = CHIP_NO_ERROR; + CHIP_ERROR err = Platform::MemoryInit(); gSystemLayer.Init(nullptr); @@ -46,6 +47,7 @@ CHIP_ERROR IOContext::Shutdown() CHIP_ERROR err = CHIP_NO_ERROR; ShutdownNetwork(); + Platform::MemoryShutdown(); return err; } diff --git a/src/transport/tests/TestSecureSessionMgr.cpp b/src/transport/tests/TestSecureSessionMgr.cpp index d0e7dddec10a69..0ea823cc399e55 100644 --- a/src/transport/tests/TestSecureSessionMgr.cpp +++ b/src/transport/tests/TestSecureSessionMgr.cpp @@ -190,7 +190,6 @@ void CheckMessageTest(nlTestSuite * inSuite, void * inContext) callback.ReceiveHandlerCallCount = 0; err = secureSessionMgr.SendMessage(localToRemoteSession, std::move(buffer)); - printf("err=%d\n", err); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); ctx.DriveIOUntil(1000 /* ms */, []() { return callback.ReceiveHandlerCallCount != 0; });