[thread/cleanup] Change GetThreadProvision to return OperationalDatas…

…et (project-chip#17054) * [thread] Change GetThreadProvision to return OperationalDataset OpenThread stack manager has OperationalDataset member only used as a temporary object for returning an operational dataset as ByteSpan in GetThreadProvision method. Change the interface to save >250 bytes of RAM. Additionally, add bound checks in OperationalDataset::GetExtendedPanId methods. * Restyled by clang-format Co-authored-by: Restyled.io <[email protected]>
chencheung · Apr 6, 2022 · d9d00bf · d9d00bf
1 parent 7c02bef
commit d9d00bf
Show file tree

Hide file tree

Showing 8 changed files with 41 additions and 47 deletions.
diff --git a/src/include/platform/ThreadStackManager.h b/src/include/platform/ThreadStackManager.h
@@ -36,6 +36,10 @@ struct TextEntry;
 struct DnssdService;
 } // namespace Dnssd
 
+namespace Thread {
+class OperationalDataset;
+} // namespace Thread
+
 namespace DeviceLayer {
 
 class PlatformManagerImpl;
@@ -93,7 +97,7 @@ class ThreadStackManager
     bool IsThreadEnabled();
     bool IsThreadProvisioned();
     bool IsThreadAttached();
-    CHIP_ERROR GetThreadProvision(ByteSpan & netInfo);
+    CHIP_ERROR GetThreadProvision(Thread::OperationalDataset & dataset);
     CHIP_ERROR GetAndLogThreadStatsCounters();
     CHIP_ERROR GetAndLogThreadTopologyMinimal();
     CHIP_ERROR GetAndLogThreadTopologyFull();
@@ -342,9 +346,9 @@ inline bool ThreadStackManager::IsThreadAttached()
     return static_cast<ImplClass *>(this)->_IsThreadAttached();
 }
 
-inline CHIP_ERROR ThreadStackManager::GetThreadProvision(ByteSpan & netInfo)
+inline CHIP_ERROR ThreadStackManager::GetThreadProvision(Thread::OperationalDataset & dataset)
 {
-    return static_cast<ImplClass *>(this)->_GetThreadProvision(netInfo);
+    return static_cast<ImplClass *>(this)->_GetThreadProvision(dataset);
 }
 
 inline CHIP_ERROR ThreadStackManager::SetThreadProvision(ByteSpan netInfo)

