diff --git a/src/lib/core/TLVBackingStore.h b/src/lib/core/TLVBackingStore.h index 6d3c989e116327..e7712a066b9010 100644 --- a/src/lib/core/TLVBackingStore.h +++ b/src/lib/core/TLVBackingStore.h @@ -155,6 +155,16 @@ class DLL_EXPORT TLVBackingStore * */ virtual CHIP_ERROR FinalizeBuffer(TLVWriter & writer, uint8_t * bufStart, uint32_t bufLen) = 0; + + /** + * Returns whether call to GetNewBuffer will always fail. + * + * There are some implementations of TLVBackingStore that provide some level of utility, such as access to pool + * of particular kind of buffer and/or reserving space for headers. Some implementation allow the caller to + * specify that they only intend to use a single buffer. It is useful for TLVWriter to know if this is the case. + * + */ + virtual bool GetNewBufferWillAlwaysFail() { return false; } }; } // namespace TLV diff --git a/src/lib/core/TLVWriter.cpp b/src/lib/core/TLVWriter.cpp index 1ac6433d1761fe..b20cc70adde8f0 100644 --- a/src/lib/core/TLVWriter.cpp +++ b/src/lib/core/TLVWriter.cpp @@ -128,6 +128,20 @@ CHIP_ERROR TLVWriter::Finalize() return err; } +CHIP_ERROR TLVWriter::ReserveBuffer(uint32_t aBufferSize) +{ + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mRemainingLen >= aBufferSize, CHIP_ERROR_NO_MEMORY); + + if (mBackingStore) + { + VerifyOrReturnError(mBackingStore->GetNewBufferWillAlwaysFail(), CHIP_ERROR_INCORRECT_STATE); + } + mReservedSize += aBufferSize; + mRemainingLen -= aBufferSize; + return CHIP_NO_ERROR; +} + CHIP_ERROR TLVWriter::PutBoolean(Tag tag, bool v) { return WriteElementHead((v) ? TLVElementType::BooleanTrue : TLVElementType::BooleanFalse, tag, 0); diff --git a/src/lib/core/TLVWriter.h b/src/lib/core/TLVWriter.h index eb38d5a28d9932..39e23a8e3e5ab3 100644 --- a/src/lib/core/TLVWriter.h +++ b/src/lib/core/TLVWriter.h @@ -144,15 +144,12 @@ class DLL_EXPORT TLVWriter * @retval #CHIP_NO_ERROR Successfully reserved required buffer size. * @retval #CHIP_ERROR_INCORRECT_STATE If the TLVWriter was not initialized. * @retval #CHIP_ERROR_NO_MEMORY The reserved buffer size cannot fits into the remaining buffer size. + * @retval #CHIP_ERROR_INCORRECT_STATE + * Uses TLVBackingStore and is in a state where it might allocate + * additional non-contigious memory, thus making it difficult/impossible + * to properly reserve space. */ - CHIP_ERROR ReserveBuffer(uint32_t aBufferSize) - { - VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(mRemainingLen >= aBufferSize, CHIP_ERROR_NO_MEMORY); - mReservedSize += aBufferSize; - mRemainingLen -= aBufferSize; - return CHIP_NO_ERROR; - } + CHIP_ERROR ReserveBuffer(uint32_t aBufferSize); /** * Release previously reserved buffer. diff --git a/src/system/TLVPacketBufferBackingStore.h b/src/system/TLVPacketBufferBackingStore.h index 81afe973ad3c33..c39b871233ebd3 100644 --- a/src/system/TLVPacketBufferBackingStore.h +++ b/src/system/TLVPacketBufferBackingStore.h @@ -80,6 +80,12 @@ class TLVPacketBufferBackingStore : public chip::TLV::TLVBackingStore CHIP_ERROR OnInit(chip::TLV::TLVWriter & writer, uint8_t *& bufStart, uint32_t & bufLen) override; CHIP_ERROR GetNewBuffer(chip::TLV::TLVWriter & writer, uint8_t *& bufStart, uint32_t & bufLen) override; CHIP_ERROR FinalizeBuffer(chip::TLV::TLVWriter & writer, uint8_t * bufStart, uint32_t bufLen) override; + virtual bool GetNewBufferWillAlwaysFail() override + { + // For non-chained buffers, caller is given one chunk of contiguous memory. All calls to + // GetNewBuffer will fail with CHIP_ERROR_NO_MEMORY. + return !mUseChainedBuffers; + } protected: chip::System::PacketBufferHandle mHeadBuffer; diff --git a/src/system/tests/TestTLVPacketBufferBackingStore.cpp b/src/system/tests/TestTLVPacketBufferBackingStore.cpp index fccced4cfbb4f4..e46906d2edbbb3 100644 --- a/src/system/tests/TestTLVPacketBufferBackingStore.cpp +++ b/src/system/tests/TestTLVPacketBufferBackingStore.cpp @@ -35,6 +35,16 @@ using namespace ::chip; namespace { +void WriteUntilRemainingLessThan(nlTestSuite * inSuite, PacketBufferTLVWriter & writer, const uint32_t remainingSize) +{ + uint32_t lengthRemaining = writer.GetRemainingFreeLength(); + while (lengthRemaining >= remainingSize) + { + NL_TEST_ASSERT(inSuite, writer.Put(TLV::AnonymousTag(), static_cast(7)) == CHIP_NO_ERROR); + lengthRemaining = writer.GetRemainingFreeLength(); + } +} + class TLVPacketBufferBackingStoreTest { public: @@ -43,6 +53,9 @@ class TLVPacketBufferBackingStoreTest static void BasicEncodeDecode(nlTestSuite * inSuite, void * inContext); static void MultiBufferEncode(nlTestSuite * inSuite, void * inContext); + static void NonChainedBufferCanReserve(nlTestSuite * inSuite, void * inContext); + static void TestWriterReserveUnreserveDoesNotOverflow(nlTestSuite * inSuite, void * inContext); + static void TestWriterReserve(nlTestSuite * inSuite, void * inContext); }; int TLVPacketBufferBackingStoreTest::TestSetup(void * inContext) @@ -238,14 +251,111 @@ void TLVPacketBufferBackingStoreTest::MultiBufferEncode(nlTestSuite * inSuite, v NL_TEST_ASSERT(inSuite, error == CHIP_END_OF_TLV); } +void TLVPacketBufferBackingStoreTest::NonChainedBufferCanReserve(nlTestSuite * inSuite, void * inContext) +{ + // Start with a too-small buffer. + uint32_t smallSize = 5; + uint32_t smallerSizeToReserver = smallSize - 1; + + auto buffer = PacketBufferHandle::New(smallSize, /* aReservedSize = */ 0); + + PacketBufferTLVWriter writer; + writer.Init(std::move(buffer), /* useChainedBuffers = */ false); + + CHIP_ERROR error = writer.ReserveBuffer(smallerSizeToReserver); + NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR); +} + +// This test previously was created to show that there was an overflow bug, now this test mainly +// just checks that you cannot reserve this type of TLVBackingStorage buffer. +void TLVPacketBufferBackingStoreTest::TestWriterReserveUnreserveDoesNotOverflow(nlTestSuite * inSuite, void * inContext) +{ + // Start with a too-small buffer. + uint32_t smallSize = 100; + uint32_t smallerSizeToReserver = smallSize - 1; + + auto buffer = PacketBufferHandle::New(smallSize, 0); + + PacketBufferTLVWriter writer; + writer.Init(std::move(buffer), /* useChainedBuffers = */ true); + + CHIP_ERROR error = writer.ReserveBuffer(smallerSizeToReserver); + if (error == CHIP_NO_ERROR) + { + uint32_t lengthRemaining = writer.GetRemainingFreeLength(); + NL_TEST_ASSERT(inSuite, lengthRemaining == 1); + // Lets try to overflow by getting next buffer in the chain, + // unreserving then writing until the end of the current buffer. + error = writer.Put(TLV::AnonymousTag(), static_cast(7)); + NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR); + + lengthRemaining = writer.GetRemainingFreeLength(); + NL_TEST_ASSERT(inSuite, lengthRemaining > smallerSizeToReserver); + + WriteUntilRemainingLessThan(inSuite, writer, 2); + + lengthRemaining = writer.GetRemainingFreeLength(); + NL_TEST_ASSERT(inSuite, lengthRemaining != 0); + NL_TEST_ASSERT(inSuite, lengthRemaining < smallerSizeToReserver); + + error = writer.UnreserveBuffer(smallerSizeToReserver); + NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR); + + lengthRemaining = writer.GetRemainingFreeLength(); + NL_TEST_ASSERT(inSuite, lengthRemaining > smallerSizeToReserver); + + // This is where we get overflow. + WriteUntilRemainingLessThan(inSuite, writer, 2); + + // If we get here then the overflow condition we were expecting did not happen. If that is the case, + // either we have fixed reservation for chained buffers, or expected failure didn't hit on this + // platform. + // + // If there is a fix please add reservation for chained buffers, please make sure you account for + // what happens if TLVWriter::WriteData fails to get a new buffer but we are not at max size, do + // you actually have space for what was supposed to be reserved. + NL_TEST_ASSERT(inSuite, false); + } + + // We no longer allow non-contigous buffers to be reserved. + NL_TEST_ASSERT(inSuite, error == CHIP_ERROR_INCORRECT_STATE); +} + +void TLVPacketBufferBackingStoreTest::TestWriterReserve(nlTestSuite * inSuite, void * inContext) +{ + // Start with a too-small buffer. + uint32_t smallSize = 5; + uint32_t smallerSizeToReserver = smallSize - 1; + + auto buffer = PacketBufferHandle::New(smallSize, 0); + + PacketBufferTLVWriter writer; + writer.Init(std::move(buffer), /* useChainedBuffers = */ false); + + CHIP_ERROR error = writer.ReserveBuffer(smallerSizeToReserver); + NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR); + + error = writer.Put(TLV::AnonymousTag(), static_cast(7)); + NL_TEST_ASSERT(inSuite, error == CHIP_ERROR_NO_MEMORY); + + error = writer.UnreserveBuffer(smallerSizeToReserver); + NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR); + + error = writer.Put(TLV::AnonymousTag(), static_cast(7)); + NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR); +} + /** * Test Suite. It lists all the test functions. */ // clang-format off const nlTest sTests[] = { - NL_TEST_DEF("BasicEncodeDecode", TLVPacketBufferBackingStoreTest::BasicEncodeDecode), - NL_TEST_DEF("MultiBufferEncode", TLVPacketBufferBackingStoreTest::MultiBufferEncode), + NL_TEST_DEF("BasicEncodeDecode", TLVPacketBufferBackingStoreTest::BasicEncodeDecode), + NL_TEST_DEF("MultiBufferEncode", TLVPacketBufferBackingStoreTest::MultiBufferEncode), + NL_TEST_DEF("NonChainedBufferCanReserve", TLVPacketBufferBackingStoreTest::NonChainedBufferCanReserve), + NL_TEST_DEF("TestWriterReserveUnreserveDoesNotOverflow", TLVPacketBufferBackingStoreTest::TestWriterReserveUnreserveDoesNotOverflow), + NL_TEST_DEF("TestWriterReserve", TLVPacketBufferBackingStoreTest::TestWriterReserve), NL_TEST_SENTINEL() };