[meshcop] simplify processing of Commissioner ID TLV (#9543)

This commit updates CommissionerIdTlv to be defined as `StringTlvInfo`
(a TLV with a UTF-8 string value with a specified maximum length). This
allows us to use `Tlv` helper methods to `Find` and `Append` this
TLV, simplifying the code.

This commit also adds a helper `StringCopy()` method that copies a
C string into a given target string buffer array if it fits in the array.
This method can also optionally perform an encoding check on the string,
such as a UTF-8 encoding check. This helper method is used to simplify
setting different strings, such as Commissioner ID, Provisioning URL,
Vendor Name, etc.

src/core/common/string.cpp

@@ -160,6 +160,35 @@ bool StringMatch(const char *aFirstString, const char *aSecondString, StringMatc
     return Match(aFirstString, aSecondString, aMode) == kFullMatch;
 }
 
+Error StringCopy(char *aTargetBuffer, uint16_t aTargetSize, const char *aSource, StringEncodingCheck aEncodingCheck)
+{
+    Error    error = kErrorNone;
+    uint16_t length;
+
+    if (aSource == nullptr)
+    {
+        aTargetBuffer[0] = kNullChar;
+        ExitNow();
+    }
+
+    length = StringLength(aSource, aTargetSize);
+    VerifyOrExit(length < aTargetSize, error = kErrorInvalidArgs);
+
+    switch (aEncodingCheck)
+    {
+    case kStringNoEncodingCheck:
+        break;
+    case kStringCheckUtf8Encoding:
+        VerifyOrExit(IsValidUtf8String(aSource), error = kErrorParse);
+        break;
+    }
+
+    memcpy(aTargetBuffer, aSource, length + 1); // `+ 1` to also copy null char.
+
+exit:
+    return error;
+}
+
 Error StringParseUint8(const char *&aString, uint8_t &aUint8)
 {
     return StringParseUint8(aString, aUint8, NumericLimits<uint8_t>::kMax);

src/core/common/string.hpp

@@ -67,6 +67,16 @@ enum StringMatchMode : uint8_t
     kStringCaseInsensitiveMatch, ///< Case insensitive match (uppercase and lowercase characters are treated as equal).
 };
 
+/**
+ * Represents string encoding check when copying string.
+ *
+ */
+enum StringEncodingCheck : uint8_t
+{
+    kStringNoEncodingCheck,   ///< Do not check the string encoding.
+    kStringCheckUtf8Encoding, ///< Validate that string follows UTF-8 encoding.
+};
+
 static constexpr char kNullChar = '\0'; ///< null character.
 
 /**
@@ -156,6 +166,43 @@ bool StringEndsWith(const char *aString, const char *aSubString, StringMatchMode
  */
 bool StringMatch(const char *aFirstString, const char *aSecondString, StringMatchMode aMode = kStringExactMatch);
 
+/**
+ * Copies a string into a given target buffer with a given size if it fits.
+ *
+ * @param[out] aTargetBuffer  A pointer to the target buffer to copy into.
+ * @param[out] aTargetSize    The size (number of characters) in @p aTargetBuffer array.
+ * @param[in]  aSource        A pointer to null-terminated string to copy from. Can be `nullptr` which treated as "".
+ * @param[in]  aEncodingCheck Specifies the encoding format check (e.g., UTF-8) to perform.
+ *
+ * @retval kErrorNone         The @p aSource fits in the given buffer. @p aTargetBuffer is updated.
+ * @retval kErrorInvalidArgs  The @p aSource does not fit in the given buffer.
+ * @retval kErrorParse        The @p aSource does not follow the encoding format specified by @p aEncodingCheck.
+ *
+ */
+Error StringCopy(char *TargetBuffer, uint16_t aTargetSize, const char *aSource, StringEncodingCheck aEncodingCheck);
+
+/**
+ * Copies a string into a given target buffer with a given size if it fits.
+ *
+ * @tparam kSize  The size of buffer.
+ *
+ * @param[out] aTargetBuffer  A reference to the target buffer array to copy into.
+ * @param[in]  aSource        A pointer to null-terminated string to copy from. Can be `nullptr` which treated as "".
+ * @param[in]  aEncodingCheck Specifies the encoding format check (e.g., UTF-8) to perform.
+ *
+ * @retval kErrorNone         The @p aSource fits in the given buffer. @p aTargetBuffer is updated.
+ * @retval kErrorInvalidArgs  The @p aSource does not fit in the given buffer.
+ * @retval kErrorParse        The @p aSource does not follow the encoding format specified by @p aEncodingCheck.
+ *
+ */
+template <uint16_t kSize>
+Error StringCopy(char (&aTargetBuffer)[kSize],
+                 const char         *aSource,
+                 StringEncodingCheck aEncodingCheck = kStringNoEncodingCheck)
+{
+    return StringCopy(aTargetBuffer, kSize, aSource, aEncodingCheck);
+}
+
 /**
  * Parses a decimal number from a string as `uint8_t` and skips over the parsed characters.
  *

src/core/meshcop/commissioner.cpp

@@ -349,21 +349,10 @@ exit:
 
 Error Commissioner::SetId(const char *aId)
 {
-    Error   error = kErrorNone;
-    uint8_t len;
+    Error error = kErrorNone;
 
     VerifyOrExit(IsDisabled(), error = kErrorInvalidState);
-    VerifyOrExit(aId != nullptr);
-    VerifyOrExit(IsValidUtf8String(aId), error = kErrorInvalidArgs);
-
-    len = static_cast<uint8_t>(StringLength(aId, sizeof(mCommissionerId)));
-
-    // CommissionerIdTlv::SetCommissionerId trims the string to the maximum array size.
-    // Prevent this from happening returning an error.
-    VerifyOrExit(len < CommissionerIdTlv::kMaxLength, error = kErrorInvalidArgs);
-
-    memcpy(mCommissionerId, aId, len);
-    mCommissionerId[len] = '\0';
+    error = StringCopy(mCommissionerId, aId, kStringCheckUtf8Encoding);
 
 exit:
     return error;
@@ -582,26 +571,7 @@ void Commissioner::RemoveJoiner(Joiner &aJoiner, uint32_t aDelay)
 
 Error Commissioner::SetProvisioningUrl(const char *aProvisioningUrl)
 {
-    Error   error = kErrorNone;
-    uint8_t len;
-
-    if (aProvisioningUrl == nullptr)
-    {
-        mProvisioningUrl[0] = '\0';
-        ExitNow();
-    }
-
-    VerifyOrExit(IsValidUtf8String(aProvisioningUrl), error = kErrorInvalidArgs);
-
-    len = static_cast<uint8_t>(StringLength(aProvisioningUrl, sizeof(mProvisioningUrl)));
-
-    VerifyOrExit(len < sizeof(mProvisioningUrl), error = kErrorInvalidArgs);
-
-    memcpy(mProvisioningUrl, aProvisioningUrl, len);
-    mProvisioningUrl[len] = '\0';
-
-exit:
-    return error;
+    return StringCopy(mProvisioningUrl, aProvisioningUrl, kStringCheckUtf8Encoding);
 }
 
 void Commissioner::HandleTimer(void)
@@ -779,20 +749,17 @@ exit:
 
 Error Commissioner::SendPetition(void)
 {
-    Error             error   = kErrorNone;
-    Coap::Message    *message = nullptr;
-    Tmf::MessageInfo  messageInfo(GetInstance());
-    CommissionerIdTlv commissionerIdTlv;
+    Error            error   = kErrorNone;
+    Coap::Message   *message = nullptr;
+    Tmf::MessageInfo messageInfo(GetInstance());
 
     mTransmitAttempts++;
 
     message = Get<Tmf::Agent>().NewPriorityConfirmablePostMessage(kUriLeaderPetition);
     VerifyOrExit(message != nullptr, error = kErrorNoBufs);
 
-    commissionerIdTlv.Init();
-    commissionerIdTlv.SetCommissionerId(mCommissionerId);
+    SuccessOrExit(error = Tlv::Append<CommissionerIdTlv>(*message, mCommissionerId));
 
-    SuccessOrExit(error = commissionerIdTlv.AppendTo(*message));
     SuccessOrExit(error = messageInfo.SetSockAddrToRlocPeerAddrToLeaderAloc());
     SuccessOrExit(
         error = Get<Tmf::Agent>().SendMessage(*message, messageInfo, Commissioner::HandleLeaderPetitionResponse, this));

src/core/meshcop/commissioner.hpp

@@ -616,8 +616,8 @@ private:
 
     Ip6::Netif::UnicastAddress mCommissionerAloc;
 
-    char mProvisioningUrl[OT_PROVISIONING_URL_MAX_SIZE + 1]; // + 1 is for null char at end of string.
-    char mCommissionerId[CommissionerIdTlv::kMaxLength + 1];
+    ProvisioningUrlTlv::StringType mProvisioningUrl;
+    CommissionerIdTlv::StringType  mCommissionerId;
 
     State mState;
 

src/core/meshcop/meshcop_leader.cpp

@@ -67,35 +67,28 @@ template <> void Leader::HandleTmf<kUriLeaderPetition>(Coap::Message &aMessage,
 {
     OT_UNUSED_VARIABLE(aMessageInfo);
 
-    CommissioningData data;
-    CommissionerIdTlv commissionerId;
-    StateTlv::State   state = StateTlv::kReject;
+    CommissioningData             data;
+    CommissionerIdTlv::StringType commissionerId;
+    StateTlv::State               state = StateTlv::kReject;
 
     LogInfo("Received %s", UriToString<kUriLeaderPetition>());
 
     VerifyOrExit(Get<Mle::MleRouter>().IsLeader());
 
     VerifyOrExit(Get<Mle::MleRouter>().IsRoutingLocator(aMessageInfo.GetPeerAddr()));
-    SuccessOrExit(Tlv::FindTlv(aMessage, commissionerId));
+
+    SuccessOrExit(Tlv::Find<CommissionerIdTlv>(aMessage, commissionerId));
 
     if (mTimer.IsRunning())
     {
-        VerifyOrExit((commissionerId.GetCommissionerIdLength() == mCommissionerId.GetCommissionerIdLength()) &&
-                     (!strncmp(commissionerId.GetCommissionerId(), mCommissionerId.GetCommissionerId(),
-                               commissionerId.GetCommissionerIdLength())));
-
+        VerifyOrExit(StringMatch(mCommissionerId, commissionerId));
         ResignCommissioner();
     }
 
     data.Init(aMessageInfo.GetPeerAddr().GetIid().GetLocator(), ++mSessionId);
     SuccessOrExit(Get<NetworkData::Leader>().SetCommissioningData(&data, data.GetLength()));
 
-    mCommissionerId = commissionerId;
-
-    if (mCommissionerId.GetLength() > CommissionerIdTlv::kMaxLength)
-    {
-        mCommissionerId.SetLength(CommissionerIdTlv::kMaxLength);
-    }
+    IgnoreError(StringCopy(mCommissionerId, commissionerId));
 
     state = StateTlv::kAccept;
     mTimer.Start(Time::SecToMsec(kTimeoutLeaderPetition));
@@ -118,7 +111,7 @@ void Leader::SendPetitionResponse(const Coap::Message    &aRequest,
 
     if (mTimer.IsRunning())
     {
-        SuccessOrExit(error = mCommissionerId.AppendTo(*message));
+        SuccessOrExit(error = Tlv::Append<CommissionerIdTlv>(*message, mCommissionerId));
     }
 
     if (aState == StateTlv::kAccept)

src/core/meshcop/meshcop_leader.hpp

@@ -133,8 +133,8 @@ private:
 
     uint32_t mDelayTimerMinimal;
 
-    CommissionerIdTlv mCommissionerId;
-    uint16_t          mSessionId;
+    CommissionerIdTlv::StringType mCommissionerId;
+    uint16_t                      mSessionId;
 };
 
 DeclareTmfHandler(Leader, kUriLeaderPetition);

src/core/meshcop/meshcop_tlvs.hpp

@@ -126,6 +126,7 @@ public:
      */
     static constexpr uint8_t kMaxProvisioningUrlLength = OT_PROVISIONING_URL_MAX_SIZE;
 
+    static constexpr uint8_t kMaxCommissionerIdLength  = 64; ///< Max length of Commissioner ID TLV.
     static constexpr uint8_t kMaxVendorNameLength      = 32; ///< Max length of Vendor Name TLV.
     static constexpr uint8_t kMaxVendorModelLength     = 32; ///< Max length of Vendor Model TLV.
     static constexpr uint8_t kMaxVendorSwVersionLength = 16; ///< Max length of Vendor SW Version TLV.
@@ -248,6 +249,12 @@ typedef UintTlvInfo<Tlv::kPeriod, uint16_t> PeriodTlv;
  */
 typedef UintTlvInfo<Tlv::kScanDuration, uint16_t> ScanDurationTlv;
 
+/**
+ * Defines Commissioner ID TLV constants and type.s
+ *
+ */
+typedef StringTlvInfo<Tlv::kCommissionerId, Tlv::kMaxCommissionerIdLength> CommissionerIdTlv;
+
 /**
  * Implements Channel TLV generation and parsing.
  *
@@ -756,62 +763,6 @@ private:
     uint16_t mLocator;
 } OT_TOOL_PACKED_END;
 
-/**
- * Implements the Commissioner ID TLV generation and parsing.
- *
- */
-OT_TOOL_PACKED_BEGIN
-class CommissionerIdTlv : public Tlv, public TlvInfo<Tlv::kCommissionerId>
-{
-public:
-    static constexpr uint8_t kMaxLength = 64; ///< maximum length (bytes)
-
-    /**
-     * Initializes the TLV.
-     *
-     */
-    void Init(void)
-    {
-        SetType(kCommissionerId);
-        SetLength(sizeof(*this) - sizeof(Tlv));
-    }
-
-    /**
-     * Returns the Commissioner ID length.
-     *
-     * @returns The Commissioner ID length.
-     *
-     */
-    uint8_t GetCommissionerIdLength(void) const
-    {
-        return GetLength() <= sizeof(mCommissionerId) ? GetLength() : sizeof(mCommissionerId);
-    }
-
-    /**
-     * Returns the Commissioner ID value.
-     *
-     * @returns The Commissioner ID value.
-     *
-     */
-    const char *GetCommissionerId(void) const { return mCommissionerId; }
-
-    /**
-     * Sets the Commissioner ID value.
-     *
-     * @param[in]  aCommissionerId  A pointer to the Commissioner ID value.
-     *
-     */
-    void SetCommissionerId(const char *aCommissionerId)
-    {
-        uint16_t length = StringLength(aCommissionerId, sizeof(mCommissionerId));
-        memcpy(mCommissionerId, aCommissionerId, length);
-        SetLength(static_cast<uint8_t>(length));
-    }
-
-private:
-    char mCommissionerId[kMaxLength];
-} OT_TOOL_PACKED_END;
-
 /**
  * Implements Commissioner Session ID TLV generation and parsing.
  *

src/core/thread/network_diagnostic.cpp

@@ -82,39 +82,20 @@ Server::Server(Instance &aInstance)
 
 Error Server::SetVendorName(const char *aVendorName)
 {
-    return SetVendorString(mVendorName, sizeof(mVendorName), aVendorName);
+    return StringCopy(mVendorName, aVendorName, kStringCheckUtf8Encoding);
 }
 
 Error Server::SetVendorModel(const char *aVendorModel)
 {
-    return SetVendorString(mVendorModel, sizeof(mVendorModel), aVendorModel);
+    return StringCopy(mVendorModel, aVendorModel, kStringCheckUtf8Encoding);
 }
 
 Error Server::SetVendorSwVersion(const char *aVendorSwVersion)
 {
-    return SetVendorString(mVendorSwVersion, sizeof(mVendorSwVersion), aVendorSwVersion);
+    return StringCopy(mVendorSwVersion, aVendorSwVersion, kStringCheckUtf8Encoding);
 }
 
-Error Server::SetVendorString(char *aDestString, uint16_t kMaxSize, const char *aSrcString)
-{
-    Error    error = kErrorInvalidArgs;
-    uint16_t length;
-
-    VerifyOrExit(aSrcString != nullptr);
-
-    length = StringLength(aSrcString, kMaxSize);
-    VerifyOrExit(length < kMaxSize);
-
-    VerifyOrExit(IsValidUtf8String(aSrcString));
-
-    memcpy(aDestString, aSrcString, length + 1);
-    error = kErrorNone;
-
-exit:
-    return error;
-}
-
-#endif // OPENTHREAD_CONFIG_NET_DIAG_VENDOR_INFO_SET_API_ENABLE
+#endif
 
 void Server::PrepareMessageInfoForDest(const Ip6::Address &aDestination, Tmf::MessageInfo &aMessageInfo) const
 {

src/core/thread/network_diagnostic.hpp

@@ -208,8 +208,6 @@ private:
     template <Uri kUri> void HandleTmf(Coap::Message &aMessage, const Ip6::MessageInfo &aMessageInfo);
 
 #if OPENTHREAD_CONFIG_NET_DIAG_VENDOR_INFO_SET_API_ENABLE
-    Error SetVendorString(char *aDestString, uint16_t kMaxSize, const char *aSrcString);
-
     VendorNameTlv::StringType      mVendorName;
     VendorModelTlv::StringType     mVendorModel;
     VendorSwVersionTlv::StringType mVendorSwVersion;

tests/scripts/expect/cli-commissioner.exp

@@ -52,9 +52,9 @@ expect_line "Done"
 send "commissioner id\n"
 expect "OpenThread Commissioner"
 expect_line "Done"
-send "commissioner id reallyAndUnnecessaryLongOpenthreadComisionerCustomIdShouldNotFit\n"
+send "commissioner id 12345678901234567890123456789012345678901234567890123456789012345\n"
 expect "Error 7: InvalidArgs"
-send "commissioner id reallyAndUnnecessaryLongOpenthreadCommissionerCustomIdShouldFit\n"
+send "commissioner id 1234567890123456789012345678901234567890123456789012345678901234\n"
 expect "Done"
 send "commissioner id customId\n"
 expect "Done"

tests/unit/test_string.cpp

@@ -366,6 +366,36 @@ void TestStringParseUint8(void)
     printf("\n\n -- PASS\n");
 }
 
+void TestStringCopy(void)
+{
+    char buffer[10];
+    char smallBuffer[1];
+
+    printf("\nTest 11: StringCopy() function\n");
+
+    SuccessOrQuit(StringCopy(buffer, "foo", kStringCheckUtf8Encoding));
+    VerifyOrQuit(StringMatch(buffer, "foo"));
+
+    SuccessOrQuit(StringCopy(buffer, nullptr, kStringCheckUtf8Encoding));
+    VerifyOrQuit(StringMatch(buffer, ""));
+
+    SuccessOrQuit(StringCopy(buffer, "", kStringCheckUtf8Encoding));
+    VerifyOrQuit(StringMatch(buffer, ""));
+
+    SuccessOrQuit(StringCopy(buffer, "123456789", kStringCheckUtf8Encoding));
+    VerifyOrQuit(StringMatch(buffer, "123456789"));
+
+    VerifyOrQuit(StringCopy(buffer, "1234567890") == kErrorInvalidArgs);
+    VerifyOrQuit(StringCopy(buffer, "1234567890abcdef") == kErrorInvalidArgs);
+
+    SuccessOrQuit(StringCopy(smallBuffer, "", kStringCheckUtf8Encoding));
+    VerifyOrQuit(StringMatch(smallBuffer, ""));
+
+    VerifyOrQuit(StringCopy(smallBuffer, "a") == kErrorInvalidArgs);
+
+    printf(" -- PASS\n");
+}
+
 // gcc-4 does not support constexpr function
 #if __GNUC__ > 4
 static_assert(ot::AreStringsInOrder("a", "b"), "AreStringsInOrder() failed");
@@ -389,6 +419,7 @@ int main(void)
     ot::TestStringMatch();
     ot::TestStringToLowercase();
     ot::TestStringParseUint8();
+    ot::TestStringCopy();
     printf("\nAll tests passed.\n");
     return 0;
 }