Skip to content

Commit

Permalink
Fixing UBSan issues that showed up in all-clusters app (#35580)
Browse files Browse the repository at this point in the history
* Fix for unsigned integer overflow:
Error Message: BufferWriter.cpp:76:16: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long')

when size becomes 0 and we are in while (size-- > 0), then size will become less than 0, which should be avoided since it is an unsigned (although technically it wraps around, UBSAN complains about it).
Fix: stop size from decrement past 0, by moving size-- inside the loop

* Fix null pointer passed to non-null argument in CHIPMemString.h
Error Message: CHIPMemString.h:88:22: runtime error: null pointer passed as argument 2, which is declared to never be null

when PI= is in mDNS TXT record (its value is empty) , and Dnssd::Internal::GetPairingInstruction calls CopyString, source is an empty bytespan and source.data() will return a null pointer, that will be passed to memcpy
Fix: avoid memcpy in that case.
  • Loading branch information
Alami-Amine authored and pull[bot] committed Sep 20, 2024
1 parent 7fd0162 commit 2337458
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 6 deletions.
6 changes: 4 additions & 2 deletions src/lib/support/BufferWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,17 +73,19 @@ LittleEndian::BufferWriter & LittleEndian::BufferWriter::EndianPutSigned(int64_t

BigEndian::BufferWriter & BigEndian::BufferWriter::EndianPut(uint64_t x, size_t size)
{
while (size-- > 0)
while (size > 0)
{
size--;
Put(static_cast<uint8_t>((x >> (size * 8)) & 0xff));
}
return *this;
}

BigEndian::BufferWriter & BigEndian::BufferWriter::EndianPutSigned(int64_t x, size_t size)
{
while (size-- > 0)
while (size > 0)
{
size--;
Put(static_cast<uint8_t>((x >> (size * 8)) & 0xff));
}
return *this;
Expand Down
16 changes: 12 additions & 4 deletions src/lib/support/CHIPMemString.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,20 @@ inline void CopyString(char (&dest)[N], const char * source)
*/
inline void CopyString(char * dest, size_t destLength, ByteSpan source)
{
if (dest && destLength)
if ((dest == nullptr) || (destLength == 0))
{
size_t maxChars = std::min(destLength - 1, source.size());
memcpy(dest, source.data(), maxChars);
dest[maxChars] = '\0';
return; // no space to copy anything, not even a null terminator
}

if (source.empty())
{
*dest = '\0'; // just a null terminator, we are copying empty data
return;
}

size_t maxChars = std::min(destLength - 1, source.size());
memcpy(dest, source.data(), maxChars);
dest[maxChars] = '\0';
}

/**
Expand Down

0 comments on commit 2337458

Please sign in to comment.