Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure fields are always initialized in TLVWriters #30876

Merged
merged 6 commits into from
Dec 11, 2023
Merged

Ensure fields are always initialized in TLVWriters #30876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6e5fb5e
Ensure fields are always initialized in TLVWriters
tennessee-google Dec 7, 2023
45b52be
Restyled by clang-format
restyled-commits Dec 7, 2023
dcf2d27
Save flash on solution
tennessee-google Dec 8, 2023
6882bd4
Restyled by clang-format
restyled-commits Dec 8, 2023
e69fe41
Add new unit test
tennessee-google Dec 8, 2023
2c6e339
Restyled by clang-format
restyled-commits Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/lib/core/TLVUpdater.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ CHIP_ERROR TLVUpdater::Init(TLVReader & aReader, uint32_t freeLen)
mUpdaterReader.ImplicitProfileId = aReader.ImplicitProfileId;
mUpdaterReader.AppData = aReader.AppData;

// TODO(#30825): Need to ensure we use TLVWriter public API rather than touch the innards.

// Initialize the internal writer object
mUpdaterWriter.mBackingStore = nullptr;
mUpdaterWriter.mBufStart = buf - readDataLen;
Expand All @@ -109,7 +111,8 @@ CHIP_ERROR TLVUpdater::Init(TLVReader & aReader, uint32_t freeLen)
mUpdaterWriter.SetContainerOpen(false);
mUpdaterWriter.SetCloseContainerReserved(false);

mUpdaterWriter.ImplicitProfileId = aReader.ImplicitProfileId;
mUpdaterWriter.ImplicitProfileId = aReader.ImplicitProfileId;
mUpdaterWriter.mInitializationCookie = TLVWriter::kExpectedInitializationCookie;

// Cache element start address for internal use
mElementStartAddr = buf + freeLen;
Expand Down Expand Up @@ -142,6 +145,8 @@ CHIP_ERROR TLVUpdater::Next()

