From e00647d661a58480d76ae5ce7ef61232e1a08361 Mon Sep 17 00:00:00 2001 From: anqid-g Date: Fri, 30 Jul 2021 06:49:18 -0700 Subject: [PATCH] TLV reader: Fix `float` support and remove union UB (#8531) * TLV reader: Improve floating-point support 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. * TLV writer: Remove union UB It is undefined behaviour to use unions for type-punning in C++. Use raw memcpy calls instead, as a substitute for the unavailable std::bit_cast. --- src/lib/core/CHIPTLVReader.cpp | 46 +++++++++++++++++++-------- src/lib/core/CHIPTLVWriter.cpp | 24 +++++--------- src/lib/core/tests/TestCHIPTLV.cpp | 51 ++++++++++++++++++++++++++---- 3 files changed, 85 insertions(+), 36 deletions(-) diff --git a/src/lib/core/CHIPTLVReader.cpp b/src/lib/core/CHIPTLVReader.cpp index 4c887275b3749d..72e9ea66746f14 100644 --- a/src/lib/core/CHIPTLVReader.cpp +++ b/src/lib/core/CHIPTLVReader.cpp @@ -207,28 +207,46 @@ CHIP_ERROR TLVReader::Get(uint64_t & v) return CHIP_NO_ERROR; } +namespace { +float BitCastToFloat(const uint64_t elemLenOrVal) +{ + float f; + auto u32 = static_cast(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. + +CHIP_ERROR TLVReader::Get(float & v) +{ + switch (ElementType()) + { + case TLVElementType::FloatingPointNumber32: { + v = BitCastToFloat(mElemLenOrVal); + break; + } + default: + return CHIP_ERROR_WRONG_TLV_TYPE; + } + return CHIP_NO_ERROR; +} + CHIP_ERROR TLVReader::Get(double & v) { switch (ElementType()) { case TLVElementType::FloatingPointNumber32: { - union - { - uint32_t u32; - float f; - } cvt; - cvt.u32 = static_cast(mElemLenOrVal); - v = cvt.f; + v = BitCastToFloat(mElemLenOrVal); break; } case TLVElementType::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: diff --git a/src/lib/core/CHIPTLVWriter.cpp b/src/lib/core/CHIPTLVWriter.cpp index 41553024cd3b36..1f09fee85fa372 100644 --- a/src/lib/core/CHIPTLVWriter.cpp +++ b/src/lib/core/CHIPTLVWriter.cpp @@ -211,26 +211,18 @@ CHIP_ERROR TLVWriter::Put(uint64_t tag, int64_t v, bool preserveSize) return Put(tag, v); } -CHIP_ERROR TLVWriter::Put(uint64_t tag, float v) +CHIP_ERROR TLVWriter::Put(uint64_t tag, const float v) { - union - { - float f; - uint32_t u32; - } cvt; - cvt.f = v; - return WriteElementHead(TLVElementType::FloatingPointNumber32, tag, cvt.u32); + uint32_t u32; + memcpy(&u32, &v, sizeof(u32)); + return WriteElementHead(TLVElementType::FloatingPointNumber32, tag, u32); } -CHIP_ERROR TLVWriter::Put(uint64_t tag, double v) +CHIP_ERROR TLVWriter::Put(uint64_t tag, const double v) { - union - { - double d; - uint64_t u64; - } cvt; - cvt.d = v; - return WriteElementHead(TLVElementType::FloatingPointNumber64, tag, cvt.u64); + uint64_t u64; + memcpy(&u64, &v, sizeof(u64)); + return WriteElementHead(TLVElementType::FloatingPointNumber64, tag, u64); } CHIP_ERROR TLVWriter::Put(uint64_t tag, ByteSpan data) diff --git a/src/lib/core/tests/TestCHIPTLV.cpp b/src/lib/core/tests/TestCHIPTLV.cpp index dcaf252f642e8f..abe13add3d1473 100644 --- a/src/lib/core/tests/TestCHIPTLV.cpp +++ b/src/lib/core/tests/TestCHIPTLV.cpp @@ -582,8 +582,8 @@ void ReadEncoding1(nlTestSuite * inSuite, TLVReader & reader) TestNext(inSuite, reader2); - TestGet(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), - static_cast(17.9)); + TestGet(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), 17.9f); + TestGet(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), 17.9f); TestNext(inSuite, reader2); @@ -2965,8 +2965,8 @@ void TestCHIPTLVReaderDup(nlTestSuite * inSuite) TestNext(inSuite, reader2); - TestGet(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), - static_cast(17.9)); + TestGet(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), 17.9f); + TestGet(inSuite, reader2, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_2, 65535), 17.9f); TestNext(inSuite, reader2); @@ -2994,6 +2994,11 @@ void TestCHIPTLVReaderErrorHandling(nlTestSuite * inSuite) err = reader.Get(val); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_WRONG_TLV_TYPE); + // Get(float&) + float numF; + err = reader.Get(numF); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_WRONG_TLV_TYPE); + // Get(double&) double numD; err = reader.Get(numD); @@ -3040,6 +3045,38 @@ void TestCHIPTLVReaderErrorHandling(nlTestSuite * inSuite) NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_WRONG_TLV_TYPE); chip::Platform::MemoryFree(const_cast(data)); } + +/** + * Test that CHIP TLV reader returns an error when a read is requested that + * would truncate the output. + */ +void TestCHIPTLVReaderTruncatedReads(nlTestSuite * inSuite) +{ + uint8_t buf[2048]; + TLVWriter writer; + TLVReader reader; + + CHIP_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 == CHIP_NO_ERROR); + + // Test reading values from the buffer + reader.Init(buf, sizeof(buf)); + + TestNext(inSuite, reader); + + TestGet(inSuite, reader, kTLVType_FloatingPointNumber, AnonymousTag, 12.5); + + err = reader.Get(outF); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_WRONG_TLV_TYPE); +} + /** * Test CHIP TLV Reader in a use case */ @@ -3068,8 +3105,8 @@ void TestCHIPTLVReaderInPractice(nlTestSuite * inSuite) TestNext(inSuite, reader); - TestGet(inSuite, reader, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_1, 4000000000ULL), - static_cast(1.0)); + TestGet(inSuite, reader, kTLVType_FloatingPointNumber, ProfileTag(TestProfile_1, 4000000000ULL), + static_cast(1.0)); } void TestCHIPTLVReader_NextOverContainer_ProcessElement(nlTestSuite * inSuite, TLVReader & reader, void * context) @@ -3170,6 +3207,8 @@ void CheckCHIPTLVReader(nlTestSuite * inSuite, void * inContext) TestCHIPTLVReaderErrorHandling(inSuite); + TestCHIPTLVReaderTruncatedReads(inSuite); + TestCHIPTLVReaderInPractice(inSuite); TestCHIPTLVReader_NextOverContainer(inSuite);