From 43cb7a09cf5b3567246be7f91ba5b9985fe48819 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Mon, 15 Apr 2024 12:25:30 -0700 Subject: [PATCH] [dataset] enforce maximum delay timer value and improve handling (#10023) This commit makes the following improvements to Delay Timer TLV handling in Pending Operational Datasets: - Enforces maximum delay adhering to the Thread spec's maximum Delay Timer value (72 hours), clamping larger values. - Defines min, max, and default delay timer constants in `DelayTimerTlv`. - Adds `CalculateRemainingDelay()` helper method to calculate remaining delay. - Removes code handling delay values exceeding the OpenThread `Timer` limitation (now redundant due to the stricter 72-hour maximum). --- src/core/meshcop/dataset.cpp | 14 ++------ src/core/meshcop/dataset_local.cpp | 19 ++--------- src/core/meshcop/dataset_manager.cpp | 24 +------------ src/core/meshcop/dataset_manager.hpp | 6 ---- src/core/meshcop/dataset_manager_ftd.cpp | 6 ++-- src/core/meshcop/meshcop_leader.cpp | 4 +-- src/core/meshcop/meshcop_leader.hpp | 1 - src/core/meshcop/meshcop_tlvs.cpp | 17 ++++++++++ src/core/meshcop/meshcop_tlvs.hpp | 43 +++++++++++++++++++++++- 9 files changed, 69 insertions(+), 65 deletions(-) diff --git a/src/core/meshcop/dataset.cpp b/src/core/meshcop/dataset.cpp index 3605ea0cafb..b40e1527ac4 100644 --- a/src/core/meshcop/dataset.cpp +++ b/src/core/meshcop/dataset.cpp @@ -473,19 +473,9 @@ Error Dataset::AppendMleDatasetTlv(Type aType, Message &aMessage) const } else if (cur->GetType() == Tlv::kDelayTimer) { - uint32_t elapsed = TimerMilli::GetNow() - mUpdateTime; - uint32_t delayTimer = cur->ReadValueAs(); + uint32_t remainingDelay = DelayTimerTlv::CalculateRemainingDelay(*cur, mUpdateTime); - if (delayTimer > elapsed) - { - delayTimer -= elapsed; - } - else - { - delayTimer = 0; - } - - SuccessOrExit(error = Tlv::Append(aMessage, delayTimer)); + SuccessOrExit(error = Tlv::Append(aMessage, remainingDelay)); } else { diff --git a/src/core/meshcop/dataset_local.cpp b/src/core/meshcop/dataset_local.cpp index ba4c5fc5a04..7b2a62e444e 100644 --- a/src/core/meshcop/dataset_local.cpp +++ b/src/core/meshcop/dataset_local.cpp @@ -106,25 +106,10 @@ Error DatasetLocal::Read(Dataset &aDataset) const } else { - uint32_t elapsed; - uint32_t delayTimer; - Tlv *tlv = aDataset.FindTlv(Tlv::kDelayTimer); + Tlv *tlv = aDataset.FindTlv(Tlv::kDelayTimer); VerifyOrExit(tlv != nullptr); - - elapsed = TimerMilli::GetNow() - mUpdateTime; - delayTimer = tlv->ReadValueAs(); - - if (delayTimer > elapsed) - { - delayTimer -= elapsed; - } - else - { - delayTimer = 0; - } - - tlv->WriteValueAs(delayTimer); + tlv->WriteValueAs(DelayTimerTlv::CalculateRemainingDelay(*tlv, mUpdateTime)); } aDataset.mUpdateTime = TimerMilli::GetNow(); diff --git a/src/core/meshcop/dataset_manager.cpp b/src/core/meshcop/dataset_manager.cpp index 09f0682fb84..83854f02def 100644 --- a/src/core/meshcop/dataset_manager.cpp +++ b/src/core/meshcop/dataset_manager.cpp @@ -725,10 +725,7 @@ void PendingDatasetManager::StartDelayTimer(void) tlv = dataset.FindTlv(Tlv::kDelayTimer); VerifyOrExit(tlv != nullptr); - delay = tlv->ReadValueAs(); - - // the Timer implementation does not support the full 32 bit range - delay = Min(delay, Timer::kMaxDelay); + delay = Min(tlv->ReadValueAs(), DelayTimerTlv::kMaxDelay); mDelayTimer.StartAt(dataset.GetUpdateTime(), delay); LogInfo("delay timer started %lu", ToUlong(delay)); @@ -739,25 +736,9 @@ void PendingDatasetManager::StartDelayTimer(void) void PendingDatasetManager::HandleDelayTimer(void) { - Tlv *tlv; Dataset dataset; IgnoreError(Read(dataset)); - - // if the Delay Timer value is larger than what our Timer implementation can handle, we have to compute - // the remainder and wait some more. - if ((tlv = dataset.FindTlv(Tlv::kDelayTimer)) != nullptr) - { - uint32_t elapsed = mDelayTimer.GetFireTime() - dataset.GetUpdateTime(); - uint32_t delay = tlv->ReadValueAs(); - - if (elapsed < delay) - { - mDelayTimer.StartAt(mDelayTimer.GetFireTime(), delay - elapsed); - ExitNow(); - } - } - LogInfo("pending delay timer expired"); dataset.ConvertToActive(); @@ -765,9 +746,6 @@ void PendingDatasetManager::HandleDelayTimer(void) Get().Save(dataset); Clear(); - -exit: - return; } template <> diff --git a/src/core/meshcop/dataset_manager.hpp b/src/core/meshcop/dataset_manager.hpp index 5c17de7a2f4..5972397bff0 100644 --- a/src/core/meshcop/dataset_manager.hpp +++ b/src/core/meshcop/dataset_manager.hpp @@ -182,12 +182,6 @@ class DatasetManager : public InstanceLocator #endif protected: - /** - * Default Delay Timer value for a Pending Operational Dataset (ms) - * - */ - static constexpr uint32_t kDefaultDelayTimer = OPENTHREAD_CONFIG_TMF_PENDING_DATASET_DEFAULT_DELAY; - /** * Defines a generic Dataset TLV to read from a message. * diff --git a/src/core/meshcop/dataset_manager_ftd.cpp b/src/core/meshcop/dataset_manager_ftd.cpp index 5f74842ef30..897af1a4e17 100644 --- a/src/core/meshcop/dataset_manager_ftd.cpp +++ b/src/core/meshcop/dataset_manager_ftd.cpp @@ -202,11 +202,11 @@ Error DatasetManager::HandleSet(Coap::Message &aMessage, const Ip6::MessageInfo case Tlv::kDelayTimer: { - uint32_t delayTimer = datasetTlv.ReadValueAs(); + uint32_t delayTimer = Min(datasetTlv.ReadValueAs(), DelayTimerTlv::kMaxDelay); - if (doesAffectNetworkKey && delayTimer < kDefaultDelayTimer) + if (doesAffectNetworkKey && delayTimer < DelayTimerTlv::kDefaultDelay) { - delayTimer = kDefaultDelayTimer; + delayTimer = DelayTimerTlv::kDefaultDelay; } else { diff --git a/src/core/meshcop/meshcop_leader.cpp b/src/core/meshcop/meshcop_leader.cpp index 4fc83854d82..98441479601 100644 --- a/src/core/meshcop/meshcop_leader.cpp +++ b/src/core/meshcop/meshcop_leader.cpp @@ -58,7 +58,7 @@ RegisterLogModule("MeshCoPLeader"); Leader::Leader(Instance &aInstance) : InstanceLocator(aInstance) , mTimer(aInstance) - , mDelayTimerMinimal(kMinDelayTimer) + , mDelayTimerMinimal(DelayTimerTlv::kMinDelay) , mSessionId(Random::NonCrypto::GetUint16()) { } @@ -218,7 +218,7 @@ Error Leader::SetDelayTimerMinimal(uint32_t aDelayTimerMinimal) { Error error = kErrorNone; - VerifyOrExit((aDelayTimerMinimal != 0 && aDelayTimerMinimal < kMinDelayTimer), error = kErrorInvalidArgs); + VerifyOrExit((aDelayTimerMinimal != 0 && aDelayTimerMinimal < DelayTimerTlv::kMinDelay), error = kErrorInvalidArgs); mDelayTimerMinimal = aDelayTimerMinimal; exit: diff --git a/src/core/meshcop/meshcop_leader.hpp b/src/core/meshcop/meshcop_leader.hpp index 50436b3c940..54fe7e949ce 100644 --- a/src/core/meshcop/meshcop_leader.hpp +++ b/src/core/meshcop/meshcop_leader.hpp @@ -104,7 +104,6 @@ class Leader : public InstanceLocator, private NonCopyable void SetEmptyCommissionerData(void); private: - static constexpr uint32_t kMinDelayTimer = OPENTHREAD_CONFIG_TMF_PENDING_DATASET_MINIMUM_DELAY; // (msec) static constexpr uint32_t kTimeoutLeaderPetition = 50; // TIMEOUT_LEAD_PET (seconds) OT_TOOL_PACKED_BEGIN diff --git a/src/core/meshcop/meshcop_tlvs.cpp b/src/core/meshcop/meshcop_tlvs.cpp index 151abcd421e..a7f84eaa3af 100644 --- a/src/core/meshcop/meshcop_tlvs.cpp +++ b/src/core/meshcop/meshcop_tlvs.cpp @@ -158,6 +158,23 @@ const char *StateTlv::StateToString(State aState) return aState == kReject ? kStateStrings[2] : kStateStrings[aState]; } +uint32_t DelayTimerTlv::CalculateRemainingDelay(const Tlv &aDelayTimerTlv, TimeMilli aUpdateTime) +{ + uint32_t delay = Min(aDelayTimerTlv.ReadValueAs(), kMaxDelay); + uint32_t elapsed = TimerMilli::GetNow() - aUpdateTime; + + if (delay > elapsed) + { + delay -= elapsed; + } + else + { + delay = 0; + } + + return delay; +} + bool ChannelMaskTlv::IsValid(void) const { uint32_t channelMask; diff --git a/src/core/meshcop/meshcop_tlvs.hpp b/src/core/meshcop/meshcop_tlvs.hpp index 2f2b1118688..7605392dc41 100644 --- a/src/core/meshcop/meshcop_tlvs.hpp +++ b/src/core/meshcop/meshcop_tlvs.hpp @@ -611,7 +611,48 @@ typedef SimpleTlvInfo PendingTimestampTlv; * Defines Delay Timer TLV constants and types. * */ -typedef UintTlvInfo DelayTimerTlv; +class DelayTimerTlv : public UintTlvInfo +{ +public: + /** + * Minimum Delay Timer value (in msec). + * + */ + static constexpr uint32_t kMinDelay = OPENTHREAD_CONFIG_TMF_PENDING_DATASET_MINIMUM_DELAY; + + /** + * Maximum Delay Timer value (in msec). + * + */ + static constexpr uint32_t kMaxDelay = (72 * Time::kOneHourInMsec); + + /** + * Default Delay Timer value (in msec). + * + */ + static constexpr uint32_t kDefaultDelay = OPENTHREAD_CONFIG_TMF_PENDING_DATASET_DEFAULT_DELAY; + + /** + * Calculates the remaining delay in milliseconds, based on the value read from a Delay Timer TLV and the specified + * update time. + * + * Ensures that the calculated delay does not exceed `kMaxDelay`. Also accounts for time already elapsed since + * @p aUpdateTime. + * + * Caller MUST ensure that @p aDelayTimerTlv is a Delay Timer TLV, otherwise behavior is undefined. + * + * @param[in] aDelayTimerTlv The delay timer TLV to read delay from. + * @param[in] aUpdateTimer The update time of the Dataset. + * + * @return The remaining delay (in msec). + * + */ + static uint32_t CalculateRemainingDelay(const Tlv &aDelayTimerTlv, TimeMilli aUpdateTime); + + static_assert(kMinDelay <= kMaxDelay, "TMF_PENDING_DATASET_MINIMUM_DELAY is larger than max allowed"); + static_assert(kDefaultDelay <= kMaxDelay, "TMF_PENDING_DATASET_DEFAULT_DELAY is larger than max allowed"); + static_assert(kDefaultDelay >= kMinDelay, "TMF_PENDING_DATASET_DEFAULT_DELAY is smaller than min allowed"); +}; /** * Implements Channel Mask TLV generation and parsing.