Skip to content

Commit

Permalink
Use safe System::Clock types in src/protocols/bdx (#10860)
Browse files Browse the repository at this point in the history 
#### Problem

Code uses plain integers to represent time values and relies on
users to get the unit scale correct.

Part of #10062 _Some operations on System::Clock types are not safe_

#### Change overview

Change code under `src/protocols/bdx` to use the safer `Clock` types.

#### Testing

CI; no change to functionality intended.
  • Loading branch information
@kpschoedel @pull
kpschoedel authored and pull[bot] committed Dec 3, 2021
1 parent 512ef94 commit 5692289
Show file tree
Hide file tree
Showing 7 changed files with 87 additions and 81 deletions.
8 changes: 4 additions & 4 deletions examples/ota-provider-app/linux/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ constexpr uint16_t kOptionQueryImageBehavior = 'q';
constexpr uint16_t kOptionDelayedActionTimeSec = 'd';

// Arbitrary BDX Transfer Params
constexpr uint32_t kMaxBdxBlockSize = 1024;
constexpr uint32_t kBdxTimeoutMs = 5 * 60 * 1000; // OTA Spec mandates >= 5 minutes
constexpr uint32_t kBdxPollFreqMs = 500;
constexpr uint32_t kMaxBdxBlockSize = 1024;
constexpr chip::System::Clock::Timeout kBdxTimeout = chip::System::Clock::Seconds16(5 * 60); // OTA Spec mandates >= 5 minutes
constexpr chip::System::Clock::Timeout kBdxPollFreq = chip::System::Clock::Milliseconds32(500);

// Global variables used for passing the CLI arguments to the OTAProviderExample object
OTAProviderExample::queryImageBehaviorType gQueryImageBehavior = OTAProviderExample::kRespondWithUpdateAvailable;
Expand Down Expand Up @@ -188,7 +188,7 @@ int main(int argc, char * argv[])
BitFlags<TransferControlFlags> bdxFlags;
bdxFlags.Set(TransferControlFlags::kReceiverDrive);
err = bdxServer.PrepareForTransfer(&chip::DeviceLayer::SystemLayer(), chip::bdx::TransferRole::kSender, bdxFlags,
kMaxBdxBlockSize, kBdxTimeoutMs, kBdxPollFreqMs);
kMaxBdxBlockSize, kBdxTimeout, kBdxPollFreq);
if (err != CHIP_NO_ERROR)
{
ChipLogError(BDX, "failed to init BDX server: %s", chip::ErrorStr(err));
Expand Down
3 changes: 2 additions & 1 deletion examples/ota-requestor-app/linux/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ void OnQueryImageResponse(void * context, uint8_t status, uint32_t delayedAction
bdxDownloader.SetInitialExchange(exchangeCtx);

// This will kick of a timer which will regularly check for updates to the bdx::TransferSession state machine.
bdxDownloader.InitiateTransfer(&chip::DeviceLayer::SystemLayer(), chip::bdx::TransferRole::kReceiver, initOptions, 20000);
bdxDownloader.InitiateTransfer(&chip::DeviceLayer::SystemLayer(), chip::bdx::TransferRole::kReceiver, initOptions,
chip::System::Clock::Seconds16(20));
}

void OnQueryFailure(void * context, uint8_t status)
Expand Down
28 changes: 14 additions & 14 deletions src/protocols/bdx/BdxTransferSession.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,17 @@ TransferSession::TransferSession()
mSuppportedXferOpts.ClearAll();
}

void TransferSession::PollOutput(OutputEvent & event, uint64_t curTimeMs)
void TransferSession::PollOutput(OutputEvent & event, System::Clock::Timestamp curTime)
{
event = OutputEvent(OutputEventType::kNone);

if (mShouldInitTimeoutStart)
{
mTimeoutStartTimeMs = curTimeMs;
mTimeoutStartTime = curTime;
mShouldInitTimeoutStart = false;
}

if (mAwaitingResponse && ((curTimeMs - mTimeoutStartTimeMs) >= mTimeoutMs))
if (mAwaitingResponse && ((curTime - mTimeoutStartTime) >= mTimeout))
{
event = OutputEvent(OutputEventType::kTransferTimeout);
mState = TransferState::kErrorState;
Expand All @@ -94,8 +94,8 @@ void TransferSession::PollOutput(OutputEvent & event, uint64_t curTimeMs)
event = OutputEvent::StatusReportEvent(OutputEventType::kStatusReceived, mStatusReportData);
break;
case OutputEventType::kMsgToSend:
event = OutputEvent::MsgToSendEvent(mMsgTypeData, std::move(mPendingMsgHandle));
mTimeoutStartTimeMs = curTimeMs;
event = OutputEvent::MsgToSendEvent(mMsgTypeData, std::move(mPendingMsgHandle));
mTimeoutStartTime = curTime;
break;
case OutputEventType::kInitReceived:
event = OutputEvent::TransferInitEvent(mTransferRequestData, std::move(mPendingMsgHandle));
Expand Down Expand Up @@ -131,12 +131,12 @@ void TransferSession::PollOutput(OutputEvent & event, uint64_t curTimeMs)
mPendingOutput = OutputEventType::kNone;
}

CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitData & initData, uint32_t timeoutMs)
CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitData & initData, System::Clock::Timeout timeout)
{
VerifyOrReturnError(mState == TransferState::kUnitialized, CHIP_ERROR_INCORRECT_STATE);

mRole = role;
mTimeoutMs = timeoutMs;
mRole = role;
mTimeout = timeout;

// Set transfer parameters. They may be overridden later by an Accept message
mSuppportedXferOpts = initData.TransferCtlFlags;
Expand Down Expand Up @@ -169,13 +169,13 @@ CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitD
}

