diff --git a/CHANGELOG.md b/CHANGELOG.md index 7413398..7550e97 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/dest.rs b/src/dest.rs index 0279239..6e19461 100644 --- a/src/dest.rs +++ b/src/dest.rs @@ -172,7 +172,7 @@ struct TransactionParams { 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, transaction_id: Option, metadata_params: MetadataGenericParams, @@ -226,7 +226,7 @@ impl Default for TransactionParams { 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!"; diff --git a/src/lib.rs b/src/lib.rs index 4ee8241..1499c1a 100644 --- a/src/lib.rs +++ b/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()); diff --git a/src/source.rs b/src/source.rs index b050232..0108bf2 100644 --- a/src/source.rs +++ b/src/source.rs @@ -287,8 +287,7 @@ impl Default for TransactionParams TransactionParams { #[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, + ); + } }