Skip to content

Commit

Permalink
Disable the CharSpan("literal") footgun (project-chip#30400)
Browse files Browse the repository at this point in the history
CharSpan("literal") includes the trailing 0 byte in the value which
is almost never what is intended.
  • Loading branch information
ksperling-apple authored Nov 10, 2023
1 parent dbd6d1d commit 6d9cb39
Show file tree
Hide file tree
Showing 9 changed files with 41 additions and 22 deletions.
2 changes: 1 addition & 1 deletion examples/all-clusters-app/nxp/mw320/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion examples/chef/common/stubs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion src/app/clusters/door-lock-server/door-lock-server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down
28 changes: 14 additions & 14 deletions src/app/tests/TestSceneTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/credentials/FabricTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
2 changes: 1 addition & 1 deletion src/credentials/tests/TestFabricTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 16 additions & 1 deletion src/lib/support/Span.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 <class U, size_t N, typename = std::enable_if_t<sizeof(U) == sizeof(T) && std::is_convertible<U *, T *>::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<!std::is_same_v<U, const char> && sizeof(U) == sizeof(T) && std::is_convertible_v<U *, T *>, 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<std::is_same_v<U, const char> && 1 == sizeof(T) && std::is_convertible_v<const char *, T *>, bool> = true>
constexpr explicit Span(U (&databuf)[N]) = delete;

// Creates a (potentially mutable) Span view of an std::array
template <class U, size_t N, typename = std::enable_if_t<sizeof(U) == sizeof(T) && std::is_convertible<U *, T *>::value>>
constexpr Span(std::array<U, N> & arr) : mDataBuf(arr.data()), mDataLen(N)
Expand Down
5 changes: 5 additions & 0 deletions src/lib/support/tests/TestSpan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 1 addition & 2 deletions src/lib/support/tests/TestTlvJson.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 6d9cb39

Please sign in to comment.