Fix metadata-only transaction handling and reset bugs

- Fix destination and source handler TransactionParams::reset only
  clearing 2 of ~11-18 fields. Stale state broke every transaction
  after the first one on a reused handler instance.
- Fix metadata-only transactions (e.g. a Proxy Put Request) to
  correctly send and expect an EOF PDU per CCSDS 727.0-B-5 4.6.1.1.9.
  The source handler no longer checksums a nonexistent source file,
  and the destination handler no longer touches a destination file
  that was never named.
- Fix a panic when a metadata PDU's Message To User TLVs overflow the
  destination handler's internal buffer. Return
  DestError::MsgsToUserBufferTooSmall instead, and bump the buffer
  from 1024 to 2048 bytes.
- Add regression tests for all of the above.
- Update the changelog.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012A4YswnSmvS9Y56WZAG9PQ
This commit is contained in:
Robin Mueller
2026-09-08 19:08:40 +02:00
co-authored by Claude Sonnet 5
parent 9ac3cc90ca
commit ea7898937f
4 changed files with 323 additions and 40 deletions
+10
View File
@@ -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
+190 -7
View File
@@ -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!";
+6 -1
View File
@@ -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());
+117 -32
View File
@@ -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,
);
}
}