diff --git a/src/lib/support/ThreadOperationalDataset.cpp b/src/lib/support/ThreadOperationalDataset.cpp
@@ -277,28 +277,34 @@ CHIP_ERROR OperationalDataset::SetChannel(uint16_t aChannel)
 
 CHIP_ERROR OperationalDataset::GetExtendedPanId(uint8_t (&aExtendedPanId)[kSizeExtendedPanId]) const
 {
-    const ThreadTLV * tlv = Locate(ThreadTLV::kExtendedPanId);
+    ByteSpan extPanIdSpan;
+    CHIP_ERROR error = GetExtendedPanIdAsByteSpan(extPanIdSpan);
 
-    if (tlv != nullptr)
+    if (error != CHIP_NO_ERROR)
     {
-        memcpy(aExtendedPanId, tlv->GetValue(), sizeof(aExtendedPanId));
-        return CHIP_NO_ERROR;
+        return error;
     }
 
-    return CHIP_ERROR_TLV_TAG_NOT_FOUND;
+    memcpy(aExtendedPanId, extPanIdSpan.data(), extPanIdSpan.size());
+    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR OperationalDataset::GetExtendedPanIdAsByteSpan(ByteSpan & span) const
 {
     const ThreadTLV * tlv = Locate(ThreadTLV::kExtendedPanId);
 
-    if (tlv != nullptr)
+    if (tlv == nullptr)
     {
-        span = ByteSpan(reinterpret_cast<const uint8_t *>(tlv->GetValue()), tlv->GetLength());
-        return CHIP_NO_ERROR;
+        return CHIP_ERROR_TLV_TAG_NOT_FOUND;
     }
 
-    return CHIP_ERROR_TLV_TAG_NOT_FOUND;
+    if (tlv->GetLength() != kSizeExtendedPanId)
+    {
+        return CHIP_ERROR_INVALID_TLV_ELEMENT;
+    }
+
+    span = ByteSpan(static_cast<const uint8_t *>(tlv->GetValue()), tlv->GetLength());
+    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR OperationalDataset::SetExtendedPanId(const uint8_t (&aExtendedPanId)[kSizeExtendedPanId])

diff --git a/src/platform/Linux/NetworkCommissioningThreadDriver.cpp b/src/platform/Linux/NetworkCommissioningThreadDriver.cpp
@@ -43,12 +43,10 @@ namespace NetworkCommissioning {
 
 CHIP_ERROR LinuxThreadDriver::Init(BaseDriver::NetworkStatusChangeCallback * networkStatusChangeCallback)
 {
-    ByteSpan currentProvision;
     VerifyOrReturnError(ConnectivityMgrImpl().IsThreadAttached(), CHIP_NO_ERROR);
-    VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentProvision) == CHIP_NO_ERROR, CHIP_NO_ERROR);
+    VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(mStagingNetwork) == CHIP_NO_ERROR, CHIP_NO_ERROR);
 
-    mSavedNetwork.Init(currentProvision);
-    mStagingNetwork.Init(currentProvision);
+    mSavedNetwork.Init(mStagingNetwork.AsByteSpan());
 
     ThreadStackMgrImpl().SetNetworkStatusChangeCallback(networkStatusChangeCallback);
 
@@ -185,14 +183,12 @@ bool LinuxThreadDriver::ThreadNetworkIterator::Next(Network & item)
     item.connected    = false;
     exhausted         = true;
 
-    ByteSpan currentProvision;
     Thread::OperationalDataset currentDataset;
     uint8_t enabledExtPanId[Thread::kSizeExtendedPanId];
 
     // The Thread network is not actually enabled.
     VerifyOrReturnError(ConnectivityMgrImpl().IsThreadAttached(), true);
-    VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentProvision) == CHIP_NO_ERROR, true);
-    VerifyOrReturnError(currentDataset.Init(currentProvision) == CHIP_NO_ERROR, true);
+    VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentDataset) == CHIP_NO_ERROR, true);
     // The Thread network is not enabled, but has a different extended pan id.
     VerifyOrReturnError(currentDataset.GetExtendedPanId(enabledExtPanId) == CHIP_NO_ERROR, true);
     VerifyOrReturnError(memcmp(extpanid, enabledExtPanId, kSizeExtendedPanId) == 0, true);

diff --git a/src/platform/Linux/ThreadStackManagerImpl.cpp b/src/platform/Linux/ThreadStackManagerImpl.cpp
@@ -235,7 +235,7 @@ CHIP_ERROR ThreadStackManagerImpl::_SetThreadProvision(ByteSpan netInfo)
     return PlatformMgr().PostEvent(&event);
 }
 
-CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(ByteSpan & netInfo)
+CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(Thread::OperationalDataset & dataset)
 {
     VerifyOrReturnError(mProxy, CHIP_ERROR_INCORRECT_STATE);
 
@@ -257,23 +257,20 @@ CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(ByteSpan & netInfo)
 
         if (response == nullptr)
         {
-            netInfo = ByteSpan();
             return CHIP_ERROR_KEY_NOT_FOUND;
         }
 
         std::unique_ptr<GVariant, GVariantDeleter> tupleContent(g_variant_get_child_value(response.get(), 0));
 
         if (tupleContent == nullptr)
         {
-            netInfo = ByteSpan();
             return CHIP_ERROR_KEY_NOT_FOUND;
         }
 
         std::unique_ptr<GVariant, GVariantDeleter> value(g_variant_get_variant(tupleContent.get()));
 
         if (value == nullptr)
         {
-            netInfo = ByteSpan();
             return CHIP_ERROR_KEY_NOT_FOUND;
         }
 
@@ -282,7 +279,7 @@ CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(ByteSpan & netInfo)
         ReturnErrorOnFailure(mDataset.Init(ByteSpan(data, size)));
     }
 
