Skip to content

Commit

Permalink
TLV reader: Improve floating-point support
Browse files Browse the repository at this point in the history
1. For some reason the Get(float&) signature was declared in the .h file
   but was never defined. Define it as well. Disallow reading a 64-bit
   floating point number to float, to avoid silent truncation / loss of
   precision.
2. It is undefined behaviour to use unions for type-punning in C++. Use
   raw memcpy calls instead, which is defined by the C++ standard
   (std::bit_cast is not available).

   Compilers will generally be able to optimize this memcpy away, unless
   a freestanding implementation (with possibly non-standard memcpy) is
   used.

This is a backport from an analogous commit to CHIP:
project-chip/connectedhomeip#8531
  • Loading branch information
anqid-g committed Aug 3, 2021
1 parent 7a74093 commit 7aa50ec
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 19 deletions.
57 changes: 43 additions & 14 deletions src/lib/core/WeaveTLVReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,45 @@ WEAVE_ERROR TLVReader::Get(uint64_t& v)
return WEAVE_NO_ERROR;
}

namespace {
float BitCastToFloat(const uint64_t elemLenOrVal)
{
float f;
auto u32 = static_cast<uint32_t>(elemLenOrVal);
memcpy(&f, &u32, sizeof(f));
return f;
}
} // namespace

// Note: Unlike the integer Get functions, this code avoids doing conversions
// between float and double wherever possible, because these conversions are
// relatively expensive on platforms that use soft-float instruction sets.

/**
* Get the value of the current element as a single-precision floating point number.
*
* @param[out] v Receives the value associated with current TLV element.
*
* @retval #WEAVE_NO_ERROR If the method succeeded.
* @retval #WEAVE_ERROR_WRONG_TLV_TYPE If the current element is not a TLV floating point type, or
* the reader is not positioned on an element.
*
*/
WEAVE_ERROR TLVReader::Get(float & v)
{
switch (ElementType())
{
case kTLVElementType_FloatingPointNumber32:
{
v = BitCastToFloat(mElemLenOrVal);
break;
}
default:
return WEAVE_ERROR_WRONG_TLV_TYPE;
}
return WEAVE_NO_ERROR;
}

/**
* Get the value of the current element as a double-precision floating point number.
*
Expand All @@ -557,24 +596,14 @@ WEAVE_ERROR TLVReader::Get(double& v)
{
case kTLVElementType_FloatingPointNumber32:
{
union
{
uint32_t u32;
float f;
} cvt;
cvt.u32 = (uint32_t)mElemLenOrVal;
v = cvt.f;
v = BitCastToFloat(mElemLenOrVal);
break;
}
case kTLVElementType_FloatingPointNumber64:
{
union
{
uint64_t u64;
double d;
} cvt;
cvt.u64 = mElemLenOrVal;
v = cvt.d;
double d;
memcpy(&d, &mElemLenOrVal, sizeof(d));
v = d;
break;
}
default:
Expand Down
51 changes: 46 additions & 5 deletions src/test-apps/TestTLV.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -580,11 +580,12 @@ void ReadEncoding1(nlTestSuite *inSuite, TLVReader& reader)

TestNext<TLVReader>(inSuite, reader2);

TestGet<TLVReader, double>(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), (float)17.9);
TestGet<TLVReader, float>(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), 17.9f);
TestGet<TLVReader, double>(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), 17.9f);

TestNext<TLVReader>(inSuite, reader2);

TestGet<TLVReader, double>(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65536), (double)17.9);
TestGet<TLVReader, double>(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65536), 17.9);

TestEndAndCloseContainer(inSuite, reader, reader2);
}
Expand Down Expand Up @@ -2946,11 +2947,12 @@ void TestWeaveTLVReaderDup(nlTestSuite *inSuite)

TestNext<TLVReader>(inSuite, reader2);

TestGet<TLVReader, double>(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), (float)17.9);
TestGet<TLVReader, float>(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), 17.9f);
TestGet<TLVReader, double>(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), 17.9f);

TestNext<TLVReader>(inSuite, reader2);

TestGet<TLVReader, double>(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65536), (double)17.9);
TestGet<TLVReader, double>(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65536), 17.9);

TestEndAndCloseContainer(inSuite, reader, reader2);
}
Expand All @@ -2974,6 +2976,11 @@ void TestWeaveTLVReaderErrorHandling(nlTestSuite *inSuite)
err = reader.Get(val);
NL_TEST_ASSERT(inSuite, err == WEAVE_ERROR_WRONG_TLV_TYPE);

// Get(float&)
float numF;
err = reader.Get(numF);
NL_TEST_ASSERT(inSuite, err == WEAVE_ERROR_WRONG_TLV_TYPE);

// Get(double&)
double numD;
err = reader.Get(numD);
Expand Down Expand Up @@ -3020,6 +3027,38 @@ void TestWeaveTLVReaderErrorHandling(nlTestSuite *inSuite)
NL_TEST_ASSERT(inSuite, err == WEAVE_ERROR_WRONG_TLV_TYPE);
free((void *)data);
}

/**
* Test that Weave TLV reader returns an error when a read is requested that
* would truncate the output.
*/
void TestWeaveTLVReaderTruncatedReads(nlTestSuite * inSuite)
{
uint8_t buf[2048];
TLVWriter writer;
TLVReader reader;

WEAVE_ERROR err;
float outF;

// Setup: Write some values into the buffer
writer.Init(buf, sizeof(buf));
writer.ImplicitProfileId = TestProfile_2;

err = writer.Put(AnonymousTag, double(12.5));
NL_TEST_ASSERT(inSuite, err == WEAVE_NO_ERROR);

// Test reading values from the buffer
reader.Init(buf, sizeof(buf));

TestNext<TLVReader>(inSuite, reader);

TestGet<TLVReader, double>(inSuite, reader, kTLVType_FloatingPointNumber, AnonymousTag, 12.5);

err = reader.Get(outF);
NL_TEST_ASSERT(inSuite, err == WEAVE_ERROR_WRONG_TLV_TYPE);
}

/**
* Test Weave TLV Reader in a use case
*/
Expand All @@ -3046,7 +3085,7 @@ void TestWeaveTLVReaderInPractice(nlTestSuite *inSuite)

TestNext<TLVReader>(inSuite, reader);

TestGet<TLVReader, double>(inSuite, reader, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_1, 4000000000ULL), (float) 1.0);
TestGet<TLVReader, float>(inSuite, reader, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_1, 4000000000ULL), (float) 1.0);
}

void TestWeaveTLVReader_NextOverContainer_ProcessElement(nlTestSuite *inSuite, TLVReader& reader, void *context)
Expand Down Expand Up @@ -3147,6 +3186,8 @@ void CheckWeaveTLVReader(nlTestSuite *inSuite, void *inContext)

TestWeaveTLVReaderErrorHandling(inSuite);

TestWeaveTLVReaderTruncatedReads(inSuite);

TestWeaveTLVReaderInPractice(inSuite);

TestWeaveTLVReader_NextOverContainer(inSuite);
Expand Down

0 comments on commit 7aa50ec

Please sign in to comment.