From ce5e73bb9d5f38e85a608a378bbcf7059b94cbe4 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Mon, 23 Jan 2017 19:40:16 -0800 Subject: [PATCH] Ensure spinel messages are not overwritten by log messages. (#1195) This commit addresses the following issue: while `NcpBase` is in middle of forming a spinel message, an OT api could trigger a call to `otNcpWriteStream()` (writing a log message through spinel) which would then overwrite the spinel message being formed. This commit contains two changes to address this issue: - In many cases (e.g., get handler for `THREAD_NETWORK_DATA`) we call the OT api first and then form the entire spinel message afterwards. - In cases where the above approach is not possible (e.g,, get handler for `CHILD_TABLE` where we need to iteratively call an OT api to get info about the next child and append it to the current spinel message), a new flag variable is used to temporarily disable the `otNcpWriteStream`. --- src/ncp/ncp_base.cpp | 95 ++++++++++++++++++++++++++------------------ src/ncp/ncp_base.hpp | 5 ++- 2 files changed, 60 insertions(+), 40 deletions(-) diff --git a/src/ncp/ncp_base.cpp b/src/ncp/ncp_base.cpp index 320fa9c9e..43aa76e95 100644 --- a/src/ncp/ncp_base.cpp +++ b/src/ncp/ncp_base.cpp @@ -480,6 +480,7 @@ NcpBase::NcpBase(otInstance *aInstance): mAllowLocalNetworkDataChange(false), mRequireJoinExistingNetwork(false), mIsRawStreamEnabled(false), + mDisableStreamWrite(false), mFramingErrorCounter(0), mRxSpinelFrameCounter(0), @@ -522,6 +523,7 @@ void NcpBase::HandleDatagramFromStack(otMessage aMessage) { ThreadError errorCode = kThreadError_None; bool isSecure = otIsMessageLinkSecurityEnabled(aMessage); + uint16_t length = otGetMessageLength(aMessage); SuccessOrExit(errorCode = OutboundFrameBegin()); @@ -533,7 +535,7 @@ void NcpBase::HandleDatagramFromStack(otMessage aMessage) isSecure ? SPINEL_PROP_STREAM_NET : SPINEL_PROP_STREAM_NET_INSECURE, - otGetMessageLength(aMessage) + length )); SuccessOrExit(errorCode = OutboundFrameFeedMessage(aMessage)); @@ -593,7 +595,6 @@ void NcpBase::HandleRawFrame(const RadioPacket *aFrame) SuccessOrExit(errorCode = OutboundFrameBegin()); - if (aFrame->mDidTX) { flags |= SPINEL_MD_FLAG_TX; @@ -736,7 +737,7 @@ void NcpBase::HandleEnergyScanResult(otEnergyScanResult *aResult) SPINEL_SCAN_STATE_IDLE ); - // If we could not send the end of scan inidciator message now (no + // If we could not send the end of scan indicator message now (no // buffer space), we set `mShouldSignalEndOfScan` to true to send // it out when buffer space becomes available. if (errorCode != kThreadError_None) @@ -1510,15 +1511,12 @@ ThreadError NcpBase::GetPropertyHandler_CAPS(uint8_t header, spinel_prop_key_t k ThreadError errorCode = kThreadError_None; SuccessOrExit(errorCode = OutboundFrameBegin()); - SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); // Begin adding capabilities ////////////////////////////////////////////// SuccessOrExit(errorCode = OutboundFrameFeedPacked(SPINEL_DATATYPE_UINT_PACKED_S, SPINEL_CAP_NET_THREAD_1_0)); - SuccessOrExit(errorCode = OutboundFrameFeedPacked(SPINEL_DATATYPE_UINT_PACKED_S, SPINEL_CAP_COUNTERS)); - SuccessOrExit(errorCode = OutboundFrameFeedPacked(SPINEL_DATATYPE_UINT_PACKED_S, SPINEL_CAP_MAC_WHITELIST)); #if OPENTHREAD_ENABLE_JAM_DETECTION @@ -1717,7 +1715,6 @@ ThreadError NcpBase::GetPropertyHandler_ChannelMaskHelper(uint8_t header, spinel ThreadError errorCode = kThreadError_None; SuccessOrExit(errorCode = OutboundFrameBegin()); - SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); for (int i = 0; i < 32; i++) @@ -1953,14 +1950,15 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_NETWORK_DATA(uint8_t header, spin uint8_t network_data[255]; uint8_t network_data_len = 255; - SuccessOrExit(errorCode = OutboundFrameBegin()); - SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); otGetNetworkDataLocal( mInstance, false, // Stable? network_data, &network_data_len ); + + SuccessOrExit(errorCode = OutboundFrameBegin()); + SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); SuccessOrExit(errorCode = OutboundFrameFeedData(network_data, network_data_len)); SuccessOrExit(errorCode = OutboundFrameSend()); @@ -1974,10 +1972,6 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_STABLE_NETWORK_DATA(uint8_t heade uint8_t network_data[255]; uint8_t network_data_len = 255; - - SuccessOrExit(errorCode = OutboundFrameBegin()); - - SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); otGetNetworkDataLocal( mInstance, true, // Stable? @@ -1985,6 +1979,8 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_STABLE_NETWORK_DATA(uint8_t heade &network_data_len ); + SuccessOrExit(errorCode = OutboundFrameBegin()); + SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); SuccessOrExit(errorCode = OutboundFrameFeedData(network_data, network_data_len)); SuccessOrExit(errorCode = OutboundFrameSend()); @@ -1998,14 +1994,15 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_LEADER_NETWORK_DATA(uint8_t heade uint8_t network_data[255]; uint8_t network_data_len = 255; - SuccessOrExit(errorCode = OutboundFrameBegin()); - SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); otGetNetworkDataLeader( mInstance, false, // Stable? network_data, &network_data_len ); + + SuccessOrExit(errorCode = OutboundFrameBegin()); + SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); SuccessOrExit(errorCode = OutboundFrameFeedData(network_data, network_data_len)); SuccessOrExit(errorCode = OutboundFrameSend()); @@ -2019,10 +2016,6 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_STABLE_LEADER_NETWORK_DATA(uint8_ uint8_t network_data[255]; uint8_t network_data_len = 255; - - SuccessOrExit(errorCode = OutboundFrameBegin()); - - SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); otGetNetworkDataLeader( mInstance, true, // Stable? @@ -2030,6 +2023,8 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_STABLE_LEADER_NETWORK_DATA(uint8_ &network_data_len ); + SuccessOrExit(errorCode = OutboundFrameBegin()); + SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); SuccessOrExit(errorCode = OutboundFrameFeedData(network_data, network_data_len)); SuccessOrExit(errorCode = OutboundFrameSend()); @@ -2128,6 +2123,8 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_CHILD_TABLE(uint8_t header, spine uint8_t index; uint8_t modeFlags; + mDisableStreamWrite = true; + SuccessOrExit(errorCode = OutboundFrameBegin()); SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); @@ -2188,6 +2185,7 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_CHILD_TABLE(uint8_t header, spine SuccessOrExit(errorCode = OutboundFrameSend()); exit: + mDisableStreamWrite = false; return errorCode; } @@ -2198,6 +2196,8 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_NEIGHBOR_TABLE(uint8_t header, sp otNeighborInfo neighInfo; uint8_t modeFlags; + mDisableStreamWrite = true; + SuccessOrExit(errorCode = OutboundFrameBegin()); SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); @@ -2253,6 +2253,7 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_NEIGHBOR_TABLE(uint8_t header, sp SuccessOrExit(errorCode = OutboundFrameSend()); exit: + mDisableStreamWrite = false; return errorCode; } @@ -2263,7 +2264,6 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_ASSISTING_PORTS(uint8_t header, s const uint16_t *ports = otGetUnsecurePorts(mInstance, &num_entries); SuccessOrExit(errorCode = OutboundFrameBegin()); - SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); for (; num_entries != 0; ports++, num_entries--) @@ -2305,8 +2305,9 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_ON_MESH_NETS(uint8_t header, spin otBorderRouterConfig border_router_config; uint8_t flags; - SuccessOrExit(errorCode = OutboundFrameBegin()); + mDisableStreamWrite = true; + SuccessOrExit(errorCode = OutboundFrameBegin()); SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); // Fill from non-local network data first @@ -2368,6 +2369,7 @@ ThreadError NcpBase::GetPropertyHandler_THREAD_ON_MESH_NETS(uint8_t header, spin SuccessOrExit(errorCode = OutboundFrameSend()); exit: + mDisableStreamWrite = false; return errorCode; } @@ -2448,9 +2450,9 @@ ThreadError NcpBase::GetPropertyHandler_IPV6_ADDRESS_TABLE(uint8_t header, spine { ThreadError errorCode = kThreadError_None; + mDisableStreamWrite = true; + SuccessOrExit(errorCode = OutboundFrameBegin()); - - SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); for (const otNetifAddress *address = otGetUnicastAddresses(mInstance); address; address = address->mNext) @@ -2468,6 +2470,7 @@ ThreadError NcpBase::GetPropertyHandler_IPV6_ADDRESS_TABLE(uint8_t header, spine SuccessOrExit(errorCode = OutboundFrameSend()); exit: + mDisableStreamWrite = false; return errorCode; } @@ -2723,7 +2726,6 @@ ThreadError NcpBase::GetPropertyHandler_MAC_CNTR(uint8_t header, spinel_prop_key value = macCounters->mRxErrOther; break; - default: errorCode = SendLastStatus(header, SPINEL_STATUS_INTERNAL_ERROR); goto bail; @@ -2811,9 +2813,7 @@ ThreadError NcpBase::GetPropertyHandler_MSG_BUFFER_COUNTERS(uint8_t header, spin otGetMessageBufferInfo(mInstance, &bufferInfo); SuccessOrExit(errorCode = OutboundFrameBegin()); - SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); - SuccessOrExit(errorCode = OutboundFrameFeedPacked("T(SSSSSSSSSSSSSSSS)", bufferInfo.mTotalBuffers, bufferInfo.mFreeBuffers, @@ -2832,7 +2832,6 @@ ThreadError NcpBase::GetPropertyHandler_MSG_BUFFER_COUNTERS(uint8_t header, spin bufferInfo.mCoapClientMessages, bufferInfo.mCoapClientBuffers )); - SuccessOrExit(errorCode = OutboundFrameSend()); exit: @@ -2844,8 +2843,9 @@ ThreadError NcpBase::GetPropertyHandler_MAC_WHITELIST(uint8_t header, spinel_pro otMacWhitelistEntry entry; ThreadError errorCode = kThreadError_None; - SuccessOrExit(errorCode = OutboundFrameBegin()); + mDisableStreamWrite = true; + SuccessOrExit(errorCode = OutboundFrameBegin()); SuccessOrExit(errorCode = OutboundFrameFeedPacked("Cii", header, SPINEL_CMD_PROP_VALUE_IS, key)); for (uint8_t i = 0; (i != 255) && (errorCode == kThreadError_None); i++) @@ -2871,6 +2871,7 @@ ThreadError NcpBase::GetPropertyHandler_MAC_WHITELIST(uint8_t header, spinel_pro SuccessOrExit(errorCode = OutboundFrameSend()); exit: + mDisableStreamWrite = false; return errorCode; } @@ -5452,23 +5453,18 @@ exit: #endif // OPENTHREAD_ENABLE_LEGACY -} // namespace Thread - - -// ---------------------------------------------------------------------------- -// MARK: Virtual Datastream I/O (Public API) -// ---------------------------------------------------------------------------- - -ThreadError otNcpStreamWrite(int aStreamId, const uint8_t* aDataPtr, int aDataLen) +ThreadError NcpBase::StreamWrite(int aStreamId, const uint8_t *aDataPtr, int aDataLen) { + ThreadError errorCode = kThreadError_None; + if (aStreamId == 0) { aStreamId = SPINEL_PROP_STREAM_DEBUG; } - if (Thread::sNcpContext) + if (!mDisableStreamWrite) { - return Thread::sNcpContext->SendPropertyUpdate( + errorCode = SendPropertyUpdate( SPINEL_HEADER_FLAG | SPINEL_HEADER_IID_0, SPINEL_CMD_PROP_VALUE_IS, static_cast(aStreamId), @@ -5478,8 +5474,29 @@ ThreadError otNcpStreamWrite(int aStreamId, const uint8_t* aDataPtr, int aDataLe } else { - return kThreadError_InvalidState; + errorCode = kThreadError_InvalidState; } + + return errorCode; +} + +} // namespace Thread + + +// ---------------------------------------------------------------------------- +// MARK: Virtual Datastream I/O (Public API) +// ---------------------------------------------------------------------------- + +ThreadError otNcpStreamWrite(int aStreamId, const uint8_t* aDataPtr, int aDataLen) +{ + ThreadError errorCode = kThreadError_InvalidState; + + if (Thread::sNcpContext) + { + errorCode = Thread::sNcpContext->StreamWrite(aStreamId, aDataPtr, aDataLen); + } + + return errorCode; } // ---------------------------------------------------------------------------- diff --git a/src/ncp/ncp_base.hpp b/src/ncp/ncp_base.hpp index c74e6a7b9..0cbfc8136 100644 --- a/src/ncp/ncp_base.hpp +++ b/src/ncp/ncp_base.hpp @@ -213,7 +213,7 @@ private: ThreadError SendLastStatus(uint8_t header, spinel_status_t lastStatus); -public: +private: ThreadError SendPropertyUpdate(uint8_t header, uint8_t command, spinel_prop_key_t key, const uint8_t *value_ptr, uint16_t value_len); @@ -484,6 +484,8 @@ private: ThreadError RemovePropertyHandler_MAC_WHITELIST(uint8_t header, spinel_prop_key_t key, const uint8_t *value_ptr, uint16_t value_len); ThreadError RemovePropertyHandler_THREAD_ACTIVE_ROUTER_IDS(uint8_t header, spinel_prop_key_t key, const uint8_t *value_ptr, uint16_t value_len); +public: + ThreadError StreamWrite(int aStreamId, const uint8_t *aDataPtr, int aDataLen); #if OPENTHREAD_ENABLE_LEGACY public: @@ -519,6 +521,7 @@ private: bool mAllowLocalNetworkDataChange; bool mRequireJoinExistingNetwork; bool mIsRawStreamEnabled; + bool mDisableStreamWrite; uint32_t mFramingErrorCounter; // Number of improperly formed received spinel frames. uint32_t mRxSpinelFrameCounter; // Number of received (inbound) spinel frames.