-    netInfo = mDataset.AsByteSpan();
+    dataset.Init(mDataset.AsByteSpan());
 
     return CHIP_NO_ERROR;
 }
@@ -716,20 +713,18 @@ void ThreadStackManagerImpl::_UpdateNetworkStatus()
     // Thread is not enabled, then we are not trying to connect to the network.
     VerifyOrReturn(IsThreadEnabled() && mpStatusChangeCallback != nullptr);
 
-    ByteSpan datasetTLV;
     Thread::OperationalDataset dataset;
     uint8_t extpanid[Thread::kSizeExtendedPanId];
 
     // If we have not provisioned any Thread network, return the status from last network scan,
     // If we have provisioned a network, we assume the ot-br-posix is activitely connecting to that network.
-    CHIP_ERROR err = ThreadStackMgrImpl().GetThreadProvision(datasetTLV);
+    CHIP_ERROR err = ThreadStackMgrImpl().GetThreadProvision(dataset);
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(DeviceLayer, "Failed to get configured network when updating network status: %s", err.AsString());
         return;
     }
 
-    VerifyOrReturn(dataset.Init(datasetTLV) == CHIP_NO_ERROR);
     // The Thread network is not enabled, but has a different extended pan id.
     VerifyOrReturn(dataset.GetExtendedPanId(extpanid) == CHIP_NO_ERROR);
 

diff --git a/src/platform/Linux/ThreadStackManagerImpl.h b/src/platform/Linux/ThreadStackManagerImpl.h
@@ -54,7 +54,7 @@ class ThreadStackManagerImpl : public ThreadStackManager
 
     void _OnPlatformEvent(const ChipDeviceEvent * event);
 
-    CHIP_ERROR _GetThreadProvision(ByteSpan & netInfo);
+    CHIP_ERROR _GetThreadProvision(Thread::OperationalDataset & dataset);
 
     CHIP_ERROR _SetThreadProvision(ByteSpan netInfo);
 

diff --git a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp b/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp
@@ -37,14 +37,12 @@ namespace NetworkCommissioning {
 
 CHIP_ERROR GenericThreadDriver::Init(Internal::BaseDriver::NetworkStatusChangeCallback * statusChangeCallback)
 {
-    ByteSpan currentProvision;
     ThreadStackMgrImpl().SetNetworkStatusChangeCallback(statusChangeCallback);
 
     VerifyOrReturnError(ThreadStackMgrImpl().IsThreadAttached(), CHIP_NO_ERROR);
-    VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentProvision) == CHIP_NO_ERROR, CHIP_NO_ERROR);
+    VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(mStagingNetwork) == CHIP_NO_ERROR, CHIP_NO_ERROR);
 
-    mSavedNetwork.Init(currentProvision);
-    mStagingNetwork.Init(currentProvision);
+    mSavedNetwork.Init(mStagingNetwork.AsByteSpan());
 
     return CHIP_NO_ERROR;
 }
@@ -182,14 +180,12 @@ bool GenericThreadDriver::ThreadNetworkIterator::Next(Network & item)
     item.connected    = false;
     exhausted         = true;
 
-    ByteSpan currentProvision;
     Thread::OperationalDataset currentDataset;
     uint8_t enabledExtPanId[Thread::kSizeExtendedPanId];
 
     // The Thread network is not actually enabled.
     VerifyOrReturnError(ConnectivityMgrImpl().IsThreadAttached(), true);
-    VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentProvision) == CHIP_NO_ERROR, true);
-    VerifyOrReturnError(currentDataset.Init(currentProvision) == CHIP_NO_ERROR, true);
+    VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentDataset) == CHIP_NO_ERROR, true);
     // The Thread network is not enabled, but has a different extended pan id.
     VerifyOrReturnError(currentDataset.GetExtendedPanId(enabledExtPanId) == CHIP_NO_ERROR, true);
     VerifyOrReturnError(memcmp(extpanid, enabledExtPanId, kSizeExtendedPanId) == 0, true);

diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp
@@ -304,7 +304,7 @@ bool GenericThreadStackManagerImpl_OpenThread<ImplClass>::_IsThreadProvisioned(v
 }
 
 template <class ImplClass>
-CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_GetThreadProvision(ByteSpan & netInfo)
+CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_GetThreadProvision(Thread::OperationalDataset & dataset)
 {
     VerifyOrReturnError(Impl()->IsThreadProvisioned(), CHIP_ERROR_INCORRECT_STATE);
     otOperationalDatasetTlvs datasetTlv;
@@ -317,8 +317,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_GetThreadProvis
         return MapOpenThreadError(otErr);
     }
 
-    ReturnErrorOnFailure(mActiveDataset.Init(ByteSpan(datasetTlv.mTlvs, datasetTlv.mLength)));
-    netInfo = mActiveDataset.AsByteSpan();
+    ReturnErrorOnFailure(dataset.Init(ByteSpan(datasetTlv.mTlvs, datasetTlv.mLength)));
 
     return CHIP_NO_ERROR;
 }
@@ -1853,24 +1852,23 @@ void GenericThreadStackManagerImpl_OpenThread<ImplClass>::_UpdateNetworkStatus()
 
     ByteSpan datasetTLV;
     Thread::OperationalDataset dataset;
-    uint8_t extpanid[chip::Thread::kSizeExtendedPanId];
+    ByteSpan extpanid;
 
     // If we have not provisioned any Thread network, return the status from last network scan,
     // If we have provisioned a network, we assume the ot-br-posix is activitely connecting to that network.
-    ReturnOnFailure(ThreadStackMgrImpl().GetThreadProvision(datasetTLV));
-    ReturnOnFailure(dataset.Init(datasetTLV));
+    ReturnOnFailure(ThreadStackMgrImpl().GetThreadProvision(dataset));
     // The Thread network is not enabled, but has a different extended pan id.
-    ReturnOnFailure(dataset.GetExtendedPanId(extpanid));
+    ReturnOnFailure(dataset.GetExtendedPanIdAsByteSpan(extpanid));
     // If we don't have a valid dataset, we are not attempting to connect the network.
 
     // We have already connected to the network, thus return success.
     if (ThreadStackMgrImpl().IsThreadAttached())
     {
-        mpStatusChangeCallback->OnNetworkingStatusChange(Status::kSuccess, MakeOptional(ByteSpan(extpanid)), NullOptional);
+        mpStatusChangeCallback->OnNetworkingStatusChange(Status::kSuccess, MakeOptional(extpanid), NullOptional);
     }
     else
     {
-        mpStatusChangeCallback->OnNetworkingStatusChange(Status::kNetworkNotFound, MakeOptional(ByteSpan(extpanid)),
+        mpStatusChangeCallback->OnNetworkingStatusChange(Status::kNetworkNotFound, MakeOptional(extpanid),
                                                          MakeOptional(static_cast<int32_t>(OT_ERROR_DETACHED)));
     }
 }

diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h
@@ -87,7 +87,7 @@ class GenericThreadStackManagerImpl_OpenThread
 
     bool _IsThreadProvisioned(void);
     bool _IsThreadAttached(void);
-    CHIP_ERROR _GetThreadProvision(ByteSpan & netInfo);
+    CHIP_ERROR _GetThreadProvision(Thread::OperationalDataset & dataset);
     CHIP_ERROR _SetThreadProvision(ByteSpan netInfo);
     CHIP_ERROR _AttachToThreadNetwork(ByteSpan netInfo, NetworkCommissioning::Internal::WirelessDriver::ConnectCallback * callback);
     void _OnThreadAttachFinished(void);
@@ -149,8 +149,7 @@ class GenericThreadStackManagerImpl_OpenThread
     // ===== Private members for use by this class only.
 
     otInstance * mOTInst;
-    uint64_t mOverrunCount                    = 0;
-    Thread::OperationalDataset mActiveDataset = {};
+    uint64_t mOverrunCount = 0;
 
     NetworkCommissioning::ThreadDriver::ScanCallback * mpScanCallback;
     NetworkCommissioning::Internal::WirelessDriver::ConnectCallback * mpConnectCallback;