diff --git a/src/fsfw/cfdp/pdu/FinishedPduReader.cpp b/src/fsfw/cfdp/pdu/FinishedPduReader.cpp index 08dd3a82..1785e57d 100644 --- a/src/fsfw/cfdp/pdu/FinishedPduReader.cpp +++ b/src/fsfw/cfdp/pdu/FinishedPduReader.cpp @@ -11,6 +11,15 @@ ReturnValue_t FinishPduReader::parseData() { size_t currentIdx = FileDirectiveReader::getHeaderSize(); const uint8_t* buf = pointers.rawPtr + currentIdx; size_t remSize = FileDirectiveReader::getWholePduSize() - currentIdx; + // Drop the PDU CRC from the parsed range before parseTlvs below, which runs until the range is + // exhausted. Without this a Finished PDU carrying a CRC is rejected as an invalid TLV type - + // including the common NO_ERROR case, where the CRC is the only thing left after the first byte. + if (getCrcFlag()) { + if (remSize < 2) { + return SerializeIF::STREAM_TOO_SHORT; + } + remSize -= 2; + } if (remSize < 1) { return SerializeIF::STREAM_TOO_SHORT; } diff --git a/src/fsfw/cfdp/pdu/MetadataPduReader.cpp b/src/fsfw/cfdp/pdu/MetadataPduReader.cpp index aaf1e08b..484eb5e6 100644 --- a/src/fsfw/cfdp/pdu/MetadataPduReader.cpp +++ b/src/fsfw/cfdp/pdu/MetadataPduReader.cpp @@ -17,6 +17,15 @@ ReturnValue_t MetadataPduReader::parseData() { size_t currentIdx = FileDirectiveReader::getHeaderSize(); const uint8_t* buf = pointers.rawPtr + currentIdx; size_t remSize = FileDirectiveReader::getWholePduSize() - currentIdx; + // The PDU CRC occupies the last two bytes of the PDU. Take it out of the parsed range up front, + // like FileDataReader does: the option loop below consumes bytes until the range is exhausted, + // so a CRC left in place would be deserialized as another TLV and rejected as an invalid type. + if (getCrcFlag()) { + if (remSize < 2) { + return SerializeIF::STREAM_TOO_SHORT; + } + remSize -= 2; + } if (remSize < 1) { return SerializeIF::STREAM_TOO_SHORT; } @@ -38,10 +47,6 @@ ReturnValue_t MetadataPduReader::parseData() { return result; } - if (getCrcFlag() && remSize == 2) { - return returnvalue::OK; - } - if (remSize > 0) { if (optionArrayMaxSize == 0 or optionArray == nullptr) { return cfdp::METADATA_CANT_PARSE_OPTIONS; diff --git a/src/fsfw/cfdp/pdu/NakPduReader.cpp b/src/fsfw/cfdp/pdu/NakPduReader.cpp index 82a5c79f..0746588b 100644 --- a/src/fsfw/cfdp/pdu/NakPduReader.cpp +++ b/src/fsfw/cfdp/pdu/NakPduReader.cpp @@ -11,6 +11,14 @@ ReturnValue_t NakPduReader::parseData() { size_t currentIdx = FileDirectiveReader::getHeaderSize(); const uint8_t* buffer = pointers.rawPtr + currentIdx; size_t remSize = FileDirectiveReader::getWholePduSize() - currentIdx; + // Drop the PDU CRC from the parsed range before the segment request loop below, which runs + // until the range is exhausted and would otherwise read the CRC as a truncated segment request. + if (getCrcFlag()) { + if (remSize < 2) { + return SerializeIF::STREAM_TOO_SHORT; + } + remSize -= 2; + } if (remSize < 1) { return SerializeIF::STREAM_TOO_SHORT; } diff --git a/unittests/cfdp/pdu/PduCrcHelper.h b/unittests/cfdp/pdu/PduCrcHelper.h new file mode 100644 index 00000000..e02b148c --- /dev/null +++ b/unittests/cfdp/pdu/PduCrcHelper.h @@ -0,0 +1,38 @@ +#ifndef FSFW_UNITTESTS_CFDP_PDU_PDUCRCHELPER_H_ +#define FSFW_UNITTESTS_CFDP_PDU_PDUCRCHELPER_H_ + +#include +#include + +#include "fsfw/globalfunctions/CRC.h" + +namespace cfdp::test { + +/** + * Turn an already serialized PDU without a CRC into one carrying a PDU CRC, in place. + * + * The PDU creators cannot produce this themselves: they append the two CRC bytes without counting + * them in the directive data field length, so the PDU they emit is inconsistent with its own + * header. Ground implementations do set the flag - cfdppy does by default - so the readers have to + * cope with it, and building the reference bytes here keeps these tests independent of that + * separate creator defect. + * + * Sets the CRC flag in the first header byte, grows the PDU data field length by two and appends + * the CRC-16/CCITT over the whole PDU, so that a CRC run across the result yields zero - which is + * what PduHeaderReader::performCrcCheckIfApplicable checks. + */ +inline void addPduCrc(uint8_t* buf, size_t& size) { + buf[0] |= 0x02; + auto dataFieldLen = static_cast((buf[1] << 8) | buf[2]); + dataFieldLen += 2; + buf[1] = (dataFieldLen >> 8) & 0xff; + buf[2] = dataFieldLen & 0xff; + uint16_t crc = CRC::crc16ccitt(buf, size); + buf[size] = (crc >> 8) & 0xff; + buf[size + 1] = crc & 0xff; + size += 2; +} + +} // namespace cfdp::test + +#endif /* FSFW_UNITTESTS_CFDP_PDU_PDUCRCHELPER_H_ */ diff --git a/unittests/cfdp/pdu/testFinishedPdu.cpp b/unittests/cfdp/pdu/testFinishedPdu.cpp index b8b395cb..cc28491a 100644 --- a/unittests/cfdp/pdu/testFinishedPdu.cpp +++ b/unittests/cfdp/pdu/testFinishedPdu.cpp @@ -1,6 +1,7 @@ #include #include +#include "PduCrcHelper.h" #include "fsfw/cfdp/pdu/FinishedPduCreator.h" #include "fsfw/cfdp/pdu/FinishedPduReader.h" #include "fsfw/globalfunctions/arrayprinter.h" @@ -187,3 +188,53 @@ TEST_CASE("Finished PDU", "[cfdp][pdu]") { } } } + +TEST_CASE("Finished PDU with CRC", "[cfdp][pdu]") { + using namespace cfdp; + std::array fnBuffer = {}; + uint8_t* buffer = fnBuffer.data(); + size_t sz = 0; + EntityId destId(WidthInBytes::TWO_BYTES, 2); + TransactionSeqNum seqNum(WidthInBytes::TWO_BYTES, 15); + EntityId sourceId(WidthInBytes::TWO_BYTES, 1); + PduConfig pduConf(sourceId, destId, TransmissionMode::ACKNOWLEDGED, seqNum); + + SECTION("Nominal completion") { + // The case every acknowledged transfer ends on: no filestore responses and no fault location, + // so the CRC is the only thing following the condition code byte. The TLV loop used to run + // straight into it and reject the PDU as an invalid TLV type. + FinishedInfo info(cfdp::ConditionCode::NO_ERROR, cfdp::FileDeliveryCode::DATA_COMPLETE, + cfdp::FileDeliveryStatus::RETAINED_IN_FILESTORE); + FinishPduCreator creator(pduConf, info); + REQUIRE(creator.serialize(&buffer, &sz, fnBuffer.size(), SerializeIF::Endianness::NETWORK) == + returnvalue::OK); + cfdp::test::addPduCrc(fnBuffer.data(), sz); + + FinishedInfo infoDeser; + FinishPduReader reader(fnBuffer.data(), sz, infoDeser); + REQUIRE(reader.parseData() == returnvalue::OK); + REQUIRE(reader.getCrcFlag()); + REQUIRE(infoDeser.getConditionCode() == cfdp::ConditionCode::NO_ERROR); + REQUIRE(infoDeser.getDeliveryCode() == cfdp::FileDeliveryCode::DATA_COMPLETE); + REQUIRE(infoDeser.getFileStatus() == cfdp::FileDeliveryStatus::RETAINED_IN_FILESTORE); + } + + SECTION("With fault location") { + EntityIdTlv faultLoc(destId); + FinishedInfo info(cfdp::ConditionCode::FILESTORE_REJECTION, + cfdp::FileDeliveryCode::DATA_INCOMPLETE, + cfdp::FileDeliveryStatus::DISCARDED_DELIBERATELY); + info.setFaultLocation(&faultLoc); + FinishPduCreator creator(pduConf, info); + REQUIRE(creator.serialize(&buffer, &sz, fnBuffer.size(), SerializeIF::Endianness::NETWORK) == + returnvalue::OK); + cfdp::test::addPduCrc(fnBuffer.data(), sz); + + EntityIdTlv faultLocDeser(destId); + FinishedInfo infoDeser; + infoDeser.setFaultLocation(&faultLocDeser); + FinishPduReader reader(fnBuffer.data(), sz, infoDeser); + REQUIRE(reader.parseData() == returnvalue::OK); + REQUIRE(infoDeser.getConditionCode() == cfdp::ConditionCode::FILESTORE_REJECTION); + } +} diff --git a/unittests/cfdp/pdu/testMetadataPdu.cpp b/unittests/cfdp/pdu/testMetadataPdu.cpp index e13b4717..bc9457ce 100644 --- a/unittests/cfdp/pdu/testMetadataPdu.cpp +++ b/unittests/cfdp/pdu/testMetadataPdu.cpp @@ -2,6 +2,7 @@ #include #include +#include "PduCrcHelper.h" #include "fsfw/cfdp/pdu/MetadataPduCreator.h" #include "fsfw/cfdp/pdu/MetadataPduReader.h" #include "fsfw/cfdp/tlv/FilestoreResponseTlv.h" @@ -222,3 +223,58 @@ TEST_CASE("Metadata PDU", "[cfdp][pdu]") { } } } + +TEST_CASE("Metadata PDU with CRC", "[cfdp][pdu]") { + using namespace cfdp; + std::array mdBuffer = {}; + uint8_t* buffer = mdBuffer.data(); + size_t sz = 0; + EntityId destId(WidthInBytes::TWO_BYTES, 2); + TransactionSeqNum seqNum(WidthInBytes::TWO_BYTES, 15); + EntityId sourceId(WidthInBytes::TWO_BYTES, 1); + PduConfig pduConf(sourceId, destId, TransmissionMode::ACKNOWLEDGED, seqNum); + Fss fileSize(0); + MetadataGenericInfo info(false, ChecksumType::NULL_CHECKSUM, fileSize); + std::array tlvDeser{}; + + SECTION("Metadata only with message to user") { + // This is the shape of a proxy put request: no file names, the request itself travels in a + // message to user TLV. Parsing it used to fail with INVALID_TLV_TYPE, because the option loop + // ran into the CRC and deserialized it as a further TLV. + cfdp::StringLv emptySourceName; + cfdp::StringLv emptyDestName; + std::array msg = {0x41, 0x42, 0x43}; + MessageToUserTlv msgToUser(msg.data(), msg.size()); + std::array options{&msgToUser}; + MetadataPduCreator creator(pduConf, info, emptySourceName, emptyDestName, options.data(), + options.size()); + creator.updateDirectiveFieldLen(); + REQUIRE(creator.serialize(&buffer, &sz, mdBuffer.size(), SerializeIF::Endianness::NETWORK) == + returnvalue::OK); + cfdp::test::addPduCrc(mdBuffer.data(), sz); + + MetadataPduReader reader(mdBuffer.data(), sz, info, tlvDeser.data(), tlvDeser.max_size()); + REQUIRE(reader.parseData() == returnvalue::OK); + REQUIRE(reader.getCrcFlag()); + REQUIRE(reader.getNumberOfParsedOptions() == 1); + REQUIRE(tlvDeser[0].getType() == cfdp::TlvType::MSG_TO_USER); + REQUIRE(tlvDeser[0].getLengthField() == msg.size()); + } + + SECTION("No options") { + // The CRC is all that follows the file names here, which the reader handled correctly even + // before the option loop was taught about it. + std::string name = "hello.txt"; + cfdp::StringLv sourceFileName(name); + cfdp::StringLv destFileName(name); + MetadataPduCreator creator(pduConf, info, sourceFileName, destFileName, nullptr, 0); + creator.updateDirectiveFieldLen(); + REQUIRE(creator.serialize(&buffer, &sz, mdBuffer.size(), SerializeIF::Endianness::NETWORK) == + returnvalue::OK); + cfdp::test::addPduCrc(mdBuffer.data(), sz); + + MetadataPduReader reader(mdBuffer.data(), sz, info, tlvDeser.data(), tlvDeser.max_size()); + REQUIRE(reader.parseData() == returnvalue::OK); + REQUIRE(reader.getNumberOfParsedOptions() == 0); + } +} diff --git a/unittests/cfdp/pdu/testNakPdu.cpp b/unittests/cfdp/pdu/testNakPdu.cpp index c3650c63..5d0ea44b 100644 --- a/unittests/cfdp/pdu/testNakPdu.cpp +++ b/unittests/cfdp/pdu/testNakPdu.cpp @@ -1,6 +1,7 @@ #include #include +#include "PduCrcHelper.h" #include "fsfw/cfdp/pdu/NakPduCreator.h" #include "fsfw/cfdp/pdu/NakPduReader.h" #include "fsfw/cfdp/pdu/PduConfig.h" @@ -150,3 +151,54 @@ TEST_CASE("NAK PDU", "[cfdp][pdu]") { REQUIRE(info.getSegmentRequestsMaxLen() == 5); } } + +TEST_CASE("NAK PDU with CRC", "[cfdp][pdu]") { + using namespace cfdp; + std::array nakBuffer = {}; + uint8_t* buffer = nakBuffer.data(); + size_t sz = 0; + EntityId destId(WidthInBytes::TWO_BYTES, 2); + TransactionSeqNum seqNum(WidthInBytes::TWO_BYTES, 15); + EntityId sourceId(WidthInBytes::TWO_BYTES, 1); + PduConfig pduConf(sourceId, destId, TransmissionMode::ACKNOWLEDGED, seqNum); + Fss startOfScope(50); + Fss endOfScope(1050); + NakInfo info(startOfScope, endOfScope); + + SECTION("With segment requests") { + std::array segReqs{ + NakInfo::SegmentRequest(cfdp::Fss(2020), cfdp::Fss(2520)), + NakInfo::SegmentRequest(cfdp::Fss(2932), cfdp::Fss(3021))}; + size_t segReqLen = segReqs.size(); + info.setSegmentRequests(segReqs.data(), &segReqLen, nullptr); + NakPduCreator creator(pduConf, info); + REQUIRE(creator.serialize(&buffer, &sz, nakBuffer.size(), SerializeIF::Endianness::NETWORK) == + returnvalue::OK); + cfdp::test::addPduCrc(nakBuffer.data(), sz); + + std::array segReqsDeser{}; + NakInfo infoDeser(Fss(0), Fss(0)); + size_t maxSegReqs = segReqsDeser.size(); + infoDeser.setSegmentRequests(segReqsDeser.data(), nullptr, &maxSegReqs); + NakPduReader reader(nakBuffer.data(), sz, infoDeser); + REQUIRE(reader.parseData() == returnvalue::OK); + REQUIRE(reader.getCrcFlag()); + REQUIRE(infoDeser.getSegmentRequestsLen() == 2); + REQUIRE(segReqsDeser[0].first.value() == 2020); + REQUIRE(segReqsDeser[1].second.value() == 3021); + } + + SECTION("Without segment requests") { + NakPduCreator creator(pduConf, info); + REQUIRE(creator.serialize(&buffer, &sz, nakBuffer.size(), SerializeIF::Endianness::NETWORK) == + returnvalue::OK); + cfdp::test::addPduCrc(nakBuffer.data(), sz); + + NakInfo infoDeser(Fss(0), Fss(0)); + NakPduReader reader(nakBuffer.data(), sz, infoDeser); + REQUIRE(reader.parseData() == returnvalue::OK); + REQUIRE(infoDeser.getStartOfScope().value() == 50); + REQUIRE(infoDeser.getEndOfScope().value() == 1050); + REQUIRE(infoDeser.getSegmentRequestsLen() == 0); + } +}