cfdp: exclude the PDU CRC from variable length PDU tails

FileDataReader takes the two CRC bytes out of the parsed range before it
reads its payload. The three readers with a variable length tail did not,
and each of them walks that tail until the range is exhausted:
MetadataPduReader over its option TLVs, NakPduReader over its segment
requests, FinishPduReader over its filestore and fault location TLVs.

With crcOnTransmission set at the sender, the CRC is therefore parsed as
one more TLV or segment request and the PDU is rejected, metadata and
Finished with INVALID_TLV_TYPE. Reception of CRC bearing PDUs only ever
worked for the PDUs which have no tail at all, which is why it went
unnoticed: a plain file uplink's metadata carries no options. A proxy put
request always carries one, and in acknowledged mode so do the NAK and
Finished PDUs a ground source sends, so this broke the OBSW as a
destination for any request carrying a message to user, and as a source
for every acknowledged downlink.

MetadataPduReader had a partial guard for this - an early return when the
CRC was the only thing left - which covered the no-options case and hid
the defect for the case that has them. It is replaced by the same up front
subtraction the other two now do.

The tests build CRC bearing PDUs by hand through the new PduCrcHelper,
because the creators cannot produce one: they append the CRC without
counting it in the directive data field length, which is a separate defect
left alone here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N4eBFnanCACYWdKCzhHMcC

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;
   }

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;

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;
   }

unittests/cfdp/pdu/PduCrcHelper.h

@@ -0,0 +1,38 @@
+#ifndef FSFW_UNITTESTS_CFDP_PDU_PDUCRCHELPER_H_
+#define FSFW_UNITTESTS_CFDP_PDU_PDUCRCHELPER_H_
+
+#include <cstddef>
+#include <cstdint>
+
+#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<uint16_t>((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_ */

unittests/cfdp/pdu/testFinishedPdu.cpp

@@ -1,6 +1,7 @@
 #include <array>
 #include <catch2/catch_test_macros.hpp>
 
+#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<uint8_t, 256> 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);
+  }
+}

unittests/cfdp/pdu/testMetadataPdu.cpp

@@ -2,6 +2,7 @@
 #include <catch2/catch_test_macros.hpp>
 #include <iostream>
 
+#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<uint8_t, 256> 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<Tlv, 5> 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<uint8_t, 3> msg = {0x41, 0x42, 0x43};
+    MessageToUserTlv msgToUser(msg.data(), msg.size());
+    std::array<Tlv*, 1> 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);
+  }
+}

unittests/cfdp/pdu/testNakPdu.cpp

@@ -1,6 +1,7 @@
 #include <array>
 #include <catch2/catch_test_macros.hpp>
 
+#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<uint8_t, 256> 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<NakInfo::SegmentRequest, 2> 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<NakInfo::SegmentRequest, 4> 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);
+  }
+}