From 0f970a1c702c1fc575e09d7b590fab1baf4042cb Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Thu, 19 Dec 2024 10:04:02 +1300 Subject: [PATCH] Simplify AutoCommissioner::SetCommissioningParameters 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. Always assign mNeedIcdRegistration when the kReadCommissioningInfo step finishes, instead of relying on SetCommissioningParameters to clear it. --- src/controller/AutoCommissioner.cpp | 67 +++-------------------------- 1 file changed, 6 insertions(+), 61 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index a373b6b34af13f..53d0297301fd63 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/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 -static bool IsUnsafeSpan(const Optional & maybeUnsafeSpan, const Optional & 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,44 +82,13 @@ 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(¶ms != 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. 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()) { @@ -146,8 +96,6 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam 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()); @@ -162,8 +110,6 @@ 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()); @@ -184,8 +130,6 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam else { ChipLogError(Controller, "Country code is too large: %u", static_cast(code.size())); - // Make sure our buffer pointers don't dangle. - mParams.ClearExternalBufferDependentValues(); return CHIP_ERROR_INVALID_ARGUMENT; } } @@ -787,6 +731,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio } } + mNeedIcdRegistration = false; if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore) { if (mDeviceCommissioningInfo.icd.isLIT && mDeviceCommissioningInfo.icd.checkInProtocolSupport)