From ae4e5171ef27c7816c9eb30690783393e7f47e3c Mon Sep 17 00:00:00 2001 From: Yakun Xu Date: Fri, 13 Jul 2018 23:50:14 +0800 Subject: [PATCH] [posix-app] fix bugs of posix apps (#2888) 1. avoid possibility of reentrant to Hdlc::Decode 2. do not send unwanted ACK to core stack --- examples/platforms/posix/radio.c | 7 ++-- src/posix/platform/frame_queue.cpp | 24 +++++-------- src/posix/platform/frame_queue.hpp | 10 ++---- src/posix/platform/radio_spinel.cpp | 54 +++++++++++------------------ src/posix/platform/radio_spinel.hpp | 26 +++++++++++--- 5 files changed, 58 insertions(+), 63 deletions(-) diff --git a/examples/platforms/posix/radio.c b/examples/platforms/posix/radio.c index 86139b2d7..2a160ab5b 100644 --- a/examples/platforms/posix/radio.c +++ b/examples/platforms/posix/radio.c @@ -561,6 +561,7 @@ bool otPlatRadioGetPromiscuous(otInstance *aInstance) void radioReceive(otInstance *aInstance) { + bool isAck; ssize_t rval = recvfrom(sSockFd, (char *)&sReceiveMessage, sizeof(sReceiveMessage), 0, NULL, NULL); if (rval < 0) @@ -581,7 +582,9 @@ void radioReceive(otInstance *aInstance) sReceiveFrame.mLength = (uint8_t)(rval - 1); - if (sAckWait && sTransmitFrame.mChannel == sReceiveMessage.mChannel && isFrameTypeAck(sReceiveFrame.mPsdu) && + isAck = isFrameTypeAck(sReceiveFrame.mPsdu); + + if (sAckWait && sTransmitFrame.mChannel == sReceiveMessage.mChannel && isAck && getDsn(sReceiveFrame.mPsdu) == getDsn(sTransmitFrame.mPsdu)) { sState = OT_RADIO_STATE_RECEIVE; @@ -590,7 +593,7 @@ void radioReceive(otInstance *aInstance) otPlatRadioTxDone(aInstance, &sTransmitFrame, &sReceiveFrame, OT_ERROR_NONE); } else if ((sState == OT_RADIO_STATE_RECEIVE || sState == OT_RADIO_STATE_TRANSMIT) && - (sReceiveFrame.mChannel == sReceiveMessage.mChannel)) + (sReceiveFrame.mChannel == sReceiveMessage.mChannel) && (!isAck || sPromiscuous)) { radioProcessFrame(aInstance); } diff --git a/src/posix/platform/frame_queue.cpp b/src/posix/platform/frame_queue.cpp index 5231e9f38..b3f86bf02 100644 --- a/src/posix/platform/frame_queue.cpp +++ b/src/posix/platform/frame_queue.cpp @@ -38,7 +38,7 @@ #include #include -#include "code_utils.h" +#include namespace ot { @@ -48,16 +48,16 @@ otError FrameQueue::Push(const uint8_t *aFrame, uint8_t aLength) uint16_t newTail = mTail + aLength + 1; assert(aFrame != NULL); - otEXPECT_ACTION(aFrame != NULL, error = OT_ERROR_INVALID_ARGS); + VerifyOrExit(aFrame != NULL, error = OT_ERROR_INVALID_ARGS); if (mHead > mTail) { - otEXPECT_ACTION(newTail < mHead, error = OT_ERROR_NO_BUFS); + VerifyOrExit(newTail < mHead, error = OT_ERROR_NO_BUFS); } else if (newTail >= sizeof(mBuffer)) { newTail -= sizeof(mBuffer); - otEXPECT_ACTION(newTail < mHead, error = OT_ERROR_NO_BUFS); + VerifyOrExit(newTail < mHead, error = OT_ERROR_NO_BUFS); } mBuffer[mTail] = aLength; @@ -79,36 +79,28 @@ exit: return error; } -void FrameQueue::Shift(void) -{ - if (mHead != mTail) - { - mHead += 1 + mBuffer[mHead]; - mHead %= sizeof(mBuffer); - } -} - -const uint8_t *FrameQueue::Peek(uint8_t *aFrame, uint8_t &aLength) +const uint8_t *FrameQueue::Shift(uint8_t *aFrame, uint8_t &aLength) { const uint8_t *frame = NULL; uint16_t next; - otEXPECT(mHead != mTail); + VerifyOrExit(mHead != mTail); aLength = mBuffer[mHead]; next = mHead + 1 + aLength; - if (next >= sizeof(mBuffer)) { uint16_t half = sizeof(mBuffer) - mHead - 1; memcpy(aFrame, mBuffer + mHead + 1, half); memcpy(aFrame + half, mBuffer, aLength - half); frame = aFrame; + next -= sizeof(mBuffer); } else { frame = mBuffer + mHead + 1; } + mHead = next; exit: return frame; diff --git a/src/posix/platform/frame_queue.hpp b/src/posix/platform/frame_queue.hpp index 976437b7d..b77e338da 100644 --- a/src/posix/platform/frame_queue.hpp +++ b/src/posix/platform/frame_queue.hpp @@ -70,12 +70,6 @@ public: */ bool IsEmpty(void) const { return mHead == mTail; } - /** - * This method removes one frame from the head. - * - */ - void Shift(void); - /** * This method pushes one frame into the queue. * @@ -89,7 +83,7 @@ public: otError Push(const uint8_t *aFrame, uint8_t aLength); /** - * This method gets one frame at head. + * This method shifts one frame at head. * * @note aFrame is only used when necessary, always use the returned pointer to access frame data. * @@ -99,7 +93,7 @@ public: * @return A pointer to the frame. * */ - const uint8_t *Peek(uint8_t *aFrame, uint8_t &aLength); + const uint8_t *Shift(uint8_t *aFrame, uint8_t &aLength); private: enum diff --git a/src/posix/platform/radio_spinel.cpp b/src/posix/platform/radio_spinel.cpp index c92baa25b..6df593f35 100644 --- a/src/posix/platform/radio_spinel.cpp +++ b/src/posix/platform/radio_spinel.cpp @@ -147,21 +147,6 @@ static inline void SuccessOrDie(otError aError) namespace ot { -/** - * This function returns if the property changed event is *UNSAFE* to be handled during `WaitResponse()`. - * - * If property could trigger another call to `ncpSet()`, it's unsafe. - * - * @param[in] aKey The identifier of the property. - * - * @returns Whether this property is *UNSAFE* to be handled during 'WaitResponse()`. - * - */ -static bool ShouldDefer(spinel_prop_key_t aKey) -{ - return aKey == SPINEL_PROP_STREAM_RAW || aKey == SPINEL_PROP_MAC_ENERGY_SCAN_RESULT; -} - static otError SpinelStatusToOtError(spinel_status_t aError) { otError ret; @@ -375,8 +360,9 @@ RadioSpinel::RadioSpinel(void) , mTxState(kIdle) , mSockFd(-1) , mState(OT_RADIO_STATE_DISABLED) - , mAckWait(false) - , mPromiscuous(false) + , mIsAckRequested(false) + , mIsDecoding(false) + , mIsPromiscuous(false) , mIsReady(false) { } @@ -492,8 +478,7 @@ void RadioSpinel::HandleNotification(const uint8_t *aBuffer, uint16_t aLength) // Some spinel properties cannot be handled during `WaitResponse()`, we must cache these events. // `mWaitingTid` is released immediately after received the response. And `mWaitingKey` is be set // to `SPINEL_PROP_LAST_STATUS` at the end of `WaitResponse()`. - VerifyOrExit(mWaitingKey == SPINEL_PROP_LAST_STATUS || !ShouldDefer(key), - error = mFrameQueue.Push(aBuffer, aLength)); + VerifyOrExit(IsSafeToHandleNow(key), error = mFrameQueue.Push(aBuffer, aLength)); HandleValueIs(key, data, static_cast(len)); break; @@ -689,7 +674,9 @@ void RadioSpinel::ReadAll(void) if (rval > 0) { + mIsDecoding = true; mHdlcDecoder.Decode(buf, static_cast(rval)); + mIsDecoding = false; } } @@ -699,10 +686,9 @@ void RadioSpinel::ProcessFrameQueue(void) uint8_t buffer[kMaxSpinelFrame]; const uint8_t *frame; - while ((frame = mFrameQueue.Peek(buffer, length)) != NULL) + while ((frame = mFrameQueue.Shift(buffer, length)) != NULL) { HandleNotification(frame, length); - mFrameQueue.Shift(); } } @@ -713,7 +699,7 @@ void RadioSpinel::RadioReceive(void) otShortAddress shortAddress; otExtAddress extAddress; - VerifyOrExit(mPromiscuous == false, error = OT_ERROR_NONE); + VerifyOrExit(mIsPromiscuous == false, error = OT_ERROR_NONE); VerifyOrExit((mState == OT_RADIO_STATE_RECEIVE || mState == OT_RADIO_STATE_TRANSMIT), error = OT_ERROR_DROP); switch (mRxRadioFrame.mPsdu[1] & IEEE802154_DST_ADDR_MASK) @@ -790,6 +776,7 @@ void RadioSpinel::Process(const fd_set &aReadFdSet, const fd_set &aWriteFdSet) { if (FD_ISSET(mSockFd, &aReadFdSet) || !mFrameQueue.IsEmpty()) { + // Handle frames received during WaitResponse() ProcessFrameQueue(); if (FD_ISSET(mSockFd, &aReadFdSet)) @@ -801,7 +788,7 @@ void RadioSpinel::Process(const fd_set &aReadFdSet, const fd_set &aWriteFdSet) if (mState == OT_RADIO_STATE_TRANSMIT && mTxState == kDone) { mState = OT_RADIO_STATE_RECEIVE; - otPlatRadioTxDone(mInstance, mTransmitFrame, (mAckWait ? &mRxRadioFrame : NULL), mTxError); + otPlatRadioTxDone(mInstance, mTransmitFrame, (mIsAckRequested ? &mRxRadioFrame : NULL), mTxError); } } @@ -820,7 +807,7 @@ otError RadioSpinel::SetPromiscuous(bool aEnable) uint8_t mode = (aEnable ? SPINEL_MAC_PROMISCUOUS_MODE_NETWORK : SPINEL_MAC_PROMISCUOUS_MODE_OFF); SuccessOrExit(error = Set(SPINEL_PROP_MAC_PROMISCUOUS_MODE, SPINEL_DATATYPE_UINT8_S, mode)); - mPromiscuous = aEnable; + mIsPromiscuous = aEnable; exit: return error; @@ -1070,12 +1057,17 @@ void RadioSpinel::RadioTransmit(void) otPlatRadioTxStarted(mInstance, mTransmitFrame); assert(mTxState == kIdle); - mAckWait = isAckRequested(mTransmitFrame->mPsdu); - error = Request(true, SPINEL_CMD_PROP_VALUE_SET, SPINEL_PROP_STREAM_RAW, + mIsAckRequested = isAckRequested(mTransmitFrame->mPsdu); + + error = Request(true, SPINEL_CMD_PROP_VALUE_SET, SPINEL_PROP_STREAM_RAW, SPINEL_DATATYPE_DATA_WLEN_S SPINEL_DATATYPE_UINT8_S SPINEL_DATATYPE_INT8_S, mTransmitFrame->mPsdu, mTransmitFrame->mLength, mTransmitFrame->mChannel, mTransmitFrame->mInfo.mRxInfo.mRssi); - if (error) + if (error == OT_ERROR_NONE) + { + mTxState = kSent; + } + else { mState = OT_RADIO_STATE_RECEIVE; @@ -1093,10 +1085,6 @@ void RadioSpinel::RadioTransmit(void) mTxState = kIdle; } - else - { - mTxState = kSent; - } } otError RadioSpinel::WriteAll(const uint8_t *aBuffer, uint16_t aLength) @@ -1264,7 +1252,7 @@ void RadioSpinel::HandleTransmitDone(uint32_t aCommand, aBuffer += unpacked; aLength -= static_cast(unpacked); - if (mAckWait) + if (mIsAckRequested) { VerifyOrExit(aLength > 0, error = OT_ERROR_FAILED); SuccessOrExit(error = ParseRadioFrame(mRxRadioFrame, aBuffer, aLength)); @@ -1492,7 +1480,7 @@ otRadioCaps otPlatRadioGetCaps(otInstance *aInstance) bool otPlatRadioGetPromiscuous(otInstance *aInstance) { OT_UNUSED_VARIABLE(aInstance); - return sRadioSpinel.GetPromiscuous(); + return sRadioSpinel.IsPromiscuous(); } void platformRadioUpdateFdSet(fd_set *aReadFdSet, fd_set *aWriteFdSet, int *aMaxFd, struct timeval *aTimeout) diff --git a/src/posix/platform/radio_spinel.hpp b/src/posix/platform/radio_spinel.hpp index 81ea886dc..cc0061a46 100644 --- a/src/posix/platform/radio_spinel.hpp +++ b/src/posix/platform/radio_spinel.hpp @@ -71,7 +71,7 @@ public: * @retval false Promiscuous mode is disabled. * */ - bool GetPromiscuous(void) const { return mPromiscuous; } + bool IsPromiscuous(void) const { return mIsPromiscuous; } /** * This method sets the status of promiscuous mode. @@ -439,6 +439,23 @@ private: } void HandleSpinelFrame(const uint8_t *aBuffer, uint16_t aLength); + /** + * This method returns if the property changed event is safe to be handled now. + * + * If a property handler will go up to core stack, it may cause reentrant issue of `Hdlc::Decode()` and + * `WaitResponse()`. + * + * @param[in] aKey The identifier of the property. + * + * @returns Whether this property is safe to be handled now. + * + */ + bool IsSafeToHandleNow(spinel_prop_key_t aKey) const + { + return !((mIsDecoding || mWaitingKey != SPINEL_PROP_LAST_STATUS) && + (aKey == SPINEL_PROP_STREAM_RAW || aKey == SPINEL_PROP_MAC_ENERGY_SCAN_RESULT)); + } + void HandleNotification(const uint8_t *aBuffer, uint16_t aLength); void HandleValueIs(spinel_prop_key_t aKey, const uint8_t *aBuffer, uint16_t aLength); @@ -482,9 +499,10 @@ private: int mSockFd; otRadioState mState; - bool mAckWait; - bool mPromiscuous; - bool mIsReady; + bool mIsAckRequested : 1; ///< Ack requested. + bool mIsDecoding : 1; ///< Decoding hdlc frames. + bool mIsPromiscuous : 1; ///< Promiscuous mode. + bool mIsReady : 1; ///< NCP ready. }; } // namespace ot