Skip to content

Commit

Permalink
Simplify AutoCommissioner::SetCommissioningParameters
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ksperling-apple
ksperling-apple committed Dec 18, 2024
1 parent 238e801 commit 0f970a1
Showing 1 changed file with 6 additions and 61 deletions.
67 changes: 6 additions & 61 deletions src/controller/AutoCommissioner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -101,53 +82,20 @@ 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.
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());
Expand All @@ -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());
Expand All @@ -184,8 +130,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;
}
}
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 0f970a1

Please sign in to comment.