CHIP_ERROR TransferSession::WaitForTransfer(TransferRole role, BitFlags<TransferControlFlags> xferControlOpts,
uint16_t maxBlockSize, uint32_t timeoutMs)
uint16_t maxBlockSize, System::Clock::Timeout timeout)
{
VerifyOrReturnError(mState == TransferState::kUnitialized, CHIP_ERROR_INCORRECT_STATE);

// Used to determine compatibility with any future TransferInit parameters
mRole = role;
mTimeoutMs = timeoutMs;
mTimeout = timeout;
mSuppportedXferOpts = xferControlOpts;
mMaxSupportedBlockSize = maxBlockSize;

Expand Down Expand Up @@ -361,22 +361,22 @@ void TransferSession::Reset()
mLastQueryNum = 0;
mNextQueryNum = 0;

mTimeoutMs = 0;
mTimeoutStartTimeMs = 0;
mTimeout = System::Clock::Zero;
mTimeoutStartTime = System::Clock::Zero;
mShouldInitTimeoutStart = true;
mAwaitingResponse = false;
}

CHIP_ERROR TransferSession::HandleMessageReceived(const PayloadHeader & payloadHeader, System::PacketBufferHandle msg,
uint64_t curTimeMs)
System::Clock::Timestamp curTime)
{
VerifyOrReturnError(!msg.IsNull(), CHIP_ERROR_INVALID_ARGUMENT);

if (payloadHeader.HasProtocol(Protocols::BDX::Id))
{
ReturnErrorOnFailure(HandleBdxMessage(payloadHeader, std::move(msg)));

mTimeoutStartTimeMs = curTimeMs;
mTimeoutStartTime = curTime;
}
else if (payloadHeader.HasMessageType(Protocols::SecureChannel::MsgType::StatusReport))
{
Expand Down
25 changes: 13 additions & 12 deletions src/protocols/bdx/BdxTransferSession.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,14 @@ class DLL_EXPORT TransferSession
* TransferSession object.
*
* Note that if the type outputted is kMsgToSend, the caller is expected to send the message immediately, and the session
* timeout timer will begin at curTimeMs.
* timeout timer will begin at curTime.
*
* See OutputEventType for all possible output event types.
*
* @param event Reference to an OutputEvent struct that will be filled out with any pending output data
* @param curTimeMs Current time indicated by the number of milliseconds since some epoch defined by the platform
* @param curTime Current time
*/
void PollOutput(OutputEvent & event, uint64_t curTimeMs);
void PollOutput(OutputEvent & event, System::Clock::Timestamp curTime);

/**
* @brief
Expand All @@ -162,13 +162,13 @@ class DLL_EXPORT TransferSession
* @param role Inidcates whether this object will be sending or receiving data
* @param initData Data for initializing this object and for populating a TransferInit message
* The role parameter will determine whether to populate a ReceiveInit or SendInit
* @param timeoutMs The amount of time to wait for a response before considering the transfer failed (milliseconds)
* @param curTimeMs The current time since epoch in milliseconds. Needed to set a start time for the transfer timeout.
* @param timeout The amount of time to wait for a response before considering the transfer failed
* @param curTime The current time since epoch. Needed to set a start time for the transfer timeout.
*
* @return CHIP_ERROR Result of initialization and preparation of a TransferInit message. May also indicate if the
* TransferSession object is unable to handle this request.
*/
CHIP_ERROR StartTransfer(TransferRole role, const TransferInitData & initData, uint32_t timeoutMs);
CHIP_ERROR StartTransfer(TransferRole role, const TransferInitData & initData, System::Clock::Timeout timeout);

/**
* @brief
Expand All @@ -179,13 +179,13 @@ class DLL_EXPORT TransferSession
* @param role Inidcates whether this object will be sending or receiving data
* @param xferControlOpts Indicates all supported control modes. Used to respond to a TransferInit message
* @param maxBlockSize The max Block size that this object supports.
* @param timeoutMs The amount of time to wait for a response before considering the transfer failed (milliseconds)
* @param timeout The amount of time to wait for a response before considering the transfer failed
*
* @return CHIP_ERROR Result of initialization. May also indicate if the TransferSession object is unable to handle this
* request.
*/
CHIP_ERROR WaitForTransfer(TransferRole role, BitFlags<TransferControlFlags> xferControlOpts, uint16_t maxBlockSize,
uint32_t timeoutMs);
System::Clock::Timeout timeout);

/**
* @brief
Expand Down Expand Up @@ -263,12 +263,13 @@ class DLL_EXPORT TransferSession
* @param payloadHeader A PayloadHeader containing the Protocol type and Message Type
* @param msg A PacketBufferHandle pointing to the message buffer to process. May be BDX or StatusReport protocol.
* Buffer is expected to start at data (not header).
* @param curTimeMs Current time indicated by the number of milliseconds since some epoch defined by the platform
* @param curTime Current time
*
* @return CHIP_ERROR Indicates any problems in decoding the message, or if the message is not of the BDX or StatusReport
* protocols.
*/
CHIP_ERROR HandleMessageReceived(const PayloadHeader & payloadHeader, System::PacketBufferHandle msg, uint64_t curTimeMs);
CHIP_ERROR HandleMessageReceived(const PayloadHeader & payloadHeader, System::PacketBufferHandle msg,
System::Clock::Timestamp curTime);

TransferControlFlags GetControlMode() const { return mControlMode; }
uint64_t GetStartOffset() const { return mStartOffset; }
Expand Down Expand Up @@ -349,8 +350,8 @@ class DLL_EXPORT TransferSession
uint32_t mLastQueryNum = 0;
uint32_t mNextQueryNum = 0;

uint32_t mTimeoutMs = 0;
uint64_t mTimeoutStartTimeMs = 0;
System::Clock::Timeout mTimeout{ 0 };
System::Clock::Timestamp mTimeoutStartTime{ 0 };
bool mShouldInitTimeoutStart = true;
bool mAwaitingResponse = false;
};
Expand Down
27 changes: 15 additions & 12 deletions src/protocols/bdx/TransferFacilitator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@
namespace chip {
namespace bdx {

constexpr System::Clock::Timeout TransferFacilitator::kDefaultPollFreq;
constexpr System::Clock::Timeout TransferFacilitator::kImmediatePollDelay;

CHIP_ERROR TransferFacilitator::OnMessageReceived(chip::Messaging::ExchangeContext * ec, const chip::PayloadHeader & payloadHeader,
chip::System::PacketBufferHandle && payload)
{
Expand All @@ -40,7 +43,7 @@ CHIP_ERROR TransferFacilitator::OnMessageReceived(chip::Messaging::ExchangeConte
ChipLogDetail(BDX, "%s: message " ChipLogFormatMessageType " protocol " ChipLogFormatProtocolId, __FUNCTION__,
payloadHeader.GetMessageType(), ChipLogValueProtocolId(payloadHeader.GetProtocolID()));
CHIP_ERROR err =
mTransfer.HandleMessageReceived(payloadHeader, std::move(payload), System::SystemClock().GetMonotonicMilliseconds());
mTransfer.HandleMessageReceived(payloadHeader, std::move(payload), System::SystemClock().GetMonotonicTimestamp());
if (err != CHIP_NO_ERROR)
{
ChipLogError(BDX, "failed to handle message: %s", ErrorStr(err));
Expand Down Expand Up @@ -71,44 +74,44 @@ void TransferFacilitator::PollTimerHandler(chip::System::Layer * systemLayer, vo
void TransferFacilitator::PollForOutput()
{
TransferSession::OutputEvent outEvent;
mTransfer.PollOutput(outEvent, System::SystemClock().GetMonotonicMilliseconds());
mTransfer.PollOutput(outEvent, System::SystemClock().GetMonotonicTimestamp());
HandleTransferSessionOutput(outEvent);

VerifyOrReturn(mSystemLayer != nullptr, ChipLogError(BDX, "%s mSystemLayer is null", __FUNCTION__));
mSystemLayer->StartTimer(System::Clock::Milliseconds32(mPollFreqMs), PollTimerHandler, this);
mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
}

void TransferFacilitator::ScheduleImmediatePoll()
{
VerifyOrReturn(mSystemLayer != nullptr, ChipLogError(BDX, "%s mSystemLayer is null", __FUNCTION__));
mSystemLayer->StartTimer(System::Clock::Milliseconds32(kImmediatePollDelayMs), PollTimerHandler, this);
mSystemLayer->StartTimer(System::Clock::Milliseconds32(kImmediatePollDelay), PollTimerHandler, this);
}

CHIP_ERROR Responder::PrepareForTransfer(System::Layer * layer, TransferRole role, BitFlags<TransferControlFlags> xferControlOpts,
uint16_t maxBlockSize, uint32_t timeoutMs, uint32_t pollFreqMs)
uint16_t maxBlockSize, System::Clock::Timeout timeout, System::Clock::Timeout pollFreq)
{
VerifyOrReturnError(layer != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

mPollFreqMs = pollFreqMs;
mPollFreq = pollFreq;
mSystemLayer = layer;

ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize, timeoutMs));
ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize, timeout));

mSystemLayer->StartTimer(System::Clock::Milliseconds32(mPollFreqMs), PollTimerHandler, this);
mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
return CHIP_NO_ERROR;
}

CHIP_ERROR Initiator::InitiateTransfer(System::Layer * layer, TransferRole role, const TransferSession::TransferInitData & initData,
uint32_t timeoutMs, uint32_t pollFreqMs)
System::Clock::Timeout timeout, System::Clock::Timeout pollFreq)
{
VerifyOrReturnError(layer != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

mPollFreqMs = pollFreqMs;
mPollFreq = pollFreq;
mSystemLayer = layer;

ReturnErrorOnFailure(mTransfer.StartTransfer(role, initData, timeoutMs));
ReturnErrorOnFailure(mTransfer.StartTransfer(role, initData, timeout));

mSystemLayer->StartTimer(System::Clock::Milliseconds32(mPollFreqMs), PollTimerHandler, this);
mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
return CHIP_NO_ERROR;
}

Expand Down
19 changes: 10 additions & 9 deletions src/protocols/bdx/TransferFacilitator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ namespace bdx {
class TransferFacilitator : public Messaging::ExchangeDelegate
{
public:
TransferFacilitator() : mExchangeCtx(nullptr), mSystemLayer(nullptr), mPollFreqMs(kDefaultPollFreqMs) {}
TransferFacilitator() : mExchangeCtx(nullptr), mSystemLayer(nullptr), mPollFreq(kDefaultPollFreq) {}
~TransferFacilitator() = default;

private:
Expand Down Expand Up @@ -82,9 +82,9 @@ class TransferFacilitator : public Messaging::ExchangeDelegate
TransferSession mTransfer;
Messaging::ExchangeContext * mExchangeCtx;
System::Layer * mSystemLayer;
uint32_t mPollFreqMs;
static constexpr uint32_t kDefaultPollFreqMs = 500;
static constexpr uint32_t kImmediatePollDelayMs = 1;
System::Clock::Timeout mPollFreq;
static constexpr System::Clock::Timeout kDefaultPollFreq = System::Clock::Milliseconds32(500);
static constexpr System::Clock::Timeout kImmediatePollDelay = System::Clock::Milliseconds32(1);
};

/**
Expand All @@ -103,12 +103,12 @@ class Responder : public TransferFacilitator
* @param[in] role The role of the Responder: Sender or Receiver of BDX data
* @param[in] xferControlOpts Supported transfer modes (see TransferControlFlags)
* @param[in] maxBlockSize The supported maximum size of BDX Block data
* @param[in] timeoutMs The chosen timeout delay for the BDX transfer in milliseconds
* @param[in] pollFreqMs The period for the TransferSession poll timer in milliseconds
* @param[in] timeout The chosen timeout delay for the BDX transfer
* @param[in] pollFreq The period for the TransferSession poll timer
*/
CHIP_ERROR PrepareForTransfer(System::Layer * layer, TransferRole role, BitFlags<TransferControlFlags> xferControlOpts,
uint16_t maxBlockSize, uint32_t timeoutMs,
uint32_t pollFreqMs = TransferFacilitator::kDefaultPollFreqMs);
uint16_t maxBlockSize, System::Clock::Timeout timeout,
System::Clock::Timeout pollFreq = TransferFacilitator::kDefaultPollFreq);
};

/**
Expand All @@ -131,7 +131,8 @@ class Initiator : public TransferFacilitator
* @param[in] pollFreqMs The period for the TransferSession poll timer in milliseconds
*/
CHIP_ERROR InitiateTransfer(System::Layer * layer, TransferRole role, const TransferSession::TransferInitData & initData,
uint32_t timeoutMs, uint32_t pollFreqMs = TransferFacilitator::kDefaultPollFreqMs);
System::Clock::Timeout timeout,
System::Clock::Timeout pollFreq = TransferFacilitator::kDefaultPollFreq);
};

} // namespace bdx
Expand Down
Loading

0 comments on commit 5692289

Please sign in to comment.