Skip to content

Commit

Permalink
Ensure fields are always initialized in TLVWriters (#30876)
Browse files Browse the repository at this point in the history 
* Ensure fields are always initialized in TLVWriters

- It was found through static analysis that in some cases, TLVWriters
  did not have all fields initialized and this led to no errors, but
  possibly could lead to bad states.
- None of the code making use of uninitialized TLVWriters was found
  in the SDK, but there may be usages.
- In adding the checks, marked several areas that could be improved
  in TLVWriter data access/management.

Issue #30825

Testing done:
- All tests still pass.
- There is never uninitialized used of a TLVWriter.

---------

Co-authored-by: [email protected] <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
  • Loading branch information
@tcarmelveilleux @tennessee-google @restyled-commits
3 people authored Dec 11, 2023
1 parent 86b53e1 commit 6e2aa2b
Show file tree
Hide file tree
Showing 4 changed files with 420 additions and 25 deletions.
14 changes: 13 additions & 1 deletion src/lib/core/TLVUpdater.cpp
View file
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
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
#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

0 comments on commit 6e2aa2b

Please sign in to comment.