From 6d9cb39aaac264bd22333b0ebff7ebda8d2f10ce Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Fri, 10 Nov 2023 16:13:58 +1300 Subject: [PATCH] Disable the CharSpan("literal") footgun (#30400) CharSpan("literal") includes the trailing 0 byte in the value which is almost never what is intended. --- examples/all-clusters-app/nxp/mw320/main.cpp | 2 +- examples/chef/common/stubs.cpp | 2 +- .../door-lock-server/door-lock-server.cpp | 2 +- src/app/tests/TestSceneTable.cpp | 28 +++++++++---------- src/credentials/FabricTable.cpp | 2 +- src/credentials/tests/TestFabricTable.cpp | 2 +- src/lib/support/Span.h | 17 ++++++++++- src/lib/support/tests/TestSpan.cpp | 5 ++++ src/lib/support/tests/TestTlvJson.cpp | 3 +- 9 files changed, 41 insertions(+), 22 deletions(-) diff --git a/examples/all-clusters-app/nxp/mw320/main.cpp b/examples/all-clusters-app/nxp/mw320/main.cpp index 8344be7cda1b80..5e29a11cc9b290 100644 --- a/examples/all-clusters-app/nxp/mw320/main.cpp +++ b/examples/all-clusters-app/nxp/mw320/main.cpp @@ -172,7 +172,7 @@ void InitOTARequestor(void) // TODO: instatiate and initialize these values when QueryImageResponse tells us an image is available // TODO: add API for OTARequestor to pass QueryImageResponse info to the application to use for OTADownloader init // OTAImageProcessor ipParams; - // ipParams.imageFile = CharSpan("dnld_img.txt"); + // ipParams.imageFile = "dnld_img.txt"_span; // gImageProcessor.SetOTAImageProcessorParams(ipParams); gImageProcessor.SetOTADownloader(&gDownloader); diff --git a/examples/chef/common/stubs.cpp b/examples/chef/common/stubs.cpp index f686815b2bf9d2..b5878496182a0b 100644 --- a/examples/chef/common/stubs.cpp +++ b/examples/chef/common/stubs.cpp @@ -186,7 +186,7 @@ class LockManager endpoints[0].id = 1; uint8_t pin[6] = { 0x31, 0x32, 0x33, 0x34, 0x35, 0x36 }; endpoints[0].credentials[0].set(DlCredentialStatus::kOccupied, CredentialTypeEnum::kPin, chip::ByteSpan(pin)); - endpoints[0].users[0].set(chip::CharSpan("default"), 1, UserStatusEnum::kOccupiedEnabled, UserTypeEnum::kUnrestrictedUser, + endpoints[0].users[0].set("default"_span, 1, UserStatusEnum::kOccupiedEnabled, UserTypeEnum::kUnrestrictedUser, CredentialRuleEnum::kSingle); endpoints[0].users[0].addCredential(CredentialTypeEnum::kPin, 1); } diff --git a/src/app/clusters/door-lock-server/door-lock-server.cpp b/src/app/clusters/door-lock-server/door-lock-server.cpp index 3912b022f90065..aa2be853f6277c 100644 --- a/src/app/clusters/door-lock-server/door-lock-server.cpp +++ b/src/app/clusters/door-lock-server/door-lock-server.cpp @@ -1985,7 +1985,7 @@ Status DoorLockServer::clearUser(chip::EndpointId endpointId, chip::FabricIndex } // Remove the user entry - if (!emberAfPluginDoorLockSetUser(endpointId, userIndex, kUndefinedFabricIndex, kUndefinedFabricIndex, chip::CharSpan(""), 0, + if (!emberAfPluginDoorLockSetUser(endpointId, userIndex, kUndefinedFabricIndex, kUndefinedFabricIndex, ""_span, 0, UserStatusEnum::kAvailable, UserTypeEnum::kUnrestrictedUser, CredentialRuleEnum::kSingle, nullptr, 0)) { diff --git a/src/app/tests/TestSceneTable.cpp b/src/app/tests/TestSceneTable.cpp index 08ada73b3729a1..743489450c7a8f 100644 --- a/src/app/tests/TestSceneTable.cpp +++ b/src/app/tests/TestSceneTable.cpp @@ -102,21 +102,21 @@ static const SceneStorageId sceneId12(kScene6, kGroup4); CharSpan empty; // Scene data -static const SceneData sceneData1(CharSpan("Scene #1")); -static const SceneData sceneData2(CharSpan("Scene #2"), 2000); -static const SceneData sceneData3(CharSpan("Scene #3"), 250); -static const SceneData sceneData4(CharSpan("Scene num4"), 5000); +static const SceneData sceneData1("Scene #1"_span); +static const SceneData sceneData2("Scene #2"_span, 2000); +static const SceneData sceneData3("Scene #3"_span, 250); +static const SceneData sceneData4("Scene num4"_span, 5000); static const SceneData sceneData5(empty); -static const SceneData sceneData6(CharSpan("Scene #6"), 3000); -static const SceneData sceneData7(CharSpan("Scene #7"), 20000); -static const SceneData sceneData8(CharSpan("NAME TOO LOOONNG!"), 15000); -static const SceneData sceneData9(CharSpan("Scene #9"), 3000); -static const SceneData sceneData10(CharSpan("Scene #10"), 1000); -static const SceneData sceneData11(CharSpan("Scene #11"), 50); -static const SceneData sceneData12(CharSpan("Scene #12"), 100); -static const SceneData sceneData13(CharSpan("Scene #13"), 100); -static const SceneData sceneData14(CharSpan("Scene #14"), 100); -static const SceneData sceneData15(CharSpan("Scene #15"), 100); +static const SceneData sceneData6("Scene #6"_span, 3000); +static const SceneData sceneData7("Scene #7"_span, 20000); +static const SceneData sceneData8("NAME TOO LOOONNG!"_span, 15000); +static const SceneData sceneData9("Scene #9"_span, 3000); +static const SceneData sceneData10("Scene #10"_span, 1000); +static const SceneData sceneData11("Scene #11"_span, 50); +static const SceneData sceneData12("Scene #12"_span, 100); +static const SceneData sceneData13("Scene #13"_span, 100); +static const SceneData sceneData14("Scene #14"_span, 100); +static const SceneData sceneData15("Scene #15"_span, 100); // Scenes SceneTableEntry scene1(sceneId1, sceneData1); diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index 474d67959db1f2..0091fdec291c30 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -782,7 +782,7 @@ FabricTable::AddOrUpdateInner(FabricIndex fabricIndex, bool isAddition, Crypto:: FabricInfo::InitParams newFabricInfo; FabricInfo * fabricEntry = nullptr; FabricId fabricIdToValidate = kUndefinedFabricId; - CharSpan fabricLabel(""); + CharSpan fabricLabel; if (isAddition) { diff --git a/src/credentials/tests/TestFabricTable.cpp b/src/credentials/tests/TestFabricTable.cpp index 2226b399e75bce..8dd65192441aea 100644 --- a/src/credentials/tests/TestFabricTable.cpp +++ b/src/credentials/tests/TestFabricTable.cpp @@ -943,7 +943,7 @@ void TestBasicAddNocUpdateNocFlow(nlTestSuite * inSuite, void * inContext) // Sequence 4: Rename fabric index 2, applies immediately when nothing pending { NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 2); - NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SetFabricLabel(2, CharSpan("roboto"))); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SetFabricLabel(2, "roboto"_span)); NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 2); NL_TEST_ASSERT_EQUALS(inSuite, storage.GetNumKeys(), numStorageAfterUpdate); // Number of keys unchanged diff --git a/src/lib/support/Span.h b/src/lib/support/Span.h index 7b7bcb8cd48b1f..2554cd4a695219 100644 --- a/src/lib/support/Span.h +++ b/src/lib/support/Span.h @@ -58,10 +58,25 @@ class Span // should be used to construct empty Spans. All other cases involving null are invalid. Span(std::nullptr_t null, size_t size) = delete; - template ::value>> + // Creates a Span view of a plain array. + // + // Note that this constructor template explicitly excludes `const char[]`, see below. + template < + class U, size_t N, + std::enable_if_t && sizeof(U) == sizeof(T) && std::is_convertible_v, bool> = true> constexpr explicit Span(U (&databuf)[N]) : mDataBuf(databuf), mDataLen(N) {} + // Explicitly disallow the creation of a CharSpan from a `const char[]` to prevent the + // creation of spans from string literals that incorrectly include the trailing '\0' byte. + // If CharSpan("Hi!") were allowed, it would be a span of length 4, not 3 as intended. + // + // To create a CharSpan literal, use the `_span` operator instead, e.g. "Hi!"_span. + template < + class U, size_t N, + std::enable_if_t && 1 == sizeof(T) && std::is_convertible_v, bool> = true> + constexpr explicit Span(U (&databuf)[N]) = delete; + // Creates a (potentially mutable) Span view of an std::array template ::value>> constexpr Span(std::array & arr) : mDataBuf(arr.data()), mDataLen(N) diff --git a/src/lib/support/tests/TestSpan.cpp b/src/lib/support/tests/TestSpan.cpp index 6490755ca57d7d..e1ff254a35b0a0 100644 --- a/src/lib/support/tests/TestSpan.cpp +++ b/src/lib/support/tests/TestSpan.cpp @@ -299,6 +299,11 @@ static void TestLiteral(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, literal.size() == 3); NL_TEST_ASSERT(inSuite, literal.data_equal(CharSpan::fromCharString("HI!"))); NL_TEST_ASSERT(inSuite, ""_span.size() == 0); + + // These should be compile errors -- if they were allowed they would produce + // a CharSpan that includes the trailing '\0' byte in the value. + // constexpr CharSpan disallowed1("abcd"); + // constexpr CharSpan disallowed2{ "abcd" }; } static void TestConversionConstructors(nlTestSuite * inSuite, void * inContext) diff --git a/src/lib/support/tests/TestTlvJson.cpp b/src/lib/support/tests/TestTlvJson.cpp index 95a8967820317e..9d9a91e243a3c1 100644 --- a/src/lib/support/tests/TestTlvJson.cpp +++ b/src/lib/support/tests/TestTlvJson.cpp @@ -133,8 +133,7 @@ void TestConverter(nlTestSuite * inSuite, void * inContext) " \"value\" : 1.0\n" "}\n"); - const char charBuf[] = "hello"; - CharSpan charSpan(charBuf); + CharSpan charSpan = "hello"_span; EncodeAndValidate(charSpan, "{\n" " \"value\" : \"hello\"\n"