Merge pull request #45 from us-irs/dest-handler-fixes-eof-cancel-handling

fixes for EOF cancel handling

Author: robamu

CHANGELOG.md

@@ -19,6 +19,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - Replaced `*Cfg*` abbreviation with `*Config*`
 
+## Fixed
+
+- Corrections for EOF (Cancel) Handling: Perform proper checks on whether the file is actually
+  completed using the supplied file size and checksum.
+
 # [v0.5.1] 2025-02-10
 
 - Bump allowed `spacepackets` to v0.28.0

src/cfdppy/handler/dest.py

@@ -616,7 +616,7 @@ def _eof_ack_pdu_done(self) -> None:
         ):
             self._start_deferred_lost_segment_handling()
             return
-        self._checksum_verify()
+        assert self._params.fp.crc32 is not None
         self.states.step = TransactionStep.TRANSFER_COMPLETION
 
     def _start_transaction_first_packet_file_data(self, fd_pdu: FileDataPdu) -> None:
@@ -902,7 +902,13 @@ def _deferred_lost_segment_handling(self) -> None:
             and not self._params.acked_params.metadata_missing
         ):
             # We are done and have received everything.
-            self._checksum_verify()
+            assert self._params.fp.crc32 is not None
+            if self._checksum_verify(self._params.fp.progress, self._params.fp.crc32):
+                self._params.finished_params.delivery_code = DeliveryCode.DATA_COMPLETE
+                self._params.finished_params.condition_code = ConditionCode.NO_ERROR
+            else:
+                self._params.finished_params.delivery_code = DeliveryCode.DATA_INCOMPLETE
+                self._params.finished_params.condition_code = ConditionCode.FILE_CHECKSUM_FAILURE
             self.states.step = TransactionStep.TRANSFER_COMPLETION
             self._params.acked_params.deferred_lost_segment_detection_active = False
             return
@@ -923,6 +929,7 @@ def _deferred_lost_segment_handling(self) -> None:
             and self._params.acked_params.nak_activity_counter + 1
             == self._params.remote_cfg.nak_timer_expiration_limit
         ):
+            self._params.finished_params.delivery_code = DeliveryCode.DATA_INCOMPLETE
             self._declare_fault(ConditionCode.NAK_LIMIT_REACHED)
             return
         # This is not the first NAK issuance and the timer expired.
@@ -983,8 +990,18 @@ def _handle_cancel_eof(self, eof_pdu: EofPdu) -> None:
             eof_pdu.condition_code,
             EntityIdTlv(self._params.remote_cfg.entity_id.as_bytes),
         )
-        # Store this as progress for the checksum calculation.
-        self._params.fp.progress = self._params.fp.file_size
+        # Store this as progress for the checksum calculation as well.
+        self._params.fp.progress = eof_pdu.file_size
+        if self._params.acked_params.metadata_missing:
+            self._params.finished_params.delivery_code = DeliveryCode.DATA_INCOMPLETE
+            return
+        if self._params.fp.file_size == 0:
+            # Empty file, no file data PDU.
+            self._params.finished_params.delivery_code = DeliveryCode.DATA_COMPLETE
+            return
+        if self._checksum_verify(self._params.fp.progress, self._params.fp.crc32):
+            self._params.finished_params.delivery_code = DeliveryCode.DATA_COMPLETE
+            return
         self._params.finished_params.delivery_code = DeliveryCode.DATA_INCOMPLETE
 
     def _handle_no_error_eof(self) -> bool:
@@ -1004,15 +1021,12 @@ def _handle_no_error_eof(self) -> bool:
             )
         if (
             self.transmission_mode == TransmissionMode.UNACKNOWLEDGED
-            and not self._checksum_verify()
+            and not self._checksum_verify(self._params.fp.progress, self._params.fp.crc32)  # type: ignore
         ):
-            if (
-                self._declare_fault(ConditionCode.FILE_CHECKSUM_FAILURE)
-                != FaultHandlerCode.IGNORE_ERROR
-            ):
-                return False
             self._start_check_limit_handling()
             return False
+        self._params.finished_params.delivery_code = DeliveryCode.DATA_COMPLETE
+        self._params.finished_params.condition_code = ConditionCode.NO_ERROR
         return True
 
     def _start_deferred_lost_segment_handling(self) -> None:
@@ -1035,27 +1049,21 @@ def _prepare_eof_ack_packet(self) -> None:
         )
         self._add_packet_to_be_sent(ack_pdu)
 
-    def _checksum_verify(self) -> bool:
-        file_delivery_complete = False
+    def _checksum_verify(self, verify_len: int, expected_crc32: bytes) -> bool:
         if (
             self._params.checksum_type == ChecksumType.NULL_CHECKSUM
             or self._params.fp.metadata_only
         ):
