From 92ab9f0177a806fe8c59bede32a7c5df231e4c17 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 19 May 2021 10:48:15 -0700 Subject: [PATCH 01/10] Persist and reload message counters on the controller --- src/controller/CHIPDevice.cpp | 62 +++++++++++++++++-- src/controller/CHIPDevice.h | 44 ++++++------- src/controller/CHIPDeviceController.cpp | 19 +----- .../QRCode/QRCodeViewController.m | 4 +- src/lib/support/BUILD.gn | 1 + src/lib/support/PersistentStorageUtils.h | 48 ++++++++++++++ src/messaging/ExchangeMgr.cpp | 4 +- src/transport/MessageCounter.h | 22 +++++-- src/transport/PeerMessageCounter.h | 1 - src/transport/SecureSessionMgr.cpp | 6 +- 10 files changed, 151 insertions(+), 60 deletions(-) create mode 100644 src/lib/support/PersistentStorageUtils.h diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index 41bd381dd6ca14..67898d83b101be 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -46,9 +46,12 @@ #include #include #include +#include #include #include #include +#include +#include using namespace chip::Inet; using namespace chip::System; @@ -150,8 +153,10 @@ CHIP_ERROR Device::SendCommands(app::CommandSender * commandObj) CHIP_ERROR Device::Serialize(SerializedDevice & output) { - CHIP_ERROR error = CHIP_NO_ERROR; - uint16_t serializedLen = 0; + CHIP_ERROR error = CHIP_NO_ERROR; + uint16_t serializedLen = 0; + uint32_t localMessageCounter = 0; + uint32_t peerMessageCounter = 0; SerializableDevice serializable; static_assert(BASE64_ENCODED_LEN(sizeof(serializable)) <= sizeof(output.inner), @@ -164,6 +169,14 @@ CHIP_ERROR Device::Serialize(SerializedDevice & output) serializable.mDevicePort = Encoding::LittleEndian::HostSwap16(mDeviceAddress.GetPort()); serializable.mAdminId = Encoding::LittleEndian::HostSwap16(mAdminId); + Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession); + VerifyOrExit(connectionState != nullptr, error = CHIP_ERROR_INCORRECT_STATE); + localMessageCounter = connectionState->GetSessionMessageCounter().GetLocalMessageCounter().Value(); + peerMessageCounter = connectionState->GetSessionMessageCounter().GetPeerMessageCounter().GetCounter(); + + serializable.mLocalMessageCounter = Encoding::LittleEndian::HostSwap32(localMessageCounter); + serializable.mPeerMessageCounter = Encoding::LittleEndian::HostSwap32(peerMessageCounter); + serializable.mCASESessionKeyId = Encoding::LittleEndian::HostSwap16(mCASESessionKeyId); serializable.mDeviceProvisioningComplete = (mDeviceProvisioningComplete) ? 1 : 0; @@ -215,10 +228,12 @@ CHIP_ERROR Device::Deserialize(const SerializedDevice & input) IPAddress::FromString(Uint8::to_const_char(serializable.mDeviceAddr), sizeof(serializable.mDeviceAddr) - 1, ipAddress), error = CHIP_ERROR_INVALID_ADDRESS); - mPairing = serializable.mOpsCreds; - mDeviceId = Encoding::LittleEndian::HostSwap64(serializable.mDeviceId); - port = Encoding::LittleEndian::HostSwap16(serializable.mDevicePort); - mAdminId = Encoding::LittleEndian::HostSwap16(serializable.mAdminId); + mPairing = serializable.mOpsCreds; + mDeviceId = Encoding::LittleEndian::HostSwap64(serializable.mDeviceId); + port = Encoding::LittleEndian::HostSwap16(serializable.mDevicePort); + mAdminId = Encoding::LittleEndian::HostSwap16(serializable.mAdminId); + mLocalMessageCounter = Encoding::LittleEndian::HostSwap32(serializable.mLocalMessageCounter); + mPeerMessageCounter = Encoding::LittleEndian::HostSwap32(serializable.mPeerMessageCounter); mCASESessionKeyId = Encoding::LittleEndian::HostSwap16(serializable.mCASESessionKeyId); mDeviceProvisioningComplete = (serializable.mDeviceProvisioningComplete != 0); @@ -262,6 +277,17 @@ void Device::OnNewConnection(SecureSessionHandle session) { mState = ConnectionState::SecureConnected; mSecureSession = session; + + // Reset the message counters here because this is the first time we get a handle to the secure session. + Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession); + VerifyOrReturn(connectionState != nullptr); + MessageCounter & localCounter = connectionState->GetSessionMessageCounter().GetLocalMessageCounter(); + if (localCounter.SetCounter(mLocalMessageCounter)) + { + ChipLogError(Controller, "Unable to restore local counter to %d", mLocalMessageCounter); + }; + Transport::PeerMessageCounter & peerCounter = connectionState->GetSessionMessageCounter().GetPeerMessageCounter(); + peerCounter.SetCounter(mPeerMessageCounter); } void Device::OnConnectionExpired(SecureSessionHandle session) @@ -470,5 +496,29 @@ void Device::AddReportHandler(EndpointId endpoint, ClusterId cluster, AttributeI mCallbacksMgr.AddReportCallback(mDeviceId, endpoint, cluster, attribute, onReportCallback); } +Device::~Device() +{ + if (mExchangeMgr) + { + // Ensure that any exchange contexts we have open get closed now, + // because we don't want them to call back in to us after this + // point. + mExchangeMgr->CloseAllContextsForDelegate(this); + } + + if (mStorageDelegate) + { + // Store the current device in persistent storage so we have the latest + // message counters available next time. + Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession); + VerifyOrReturn(connectionState != nullptr); + SerializedDevice serialized; + Serialize(serialized); + // TODO: no need to base-64 the serialized values AGAIN + PERSISTENT_KEY_OP(GetDeviceId(), kPairedDeviceKeyPrefix, key, + mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner))); + } +} + } // namespace Controller } // namespace chip diff --git a/src/controller/CHIPDevice.h b/src/controller/CHIPDevice.h index 41b54d8d5f1868..e8d673738d8a6c 100644 --- a/src/controller/CHIPDevice.h +++ b/src/controller/CHIPDevice.h @@ -73,11 +73,11 @@ using DeviceTransportMgr = TransportMgrCloseAllContextsForDelegate(this); - } - } + ~Device(); enum class PairingWindowOption { @@ -173,13 +164,14 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta */ void Init(ControllerDeviceInitParams params, uint16_t listenPort, Transport::AdminId admin) { - mTransportMgr = params.transportMgr; - mSessionManager = params.sessionMgr; - mExchangeMgr = params.exchangeMgr; - mInetLayer = params.inetLayer; - mListenPort = listenPort; - mAdminId = admin; - mCredentials = params.credentials; + mTransportMgr = params.transportMgr; + mSessionManager = params.sessionMgr; + mExchangeMgr = params.exchangeMgr; + mInetLayer = params.inetLayer; + mListenPort = listenPort; + mAdminId = admin; + mStorageDelegate = params.storageDelegate; + mCredentials = params.credentials; #if CONFIG_NETWORK_LAYER_BLE mBleLayer = params.bleLayer; #endif @@ -406,6 +398,10 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta uint8_t mSequenceNumber = 0; + // Message counts start at 1 + uint32_t mLocalMessageCounter = 1; + uint32_t mPeerMessageCounter = 1; + app::CHIPDeviceCallbacksMgr & mCallbacksMgr = app::CHIPDeviceCallbacksMgr::GetInstance(); /** @@ -440,6 +436,8 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta uint16_t mCASESessionKeyId = 0; Credentials::OperationalCredentialSet * mCredentials = nullptr; + + PersistentStorageDelegate * mStorageDelegate = nullptr; }; /** @@ -494,6 +492,8 @@ typedef struct SerializableDevice uint8_t mDeviceTransport; uint8_t mDeviceProvisioningComplete; uint8_t mInterfaceName[kMaxInterfaceName]; + uint32_t mLocalMessageCounter; /* This field is serialized in LittleEndian byte order */ + uint32_t mPeerMessageCounter; /* This field is serialized in LittleEndian byte order */ } SerializableDevice; typedef struct SerializedDevice diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 07a84217c3504b..d9cf67f5b2fcc9 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -53,6 +53,7 @@ #include #include #include +#include #include #include #include @@ -89,10 +90,6 @@ namespace Controller { using namespace chip::Encoding; -constexpr const char kPairedDeviceListKeyPrefix[] = "ListPairedDevices"; -constexpr const char kPairedDeviceKeyPrefix[] = "PairedDevice"; -constexpr const char kNextAvailableKeyID[] = "StartKeyID"; - #if CHIP_DEVICE_CONFIG_ENABLE_MDNS constexpr uint16_t kMdnsPort = 5353; #endif @@ -103,20 +100,6 @@ constexpr uint32_t kMaxCHIPOpCertLength = 1024; constexpr uint32_t kMaxCHIPCSRLength = 1024; constexpr uint32_t kOpCSRNonceLength = 32; -// This macro generates a key using node ID an key prefix, and performs the given action -// on that key. -#define PERSISTENT_KEY_OP(node, keyPrefix, key, action) \ - do \ - { \ - constexpr size_t len = std::extent::value; \ - nlSTATIC_ASSERT_PRINT(len > 0, "keyPrefix length must be known at compile time"); \ - /* 2 * sizeof(NodeId) to accomodate 2 character for each byte in Node Id */ \ - char key[len + 2 * sizeof(NodeId) + 1]; \ - nlSTATIC_ASSERT_PRINT(sizeof(node) <= sizeof(uint64_t), "Node ID size is greater than expected"); \ - snprintf(key, sizeof(key), "%s%" PRIx64, keyPrefix, node); \ - action; \ - } while (0) - DeviceController::DeviceController() { mState = State::NotInitialized; diff --git a/src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m b/src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m index 834d6adf23138d..8816c86b42717c 100644 --- a/src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m +++ b/src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m @@ -392,10 +392,9 @@ - (void)onPairingComplete:(NSError *)error if (error.code != CHIPSuccess) { NSLog(@"Got pairing error back %@", error); } else { - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, DISPATCH_TIME_NOW), dispatch_get_main_queue(), ^{ + dispatch_async(dispatch_get_main_queue(), ^{ [self->_deviceList refreshDeviceList]; [self retrieveAndSendWifiCredentials]; - [self setVendorIDOnAccessory]; }); } } @@ -623,6 +622,7 @@ - (void)onAddressUpdated:(NSError *)error NSLog(@"Error retrieving device informations over Mdns: %@", error); return; } + [self setVendorIDOnAccessory]; } - (void)updateUIFields:(CHIPSetupPayload *)payload decimalString:(nullable NSString *)decimalString diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index f88bbc0603e18b..3e701e70d2ec78 100644 --- a/src/lib/support/BUILD.gn +++ b/src/lib/support/BUILD.gn @@ -77,6 +77,7 @@ static_library("support") { "LifetimePersistedCounter.h", "PersistedCounter.cpp", "PersistedCounter.h", + "PersistentStorageUtils.h", "Pool.cpp", "Pool.h", "PrivateHeap.cpp", diff --git a/src/lib/support/PersistentStorageUtils.h b/src/lib/support/PersistentStorageUtils.h new file mode 100644 index 00000000000000..3b79004b5344b1 --- /dev/null +++ b/src/lib/support/PersistentStorageUtils.h @@ -0,0 +1,48 @@ +/* + * + * Copyright (c) 2021 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * This file defines and implements some utlities for generating persistent storage keys + * + */ + +#pragma once + +#include + +namespace chip { + +constexpr const char kPairedDeviceListKeyPrefix[] = "ListPairedDevices"; +constexpr const char kPairedDeviceKeyPrefix[] = "PairedDevice"; +constexpr const char kNextAvailableKeyID[] = "StartKeyID"; + +// This macro generates a key for storage using a node ID and a key prefix, and performs the given action +// on that key. +#define PERSISTENT_KEY_OP(node, keyPrefix, key, action) \ + do \ + { \ + constexpr size_t len = std::extent::value; \ + nlSTATIC_ASSERT_PRINT(len > 0, "keyPrefix length must be known at compile time"); \ + /* 2 * sizeof(NodeId) to accomodate 2 character for each byte in Node Id */ \ + char key[len + 2 * sizeof(NodeId) + 1]; \ + nlSTATIC_ASSERT_PRINT(sizeof(node) <= sizeof(uint64_t), "Node ID size is greater than expected"); \ + snprintf(key, sizeof(key), "%s%" PRIx64, keyPrefix, node); \ + action; \ + } while (0) + +} // namespace chip diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index 1215751347af48..2c990786bb17b8 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -198,8 +198,8 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const UnsolicitedMessageHandler * matchingUMH = nullptr; bool sendAckAndCloseExchange = false; - ChipLogProgress(ExchangeManager, "Received message of type %d and protocolId %d", payloadHeader.GetMessageType(), - payloadHeader.GetProtocolID()); + ChipLogProgress(ExchangeManager, "Received message of type %d and protocolId %d on exchange %d", payloadHeader.GetMessageType(), + payloadHeader.GetProtocolID(), payloadHeader.GetExchangeID()); // Search for an existing exchange that the message applies to. If a match is found... bool found = false; diff --git a/src/transport/MessageCounter.h b/src/transport/MessageCounter.h index 49d044dc7b4f0e..84ecce104a7419 100644 --- a/src/transport/MessageCounter.h +++ b/src/transport/MessageCounter.h @@ -47,10 +47,11 @@ class MessageCounter virtual ~MessageCounter() = 0; - virtual Type GetType() = 0; - virtual void Reset() = 0; - virtual uint32_t Value() = 0; /** Get current value */ - virtual CHIP_ERROR Advance() = 0; /** Advance the counter */ + virtual Type GetType() = 0; + virtual void Reset() = 0; + virtual uint32_t Value() = 0; /** Get current value */ + virtual CHIP_ERROR Advance() = 0; /** Advance the counter */ + virtual CHIP_ERROR SetCounter(uint32_t count) = 0; /** Set the counter to the specified value */ }; inline MessageCounter::~MessageCounter() {} @@ -71,6 +72,12 @@ class GlobalUnencryptedMessageCounter : public MessageCounter ++value; return CHIP_NO_ERROR; } + CHIP_ERROR SetCounter(uint32_t count) override + { + Reset(); + value = count; + return CHIP_NO_ERROR; + } private: uint32_t value; @@ -89,6 +96,7 @@ class GlobalEncryptedMessageCounter : public MessageCounter } uint32_t Value() override { return persisted.GetValue(); } CHIP_ERROR Advance() override { return persisted.Advance(); } + CHIP_ERROR SetCounter(uint32_t count) override { return CHIP_ERROR_NOT_IMPLEMENTED; } private: #if CONFIG_DEVICE_LAYER @@ -127,6 +135,12 @@ class LocalSessionMessageCounter : public MessageCounter ++value; return CHIP_NO_ERROR; } + CHIP_ERROR SetCounter(uint32_t count) override + { + Reset(); + value = count; + return CHIP_NO_ERROR; + } private: uint32_t value; diff --git a/src/transport/PeerMessageCounter.h b/src/transport/PeerMessageCounter.h index 476150938f4171..2048c98b0af26d 100644 --- a/src/transport/PeerMessageCounter.h +++ b/src/transport/PeerMessageCounter.h @@ -148,7 +148,6 @@ class PeerMessageCounter mSynced.mWindow.reset(); } - /* Test-only */ uint32_t GetCounter() { return mSynced.mMaxCounter; } private: diff --git a/src/transport/SecureSessionMgr.cpp b/src/transport/SecureSessionMgr.cpp index 6d7864af2f7b96..8b01f0dc0258fb 100644 --- a/src/transport/SecureSessionMgr.cpp +++ b/src/transport/SecureSessionMgr.cpp @@ -370,11 +370,7 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader, { ChipLogError(Inet, "Message counter verify failed, err = %d", err); } - // TODO - Enable exit on error for message counter verification failure. - // We are now using IM messages in commissioner class to provision op creds and - // other device commissioning steps. This is somehow causing issues with message counter - // verification. Disabling this check for now. Enable it after debugging the cause. - // SuccessOrExit(err); + SuccessOrExit(err); } admin = mAdmins->FindAdminWithId(state->GetAdminId()); From c9c017dbd53fb647d51fe9b7bc42a4940e205029 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Tue, 25 May 2021 14:10:47 -0700 Subject: [PATCH 02/10] Refactor CHIPDevice storage into an API on CHIPDevice --- src/controller/CHIPDevice.cpp | 21 ++++++++++++++++----- src/controller/CHIPDevice.h | 7 +++++++ src/controller/CHIPDeviceController.cpp | 9 ++------- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index 67898d83b101be..509e97f1e435f0 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -273,6 +273,21 @@ CHIP_ERROR Device::Deserialize(const SerializedDevice & input) return error; } +CHIP_ERROR Device::Persist() +{ + CHIP_ERROR error = CHIP_NO_ERROR; + if (mStorageDelegate != nullptr) + { + SerializedDevice serialized; + Serialize(serialized); + + // TODO: no need to base-64 the serialized values AGAIN + PERSISTENT_KEY_OP(GetDeviceId(), kPairedDeviceKeyPrefix, key, + error = mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner))); + } + return error; +} + void Device::OnNewConnection(SecureSessionHandle session) { mState = ConnectionState::SecureConnected; @@ -512,11 +527,7 @@ Device::~Device() // message counters available next time. Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession); VerifyOrReturn(connectionState != nullptr); - SerializedDevice serialized; - Serialize(serialized); - // TODO: no need to base-64 the serialized values AGAIN - PERSISTENT_KEY_OP(GetDeviceId(), kPairedDeviceKeyPrefix, key, - mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner))); + Persist(); } } diff --git a/src/controller/CHIPDevice.h b/src/controller/CHIPDevice.h index e8d673738d8a6c..44c4f6d88d4a63 100644 --- a/src/controller/CHIPDevice.h +++ b/src/controller/CHIPDevice.h @@ -218,6 +218,13 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta **/ CHIP_ERROR Deserialize(const SerializedDevice & input); + /** + * @brief Serialize and store the Device in persistent storage + * + * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise + */ + CHIP_ERROR Persist(); + /** * @brief * Called when a new pairing is being established diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index d9cf67f5b2fcc9..8228de37d99632 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -473,14 +473,9 @@ void DeviceController::PersistDevice(Device * device) // mainly by test applications, do not require a storage delegate. This is to // reduce overheads on these tests. // Let's make sure the delegate object is available before calling into it. - if (mStorageDelegate != nullptr && mState == State::Initialized) + if (mState == State::Initialized) { - SerializedDevice serialized; - device->Serialize(serialized); - - // TODO: no need to base-64 the serialized values AGAIN - PERSISTENT_KEY_OP(device->GetDeviceId(), kPairedDeviceKeyPrefix, key, - mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner))); + device->Persist(); } } From 71468cce3a47939418b33319ad549fd64ede5c9f Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Tue, 25 May 2021 14:23:01 -0700 Subject: [PATCH 03/10] Rename PersistentStorageUtils -> PersistentStorageMacros --- src/controller/CHIPDevice.cpp | 2 +- src/controller/CHIPDeviceController.cpp | 2 +- src/lib/support/BUILD.gn | 2 +- .../{PersistentStorageUtils.h => PersistentStorageMacros.h} | 0 4 files changed, 3 insertions(+), 3 deletions(-) rename src/lib/support/{PersistentStorageUtils.h => PersistentStorageMacros.h} (100%) diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index 509e97f1e435f0..fbfcf733b5d590 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -46,7 +46,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 8228de37d99632..b0e092fbfc2e60 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -53,7 +53,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index 3e701e70d2ec78..72b29d92093350 100644 --- a/src/lib/support/BUILD.gn +++ b/src/lib/support/BUILD.gn @@ -77,7 +77,7 @@ static_library("support") { "LifetimePersistedCounter.h", "PersistedCounter.cpp", "PersistedCounter.h", - "PersistentStorageUtils.h", + "PersistentStorageMacros.h", "Pool.cpp", "Pool.h", "PrivateHeap.cpp", diff --git a/src/lib/support/PersistentStorageUtils.h b/src/lib/support/PersistentStorageMacros.h similarity index 100% rename from src/lib/support/PersistentStorageUtils.h rename to src/lib/support/PersistentStorageMacros.h From b30b7ceba6ea5d690ef6dee82bcf1ba28c9329df Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 26 May 2021 12:06:13 -0700 Subject: [PATCH 04/10] Fix the cluster tests --- src/controller/CHIPDevice.cpp | 2 +- src/controller/CHIPDeviceController.cpp | 18 ++++++++---------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index fbfcf733b5d590..3d18dd8986eec0 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -521,7 +521,7 @@ Device::~Device() mExchangeMgr->CloseAllContextsForDelegate(this); } - if (mStorageDelegate) + if (mStorageDelegate != nullptr && mSessionManager != nullptr) { // Store the current device in persistent storage so we have the latest // message counters available next time. diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index b0e092fbfc2e60..8bc91ca9a2e395 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -468,11 +468,6 @@ CHIP_ERROR DeviceController::UpdateDevice(Device * device, uint64_t fabricId) void DeviceController::PersistDevice(Device * device) { - // mStorageDelegate would not be null for a typical pairing scenario, as Pair() - // requires a valid storage delegate. However, test pairing usecase, that's used - // mainly by test applications, do not require a storage delegate. This is to - // reduce overheads on these tests. - // Let's make sure the delegate object is available before calling into it. if (mState == State::Initialized) { device->Persist(); @@ -754,11 +749,14 @@ void DeviceController::OnCommissionableNodeFound(const chip::Mdns::Commissionabl ControllerDeviceInitParams DeviceController::GetControllerDeviceInitParams() { - return ControllerDeviceInitParams{ .transportMgr = mTransportMgr, - .sessionMgr = mSessionMgr, - .exchangeMgr = mExchangeMgr, - .inetLayer = mInetLayer, - .credentials = &mCredentials }; + return ControllerDeviceInitParams{ + .transportMgr = mTransportMgr, + .sessionMgr = mSessionMgr, + .exchangeMgr = mExchangeMgr, + .inetLayer = mInetLayer, + .credentials = &mCredentials, + .storageDelegate = mStorageDelegate, + }; } DeviceCommissioner::DeviceCommissioner() : From 95c2c655dda086a8174c27bd8b1f64c0cca38f9f Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 26 May 2021 14:23:30 -0700 Subject: [PATCH 05/10] Fix ordering of initializers --- src/controller/CHIPDeviceController.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 8bc91ca9a2e395..2a1a338e59d3ff 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -754,8 +754,8 @@ ControllerDeviceInitParams DeviceController::GetControllerDeviceInitParams() .sessionMgr = mSessionMgr, .exchangeMgr = mExchangeMgr, .inetLayer = mInetLayer, - .credentials = &mCredentials, .storageDelegate = mStorageDelegate, + .credentials = &mCredentials, }; } From f49beb205fec0d0d8ce4b705294530e229baadc6 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 26 May 2021 17:33:29 -0700 Subject: [PATCH 06/10] Update comment to describe why counters are being stored --- src/controller/CHIPDevice.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index 3d18dd8986eec0..671b0f0cdef015 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -523,8 +523,9 @@ Device::~Device() if (mStorageDelegate != nullptr && mSessionManager != nullptr) { - // Store the current device in persistent storage so we have the latest - // message counters available next time. + // Since CHIPDevices can be serialized/deserialized in the middle of what is conceptually a single PASE session + // We need to store the session counters along with the the session information so that when we deserialize + // this device next time, we can recreate the session correctly. Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession); VerifyOrReturn(connectionState != nullptr); Persist(); From d730cc61ba166427c68ca0a9e5309b2051aab6c7 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 26 May 2021 17:47:47 -0700 Subject: [PATCH 07/10] Moved comments around --- src/controller/CHIPDevice.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index 671b0f0cdef015..d3711ba14ca219 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -294,6 +294,8 @@ void Device::OnNewConnection(SecureSessionHandle session) mSecureSession = session; // Reset the message counters here because this is the first time we get a handle to the secure session. + // Since CHIPDevices can be serialized/deserialized in the middle of what is conceptually a single PASE session + // we need to restore the session counters along with the the session information. Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession); VerifyOrReturn(connectionState != nullptr); MessageCounter & localCounter = connectionState->GetSessionMessageCounter().GetLocalMessageCounter(); @@ -523,9 +525,7 @@ Device::~Device() if (mStorageDelegate != nullptr && mSessionManager != nullptr) { - // Since CHIPDevices can be serialized/deserialized in the middle of what is conceptually a single PASE session - // We need to store the session counters along with the the session information so that when we deserialize - // this device next time, we can recreate the session correctly. + // If a session can be found, persist the device so that we track the newest message counter values Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession); VerifyOrReturn(connectionState != nullptr); Persist(); From cff79793a1f3a025791a56561d68b638499b7d40 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 27 May 2021 10:24:33 -0400 Subject: [PATCH 08/10] Fix typo --- src/controller/CHIPDevice.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index d3711ba14ca219..ec002f628b5da6 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -295,7 +295,7 @@ void Device::OnNewConnection(SecureSessionHandle session) // Reset the message counters here because this is the first time we get a handle to the secure session. // Since CHIPDevices can be serialized/deserialized in the middle of what is conceptually a single PASE session - // we need to restore the session counters along with the the session information. + // we need to restore the session counters along with the session information. Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession); VerifyOrReturn(connectionState != nullptr); MessageCounter & localCounter = connectionState->GetSessionMessageCounter().GetLocalMessageCounter(); From 8d809b9bd9cb72a368470a2698a1d1aaf6d6f444 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Thu, 27 May 2021 09:46:41 -0700 Subject: [PATCH 09/10] Address review comments --- src/controller/CHIPDevice.cpp | 13 ++++++++++--- src/controller/CHIPDevice.h | 7 +++---- src/controller/CHIPDeviceController.cpp | 4 ++++ 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index ec002f628b5da6..e338f3b38746c8 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -279,12 +279,17 @@ CHIP_ERROR Device::Persist() if (mStorageDelegate != nullptr) { SerializedDevice serialized; - Serialize(serialized); + SuccessOrExit(error = Serialize(serialized)); // TODO: no need to base-64 the serialized values AGAIN PERSISTENT_KEY_OP(GetDeviceId(), kPairedDeviceKeyPrefix, key, error = mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner))); + if (error != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Failed to persist device %d", error); + } } +exit: return error; } @@ -527,8 +532,10 @@ Device::~Device() { // If a session can be found, persist the device so that we track the newest message counter values Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession); - VerifyOrReturn(connectionState != nullptr); - Persist(); + if (connectionState != nullptr) + { + Persist(); + } } } diff --git a/src/controller/CHIPDevice.h b/src/controller/CHIPDevice.h index 44c4f6d88d4a63..ef8e8cd917e030 100644 --- a/src/controller/CHIPDevice.h +++ b/src/controller/CHIPDevice.h @@ -221,7 +221,7 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta /** * @brief Serialize and store the Device in persistent storage * - * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise + * @return Returns a CHIP_ERROR if either serialization or storage fails */ CHIP_ERROR Persist(); @@ -405,9 +405,8 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta uint8_t mSequenceNumber = 0; - // Message counts start at 1 - uint32_t mLocalMessageCounter = 1; - uint32_t mPeerMessageCounter = 1; + uint32_t mLocalMessageCounter = 0; + uint32_t mPeerMessageCounter = 0; app::CHIPDeviceCallbacksMgr & mCallbacksMgr = app::CHIPDeviceCallbacksMgr::GetInstance(); diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 2a1a338e59d3ff..8404d74ef95596 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -472,6 +472,10 @@ void DeviceController::PersistDevice(Device * device) { device->Persist(); } + else + { + ChipLogError(Controller, "Failed to persist device. Controller not initialized."); + } } CHIP_ERROR DeviceController::ServiceEvents() From 96b735046dee16a2c2a501a76f179a04ab91bcc3 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Thu, 27 May 2021 12:01:52 -0700 Subject: [PATCH 10/10] remove unnecessary semicolon --- src/controller/CHIPDevice.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index e338f3b38746c8..d0d0c3a70727b9 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -307,7 +307,7 @@ void Device::OnNewConnection(SecureSessionHandle session) if (localCounter.SetCounter(mLocalMessageCounter)) { ChipLogError(Controller, "Unable to restore local counter to %d", mLocalMessageCounter); - }; + } Transport::PeerMessageCounter & peerCounter = connectionState->GetSessionMessageCounter().GetPeerMessageCounter(); peerCounter.SetCounter(mPeerMessageCounter); }