Simplify AutoCommissioner::SetCommissioningParameters (project-chip#36896)

Unconditionally clear members that point to buffers, and handle them via the
explicit code that copies the pointed-to data. The logic to work out whether
any pointers need to be cleared was quite complicated.

Use memmove() instead of memcpy() since src and dst may overlap / be identical.

Always assign mNeedIcdRegistration when the kReadCommissioningInfo step
finishes, instead of relying on SetCommissioningParameters to clear it.

Author: ksperling-apple

src/controller/AutoCommissioner.cpp

@@ -49,25 +49,6 @@ void AutoCommissioner::SetOperationalCredentialsDelegate(OperationalCredentialsD
     mOperationalCredentialsDelegate = operationalCredentialsDelegate;
 }
 
-// Returns true if maybeUnsafeSpan is pointing to a buffer that we're not sure
-// will live for long enough.  knownSafeSpan, if it has a value, points to a
-// buffer that we _are_ sure will live for long enough.
-template <typename SpanType>
-static bool IsUnsafeSpan(const Optional<SpanType> & maybeUnsafeSpan, const Optional<SpanType> & knownSafeSpan)
-{
-    if (!maybeUnsafeSpan.HasValue())
-    {
-        return false;
-    }
-
-    if (!knownSafeSpan.HasValue())
-    {
-        return true;
-    }
-
-    return maybeUnsafeSpan.Value().data() != knownSafeSpan.Value().data();
-}
-
 CHIP_ERROR AutoCommissioner::VerifyICDRegistrationInfo(const CommissioningParameters & params)
 {
     ChipLogProgress(Controller, "Checking ICD registration parameters");
@@ -101,56 +82,26 @@ CHIP_ERROR AutoCommissioner::VerifyICDRegistrationInfo(const CommissioningParame
 
 CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParameters & params)
 {
-    // Make sure any members that point to buffers that we are not pointing to
-    // our own buffers are not going to dangle.  We can skip this step if all
-    // the buffers pointers that we don't plan to re-point to our own buffers
-    // below are already pointing to the same things as our own buffer pointers
-    // (so that we know they have to be safe somehow).
-    //
-    // The checks are a bit painful, because Span does not have a usable
-    // operator==, and in any case, we want to compare for pointer equality, not
-    // data equality.
-    bool haveMaybeDanglingBufferPointers =
-        ((params.GetNOCChainGenerationParameters().HasValue() &&
-          (!mParams.GetNOCChainGenerationParameters().HasValue() ||
-           params.GetNOCChainGenerationParameters().Value().nocsrElements.data() !=
-               mParams.GetNOCChainGenerationParameters().Value().nocsrElements.data() ||
-           params.GetNOCChainGenerationParameters().Value().signature.data() !=
-               mParams.GetNOCChainGenerationParameters().Value().signature.data())) ||
-         IsUnsafeSpan(params.GetRootCert(), mParams.GetRootCert()) || IsUnsafeSpan(params.GetNoc(), mParams.GetNoc()) ||
-         IsUnsafeSpan(params.GetIcac(), mParams.GetIcac()) || IsUnsafeSpan(params.GetIpk(), mParams.GetIpk()) ||
-         IsUnsafeSpan(params.GetAttestationElements(), mParams.GetAttestationElements()) ||
-         IsUnsafeSpan(params.GetAttestationSignature(), mParams.GetAttestationSignature()) ||
-         IsUnsafeSpan(params.GetPAI(), mParams.GetPAI()) || IsUnsafeSpan(params.GetDAC(), mParams.GetDAC()) ||
-         IsUnsafeSpan(params.GetTimeZone(), mParams.GetTimeZone()) ||
-         IsUnsafeSpan(params.GetDSTOffsets(), mParams.GetDSTOffsets()) ||
-         IsUnsafeSpan(params.GetICDSymmetricKey(), mParams.GetICDSymmetricKey()) ||
-         (params.GetDefaultNTP().HasValue() && !params.GetDefaultNTP().Value().IsNull() &&
-          params.GetDefaultNTP().Value().Value().data() != mDefaultNtp));
-
+    // Our logic below assumes that we can modify mParams without affecting params.
+    VerifyOrReturnError(&params != &mParams, CHIP_NO_ERROR);
+
+    // Copy the whole struct (scalars and pointers), but clear any members that might point to
+    // external buffers. For those members we have to copy the data over into our own buffers below.
+    // Note that all of the copy operations use memmove() instead of memcpy(), because the caller
+    // may be passing a modified shallow copy of our CommissioningParmeters, i.e. where various spans
+    // already point into the buffers we're copying into, and memcpy() with overlapping buffers is UB.
     mParams = params;
-
-    mNeedIcdRegistration = false;
-
-    if (haveMaybeDanglingBufferPointers)
-    {
-        mParams.ClearExternalBufferDependentValues();
-    }
-
-    // For members of params that point to some sort of buffer, we have to copy
-    // the data over into our own buffers.
+    mParams.ClearExternalBufferDependentValues();
 
     if (params.GetThreadOperationalDataset().HasValue())
     {
         ByteSpan dataset = params.GetThreadOperationalDataset().Value();
         if (dataset.size() > CommissioningParameters::kMaxThreadDatasetLen)
         {
             ChipLogError(Controller, "Thread operational data set is too large");
-            // Make sure our buffer pointers don't dangle.
-            mParams.ClearExternalBufferDependentValues();
             return CHIP_ERROR_INVALID_ARGUMENT;
         }
-        memcpy(mThreadOperationalDataset, dataset.data(), dataset.size());
+        memmove(mThreadOperationalDataset, dataset.data(), dataset.size());
         ChipLogProgress(Controller, "Setting thread operational dataset from parameters");
         mParams.SetThreadOperationalDataset(ByteSpan(mThreadOperationalDataset, dataset.size()));
     }
@@ -162,12 +113,10 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
             creds.credentials.size() > CommissioningParameters::kMaxCredentialsLen)
         {
             ChipLogError(Controller, "Wifi credentials are too large");
-            // Make sure our buffer pointers don't dangle.
-            mParams.ClearExternalBufferDependentValues();
             return CHIP_ERROR_INVALID_ARGUMENT;
         }
-        memcpy(mSsid, creds.ssid.data(), creds.ssid.size());
-        memcpy(mCredentials, creds.credentials.data(), creds.credentials.size());
+        memmove(mSsid, creds.ssid.data(), creds.ssid.size());
+        memmove(mCredentials, creds.credentials.data(), creds.credentials.size());
         ChipLogProgress(Controller, "Setting wifi credentials from parameters");
         mParams.SetWiFiCredentials(
             WiFiCredentials(ByteSpan(mSsid, creds.ssid.size()), ByteSpan(mCredentials, creds.credentials.size())));
@@ -184,8 +133,6 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
         else
         {
             ChipLogError(Controller, "Country code is too large: %u", static_cast<unsigned>(code.size()));
-            // Make sure our buffer pointers don't dangle.
-            mParams.ClearExternalBufferDependentValues();
             return CHIP_ERROR_INVALID_ARGUMENT;
         }
     }
@@ -195,7 +142,7 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
     {
         ChipLogProgress(Controller, "Setting attestation nonce from parameters");
         VerifyOrReturnError(params.GetAttestationNonce().Value().size() == sizeof(mAttestationNonce), CHIP_ERROR_INVALID_ARGUMENT);
-        memcpy(mAttestationNonce, params.GetAttestationNonce().Value().data(), params.GetAttestationNonce().Value().size());
+        memmove(mAttestationNonce, params.GetAttestationNonce().Value().data(), params.GetAttestationNonce().Value().size());
     }
     else
     {
@@ -208,7 +155,7 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
     {
         ChipLogProgress(Controller, "Setting CSR nonce from parameters");
         VerifyOrReturnError(params.GetCSRNonce().Value().size() == sizeof(mCSRNonce), CHIP_ERROR_INVALID_ARGUMENT);
-        memcpy(mCSRNonce, params.GetCSRNonce().Value().data(), params.GetCSRNonce().Value().size());
+        memmove(mCSRNonce, params.GetCSRNonce().Value().data(), params.GetCSRNonce().Value().size());
     }
     else
     {
@@ -271,7 +218,7 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
         ReturnErrorOnFailure(VerifyICDRegistrationInfo(params));
 
         // The values must be valid now.
-        memcpy(mICDSymmetricKey, params.GetICDSymmetricKey().Value().data(), params.GetICDSymmetricKey().Value().size());
+        memmove(mICDSymmetricKey, params.GetICDSymmetricKey().Value().data(), params.GetICDSymmetricKey().Value().size());
         mParams.SetICDSymmetricKey(ByteSpan(mICDSymmetricKey));
         mParams.SetICDCheckInNodeId(params.GetICDCheckInNodeId().Value());
         mParams.SetICDMonitoredSubject(params.GetICDMonitoredSubject().Value());
@@ -787,6 +734,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
                 }
             }
 
+            mNeedIcdRegistration = false;
             if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore)
             {
                 if (mDeviceCommissioningInfo.icd.isLIT && mDeviceCommissioningInfo.icd.checkInProtocolSupport)

src/lib/support/Span.h

@@ -374,7 +374,8 @@ inline CHIP_ERROR CopySpanToMutableSpan(ByteSpan span_to_copy, MutableByteSpan &
 {
     VerifyOrReturnError(out_buf.size() >= span_to_copy.size(), CHIP_ERROR_BUFFER_TOO_SMALL);
 
-    memcpy(out_buf.data(), span_to_copy.data(), span_to_copy.size());
+    // There is no guarantee that span_to_copy and out_buf don't overlap, so use memmove()
+    memmove(out_buf.data(), span_to_copy.data(), span_to_copy.size());
     out_buf.reduce_size(span_to_copy.size());
 
     return CHIP_NO_ERROR;
@@ -384,7 +385,8 @@ inline CHIP_ERROR CopyCharSpanToMutableCharSpan(CharSpan cspan_to_copy, MutableC
 {
     VerifyOrReturnError(out_buf.size() >= cspan_to_copy.size(), CHIP_ERROR_BUFFER_TOO_SMALL);
 
-    memcpy(out_buf.data(), cspan_to_copy.data(), cspan_to_copy.size());
+    // There is no guarantee that cspan_to_copy and out_buf don't overlap, so use memmove()
+    memmove(out_buf.data(), cspan_to_copy.data(), cspan_to_copy.size());
     out_buf.reduce_size(cspan_to_copy.size());
 
     return CHIP_NO_ERROR;
@@ -400,7 +402,8 @@ inline void CopyCharSpanToMutableCharSpanWithTruncation(CharSpan span_to_copy, M
 {
     size_t size_to_copy = std::min(span_to_copy.size(), out_span.size());
 
-    memcpy(out_span.data(), span_to_copy.data(), size_to_copy);
+    // There is no guarantee that span_to_copy and out_buf don't overlap, so use memmove()
+    memmove(out_span.data(), span_to_copy.data(), size_to_copy);
     out_span.reduce_size(size_to_copy);
 }