-            file_delivery_complete = True
-        else:
-            crc32 = self.user.vfs.calculate_checksum(
-                self._params.checksum_type,
-                self._params.fp.file_name,
-                self._params.fp.progress,
-            )
-            if crc32 == self._params.fp.crc32:
-                file_delivery_complete = True
-            else:
-                self._declare_fault(ConditionCode.FILE_CHECKSUM_FAILURE)
-        if file_delivery_complete:
-            self._params.finished_params.delivery_code = DeliveryCode.DATA_COMPLETE
-            self._params.finished_params.condition_code = ConditionCode.NO_ERROR
-        return file_delivery_complete
+            return True
+        crc32 = self.user.vfs.calculate_checksum(
+            self._params.checksum_type,
+            self._params.fp.file_name,
+            verify_len,
+        )
+        if crc32 == expected_crc32:
+            return True
+        self._declare_fault(ConditionCode.FILE_CHECKSUM_FAILURE)
+        return False
 
     def _file_transfer_complete_transition(self) -> None:
         if self.transmission_mode == TransmissionMode.UNACKNOWLEDGED:
@@ -1126,8 +1134,11 @@ def _add_packet_to_be_sent(self, packet: GenericPduPacket) -> None:
     def _check_limit_handling(self) -> None:
         assert self._params.check_timer is not None
         assert self._params.remote_cfg is not None
+        assert self._params.fp.crc32 is not None
         if self._params.check_timer.timed_out():
-            if self._checksum_verify():
+            if self._checksum_verify(self._params.fp.progress, self._params.fp.crc32):
+                self._params.finished_params.delivery_code = DeliveryCode.DATA_COMPLETE
+                self._params.finished_params.condition_code = ConditionCode.NO_ERROR
                 self._file_transfer_complete_transition()
                 return
             if self._params.current_check_count + 1 >= self._params.remote_cfg.check_limit:

tests/test_dest_handler.py

