Update and fix checksum handling

CHANGELOG.md

@@ -13,11 +13,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ## Fixed
 
 - The large file flag was not set properly in the source handler for large file transfers.
+- The CRC algorithms will now be used for empty files as well instead of hardcoding the
+  checksum type to the NULL checksum. This was a bug which did not show directly for
+  checksums like CRC32 because those have an initial value of 0x0
 
 ## Changed
 
 - Added `file_size` abstract method to `VirtualFilestore`
 - Renamed `HostFilestore` to `NativeFilestore`, but keep old name alias for backwards compatibility.
+- Added `calculate_checksum` and `verify_checksum` to `VirtualFilestore` interface.
 
 # [v0.1.2] 2024-06-04
 

src/cfdppy/filestore.py

@@ -10,7 +10,7 @@
 from crcmod.predefined import PredefinedCrc
 from spacepackets.cfdp.defs import NULL_CHECKSUM_U32, ChecksumType
 from spacepackets.cfdp.tlv import FilestoreResponseStatusCode
-from cfdppy.exceptions import ChecksumNotImplemented , SourceFileDoesNotExist
+from cfdppy.exceptions import ChecksumNotImplemented, SourceFileDoesNotExist
 
 
 _LOGGER = logging.getLogger(__name__)
@@ -108,14 +108,26 @@ def list_directory(
 
     @abc.abstractmethod
     def calc_checksum(
-        self, checksum_type: ChecksumType, file_path: Path, segment_len: int = 4096
+        self,
+        checksum_type: ChecksumType,
+        file_path: Path,
+        file_sz: int,
+        segment_len: int = 4096,
     ) -> bytes:
         pass
 
     def verify_checksum(
-        self, checksum: bytes, checksum_type: ChecksumType, file_path: Path, segment_len: int = 4096
+        self,
+        checksum: bytes,
+        checksum_type: ChecksumType,
+        file_path: Path,
+        file_sz: int,
+        segment_len: int = 4096,
     ) -> bool:
-        return self.calc_checksum(checksum_type, file_path, segment_len) == checksum
+        return (
+            self.calc_checksum(checksum_type, file_path, file_sz, segment_len)
+            == checksum
+        )
 
 
 class NativeFilestore(VirtualFilestore):
@@ -281,28 +293,30 @@ def list_directory(
             os.chdir(curr_path)
         return FilestoreResponseStatusCode.SUCCESS
 
-
     def _verify_checksum(self, checksum_type: ChecksumType):
         if checksum_type not in [
-            ChecksumType.NULL_CHECKSUM,
             ChecksumType.CRC_32,
             ChecksumType.CRC_32C,
         ]:
             raise ChecksumNotImplemented(checksum_type)
 
-    def checksum_type_to_crcmod_str(self, checksum_type: Optional[ChecksumType]) -> Optional[str]:
-        if checksum_type is None or checksum_type == ChecksumType.NULL_CHECKSUM:
-            return None
+    def checksum_type_to_crcmod_str(self, checksum_type: ChecksumType) -> Optional[str]:
         if checksum_type == ChecksumType.CRC_32:
             return "crc32"
         elif checksum_type == ChecksumType.CRC_32C:
             return "crc32c"
+        raise ChecksumNotImplemented(checksum_type)
 
     def _generate_crc_calculator(self, checksum_type: ChecksumType) -> PredefinedCrc:
-        return PredefinedCrc(self.checksum_type_to_crcmod_str(self._verify_checksum(checksum_type)))
+        self._verify_checksum(checksum_type)
+        return PredefinedCrc(self.checksum_type_to_crcmod_str(checksum_type))
 
     def calc_checksum(
-        self, checksum_type: ChecksumType, file_path: Path, segment_len: int = 4096
+        self,
+        checksum_type: ChecksumType,
+        file_path: Path,
+        file_sz: int,
+        segment_len: int = 4096,
     ) -> bytes:
         if checksum_type == ChecksumType.NULL_CHECKSUM:
             return NULL_CHECKSUM_U32
@@ -314,7 +328,6 @@ def calc_checksum(
         if not file_path.exists():
             raise SourceFileDoesNotExist(file_path)
         current_offset = 0
-        file_sz = os.path.getsize(file_path)
         # Calculate the file CRC
         with open(file_path, "rb") as file:
             while current_offset < file_sz:

src/cfdppy/handler/source.py

@@ -28,16 +28,13 @@
     UnretrievedPdusToBeSent,
 )
 from cfdppy.handler.common import _PositiveAckProcedureParams
-from cfdppy.handler.crc import CrcHelper
 from cfdppy.handler.defs import (
     _FileParamsBase,
 )
 from cfdppy.mib import CheckTimerProvider, EntityType, RemoteEntityCfgTable
 from cfdppy.request import PutRequest
 from cfdppy.user import TransactionFinishedParams, TransactionParams
 from spacepackets.cfdp import (
-    NULL_CHECKSUM_U32,
-    ChecksumType,
     ConditionCode,
     CrcFlag,
     Direction,
@@ -49,6 +46,7 @@
     TransactionId,
     TransmissionMode,
 )
+from spacepackets.cfdp.defs import ChecksumType
 from spacepackets.cfdp.pdu import (
     AbstractFileDirectiveBase,
     AckPdu,
@@ -639,7 +637,7 @@ def _prepare_metadata_pdu(self):
             assert self._params.remote_cfg is not None
             params = MetadataParams(
                 closure_requested=self._params.closure_requested,
-                checksum_type=self._params.remote_cfg.crc_type,
+                checksum_type=ChecksumType.NULL_CHECKSUM,
                 file_size=0,
                 dest_file_name=None,
                 source_file_name=None,
@@ -1006,17 +1004,13 @@ def _abandon_transaction(self):
         self.reset()
 
     def _checksum_calculation(self, size_to_calculate: int) -> bytes:
-        if self._params.fp.file_size == 0:
-            # Empty file, use null checksum
-            crc = NULL_CHECKSUM_U32
-        else:
-            assert self._put_req is not None
-            assert self._put_req.source_file is not None
-            assert self._params.remote_cfg is not None
+        assert self._put_req is not None
+        assert self._put_req.source_file is not None
+        assert self._params.remote_cfg is not None
 
-            crc = self.user.vfs.calc_checksum(
-                checksum_type=self._params.remote_cfg.crc_type,
-                file_path=self._put_req.source_file,
-                segment_len=self._params.fp.segment_len,
-            )
-        return crc
+        return self.user.vfs.calc_checksum(
+            checksum_type=self._params.remote_cfg.crc_type,
+            file_path=self._put_req.source_file,
+            file_sz=size_to_calculate,
+            segment_len=self._params.fp.segment_len,
+        )

tests/test_filestore.py

@@ -1,10 +1,32 @@
 import os.path
 from pathlib import Path
 import tempfile
+import struct
 
+from cfdppy.crc import calc_modular_checksum
 from pyfakefs.fake_filesystem_unittest import TestCase
 from cfdppy.filestore import NativeFilestore, FilestoreResult
 
+EXAMPLE_DATA_CFDP = bytes(
+    [
+        0x00,
+        0x01,
+        0x02,
+        0x03,
+        0x04,
+        0x05,
+        0x06,
+        0x07,
+        0x08,
+        0x09,
+        0x0A,
+        0x0B,
+        0x0C,
+        0x0D,
+        0x0E,
+    ]
+)
+
 
 class TestCfdpHostFilestore(TestCase):
     def setUp(self):
@@ -17,6 +39,29 @@ def setUp(self):
         self.test_list_dir_name = Path(f"{self.temp_dir}/list-dir-test.txt")
         self.filestore = NativeFilestore()
 
+        self.file_path = Path(f"{tempfile.gettempdir()}/crc_file")
+        with open(self.file_path, "wb") as file:
+            file.write(EXAMPLE_DATA_CFDP)
+        # Kind of re-writing the modular checksum impl here which we are trying to test, but the
+        # numbers/correctness were verified manually using calculators, so this is okay.
+        segments_to_add = []
+        for i in range(4):
+            if (i + 1) * 4 > len(EXAMPLE_DATA_CFDP):
+                data_to_add = EXAMPLE_DATA_CFDP[i * 4 :].ljust(4, bytes([0]))
+            else:
+                data_to_add = EXAMPLE_DATA_CFDP[i * 4 : (i + 1) * 4]
+            segments_to_add.append(
+                int.from_bytes(
+                    data_to_add,
+                    byteorder="big",
+                    signed=False,
+                )
+            )
+        full_sum = sum(segments_to_add)
+        full_sum %= 2**32
+
+        self.expected_checksum_for_example = struct.pack("!I", full_sum)
+
     def test_creation(self):
         res = self.filestore.create_file(self.test_file_name_0)
         self.assertTrue(res == FilestoreResult.CREATE_SUCCESS)
@@ -93,5 +138,11 @@ def test_list_dir(self):
         )
         self.assertTrue(res == FilestoreResult.SUCCESS)
 
+    def test_modular_checksum(self):
+        self.assertEqual(
+            calc_modular_checksum(self.file_path), self.expected_checksum_for_example
+        )
+
     def tearDown(self):
-        pass
+        if self.file_path.exists():
+            os.remove(self.file_path)

tests/test_src_handler.py

@@ -124,7 +124,8 @@ def _common_empty_file_test(
             closure_requested=None,
         )
         metadata_pdu, transaction_id = self._start_source_transaction(put_req)
-        eof_pdu = self._handle_eof_pdu(transaction_id, NULL_CHECKSUM_U32, 0)
+        crc32 = PredefinedCrc("crc32").digest()
+        eof_pdu = self._handle_eof_pdu(transaction_id, crc32, 0)
         return transaction_id, metadata_pdu, eof_pdu
 
     def _handle_eof_pdu(
@@ -236,6 +237,7 @@ def _transaction_with_file_data_wrapper(
         put_req: PutRequest,
         data: Optional[bytes],
         originating_transaction_id: Optional[TransactionId] = None,
+        crc_type: ChecksumType = ChecksumType.CRC_32,
     ) -> TransactionStartParams:
         file_size = None
         crc32 = None
@@ -245,7 +247,7 @@ def _transaction_with_file_data_wrapper(
             file_size = put_req.source_file.stat().st_size
         self.local_cfg.local_entity_id = self.source_id
         metadata_pdu, transaction_id = self._start_source_transaction(
-            put_req, originating_transaction_id
+            put_req, originating_transaction_id, crc_type
         )
         self.assertEqual(transaction_id.source_id.value, self.source_id.value)
         self.assertEqual(transaction_id.seq_num.value, self.expected_seq_num)
@@ -284,6 +286,7 @@ def _start_source_transaction(
         self,
         put_request: PutRequest,
         expected_originating_id: Optional[TransactionId] = None,
+        crc_type: ChecksumType = ChecksumType.CRC_32,
     ) -> Tuple[MetadataPdu, TransactionId]:
         self.source_handler.put_request(put_request)
         fsm_res = self.source_handler.state_machine()
@@ -305,7 +308,7 @@ def _start_source_transaction(
             self.assertEqual(
                 metadata_pdu.params.closure_requested, put_request.closure_requested
             )
-        self.assertEqual(metadata_pdu.checksum_type, ChecksumType.CRC_32)
+        self.assertEqual(metadata_pdu.checksum_type, crc_type)
         source_file_as_posix = None
         if put_request.source_file is not None:
             source_file_as_posix = put_request.source_file.as_posix()

tests/test_src_handler_nak_no_closure.py

@@ -17,6 +17,7 @@
     TransactionId,
     TransmissionMode,
 )
+from spacepackets.cfdp.defs import ChecksumType
 from spacepackets.cfdp.pdu import DeliveryCode, FileStatus
 from spacepackets.cfdp.pdu.finished import FinishedParams
 from spacepackets.cfdp.tlv import (
@@ -239,7 +240,10 @@ def test_proxy_put_response_no_originating_id(self):
         self.dest_id = ByteFieldU8(2)
         self.source_handler.source_id = self.source_id
         self._transaction_with_file_data_wrapper(
-            put_req, data=None, originating_transaction_id=None
+            put_req,
+            data=None,
+            originating_transaction_id=None,
+            crc_type=ChecksumType.NULL_CHECKSUM,
         )
         self.source_handler.state_machine()
         self._test_transaction_completion()