diff --git a/CHANGELOG.md b/CHANGELOG.md index c85a23d..a657fd6 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/cfdppy/handler/dest.py b/src/cfdppy/handler/dest.py index 6ae3cf6..34b24fb 100644 --- a/src/cfdppy/handler/dest.py +++ b/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: diff --git a/tests/test_dest_handler.py b/tests/test_dest_handler.py index 9acce7b..73fb2ea 100644 --- a/tests/test_dest_handler.py +++ b/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) diff --git a/tests/test_dest_handler_acked.py b/tests/test_dest_handler_acked.py index 02673bd..8f04314 100644 --- a/tests/test_dest_handler_acked.py +++ b/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, diff --git a/tests/test_dest_handler_naked.py b/tests/test_dest_handler_naked.py index bbc48d5..e50fef8 100644 --- a/tests/test_dest_handler_naked.py +++ b/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,