From 46a1a38ecd8117d33505997d13566c37adc071ee Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Tue, 21 Jan 2025 08:47:45 +1300 Subject: [PATCH] Add the ability to read extra attributes during commissioning (#36867) * Add support for ExtraReadPaths to CommissioningParameters * CHIP_CONFIG_ENABLE_READ_CLIENT strikes again * Check for alloc failure * Address further review comments --- src/app/AttributePathParams.h | 10 ++++---- src/controller/AutoCommissioner.cpp | 31 +++++++++++++++++++++++-- src/controller/AutoCommissioner.h | 14 ++++++++--- src/controller/CHIPDeviceController.cpp | 7 ++++++ src/controller/CHIPDeviceController.h | 1 + src/controller/CommissioningDelegate.h | 20 ++++++++++++++++ src/lib/support/ScopedBuffer.h | 6 +++-- 7 files changed, 77 insertions(+), 12 deletions(-) diff --git a/src/app/AttributePathParams.h b/src/app/AttributePathParams.h index 52d5bb5972162b..f69340aa3a7ea1 100644 --- a/src/app/AttributePathParams.h +++ b/src/app/AttributePathParams.h @@ -30,7 +30,7 @@ struct AttributePathParams { AttributePathParams() = default; - explicit AttributePathParams(EndpointId aEndpointId) : + constexpr explicit AttributePathParams(EndpointId aEndpointId) : AttributePathParams(aEndpointId, kInvalidClusterId, kInvalidAttributeId, kInvalidListIndex) {} @@ -38,19 +38,19 @@ struct AttributePathParams // TODO: (Issue #10596) Need to ensure that we do not encode the NodeId over the wire // if it is either not 'set', or is set to a value that matches accessing fabric // on which the interaction is undertaken. - AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId) : + constexpr AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId) : AttributePathParams(aEndpointId, aClusterId, kInvalidAttributeId, kInvalidListIndex) {} - AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId, AttributeId aAttributeId) : + constexpr AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId, AttributeId aAttributeId) : AttributePathParams(aEndpointId, aClusterId, aAttributeId, kInvalidListIndex) {} - AttributePathParams(ClusterId aClusterId, AttributeId aAttributeId) : + constexpr AttributePathParams(ClusterId aClusterId, AttributeId aAttributeId) : AttributePathParams(kInvalidEndpointId, aClusterId, aAttributeId, kInvalidListIndex) {} - AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId, AttributeId aAttributeId, ListIndex aListIndex) : + constexpr AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId, AttributeId aAttributeId, ListIndex aListIndex) : mClusterId(aClusterId), mAttributeId(aAttributeId), mEndpointId(aEndpointId), mListIndex(aListIndex) {} diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index e9dac8df879bdb..d156d3058272d2 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -18,13 +18,14 @@ #include -#include - #include #include #include #include +#include +#include + namespace chip { namespace Controller { @@ -225,6 +226,32 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam mParams.SetICDClientType(params.GetICDClientType().Value()); } + auto extraReadPaths = params.GetExtraReadPaths(); + if (extraReadPaths.size() > 0) + { + using ReadPath = std::remove_pointer_t; + static_assert(std::is_trivially_copyable_v, "can't use memmove / memcpy, not trivially copyable"); + + if (mExtraReadPaths.AllocatedSize() == extraReadPaths.size()) + { + memmove(mExtraReadPaths.Get(), extraReadPaths.data(), extraReadPaths.size() * sizeof(ReadPath)); + } + else + { + // We can't reallocate mExtraReadPaths yet as this would free the old buffer, + // and the caller might be passing a sub-span of the old paths. + decltype(mExtraReadPaths) oldReadPaths(std::move(mExtraReadPaths)); + VerifyOrReturnError(mExtraReadPaths.Alloc(extraReadPaths.size()), CHIP_ERROR_NO_MEMORY); + memcpy(mExtraReadPaths.Get(), extraReadPaths.data(), extraReadPaths.size() * sizeof(ReadPath)); + } + + mParams.SetExtraReadPaths(mExtraReadPaths.Span()); + } + else + { + mExtraReadPaths.Free(); + } + return CHIP_NO_ERROR; } diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index deacda6e3cca31..1589da4225850f 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -16,10 +16,13 @@ * limitations under the License. */ #pragma once + +#include #include #include #include #include +#include #include namespace chip { @@ -116,7 +119,9 @@ class AutoCommissioner : public CommissioningDelegate CommissioneeDeviceProxy * mCommissioneeDeviceProxy = nullptr; OperationalCredentialsDelegate * mOperationalCredentialsDelegate = nullptr; OperationalDeviceProxy mOperationalDeviceProxy; - // Memory space for the commisisoning parameters that come in as ByteSpans - the caller is not guaranteed to retain this memory + + // BEGIN memory space for commissioning parameters that are Spans or similar pointers: + // The caller is not guaranteed to retain the memory these parameters point to. uint8_t mSsid[CommissioningParameters::kMaxSsidLen]; uint8_t mCredentials[CommissioningParameters::kMaxCredentialsLen]; uint8_t mThreadOperationalDataset[CommissioningParameters::kMaxThreadDatasetLen]; @@ -136,6 +141,11 @@ class AutoCommissioner : public CommissioningDelegate static constexpr size_t kMaxDefaultNtpSize = 128; char mDefaultNtp[kMaxDefaultNtpSize]; + uint8_t mICDSymmetricKey[Crypto::kAES_CCM128_Key_Length]; + Platform::ScopedMemoryBufferWithSize mExtraReadPaths; + + // END memory space for commisisoning parameters + bool mNeedsNetworkSetup = false; ReadCommissioningInfo mDeviceCommissioningInfo; bool mNeedsDST = false; @@ -155,8 +165,6 @@ class AutoCommissioner : public CommissioningDelegate uint8_t mAttestationElements[Credentials::kMaxRspLen]; uint16_t mAttestationSignatureLen = 0; uint8_t mAttestationSignature[Crypto::kMax_ECDSA_Signature_Length]; - - uint8_t mICDSymmetricKey[Crypto::kAES_CCM128_Key_Length]; }; } // namespace Controller } // namespace chip diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 76e82d7f21ac85..e62bfb86916345 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -2329,6 +2329,12 @@ void DeviceCommissioner::ContinueReadingCommissioningInfo(const CommissioningPar Clusters::IcdManagement::Attributes::ActiveModeDuration::Id)); VerifyOrReturn(builder.AddAttributePath(kRootEndpointId, Clusters::IcdManagement::Id, Clusters::IcdManagement::Attributes::ActiveModeThreshold::Id)); + + // Extra paths requested via CommissioningParameters + for (auto const & path : params.GetExtraReadPaths()) + { + VerifyOrReturn(builder.AddAttributePath(path)); + } }(); VerifyOrDie(builder.size() > 0); // our logic is broken if there is nothing to read @@ -2363,6 +2369,7 @@ void DeviceCommissioner::FinishReadingCommissioningInfo() // up returning an error (e.g. because some mandatory information was missing). CHIP_ERROR err = CHIP_NO_ERROR; ReadCommissioningInfo info; + info.attributes = mAttributeCache.get(); AccumulateErrors(err, ParseGeneralCommissioningInfo(info)); AccumulateErrors(err, ParseBasicInformation(info)); AccumulateErrors(err, ParseNetworkCommissioningInfo(info)); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 93a0a58752900a..c9f1652c969be8 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -816,6 +816,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, Optional GetCommissioningParameters() { + // TODO: Return a non-optional const & to avoid a copy, mDefaultCommissioner is never null return mDefaultCommissioner == nullptr ? NullOptional : MakeOptional(mDefaultCommissioner->GetCommissioningParameters()); } diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 660a3d11d128f9..99763aa305cabd 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -18,11 +18,14 @@ #pragma once #include +#include +#include #include #include #include #include #include +#include #include #include #include @@ -591,6 +594,18 @@ class CommissioningParameters } void ClearICDStayActiveDurationMsec() { mICDStayActiveDurationMsec.ClearValue(); } + Span GetExtraReadPaths() const { return mExtraReadPaths; } + + // Additional attribute paths to read as part of the kReadCommissioningInfo stage. + // These values read from the device will be available in ReadCommissioningInfo.attributes. + // Clients should avoid requesting paths that are already read internally by the commissioner + // as no consolidation of internally read and extra paths provided here will be performed. + CommissioningParameters & SetExtraReadPaths(Span paths) + { + mExtraReadPaths = paths; + return *this; + } + // Clear all members that depend on some sort of external buffer. Can be // used to make sure that we are not holding any dangling pointers. void ClearExternalBufferDependentValues() @@ -613,6 +628,7 @@ class CommissioningParameters mDSTOffsets.ClearValue(); mDefaultNTP.ClearValue(); mICDSymmetricKey.ClearValue(); + mExtraReadPaths = decltype(mExtraReadPaths)(); } private: @@ -662,6 +678,7 @@ class CommissioningParameters Optional mICDStayActiveDurationMsec; ICDRegistrationStrategy mICDRegistrationStrategy = ICDRegistrationStrategy::kIgnore; bool mCheckForMatchingFabric = false; + Span mExtraReadPaths; }; struct RequestedCertificate @@ -762,6 +779,9 @@ struct ICDManagementClusterInfo struct ReadCommissioningInfo { +#if CHIP_CONFIG_ENABLE_READ_CLIENT + app::ClusterStateCache const * attributes = nullptr; +#endif NetworkClusters network; BasicClusterInfo basic; GeneralCommissioningInfo general; diff --git a/src/lib/support/ScopedBuffer.h b/src/lib/support/ScopedBuffer.h index 7a3c0aaca5902c..923c532e15b003 100644 --- a/src/lib/support/ScopedBuffer.h +++ b/src/lib/support/ScopedBuffer.h @@ -26,6 +26,7 @@ #include #include +#include #include #include @@ -218,11 +219,12 @@ class ScopedMemoryBufferWithSize : public ScopedMemoryBuffer return *this; } - ~ScopedMemoryBufferWithSize() { mCount = 0; } - // return the size as count of elements inline size_t AllocatedSize() const { return mCount; } + chip::Span Span() { return chip::Span(this->Get(), AllocatedSize()); } + chip::Span Span() const { return chip::Span(this->Get(), AllocatedSize()); } + void Free() { mCount = 0;