From 7dc645a8562f147b4ba87734ff9ff1f4dffd6a8b Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Mon, 24 Dec 2018 15:12:16 -0800 Subject: [PATCH] [hdlc] report frames smaller than FCS size as error frame (#3395) This commit changes the `Hdlc::Decoder` implementation to report non-zero length received frames shorter than FCS size as an error `OT_ERROR_PARSE` (instead of ignoring such frames during decoding). Since the recent changes in `Hdlc::Decoder` buffer model delegates the management of the frame buffer to the user of `Decoder`, clearing frame buffer after an error should be be performed by the `Decoder` user from the `FrameHandler` callback. The change in this commit addresses an issue where a small (single-byte) frame would not have been cleared from the decoder buffer and included in the next frame. This commit also updates the `test_hdlc` unit test to cover small frame error. --- src/ncp/hdlc.cpp | 15 ++++++--------- tests/unit/test_hdlc.cpp | 37 +++++++++++++++++++++++++++++++++++-- 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/ncp/hdlc.cpp b/src/ncp/hdlc.cpp index 3cff3bf56..000458b56 100644 --- a/src/ncp/hdlc.cpp +++ b/src/ncp/hdlc.cpp @@ -69,6 +69,7 @@ enum { kInitFcs = 0xffff, ///< Initial FCS value. kGoodFcs = 0xf0b8, ///< Good FCS value. + kFcsSize = 2, ///< FCS size (number of bytes). }; uint16_t UpdateFcs(uint16_t aFcs, uint8_t aByte) @@ -237,19 +238,15 @@ void Decoder::Decode(const uint8_t *aData, uint16_t aLength) case kFlagSequence: - // Ignore frames which are smaller than the size of the FCS. - if (mDecodedLength > sizeof(uint16_t)) + if (mDecodedLength > 0) { - otError error = OT_ERROR_NONE; + otError error = OT_ERROR_PARSE; - if (mFcs == kGoodFcs) + if ((mDecodedLength >= kFcsSize) && (mFcs == kGoodFcs)) { // Remove the FCS from the frame. - mWritePointer.UndoLastWrites(sizeof(uint16_t)); - } - else - { - error = OT_ERROR_PARSE; + mWritePointer.UndoLastWrites(kFcsSize); + error = OT_ERROR_NONE; } mFrameHandler(mContext, error); diff --git a/tests/unit/test_hdlc.cpp b/tests/unit/test_hdlc.cpp index 1e9c23434..dbae6bda0 100644 --- a/tests/unit/test_hdlc.cpp +++ b/tests/unit/test_hdlc.cpp @@ -360,6 +360,7 @@ void TestEncoderDecoder(void) Hdlc::Decoder decoder(decoderBuffer, ProcessDecodedFrame, &decoderContext); uint8_t * frame; uint16_t length; + uint8_t badShortFrame[3] = {kFlagSequence, 0xaa, kFlagSequence}; printf("Testing Hdlc::Encoder and Hdlc::Decoder"); @@ -383,13 +384,18 @@ void TestEncoderDecoder(void) SuccessOrQuit(encoder.EndFrame(), "Encoder::EndFrame() failed"); encoderBuffer.SaveFrame(); + SuccessOrQuit(encoder.BeginFrame(), "Encoder::BeginFrame() failed"); + // Empty frame + SuccessOrQuit(encoder.EndFrame(), "Encoder::EndFrame() failed"); + encoderBuffer.SaveFrame(); + byte = kFlagSequence; SuccessOrQuit(encoder.BeginFrame(), "Encoder::BeginFrame() failed"); SuccessOrQuit(encoder.Encode(&byte, sizeof(uint8_t)), "Encoder::Encode() failed"); SuccessOrQuit(encoder.EndFrame(), "Encoder::EndFrame() failed"); encoderBuffer.SaveFrame(); - // Feed the encoded frame to decoder and saved the content + // Feed the encoded frames to decoder and save the content while (encoderBuffer.ReadSavedFrame(frame, length) == OT_ERROR_NONE) { decoderContext.mWasCalled = false; @@ -402,7 +408,7 @@ void TestEncoderDecoder(void) decoderBuffer.SaveFrame(); } - // Verify the decoded frame match the original frames + // Verify the decoded frames match the original frames SuccessOrQuit(decoderBuffer.ReadSavedFrame(frame, length), "Incorrect decoded frame"); VerifyOrQuit(length == sizeof(sOpenThreadText) - 1, "Decoded frame length does not match original frame"); @@ -420,6 +426,9 @@ void TestEncoderDecoder(void) VerifyOrQuit(length == sizeof(sHelloText) - 1, "Decoded frame length does not match original frame"); VerifyOrQuit(memcmp(frame, sHelloText, length) == 0, "Decoded frame content does not match original frame"); + SuccessOrQuit(decoderBuffer.ReadSavedFrame(frame, length), "Incorrect decoded frame"); + VerifyOrQuit(length == 0, "Decoded frame length does not match original frame"); + SuccessOrQuit(decoderBuffer.ReadSavedFrame(frame, length), "Incorrect decoded frame"); VerifyOrQuit(length == sizeof(uint8_t), "Decoded frame length does not match original frame"); VerifyOrQuit(*frame == kFlagSequence, "Decoded frame content does not match original frame"); @@ -459,6 +468,30 @@ void TestEncoderDecoder(void) VerifyOrQuit(decoderContext.mWasCalled, "Decoder::Decode() failed"); VerifyOrQuit(decoderContext.mError == OT_ERROR_PARSE, "Decoder::Decode() did not fail with bad FCS"); + decoderBuffer.Clear(); + + // Test `Decoder` behavior with short frame (smaller than FCS) + + decoderContext.mWasCalled = false; + decoder.Decode(badShortFrame, sizeof(badShortFrame)); + VerifyOrQuit(decoderContext.mWasCalled, "Decoder::Decode() failed"); + VerifyOrQuit(decoderContext.mError == OT_ERROR_PARSE, "Decoder::Decode() did not fail for short frame"); + + decoderBuffer.Clear(); + + // Test `Decoder` with back to back `kFlagSequence` and ensure callback is not invoked. + + byte = kFlagSequence; + decoderContext.mWasCalled = false; + decoder.Decode(&byte, sizeof(uint8_t)); + VerifyOrQuit(!decoderContext.mWasCalled, "Decoder::Decode() failed"); + decoder.Decode(&byte, sizeof(uint8_t)); + VerifyOrQuit(!decoderContext.mWasCalled, "Decoder::Decode() failed"); + decoder.Decode(&byte, sizeof(uint8_t)); + VerifyOrQuit(!decoderContext.mWasCalled, "Decoder::Decode() failed"); + decoder.Decode(&byte, sizeof(uint8_t)); + VerifyOrQuit(!decoderContext.mWasCalled, "Decoder::Decode() failed"); + printf(" -- PASS\n"); }