From 0914eb3cb4a4ac1f159f256d0d7a2fa56815b27d Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 17 Sep 2021 14:22:32 -0400 Subject: [PATCH] Fix some post-landing review comments for rotating device id. Specific changes: 1) Use `MutableButeSpan foo(bar)` instead of `MutableByteSpan foo = MutableByteSpan(bar)`. 2) Use the `Put` signature that takes ByteSpan when writing to the TLVWriter. 3) Fix some comments on generateRotatingDeviceIdAsBinary to make it clearer how the "in,out" param works. 4) Stop failing out for rotating device ids that are smaller than the max possible size. Add a test for this. --- .../AdditionalDataPayloadGenerator.cpp | 22 +++++++++---------- .../AdditionalDataPayloadGenerator.h | 7 +++--- .../AdditionalDataPayloadParser.cpp | 4 ++-- .../tests/TestAdditionalDataPayload.cpp | 13 +++++++++++ 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/setup_payload/AdditionalDataPayloadGenerator.cpp b/src/setup_payload/AdditionalDataPayloadGenerator.cpp index fd88545510d11e..a864d53f030fab 100644 --- a/src/setup_payload/AdditionalDataPayloadGenerator.cpp +++ b/src/setup_payload/AdditionalDataPayloadGenerator.cpp @@ -60,15 +60,13 @@ AdditionalDataPayloadGenerator::generateAdditionalDataPayload(uint16_t lifetimeC if (additionalDataFields.Has(AdditionalDataFields::RotatingDeviceId)) { uint8_t rotatingDeviceIdInternalBuffer[RotatingDeviceId::kMaxLength]; - MutableByteSpan rotatingDeviceIdBuffer = MutableByteSpan(rotatingDeviceIdInternalBuffer); + MutableByteSpan rotatingDeviceIdBuffer(rotatingDeviceIdInternalBuffer); // Generating Device Rotating Id - ReturnErrorOnFailure(generateRotatingDeviceIdAsBinary(lifetimeCounter, serialNumberBuffer, serialNumberBufferSize, - rotatingDeviceIdBuffer)); + ReturnErrorOnFailure( + generateRotatingDeviceIdAsBinary(lifetimeCounter, serialNumberBuffer, serialNumberBufferSize, rotatingDeviceIdBuffer)); // Adding the rotating device id to the TLV data - ReturnErrorOnFailure(innerWriter.PutBytes(ContextTag(kRotatingDeviceIdTag), - rotatingDeviceIdBuffer.data(), - static_cast(rotatingDeviceIdBuffer.size()))); + ReturnErrorOnFailure(innerWriter.Put(ContextTag(kRotatingDeviceIdTag), rotatingDeviceIdBuffer)); } ReturnErrorOnFailure(writer.CloseContainer(innerWriter)); @@ -76,8 +74,10 @@ AdditionalDataPayloadGenerator::generateAdditionalDataPayload(uint16_t lifetimeC return writer.Finalize(&bufferHandle); } -CHIP_ERROR AdditionalDataPayloadGenerator::generateRotatingDeviceIdAsBinary( - uint16_t lifetimeCounter, const char * serialNumberBuffer, size_t serialNumberBufferSize, MutableByteSpan & rotatingDeviceIdBuffer) +CHIP_ERROR AdditionalDataPayloadGenerator::generateRotatingDeviceIdAsBinary(uint16_t lifetimeCounter, + const char * serialNumberBuffer, + size_t serialNumberBufferSize, + MutableByteSpan & rotatingDeviceIdBuffer) { uint8_t hashOutputBuffer[kSHA256_Hash_Length]; BufferWriter outputBufferWriter(rotatingDeviceIdBuffer); @@ -113,9 +113,9 @@ CHIP_ERROR AdditionalDataPayloadGenerator::generateRotatingDeviceIdAsHexString( size_t rotatingDeviceIdBufferSize, size_t & rotatingDeviceIdValueOutputSize) { uint8_t rotatingDeviceIdInternalBuffer[RotatingDeviceId::kMaxLength]; - MutableByteSpan rotatingDeviceIdBufferTemp = MutableByteSpan(rotatingDeviceIdInternalBuffer); - ReturnErrorOnFailure(generateRotatingDeviceIdAsBinary(lifetimeCounter, serialNumberBuffer, serialNumberBufferSize, - rotatingDeviceIdBufferTemp)); + MutableByteSpan rotatingDeviceIdBufferTemp(rotatingDeviceIdInternalBuffer); + ReturnErrorOnFailure( + generateRotatingDeviceIdAsBinary(lifetimeCounter, serialNumberBuffer, serialNumberBufferSize, rotatingDeviceIdBufferTemp)); VerifyOrReturnError(rotatingDeviceIdBufferSize >= RotatingDeviceId::kHexMaxLength, CHIP_ERROR_BUFFER_TOO_SMALL); ReturnErrorOnFailure(BytesToUppercaseHexString(rotatingDeviceIdBufferTemp.data(), rotatingDeviceIdBufferTemp.size(), diff --git a/src/setup_payload/AdditionalDataPayloadGenerator.h b/src/setup_payload/AdditionalDataPayloadGenerator.h index 307a79abd093b7..317fd83efffea5 100644 --- a/src/setup_payload/AdditionalDataPayloadGenerator.h +++ b/src/setup_payload/AdditionalDataPayloadGenerator.h @@ -78,10 +78,9 @@ class AdditionalDataPayloadGenerator * @param lifetimeCounter lifetime counter * @param serialNumberBuffer null-terminated serial number buffer * @param serialNumberBufferSize size of the serial number buffer supplied. - * @param rotatingDeviceIdBuffer rotating device id mutable byte span, it will be resized to the actual size used upon successful generation - * - * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise. - * + * @param [in,out] rotatingDeviceIdBuffer as input, the buffer to use for + * the binary data. As output, will have its size set to + * the actual size used upon successful generation */ CHIP_ERROR generateRotatingDeviceIdAsBinary(uint16_t lifetimeCounter, const char * serialNumberBuffer, size_t serialNumberBufferSize, MutableByteSpan & rotatingDeviceIdBuffer); diff --git a/src/setup_payload/AdditionalDataPayloadParser.cpp b/src/setup_payload/AdditionalDataPayloadParser.cpp index 83a49c61b82050..f555fd45b6c3ba 100644 --- a/src/setup_payload/AdditionalDataPayloadParser.cpp +++ b/src/setup_payload/AdditionalDataPayloadParser.cpp @@ -53,12 +53,12 @@ CHIP_ERROR AdditionalDataPayloadParser::populatePayload(SetupPayloadData::Additi ByteSpan rotatingDeviceId; ReturnErrorOnFailure(innerReader.GetByteView(rotatingDeviceId)); - VerifyOrReturnError(rotatingDeviceId.size() == RotatingDeviceId::kMaxLength, CHIP_ERROR_INVALID_STRING_LENGTH); + VerifyOrReturnError(rotatingDeviceId.size() <= RotatingDeviceId::kMaxLength, CHIP_ERROR_INVALID_STRING_LENGTH); char rotatingDeviceIdBufferTemp[RotatingDeviceId::kHexMaxLength]; ReturnErrorOnFailure(Encoding::BytesToUppercaseHexString(rotatingDeviceId.data(), rotatingDeviceId.size(), rotatingDeviceIdBufferTemp, RotatingDeviceId::kHexMaxLength)); - outPayload.rotatingDeviceId = std::string(rotatingDeviceIdBufferTemp, RotatingDeviceId::kHexMaxLength); + outPayload.rotatingDeviceId = std::string(rotatingDeviceIdBufferTemp, rotatingDeviceId.size() * 2); } else { diff --git a/src/setup_payload/tests/TestAdditionalDataPayload.cpp b/src/setup_payload/tests/TestAdditionalDataPayload.cpp index b2a06f6cadb2a6..694d256b92641e 100644 --- a/src/setup_payload/tests/TestAdditionalDataPayload.cpp +++ b/src/setup_payload/tests/TestAdditionalDataPayload.cpp @@ -47,8 +47,10 @@ constexpr char kAdditionalDataPayloadWithoutRotatingDeviceId[] = "1518 constexpr char kAdditionalDataPayloadWithRotatingDeviceId[] = "153000120A001998AB7130E38B7E9A401CFE9F7B79AF18"; constexpr char kAdditionalDataPayloadWithInvalidRotatingDeviceIdLength[] = "153000FF0A001998AB7130E38B7E9A401CFE9F7B79AF18"; constexpr char kAdditionalDataPayloadWithLongRotatingDeviceId[] = "153000130A00191998AB7130E38B7E9A401CFE9F7B79AF18"; +constexpr char kAdditionalDataPayloadWithShortRotatingDeviceId[] = "153000110A001998AB7130E38B7E9A401CFE9F7B7918"; constexpr char kAdditionalDataPayloadWithRotatingDeviceIdAndMaxLifetimeCounter[] = "15300012FFFFFC1670A9F9666D1C4587FCBC4811549018"; constexpr char kRotatingDeviceId[] = "0A001998AB7130E38B7E9A401CFE9F7B79AF"; +constexpr char kShortRotatingDeviceId[] = "0A001998AB7130E38B7E9A401CFE9F7B79"; constexpr uint16_t kLifetimeCounter = 10; constexpr uint16_t kAdditionalDataPayloadLength = 51; constexpr uint16_t kShortRotatingIdLength = 5; @@ -221,6 +223,16 @@ void TestParsingAdditionalDataPayloadWithLongRotatingDeviceId(nlTestSuite * inSu resultPayload) == CHIP_ERROR_INVALID_STRING_LENGTH); } +void TestParsingAdditionalDataPayloadWithShortRotatingDeviceId(nlTestSuite * inSuite, void * inContext) +{ + chip::SetupPayloadData::AdditionalDataPayload resultPayload; + NL_TEST_ASSERT(inSuite, + ParseAdditionalDataPayload(kAdditionalDataPayloadWithShortRotatingDeviceId, + strlen(kAdditionalDataPayloadWithShortRotatingDeviceId), + resultPayload) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, strcmp(resultPayload.rotatingDeviceId.c_str(), kShortRotatingDeviceId) == 0); +} + /** * Test Suite that lists all the Test functions. */ @@ -238,6 +250,7 @@ const nlTest sTests[] = NL_TEST_DEF("Test Parsing Additional Data Payload without Rotating Device Id", TestParsingAdditionalDataPayloadWithoutRotatingDeviceId), NL_TEST_DEF("Test Parsing Additional Data Payload with Invalid Rotating Device Id Length", TestParsingAdditionalDataPayloadWithInvalidRotatingDeviceIdLength), NL_TEST_DEF("Test Parsing Additional Data Payload with Long Rotating Device Id", TestParsingAdditionalDataPayloadWithLongRotatingDeviceId), + NL_TEST_DEF("Test Parsing Additional Data Payload with Short Rotating Device Id", TestParsingAdditionalDataPayloadWithShortRotatingDeviceId), NL_TEST_SENTINEL() }; // clang-format on