Merge pull request 'metadata only bugfixes' (#20) from metadata-only-bugfix into main

Reviewed-on: #20

CHANGELOG.md

@@ -16,6 +16,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   The incomplete reset left stale acknowledged-mode state behind, so a new transaction's Metadata
   PDU was mistaken for a duplicate of an already-processed one and silently dropped, breaking
   every transfer after the first one on a given destination handler instance.
+- Source handler's `TransactionParams::reset` had the same issue. A second transaction on the
+  same source handler instance could compute its EOF checksum over stale state left behind by the
+  first one.
+- Metadata-only transactions (e.g. a Proxy Put Request per CCSDS 727.0-B-5 6.1) now correctly
+  send and expect an EOF (No error) PDU, as required by 4.6.1.1.9 case (C). The source handler no
+  longer tries to checksum a source file that does not exist for this case, and the destination
+  handler no longer tries to create or truncate a destination file that was never named.
+- Destination handler no longer panics when a metadata PDU's Message To User TLVs overflow its
+  internal buffer. It now returns `DestError::MsgsToUserBufferTooSmall` instead, and the buffer
+  was bumped from 1024 to 2048 bytes.
 
 # [v0.3.0] 2025-09-25
 

src/dest.rs

@@ -172,7 +172,7 @@ struct TransactionParams<CountdownInstance: Countdown> {
     file_names: FileNames,
     msgs_to_user_size: usize,
     // TODO: Should we make this configurable?
-    msgs_to_user_buf: [u8; 1024],
+    msgs_to_user_buf: [u8; 2048],
     remote_cfg: Option<RemoteEntityConfig>,
     transaction_id: Option<TransactionId>,
     metadata_params: MetadataGenericParams,
@@ -226,7 +226,7 @@ impl<CheckTimer: Countdown> Default for TransactionParams<CheckTimer> {
             pdu_conf: Default::default(),
             msgs_to_user_size: 0,
             file_size: 0,
-            msgs_to_user_buf: [0; 1024],
+            msgs_to_user_buf: [0; 2048],
             file_names: Default::default(),
             remote_cfg: None,
             transaction_id: None,
@@ -305,6 +305,8 @@ pub enum DestError {
     InvalidRemoteConfig(RemoteEntityConfig),
     #[error("cfdp feature not implemented")]
     NotImplemented,
+    #[error("messages to user in metadata PDU exceed internal buffer size of {buf_len}")]
+    MsgsToUserBufferTooSmall { buf_len: usize },
 }
 
 /// This is the primary CFDP destination handler. It models the CFDP destination entity, which is
@@ -718,16 +720,21 @@ impl<
             self.transaction_params.file_names.dest_file_name[..dest_name.len_value()]
                 .copy_from_slice(dest_name.value());
             self.transaction_params.file_names.dest_file_name_len = dest_name.len_value();
-            self.transaction_params.msgs_to_user_size = 0;
         }
+        self.transaction_params.msgs_to_user_size = 0;
         if !metadata_pdu.options().is_empty() {
             for option_tlv in metadata_pdu.options_iter().unwrap() {
                 if option_tlv.is_standard_tlv()
                     && option_tlv.tlv_type().unwrap() == TlvType::MsgToUser
                 {
-                    self.transaction_params
-                        .msgs_to_user_buf
-                        .copy_from_slice(option_tlv.raw_data().unwrap());
+                    let raw = option_tlv.raw_data().unwrap();
+                    let start = self.transaction_params.msgs_to_user_size;
+                    let buf_len = self.transaction_params.msgs_to_user_buf.len();
+                    if start + raw.len() > buf_len {
+                        return Err(DestError::MsgsToUserBufferTooSmall { buf_len });
+                    }
+                    self.transaction_params.msgs_to_user_buf[start..start + raw.len()]
+                        .copy_from_slice(raw);
                     self.transaction_params.msgs_to_user_size += option_tlv.len_full();
                 }
             }
@@ -1641,6 +1648,16 @@ impl<
             msgs_to_user: &msgs_to_user[..num_msgs_to_user],
         };
         cfdp_user.metadata_recvd_indication(&metadata_recvd_params);
+        drop(msgs_to_user);
+
+        // A metadata-only transaction (e.g. a Proxy Put Request) has no destination file name
+        // at all, so there is nothing to create or truncate. Per CCSDS 727.0-B-5 4.6.1.1.9 the
+        // sender still issues an EOF PDU for it (case (C): no file is to be sent), so we still
+        // need to move on to receiving it rather than completing immediately.
+        if self.transaction_params.metadata_only {
+            self.set_step(TransactionStep::ReceivingFileDataPdus);
+            return Ok(());
+        }
 
         if self.vfs.exists(dest_name)? && self.vfs.is_dir(dest_name)? {
             // Create new destination path by concatenating the last part of the source source
@@ -1669,7 +1686,6 @@ impl<
             self.vfs.create_file(dest_path_str)?;
         }
         self.transaction_params.finished_params.file_status = FileStatus::Retained;
-        drop(msgs_to_user);
         self.set_step(TransactionStep::ReceivingFileDataPdus);
         Ok(())
     }
@@ -1916,6 +1932,7 @@ mod tests {
                 WritablePduPacket, finished::FinishedPduReader, metadata::MetadataPduCreator,
                 nak::NakPduReader,
             },
+            tlv::msg_to_user::MsgToUserTlv,
         },
         util::{UnsignedByteFieldU8, UnsignedEnum},
     };