CHIP_ERROR TLVUpdater::Move()
{
VerifyOrReturnError(mUpdaterWriter.IsInitialized(), CHIP_ERROR_INCORRECT_STATE);

const uint8_t * elementEnd;
uint32_t copyLen;

Expand Down Expand Up @@ -171,13 +176,16 @@ CHIP_ERROR TLVUpdater::Move()

void TLVUpdater::MoveUntilEnd()
{
VerifyOrDie(mUpdaterWriter.IsInitialized());

const uint8_t * buffEnd = mUpdaterReader.GetReadPoint() + mUpdaterReader.GetRemainingLength();

uint32_t copyLen = static_cast<uint32_t>(buffEnd - mElementStartAddr);

// Move all elements till end to output TLV
memmove(mUpdaterWriter.mWritePoint, mElementStartAddr, copyLen);

// TODO(#30825): Need to ensure public API is used rather than touching the innards.
// Adjust the updater state
mElementStartAddr += copyLen;
mUpdaterWriter.mWritePoint += copyLen;
Expand All @@ -197,6 +205,8 @@ void TLVUpdater::MoveUntilEnd()

CHIP_ERROR TLVUpdater::EnterContainer(TLVType & outerContainerType)
{
VerifyOrReturnError(mUpdaterWriter.IsInitialized(), CHIP_ERROR_INCORRECT_STATE);

TLVType containerType;

VerifyOrReturnError(TLVTypeIsContainer(static_cast<TLVType>(mUpdaterReader.mControlByte & kTLVTypeMask)),
Expand Down Expand Up @@ -232,6 +242,8 @@ CHIP_ERROR TLVUpdater::ExitContainer(TLVType outerContainerType)
*/
void TLVUpdater::AdjustInternalWriterFreeSpace()
{
VerifyOrDie(mUpdaterWriter.IsInitialized());

const uint8_t * nextElementStart = mUpdaterReader.mReadPoint;

if (nextElementStart != mElementStartAddr)
Expand Down
98 changes: 80 additions & 18 deletions src/lib/core/TLVWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,56 +44,87 @@
#define NO_INLINE __attribute__((noinline))
#endif // DOXYGEN

// You can enable this block manually to abort on usage of uninitialized writers in
// your codebase. There are no such usages in the SDK (outside of tests).
#if 0
tcarmelveilleux marked this conversation as resolved.
Show resolved Hide resolved
#define ABORT_ON_UNINITIALIZED_IF_ENABLED() VerifyOrDie(IsInitialized() == true)
#else
#define ABORT_ON_UNINITIALIZED_IF_ENABLED() \
do \
{ \
} while (0)
#endif

namespace chip {
namespace TLV {

using namespace chip::Encoding;

TLVWriter::TLVWriter() :
ImplicitProfileId(kProfileIdNotSpecified), AppData(nullptr), mBackingStore(nullptr), mBufStart(nullptr), mWritePoint(nullptr),
mRemainingLen(0), mLenWritten(0), mMaxLen(0), mReservedSize(0), mContainerType(kTLVType_NotSpecified), mInitializationCookie(0),
mContainerOpen(false), mCloseContainerReserved(true)
{}

NO_INLINE void TLVWriter::Init(uint8_t * buf, size_t maxLen)
{
// TODO: Maybe we can just make mMaxLen, mLenWritten, mRemainingLen size_t instead?
uint32_t actualMaxLen = maxLen > UINT32_MAX ? UINT32_MAX : static_cast<uint32_t>(maxLen);

// TODO(#30825): Need to ensure a single init path for this complex data.
mInitializationCookie = 0;
mBackingStore = nullptr;
mBufStart = mWritePoint = buf;
mRemainingLen = actualMaxLen;
mLenWritten = 0;
mMaxLen = actualMaxLen;
mContainerType = kTLVType_NotSpecified;
mReservedSize = 0;
mBufStart = buf;
mWritePoint = buf;
mRemainingLen = actualMaxLen;
mLenWritten = 0;
mMaxLen = actualMaxLen;
mContainerType = kTLVType_NotSpecified;
mReservedSize = 0;
SetContainerOpen(false);
SetCloseContainerReserved(true);

ImplicitProfileId = kProfileIdNotSpecified;
ImplicitProfileId = kProfileIdNotSpecified;
mInitializationCookie = kExpectedInitializationCookie;
}

CHIP_ERROR TLVWriter::Init(TLVBackingStore & backingStore, uint32_t maxLen)
CHIP_ERROR TLVWriter::Init(TLVBackingStore & backingStore, uint32_t maxLen /* = UINT32_MAX */)
{
// TODO(#30825): Need to ensure a single init path for this complex data.
Init(nullptr, maxLen);
mInitializationCookie = 0;

mBackingStore = &backingStore;
mBufStart = nullptr;
mRemainingLen = 0;
CHIP_ERROR err = mBackingStore->OnInit(*this, mBufStart, mRemainingLen);
if (err != CHIP_NO_ERROR)
return err;

mWritePoint = mBufStart;
mLenWritten = 0;
mMaxLen = maxLen;
mContainerType = kTLVType_NotSpecified;
mReservedSize = 0;
SetContainerOpen(false);
SetCloseContainerReserved(true);

ImplicitProfileId = kProfileIdNotSpecified;
mWritePoint = mBufStart;
mInitializationCookie = kExpectedInitializationCookie;
return CHIP_NO_ERROR;
}

CHIP_ERROR TLVWriter::Finalize()
{
ABORT_ON_UNINITIALIZED_IF_ENABLED();

VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);

CHIP_ERROR err = CHIP_NO_ERROR;
if (IsContainerOpen())
return CHIP_ERROR_TLV_CONTAINER_OPEN;
if (mBackingStore != nullptr)
err = mBackingStore->FinalizeBuffer(*this, mBufStart, static_cast<uint32_t>(mWritePoint - mBufStart));

// TODO(#30825) The following should be safe, but in some cases (without mBackingStore), there are incremental writes that
// start failing.
#if 0
if (err == CHIP_NO_ERROR)
mInitializationCookie = 0;
#endif

return err;
}

Expand Down Expand Up @@ -463,6 +494,9 @@ CHIP_ERROR TLVWriter::CopyElement(Tag tag, TLVReader & reader)

CHIP_ERROR TLVWriter::OpenContainer(Tag tag, TLVType containerType, TLVWriter & containerWriter)
{
ABORT_ON_UNINITIALIZED_IF_ENABLED();

VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
CHIP_ERROR err = CHIP_NO_ERROR;

VerifyOrReturnError(TLVTypeIsContainer(containerType), CHIP_ERROR_WRONG_TLV_TYPE);
Expand All @@ -483,6 +517,7 @@ CHIP_ERROR TLVWriter::OpenContainer(Tag tag, TLVType containerType, TLVWriter &
return err;
}

// TODO(#30825): Clean-up this separate init path path.
containerWriter.mBackingStore = mBackingStore;
containerWriter.mBufStart = mBufStart;
containerWriter.mWritePoint = mWritePoint;
Expand All @@ -492,7 +527,8 @@ CHIP_ERROR TLVWriter::OpenContainer(Tag tag, TLVType containerType, TLVWriter &
containerWriter.mContainerType = containerType;
containerWriter.SetContainerOpen(false);
containerWriter.SetCloseContainerReserved(IsCloseContainerReserved());
containerWriter.ImplicitProfileId = ImplicitProfileId;
containerWriter.ImplicitProfileId = ImplicitProfileId;
containerWriter.mInitializationCookie = kExpectedInitializationCookie;

SetContainerOpen(true);

Expand All @@ -501,6 +537,10 @@ CHIP_ERROR TLVWriter::OpenContainer(Tag tag, TLVType containerType, TLVWriter &

CHIP_ERROR TLVWriter::CloseContainer(TLVWriter & containerWriter)
{
ABORT_ON_UNINITIALIZED_IF_ENABLED();

VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);

if (!TLVTypeIsContainer(containerWriter.mContainerType))
return CHIP_ERROR_INCORRECT_STATE;

Expand All @@ -526,6 +566,9 @@ CHIP_ERROR TLVWriter::CloseContainer(TLVWriter & containerWriter)

CHIP_ERROR TLVWriter::StartContainer(Tag tag, TLVType containerType, TLVType & outerContainerType)
{
ABORT_ON_UNINITIALIZED_IF_ENABLED();

VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
CHIP_ERROR err = CHIP_NO_ERROR;

VerifyOrReturnError(TLVTypeIsContainer(containerType), CHIP_ERROR_WRONG_TLV_TYPE);
Expand Down Expand Up @@ -555,6 +598,10 @@ CHIP_ERROR TLVWriter::StartContainer(Tag tag, TLVType containerType, TLVType & o

CHIP_ERROR TLVWriter::EndContainer(TLVType outerContainerType)
{
ABORT_ON_UNINITIALIZED_IF_ENABLED();

VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);

if (!TLVTypeIsContainer(mContainerType))
return CHIP_ERROR_INCORRECT_STATE;

Expand Down Expand Up @@ -585,6 +632,10 @@ CHIP_ERROR TLVWriter::CopyContainer(TLVReader & container)

CHIP_ERROR TLVWriter::CopyContainer(Tag tag, TLVReader & container)
{
ABORT_ON_UNINITIALIZED_IF_ENABLED();

VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);

// NOTE: This function MUST be used with a TVLReader that is reading from a contiguous buffer.
if (container.mBackingStore != nullptr)
return CHIP_ERROR_INVALID_ARGUMENT;
Expand All @@ -611,6 +662,8 @@ CHIP_ERROR TLVWriter::CopyContainer(Tag tag, TLVReader & container)

CHIP_ERROR TLVWriter::CopyContainer(Tag tag, const uint8_t * encodedContainer, uint16_t encodedContainerLen)
{
VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);

TLVReader reader;

reader.Init(encodedContainer, encodedContainerLen);
Expand All @@ -624,6 +677,9 @@ CHIP_ERROR TLVWriter::CopyContainer(Tag tag, const uint8_t * encodedContainer, u

CHIP_ERROR TLVWriter::WriteElementHead(TLVElementType elemType, Tag tag, uint64_t lenOrVal)
{
ABORT_ON_UNINITIALIZED_IF_ENABLED();

VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
uint8_t * p;
uint8_t stagingBuf[17]; // 17 = 1 control byte + 8 tag bytes + 8 length/value bytes

Expand Down Expand Up @@ -742,6 +798,9 @@ CHIP_ERROR TLVWriter::WriteElementHead(TLVElementType elemType, Tag tag, uint64_

CHIP_ERROR TLVWriter::WriteElementWithData(TLVType type, Tag tag, const uint8_t * data, uint32_t dataLen)
{
ABORT_ON_UNINITIALIZED_IF_ENABLED();

VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
if (static_cast<uint64_t>(type) & kTLVTypeSizeMask)
{
// We won't be able to recover this type properly!
Expand All @@ -767,6 +826,9 @@ CHIP_ERROR TLVWriter::WriteElementWithData(TLVType type, Tag tag, const uint8_t

CHIP_ERROR TLVWriter::WriteData(const uint8_t * p, uint32_t len)
{
ABORT_ON_UNINITIALIZED_IF_ENABLED();

VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError((mLenWritten + len) <= mMaxLen, CHIP_ERROR_BUFFER_TOO_SMALL);

while (len > 0)
Expand Down
Loading
Loading