@@ -264,6 +264,7 @@ def _generic_finished_pdu_check_acked(
         expected_condition_code: ConditionCode,
         expected_file_status: FileStatus = FileStatus.FILE_RETAINED,
         expected_fault_location: EntityIdTlv | None = None,
+        empty_file: bool = False,
     ) -> FinishedPdu:
         return self._generic_finished_pdu_check(
             fsm_res,
@@ -272,6 +273,7 @@ def _generic_finished_pdu_check_acked(
             expected_file_status,
             expected_condition_code,
             expected_fault_location=expected_fault_location,
+            empty_file=empty_file,
         )
 
     def _generic_no_error_finished_pdu_check_acked(
@@ -308,6 +310,7 @@ def _generic_finished_pdu_check(
         expected_file_status: FileStatus = FileStatus.FILE_RETAINED,
         expected_condition_code: ConditionCode = ConditionCode.NO_ERROR,
         expected_fault_location: EntityIdTlv | None = None,
+        empty_file: bool = False,
     ) -> FinishedPdu:
         self._state_checker(fsm_res, 1, expected_state, expected_step)
         self.assertTrue(fsm_res.states.packets_ready)
@@ -319,7 +322,7 @@ def _generic_finished_pdu_check(
         finished_pdu = next_pdu.to_finished_pdu()
         self.assertEqual(finished_pdu.condition_code, expected_condition_code)
         self.assertEqual(finished_pdu.file_status, expected_file_status)
-        if expected_condition_code == ConditionCode.NO_ERROR:
+        if expected_condition_code == ConditionCode.NO_ERROR or empty_file:
             self.assertEqual(finished_pdu.delivery_code, DeliveryCode.DATA_COMPLETE)
         else:
             self.assertEqual(finished_pdu.delivery_code, DeliveryCode.DATA_INCOMPLETE)

tests/test_dest_handler_acked.py

@@ -57,16 +57,53 @@ def test_acked_small_file_transfer(self):
         self._generic_verify_transfer_completion(fsm_res, file_content)
         self._generic_insert_finished_pdu_ack(finished_pdu)
 
+    def test_cancelled_file_transfer_empty(self):
+        # Basic acknowledged empty file transfer.
+        self._generic_regular_transfer_init(0)
+        # Cancel the transfer by sending an EOF PDU with the appropriate parameters.
+        eof_pdu = EofPdu(
+            file_size=0,
+            file_checksum=NULL_CHECKSUM_U32,
+            pdu_conf=self.src_pdu_conf,
+            condition_code=ConditionCode.CANCEL_REQUEST_RECEIVED,
+        )
+        fsm_res = self.dest_handler.state_machine(eof_pdu)
+        # Should contain an ACK PDU now.
+        self._generic_verify_eof_ack_packet(
+            fsm_res,
+            TransactionStep.WAITING_FOR_FINISHED_ACK,
+            condition_code_of_acked_pdu=ConditionCode.CANCEL_REQUEST_RECEIVED,
+        )
+        finished_pdu = self._generic_finished_pdu_check_acked(
+            fsm_res,
+            expected_condition_code=ConditionCode.CANCEL_REQUEST_RECEIVED,
+            expected_fault_location=EntityIdTlv(self.src_entity_id.as_bytes),
+            empty_file=True,
+        )
+        # Complete, because this is just an empty file.
+        self._generic_verify_transfer_completion(
+            fsm_res,
+            expected_file_data=None,
+            expected_finished_params=FinishedParams(
+                condition_code=ConditionCode.CANCEL_REQUEST_RECEIVED,
+                delivery_code=DeliveryCode.DATA_COMPLETE,
+                file_status=FileStatus.FILE_RETAINED,
+                fault_location=EntityIdTlv(self.src_entity_id.as_bytes),
+            ),
+        )
+        self._generic_insert_finished_pdu_ack(finished_pdu)
+
     def test_cancelled_file_transfer(self):
         file_content = b"Hello World!"
         with open(self.src_file_path, "wb") as of:
             of.write(file_content)
+        crc32 = fastcrc.crc32.iso_hdlc(file_content)
         # Basic acknowledged empty file transfer.
         self._generic_regular_transfer_init(len(file_content))
         # Cancel the transfer by sending an EOF PDU with the appropriate parameters.
         eof_pdu = EofPdu(
-            file_size=0,
-            file_checksum=NULL_CHECKSUM_U32,
+            file_size=len(file_content),
+            file_checksum=crc32,
             pdu_conf=self.src_pdu_conf,
             condition_code=ConditionCode.CANCEL_REQUEST_RECEIVED,
         )
@@ -77,7 +114,6 @@ def test_cancelled_file_transfer(self):
             TransactionStep.WAITING_FOR_FINISHED_ACK,
             condition_code_of_acked_pdu=ConditionCode.CANCEL_REQUEST_RECEIVED,
         )
-        # fsm_res = self.dest_handler.state_machine()
         finished_pdu = self._generic_finished_pdu_check_acked(
             fsm_res,
             expected_condition_code=ConditionCode.CANCEL_REQUEST_RECEIVED,

tests/test_dest_handler_naked.py

@@ -150,19 +150,58 @@ def test_check_timer_mechanism(self):
             TransactionStep.IDLE,
         )
 
-    def test_cancelled_transfer_via_eof_pdu(self):
+    def test_cancelled_transfer_via_eof_pdu_complete(self):
         data = b"Hello World\n"
         with open(self.src_file_path, "wb") as of:
             of.write(data)
         file_size = self.src_file_path.stat().st_size
+        crc32 = struct.pack("!I", fastcrc.crc32.iso_hdlc(data))
         self._generic_regular_transfer_init(
             file_size=file_size,
         )
         self._insert_file_segment(segment=data, offset=0)
         # Cancel the transfer by sending an EOF PDU with the appropriate parameters.
         eof_pdu = EofPdu(
-            file_size=0,
-            file_checksum=NULL_CHECKSUM_U32,
+            file_size=len(data),
+            file_checksum=crc32,
+            pdu_conf=self.src_pdu_conf,
+            condition_code=ConditionCode.CANCEL_REQUEST_RECEIVED,
+        )
+        fsm_res = self.dest_handler.state_machine(eof_pdu)
+        self._generic_eof_recv_indication_check(fsm_res)
+        if self.closure_requested:
+            self._generic_finished_pdu_check(
+                fsm_res,
+                expected_state=CfdpState.IDLE,
+                expected_step=TransactionStep.IDLE,
+                expected_condition_code=ConditionCode.CANCEL_REQUEST_RECEIVED,
+                expected_fault_location=EntityIdTlv(self.src_entity_id.as_bytes),
+            )
+        # The data is still complete, checksum was verified successfully.
+        self._generic_verify_transfer_completion(
+            fsm_res,
+            expected_file_data=None,
+            expected_finished_params=FinishedParams(
+                condition_code=ConditionCode.CANCEL_REQUEST_RECEIVED,
+                delivery_code=DeliveryCode.DATA_COMPLETE,
+                file_status=FileStatus.FILE_RETAINED,
+                fault_location=EntityIdTlv(self.src_entity_id.as_bytes),
+            ),
+        )
+
+    def test_cancelled_transfer_via_eof_pdu_incomplete(self):
+        data = b"Hello World\n"
+        with open(self.src_file_path, "wb") as of:
+            of.write(data)
+        file_size = self.src_file_path.stat().st_size
+        crc32 = struct.pack("!I", fastcrc.crc32.iso_hdlc(data))
+        self._generic_regular_transfer_init(
+            file_size=file_size,
+        )
+        # Cancel the transfer by sending an EOF PDU with the appropriate parameters.
+        eof_pdu = EofPdu(
+            file_size=len(data),
+            file_checksum=crc32,
             pdu_conf=self.src_pdu_conf,
             condition_code=ConditionCode.CANCEL_REQUEST_RECEIVED,
         )
@@ -176,6 +215,7 @@ def test_cancelled_transfer_via_eof_pdu(self):
                 expected_condition_code=ConditionCode.CANCEL_REQUEST_RECEIVED,
                 expected_fault_location=EntityIdTlv(self.src_entity_id.as_bytes),
             )
+        # Data segment missing, checksum fails, data incomplete.
         self._generic_verify_transfer_completion(
             fsm_res,
             expected_file_data=None,