@@ -2844,6 +2861,172 @@ mod tests {
         }
     }
 
+    /// Regression test for a bug where messages to user exceeding the internal buffer caused a
+    /// slice index panic instead of a graceful error. A malicious or malformed metadata PDU with
+    /// enough Message To User TLVs must not be able to crash the destination handler.
+    #[test]
+    fn test_metadata_pdu_with_oversized_msgs_to_user_returns_error() {
+        let fault_handler = TestFaultHandler::default();
+        let mut tb = DestHandlerTestbench::new_with_fixed_paths(
+            fault_handler,
+            TransmissionMode::Unacknowledged,
+            false,
+        );
+        let mut user = tb.test_user_from_cached_paths(0);
+        // 9 TLVs with the maximum value length of 255 bytes each (9 * 257 = 2313 bytes) exceed
+        // the destination handler's 2048-byte messages-to-user buffer.
+        let msg_value = [0u8; 255];
+        let mut opts_buf = [0u8; 2313];
+        let mut opts_len = 0;
+        for _ in 0..9 {
+            let msg_to_user =
+                MsgToUserTlv::new(&msg_value).expect("creating msg to user tlv failed");
+            opts_len += msg_to_user
+                .write_to_bytes(&mut opts_buf[opts_len..])
+                .expect("writing msg to user tlv failed");
+        }
+        let pdu_header = PduHeader::new_for_file_directive(tb.pdu_conf, 0);
+        let metadata_pdu = MetadataPduCreator::new_with_opts(
+            pdu_header,
+            MetadataGenericParams::new(false, ChecksumType::NullChecksum, 0),
+            Lv::new_from_str(tb.src_path.to_str().unwrap()).unwrap(),
+            Lv::new_from_str(tb.dest_path.to_str().unwrap()).unwrap(),
+            &opts_buf[..opts_len],
+        );
+        let mut pdu_buf = [0u8; 4096];
+        let packet_info = create_packet_info(&metadata_pdu, &mut pdu_buf);
+        let result = tb.handler.state_machine(&mut user, Some(&packet_info));
+        assert!(matches!(
+            result,
+            Err(DestError::MsgsToUserBufferTooSmall { .. })
+        ));
+        tb.check_dest_file = false;
+        tb.check_handler_idle_at_drop = false;
+    }
+
+    /// Regression test for a bug where `msgs_to_user_buf.copy_from_slice(..)` copied a message
+    /// TLV into the full 1024-byte buffer instead of a correctly-sized offset slice, panicking
+    /// on any metadata PDU carrying a Message To User TLV.
+    #[test]
+    fn test_metadata_pdu_with_msgs_to_user_tlv_does_not_panic() {
+        let fault_handler = TestFaultHandler::default();
+        let mut tb = DestHandlerTestbench::new_with_fixed_paths(
+            fault_handler,
+            TransmissionMode::Unacknowledged,
+            false,
+        );
+        let mut user = tb.test_user_from_cached_paths(0);
+        user.expected_msgs_to_user_count = 1;
+        let msg_value = *b"hello proxy message";
+        let msg_to_user = MsgToUserTlv::new(&msg_value).expect("creating msg to user tlv failed");
+        let mut opts_buf = [0u8; 64];
+        let opts_len = msg_to_user
+            .write_to_bytes(&mut opts_buf)
+            .expect("writing msg to user tlv failed");
+        let pdu_header = PduHeader::new_for_file_directive(tb.pdu_conf, 0);
+        let metadata_pdu = MetadataPduCreator::new_with_opts(
+            pdu_header,
+            MetadataGenericParams::new(false, ChecksumType::NullChecksum, 0),
+            Lv::new_from_str(tb.src_path.to_str().unwrap()).unwrap(),
+            Lv::new_from_str(tb.dest_path.to_str().unwrap()).unwrap(),
+            &opts_buf[..opts_len],
+        );
+        let packet_info = create_packet_info(&metadata_pdu, &mut tb.buf);
+        tb.handler
+            .state_machine(&mut user, Some(&packet_info))
+            .expect("state machine failure");
+        assert_eq!(user.metadata_recv_queue.len(), 1);
+        let metadata_recvd = user.metadata_recv_queue.pop_front().unwrap();
+        assert_eq!(metadata_recvd.msgs_to_user.len(), 1);
+        assert_eq!(metadata_recvd.msgs_to_user[0], opts_buf[..opts_len]);
+        tb.check_dest_file = false;
+        tb.check_handler_idle_at_drop = false;
+    }
+
+    /// Regression test for the CFDP Proxy Put Request use case (CCSDS 727.0-B-5 6.1): a
+    /// metadata-only transaction (no source/destination file names, only a message to user).
+    /// Two bugs made this fail: the destination handler unconditionally tried to create/
+    /// truncate a destination file even though there is no destination file name, and per
+    /// 4.6.1.1.9 case (C) the sender still issues an EOF (No error) PDU ("no file is to be
+    /// sent") which the destination must still process rather than short-circuit to completion
+    /// right after the Metadata PDU.
+    #[test]
+    fn test_metadata_only_transaction_completes_without_touching_filestore() {
+        let fault_handler = TestFaultHandler::default();
+        let mut tb = DestHandlerTestbench::new_with_fixed_paths(
+            fault_handler,
+            TransmissionMode::Unacknowledged,
+            false,
+        );
+        let mut user = tb.test_user_from_cached_paths(0);
+        user.expected_msgs_to_user_count = 1;
+        user.expected_full_src_name = String::new();
+        user.expected_full_dest_name = String::new();
+        let msg_value = *b"cfdp proxy put request placeholder";
+        let msg_to_user = MsgToUserTlv::new(&msg_value).expect("creating msg to user tlv failed");
+        let mut opts_buf = [0u8; 64];
+        let opts_len = msg_to_user
+            .write_to_bytes(&mut opts_buf)
+            .expect("writing msg to user tlv failed");
+        let pdu_header = PduHeader::new_for_file_directive(tb.pdu_conf, 0);
+        let metadata_pdu = MetadataPduCreator::new_with_opts(
+            pdu_header,
+            MetadataGenericParams::new(false, ChecksumType::NullChecksum, 0),
+            Lv::new_empty(),
+            Lv::new_empty(),
+            &opts_buf[..opts_len],
+        );
+        let packet_info = create_packet_info(&metadata_pdu, &mut tb.buf);
+        tb.handler
+            .state_machine(&mut user, Some(&packet_info))
+            .expect("state machine failure processing metadata-only PDU");
+        assert_eq!(user.metadata_recv_queue.len(), 1);
+        let metadata_recvd = user.metadata_recv_queue.pop_front().unwrap();
+        assert!(metadata_recvd.src_file_name.is_empty());
+        assert!(metadata_recvd.dest_file_name.is_empty());
+        tb.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus);
+
+        tb.generic_eof_no_error(&mut user, Vec::new())
+            .expect("EOF no error insertion failed for metadata-only transaction");
+        tb.check_dest_file = false;
+        assert_eq!(user.finished_indic_queue.len(), 1);
+        let finished_indication = user.finished_indic_queue.pop_front().unwrap();
+        assert_eq!(finished_indication.file_status, FileStatus::Unreported);
+        assert_eq!(finished_indication.delivery_code, DeliveryCode::Complete);
+        assert_eq!(finished_indication.condition_code, ConditionCode::NoError);
+    }
+
+    /// Regression test for a bug where `TransactionParams::reset` only cleared 2 of ~18 fields
+    /// (e.g. leftover `acked_params`), so a transaction reusing the same handler right after a
+    /// previous one completed had its Metadata PDU silently treated as a duplicate and dropped.
+    #[test]
+    fn test_second_transaction_after_first_completes_is_not_dropped() {
+        let file_data_str = "Hello World!";
+        let file_data = file_data_str.as_bytes();
+        let file_size = file_data.len() as u64;
+        let fault_handler = TestFaultHandler::default();
+        let mut tb = DestHandlerTestbench::new_with_fixed_paths(
+            fault_handler,
+            TransmissionMode::Acknowledged,
+            false,
+        );
+        for _ in 0..2 {
+            let mut user = tb.test_user_from_cached_paths(file_size);
+            let transfer_info = tb
+                .generic_transfer_init(&mut user, file_size)
+                .expect("transfer init failed");
+            tb.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus);
+            tb.generic_file_data_insert(&mut user, 0, file_data)
+                .expect("file data insertion failed");
+            tb.generic_eof_no_error(&mut user, file_data.to_vec())
+                .expect("EOF no error insertion failed");
+            tb.check_completion_indication_success(&mut user);
+            tb.check_eof_ack_pdu(ConditionCode::NoError);
+            tb.check_finished_pdu_success();
+            tb.acknowledge_finished_pdu(&mut user, &transfer_info);
+        }
+    }
+
     #[test]
     fn test_checksum_failure_not_acked() {
         let file_data_str = "Hello World!";

src/lib.rs

@@ -1263,6 +1263,7 @@ pub(crate) mod tests {
         pub expected_full_src_name: String,
         pub expected_full_dest_name: String,
         pub expected_file_size: u64,
+        pub expected_msgs_to_user_count: usize,
         pub transaction_indication_call_count: u32,
         pub eof_sent_call_count: u32,
         pub eof_recvd_call_count: u32,
@@ -1284,6 +1285,7 @@ pub(crate) mod tests {
                 expected_full_src_name,
                 expected_full_dest_name,
                 expected_file_size,
+                expected_msgs_to_user_count: 0,
                 transaction_indication_call_count: 0,
                 eof_recvd_call_count: 0,
                 eof_sent_call_count: 0,
@@ -1361,7 +1363,10 @@ pub(crate) mod tests {
                 String::from(md_recvd_params.dest_file_name),
                 self.expected_full_dest_name
             );
-            assert_eq!(md_recvd_params.msgs_to_user.len(), 0);
+            assert_eq!(
+                md_recvd_params.msgs_to_user.len(),
+                self.expected_msgs_to_user_count
+            );
             assert_eq!(md_recvd_params.source_id, LOCAL_ID.into());
             assert_eq!(md_recvd_params.file_size, self.expected_file_size);
             self.metadata_recv_queue.push_back(md_recvd_params.into());

src/source.rs

@@ -287,8 +287,7 @@ impl<CountdownInstance: Countdown> Default for TransactionParams<CountdownInstan
 impl<CountdownInstance: Countdown> TransactionParams<CountdownInstance> {
     #[inline]
     fn reset(&mut self) {
-        self.transaction_id = None;
-        self.transmission_mode = None;
+        *self = Self::default();
     }
 }
 
@@ -826,16 +825,22 @@ impl<
     }
 
     fn eof_fsm(&mut self, user: &mut impl CfdpUser) -> Result<(), SourceError> {
-        let checksum = self.vfs.calculate_checksum(
-            self.put_request_cacher.source_file().unwrap(),
-            self.transaction_params
-                .remote_cfg
-                .as_ref()
-                .unwrap()
-                .default_crc_type,
-            self.transaction_params.file_params.file_size,
-            self.pdu_and_cksum_buffer.borrow_mut().as_mut_slice(),
-        )?;
+        // A metadata-only transaction (e.g. a Proxy Put Request) has no source file to read, so
+        // there is nothing to checksum: it always transfers zero bytes.
+        let checksum = if self.transaction_params.file_params.metadata_only {
+            0
+        } else {
+            self.vfs.calculate_checksum(
+                self.put_request_cacher.source_file().unwrap(),
+                self.transaction_params
+                    .remote_cfg
+                    .as_ref()
+                    .unwrap()
+                    .default_crc_type,
+                self.transaction_params.file_params.file_size,
+                self.pdu_and_cksum_buffer.borrow_mut().as_mut_slice(),
+            )?
+        };
         self.transaction_params.file_params.checksum_completed_file = Some(checksum);
         self.prepare_and_send_eof_pdu(user, checksum)?;
         if self.transmission_mode().unwrap() == TransmissionMode::Unacknowledged {
@@ -982,22 +987,19 @@ impl<
         {
             return Ok(ControlFlow::Break(1));
         }
+        // Per CCSDS 727.0-B-5 4.6.1.1.9, an EOF (No error) PDU is required once the Metadata
+        // PDU and all File Data PDUs have been issued, including case (C): no file is to be
+        // sent at all (a metadata-only transaction, e.g. a Proxy Put Request). A metadata-only
+        // transaction always has progress == file_size == 0, so it naturally falls into this
+        // branch already - there is deliberately no separate `metadata_only` case here.
         if self.transaction_params.file_params.empty_file
             || self.transaction_params.file_params.progress
                 >= self.transaction_params.file_params.file_size
         {
-            // EOF is still expected.
             self.set_step(TransactionStep::SendingEof);
             self.transaction_params
                 .cond_code_eof
                 .set(Some(ConditionCode::NoError));
-        } else if self.transaction_params.file_params.metadata_only {
-            // Special case: Metadata Only, no EOF required.
-            if self.transaction_params.closure_requested {
-                self.set_step(TransactionStep::WaitingForFinished);
-            } else {
-                self.set_step(TransactionStep::NoticeOfCompletion);
-            }
         }
         Ok(ControlFlow::Continue(()))
     }
@@ -1194,17 +1196,22 @@ impl<
             .cond_code_eof
             .set(Some(condition_code));
         // As specified in 4.11.2.2, prepare an EOF PDU to be sent to the remote entity. Supply
-        // the checksum for the file copy progress sent so far.
-        let checksum = self.vfs.calculate_checksum(
-            self.put_request_cacher.source_file().unwrap(),
-            self.transaction_params
-                .remote_cfg
-                .as_ref()
-                .unwrap()
-                .default_crc_type,
-            self.transaction_params.file_params.progress,
-            self.pdu_and_cksum_buffer.borrow_mut().as_mut_slice(),
-        )?;
+        // the checksum for the file copy progress sent so far. A metadata-only transaction has
+        // no source file to read, so there is nothing to checksum.
+        let checksum = if self.transaction_params.file_params.metadata_only {
+            0
+        } else {
+            self.vfs.calculate_checksum(
+                self.put_request_cacher.source_file().unwrap(),
+                self.transaction_params
+                    .remote_cfg
+                    .as_ref()
+                    .unwrap()
+                    .default_crc_type,
+                self.transaction_params.file_params.progress,
+                self.pdu_and_cksum_buffer.borrow_mut().as_mut_slice(),
+            )?
+        };
         self.prepare_and_send_eof_pdu(user, checksum)?;
         *sent_packets += 1;
         if self.transmission_mode().unwrap() == TransmissionMode::Unacknowledged {
@@ -1289,6 +1296,7 @@ mod tests {
                 file_data::FileDataPdu, finished::FinishedPduCreator, metadata::MetadataPduReader,
                 nak::NakPduCreator,
             },
+            tlv::msg_to_user::MsgToUserTlv,
         },
         util::UnsignedByteFieldU16,
     };
@@ -1691,7 +1699,7 @@ mod tests {
                     .common_pdu_conf()
                     .transaction_seq_num
                     .value(),
-                0
+                cfdp_user.next_expected_seq_num
             );
             if self.transmission_mode == TransmissionMode::Unacknowledged {
                 if !closure_requested {
@@ -2411,4 +2419,81 @@ mod tests {
             transfer_info.id,
         );
     }
+
+    /// Regression test for the CFDP Proxy Put Request use case (CCSDS 727.0-B-5 6.1): a
+    /// metadata-only transaction (no source file) still issues an EOF (No error) PDU per
+    /// 4.6.1.1.9 case (C), and that EOF carries a checksum of 0 rather than trying to checksum
+    /// a source file that does not exist.
+    #[test]
+    fn test_metadata_only_put_request_sends_eof_with_zero_checksum() {
+        let mut tb = SourceHandlerTestbench::new(TransmissionMode::Unacknowledged, false, 512);
+        let mut user = tb.create_user(0, 0);
+        let msg_to_user =
+            MsgToUserTlv::new(b"cfdp proxy put request placeholder").expect("creating tlv failed");
+        let put_request = PutRequestOwned::new_msgs_to_user_only(REMOTE_ID.into(), &[msg_to_user])
+            .expect("creating msgs to user only put request failed");
+        tb.put_request(&put_request)
+            .expect("put_request call failed");
+        assert_eq!(tb.handler.state(), State::Busy);
+        let sent_packets = tb
+            .handler
+            .state_machine_no_packet(&mut user)
+            .expect("source handler FSM failure");
+        assert_eq!(sent_packets, 2);
+        let metadata_pdu = tb.get_next_sent_pdu().unwrap();
+        assert_eq!(metadata_pdu.pdu_type, PduType::FileDirective);
+        assert_eq!(
+            metadata_pdu.file_directive_type,
+            Some(FileDirectiveType::Metadata)
+        );
+        let metadata_pdu_reader =
+            MetadataPduReader::new(&metadata_pdu.raw_pdu).expect("invalid metadata PDU format");
+        assert!(metadata_pdu_reader.src_file_name().is_empty());
+        assert!(metadata_pdu_reader.dest_file_name().is_empty());
+        assert_eq!(metadata_pdu_reader.metadata_params().file_size, 0);
+
+        let eof_pdu_sent = tb.get_next_sent_pdu().unwrap();
+        assert_eq!(eof_pdu_sent.pdu_type, PduType::FileDirective);
+        assert_eq!(
+            eof_pdu_sent.file_directive_type,
+            Some(FileDirectiveType::Eof)
+        );
+        let eof_pdu = EofPdu::from_bytes(&eof_pdu_sent.raw_pdu).expect("invalid EOF PDU format");
+        assert_eq!(eof_pdu.condition_code(), ConditionCode::NoError);
+        assert_eq!(eof_pdu.file_size(), 0);
+        assert_eq!(eof_pdu.file_checksum(), 0);
+        // No closure was requested on the put request, so it defaults to `true` and the
+        // transaction now waits for a Finished PDU instead of idling immediately.
+        assert_eq!(tb.handler.state(), State::Busy);
+        assert_eq!(tb.handler.step(), TransactionStep::WaitingForFinished);
+        tb.check_idle_on_drop = false;
+    }
+
+    /// Regression test for a bug where the source-side `TransactionParams::reset` only cleared
+    /// 2 of 11 fields, so a second transaction reused stale state left behind by the first and
+    /// computed its EOF checksum over the wrong data.
+    #[test]
+    fn test_second_transfer_after_first_completes_has_correct_checksum() {
+        let mut tb = SourceHandlerTestbench::new(TransmissionMode::Unacknowledged, false, 512);
+        let mut first_user = TestCfdpUser::default();
+        tb.common_tiny_file_transfer(&mut first_user, false);
+
+        let mut file = OpenOptions::new()
+            .write(true)
+            .open(&tb.srcfile)
+            .expect("opening file failed");
+        let second_content = b"Goodbye World!!";
+        file.write_all(second_content)
+            .expect("writing file content failed");
+        drop(file);
+        let mut second_user = tb.create_user(1, second_content.len() as u64);
+        let (transfer_info, _fd_pdus) =
+            tb.generic_file_transfer(&mut second_user, false, second_content.to_vec());
+        second_user.verify_finished_indication(
+            DeliveryCode::Complete,
+            ConditionCode::NoError,
+            transfer_info.id,
+            FileStatus::Unreported,
+        );
+    }
 }