CCSDS File Delivery Protocol - CfdpManager#5138
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
thomas-bc
left a comment
thomas-bc
left a comment
There was a problem hiding this comment.
A few AI finds that seemed relevant, and one big question about how to handle file paths.
|
|
||
| void Engine::setChannelFlowState(U8 channelId, Flow::T flowState) | ||
| { | ||
| FW_ASSERT(channelId <= Cfdp::NumChannels, channelId, Cfdp::NumChannels); |
There was a problem hiding this comment.
AI finding: Should this be channelId < Cfdp::NumChannels instead of <= ?
There was a problem hiding this comment.
Agreed, changed assertion from channelId <= Cfdp::NumChannels to channelId < Cfdp::NumChannels to prevent out-of-bounds array access.
| U32 directiveCodeOffset = 4 + (2 * eidSize) + tsnSize; | ||
|
|
||
| // Read directive code | ||
| U8 directiveCode = data[directiveCodeOffset]; |
There was a problem hiding this comment.
AI finding: This reads at an offset based of header bytes (potentially untrusted) - we should check that there is enough length in the data before trying to access.
There was a problem hiding this comment.
Agreed, added validation check if (directiveCodeOffset < buffer.getSize()) before reading directive code from calculated offset.
|
|
||
| /* store the filenames in transaction - validation already done during deserialization */ | ||
| txn->m_history->fnames.src_filename = md.getSourceFilename(); | ||
| txn->m_history->fnames.dst_filename = md.getDestFilename(); |
There was a problem hiding this comment.
One security issue that we've had reported a lot on the F Prime Svc.FileUplink component is that it allows to write files at any location on the filesystem, potentially overwriting critical system files etc.
This is risky for in case of operator oversight (shrug) but also if you receive unauthenticated (potentially malicious) commands.
My understanding is this CfdpManager has the same capability.
Did you find that CFDP makes recommendation on how to deal with that? If not, we should agree on a rationale so that we stop the spam of "hey you have a critical vulnerability here". Maybe the answer is "encrypt/authenticate your comms". But we should agree on something.
cc @bitWarrior , if you have recommendations.
There was a problem hiding this comment.
CfdpManager was patterned using the same security model as FileUplink/FileDownlink. Authentication is expected to happen at the physical or network layers, not in the application itself. In out use-cases, CFDP traffic is being sent over authenticated channels such as encrypted radios or Bundle Protocol Security, so CfdpManager assumes the source is already trusted. That matches the assumptions generally made by the CCSDS 727.0-B-5 CFDP standard.
I understand the defense-in-depth argument for adding application-layer path validation. The tradeoff is additional configuration complexity and possible conflicts with legitimate use cases like firmware updates or system file management. It also does not solve the underlying problem if the CFDP traffic itself is unauthenticated.
I added a “Security Considerations” section to the SDD to document the design rationale more clearly.
If there is interest in revisiting the design or discussing path validation for F´ components more broadly, I’d be happy to join that discussion.
This reverts commit f48314f.
… NAK processed logic
| // notably this state is distinguishable from items still on the free list | ||
| txn->m_state = TXN_STATE_INIT; | ||
| txn->m_history->dir = direction; | ||
| txn->m_chan = this; // Set channel pointer |
| | CFDP-010 | `CfdpManager` shall support configurable file archiving to move completed files instead of deletion | Preserves files for audit trails and operational analysis while managing storage | Unit Test | | ||
| | CFDP-011 | `CfdpManager` shall support both command-initiated and port-initiated file transfers | Allows both ground operators and onboard components to initiate file transfers | Unit Test, System Test | | ||
| | CFDP-012 | `CfdpManager` shall support flow control to freeze and resume channel operations | Provides mechanism to temporarily halt file transfers during critical spacecraft operations | Unit Test | | ||
|
|
There was a problem hiding this comment.
must fix SDD has no Events section — 72 events defined in Events.fppi are undocumented.
Events are ground-/operator-facing FPP surfaces. The SDD documents Commands, Parameters, and Telemetry but omits Events entirely. Ground operators rely on the SDD to understand event semantics, severities, and parameter meanings. An ## Events section with at minimum a summary table (name, severity, description) is required before this ships.
| | SetChannelFlow | Sets the flow control state for a specific CFDP channel. Can freeze (pause) or resume PDU transmission on the channel. | | ||
| | SuspendResumeTransaction | Suspend or resume a transaction. When suspended, the transaction remains in memory but stops making progress (no PDUs sent or processed, no timers tick). Useful during critical spacecraft operations. Takes an action parameter (SUSPEND or RESUME). Transactions are identified by channel ID, transaction sequence number, and entity ID. | | ||
| | CancelTransaction | Gracefully cancel a transaction with protocol close-out. Sends FIN/ACK PDUs as appropriate for the transaction type and state. Transaction is removed from memory. Transactions are identified by channel ID, transaction sequence number, and entity ID. | | ||
| | AbandonTransaction | Immediately terminate a transaction without protocol close-out. No FIN/ACK sent. Transaction is immediately removed from memory. Used for stuck or unresponsive transactions. Transactions are identified by channel ID, transaction sequence number, and entity ID. | |
There was a problem hiding this comment.
could fix Commands table is missing the ResetCounters command.
Commands.fppi defines 9 commands but the SDD table lists only 8 — ResetCounters (resets per-channel telemetry counters, with channelId 0xFF for all channels) is absent.
| | AbandonTransaction | Immediately terminate a transaction without protocol close-out. No FIN/ACK sent. Transaction is immediately removed from memory. Used for stuck or unresponsive transactions. Transactions are identified by channel ID, transaction sequence number, and entity ID. | | |
| | AbandonTransaction | Immediately terminate a transaction without protocol close-out. No FIN/ACK sent. Transaction is immediately removed from memory. Used for stuck or unresponsive transactions. Transactions are identified by channel ID, transaction sequence number, and entity ID. | | |
| | ResetCounters | Resets telemetry counters for the specified CFDP channel. Pass `channelId` 0xFF to reset all channels. | |
| #### Sent Counters | ||
| | Field | Type | Description | | ||
| |---|---|---| | ||
| | sentNakSegmentRequests | U32 | Number of NAK segment requests sent to peer entity | |
There was a problem hiding this comment.
could fix Telemetry section is missing three fields present in ChannelTelemetry struct.
Types.fpp defines recvPdu (receive counter), sentFileDataBytes and sentPdu (sent counters) but the SDD's Telemetry tables omit them. The Sent Counters table only lists sentNakSegmentRequests.
| | sentNakSegmentRequests | U32 | Number of NAK segment requests sent to peer entity | | |
| | sentNakSegmentRequests | U32 | Number of NAK segment requests sent to peer entity | | |
| | sentFileDataBytes | U64 | Total file data bytes sent across all transactions | | |
| | sentPdu | U32 | Number of PDUs sent with valid headers | |
(Also add | recvPdu | U32 | Number of PDUs received with valid headers | to the Receive Counters table.)
| instance prmDb | ||
|
|
||
| } # end topology | ||
| } # end FileHandlingCfdp Subtopology |
There was a problem hiding this comment.
suggestion New FileHandlingCfdp subtopology ships without a docs/sdd.md.
Peer subtopologies (FileHandling, ComCcsds) include design documents at docs/sdd.md. The subtopology how-to guide also recommends including a docs folder. Adding a brief SDD describing the subtopology's purpose, its component instances, wiring, and configuration knobs would keep the new surface consistent with existing subtopologies.
There was a problem hiding this comment.
nice-to-have....or I can get an AI to write it once merged.
There was a problem hiding this comment.
I had Claude write an SDD for the FileHandlingCfdp subtopology. Feel free to tear it apart!
| module Cfdp { | ||
|
|
||
| @ F' implementation of the CFDP file transfer protocol | ||
| active component CfdpManager { |
There was a problem hiding this comment.
must fix Human design adjudication required. This PR introduces CfdpManager — a new 73-file active component plus a FileHandlingCfdp subtopology — porting NASA cFS CF v3.0.0 to F Prime. Changes cross Svc, Os, and default/config layers.
The design is prima-facie reasonable (active component with async ports matching the Cyclic Notification Pattern per docs/user-manual/framework/component-and-port-selection.md), but the scope and architectural surface require human design-owner sign-off before deeper reviewer agents invest effort. Key questions for maintainers:
- Is wrapping the cFS CF engine via
friend class(Engine, Channel, Transaction accessing component EVR/TLM/PRM directly) the desired long-term integration pattern, or should these be separate components communicating via ports? - Is placing this under
Svc/Ccsds/(new namespace layer) the agreed namespace home? - Does the FileHandlingCfdp subtopology composition (CfdpManager + FileManager + PrmDb) match the intended deployment model?
cc @LeStarch @thomas-bc — design needs human adjudication before deeper review.
There was a problem hiding this comment.
- Is a minor violation of the standard. It would be better to avoid, but not critical.
- Yes
- If this is a replacement for the FileHandling subtopology, then yes!
There was a problem hiding this comment.
- If there is a better pattern for subclasses having access to component
EVR/TLM/PRMI would be happy to update the access pattern. - 👍
- Yes the
FileHandlingCfdpsubtopology is intended as a drop-in replacement for theFileHandlingsubtopology.
| @ This file defines proposed telemetry channels for CfdpManager based on | ||
| @ telemetry counters from the NASA CF (CFDP) Application. | ||
| @ | ||
| @ Note: These are currently PROPOSALS and not yet implemented. |
There was a problem hiding this comment.
must fix Telemetry.fppi header declares these channels as "currently PROPOSALS and not yet implemented," but the C++ implementation fully maintains all counters (CfdpManager.hpp lines 110–236, incrementRecv*, addSent*, etc.) and emits them every second via tlmWrite_ChannelTelemetry in run1Hz_handler (CfdpManager.cpp line 71).
The FPP is a contract; this comment creates a false contract that downstream tooling or reviewers may rely on. Remove the "PROPOSALS / not yet implemented" comment block (lines 1–8), and update the SDD § Telemetry which repeats the same claim.
There was a problem hiding this comment.
Removed the deprecated comment.
| ) | ||
|
|
||
| @ Command to start a directory playback | ||
| async command PlaybackDirectory( |
There was a problem hiding this comment.
must fix 8 of 9 new async commands have no test coverage.
Only SendFile is exercised via sendCmd_SendFile in the test suite. The remaining 8 commands — PlaybackDirectory, PollDirectory, StopPollDirectory, SetChannelFlow, SuspendResumeTransaction, CancelTransaction, AbandonTransaction, ResetCounters — have no sendCmd_* invocation, no ASSERT_CMD_RESPONSE, and no doDispatch() in any test. Each is a ground-facing async command and requires at minimum a happy-path test (sendCmd_X → doDispatch → ASSERT_CMD_RESPONSE(..., OK)).
There was a problem hiding this comment.
@pepepr08 is working on the UT coverage.
| @@ -0,0 +1,547 @@ | |||
| event BuffersExhausted severity warning low \ | |||
There was a problem hiding this comment.
must fix ~53 of 55 new events have no ASSERT_EVENTS_* coverage.
Only TxFileTransferCompleted and RxFileTransferCompleted are asserted (with both _SIZE and payload checks). All other events — including ground-observable warnings like BuffersExhausted, InvalidChannel, SendFileInitiateFail, FailPduHeaderDeserialization, RxCrcMismatch, TxAckLimitReached, TransactionCanceled, etc. — are never verified in any test. Many of these are emitted on failure paths that are also untested (see separate fail-path finding).
There was a problem hiding this comment.
@pepepr08 is working on the UT coverage.
| @@ -0,0 +1,58 @@ | |||
| @ CFDP ID to denote the current node when sending PDUs | |||
| param LocalEid: Cfdp.EntityId \ | |||
There was a problem hiding this comment.
must fix None of the 10 new parameters have paramSet_* / paramGet_* test coverage.
LocalEid, OutgoingFileChunkSize, RxCrcCalcBytesPerCycle, FileInDefaultChannel, FileInDefaultDestEntityId, FileInDefaultClass, FileInDefaultKeep, FileInDefaultPriority, and ChannelConfig are all declared but the test suite never calls paramSet_* or paramGet_*. Some parameters are consumed implicitly (via loadParameters() + default values), but the contract requires exercising set/get paths and verifying default-vs-set behavior.
There was a problem hiding this comment.
@pepepr08 is working on the UT coverage.
| output port dataOut: [NumChannels] Fw.BufferSend | ||
|
|
||
| @ Buffer that was sent via the dataOut port and is now being returned | ||
| async input port dataReturnIn: [NumChannels] Fw.BufferSend |
There was a problem hiding this comment.
must fix New async input port dataReturnIn has no test invocation.
dataReturnIn (array port with [NumChannels] instances) is never exercised via invoke_to_dataReturnIn in any test. This port handles buffer deallocation after PDU transmission — untested, it masks potential double-free or leak bugs.
There was a problem hiding this comment.
@pepepr08 is working on the UT coverage.
| @@ -0,0 +1,72 @@ | |||
| @ Command to start a CFDP file transaction | |||
| async command SendFile( | |||
There was a problem hiding this comment.
must fix SendFile (the only tested command) lacks failure-path coverage.
The implementation in CfdpManager.cpp returns EXECUTION_ERROR when the engine fails to initiate the transfer and VALIDATION_ERROR for invalid channel indices (checkCommandChannelIndex). The test only exercises the success path (Fw::CmdResponse::OK). At minimum, test with an invalid channelId (>= NumChannels) and verify ASSERT_CMD_RESPONSE(..., VALIDATION_ERROR) + ASSERT_EVENTS_InvalidChannel_SIZE(1).
There was a problem hiding this comment.
@pepepr08 is working on the UT coverage.
|
|
||
| # Downlink ports | ||
| @ Port for outputting PDU data | ||
| output port dataOut: [NumChannels] Fw.BufferSend |
There was a problem hiding this comment.
suggestion Multi-instance ports (dataOut, dataIn, bufferAllocate, etc.) are exercised on channel 0 for most test paths.
Class 2 TX command-based tests use TEST_CHANNEL_ID_1, which provides some coverage. However, RX paths and Class 1 TX exclusively use channel 0. Consider adding at least one RX test on a non-zero channel index to cover index-arithmetic in the uplink path.
There was a problem hiding this comment.
@pepepr08 is working on the UT coverage.
|
|
||
| // Verify RX counters (received Metadata + FileData + EOF = 3 PDUs minimum) | ||
| // Note: Counters are cumulative, so use >= for multi-transaction tests | ||
| EXPECT_GE(tlm[TEST_CHANNEL_ID_0].get_recvPdu(), 3u) << "recvPdu should be at least 3 (Metadata + FileData + EOF)"; |
There was a problem hiding this comment.
could fix Telemetry counter assertions use EXPECT_GE lower bounds instead of exact values.
In single-transaction tests (e.g., sendAndVerifyClass1Rx), EXPECT_GE(tlm[...].get_recvPdu(), 3u) passes even if the count is wrong (e.g., 100). Since these are isolated single-transaction tests with a fresh tester instance, the exact count is known and EXPECT_EQ would catch regressions that inflate or miscount PDUs.
| /** | ||
| * @brief C++ class encapsulation of CFDP chunk list operations | ||
| * | ||
| * This class provides modern C++ encapsulation around the gap tracking functionality |
There was a problem hiding this comment.
must fix CPP-25/STL — std::function is a banned STL type in F Prime flight code
std::function wraps a type-erased callable and may dynamically allocate to store captures (violating CPP-1 as well). Flight code must use a function pointer or a named functor. The same pattern appears in Clist.hpp (CListTraverseCallback) and Types/Types.hpp (CfdpTraverseAllTransactionsFunc).
Replace with a plain function pointer plus opaque void* context, which the codebase already uses via CListFunc.
| * This class provides modern C++ encapsulation around the gap tracking functionality | |
| using GapComputeCallback = void (*)(const Chunk* chunk, void* opaque); |
There was a problem hiding this comment.
Fixed. This required a rather large amount of re-work that should be reviewed.
| /************************************************************************/ | ||
| /** @brief Initialize a clist node. | ||
| * | ||
| * @param node Pointer to node structure to be initialized |
There was a problem hiding this comment.
must fix CPP-25/STL — std::function is a banned STL type in F Prime flight code
This CListTraverseCallback alias introduces std::function which can heap-allocate internally and pulls in STL headers compiled without -fno-exceptions guarantees. The existing CListFunc (line 94) is the correct pattern — a plain function pointer with void* context.
| * @param node Pointer to node structure to be initialized | |
| using CListTraverseCallback = CListTraverseStatus (*)(CListNode*, void*); |
| } // namespace Cfdp | ||
| } // namespace Ccsds | ||
| } // namespace Svc | ||
|
|
There was a problem hiding this comment.
must fix CPP-25/STL — std::function is a banned STL type in F Prime flight code
Same pattern as GapComputeCallback and CListTraverseCallback. Replace with a plain function pointer.
| using CfdpTraverseAllTransactionsFunc = void (*)(Transaction* txn, void* context); |
| } | ||
| } | ||
|
|
||
| void Engine::armAckTimer(Transaction* txn) { |
There was a problem hiding this comment.
must fix CPP-1 — Dynamic new allocation in Engine::init() violates the post-init memory ban
Engine::init() is called from CfdpManager::configure() which runs after component construction. The new Channel(...) call dynamically allocates heap memory at runtime. F Prime requires all dynamic storage to be allocated during boot/init via Fw::MemAllocator or pre-sized arrays. Channel objects should be pre-allocated through the same Fw::MemAllocator pattern used for the Engine itself in CfdpManager::configure(), or use placement new into a pre-allocated buffer.
| // Use operator new for raw memory (for types requiring placement new with constructor params) | ||
| m_transactions = static_cast<Transaction*>(::operator new(CFDP_NUM_TRANSACTIONS_PER_CHANNEL * sizeof(Transaction))); | ||
| m_chunks = static_cast<CfdpChunkWrapper*>( | ||
| ::operator new((CFDP_NUM_TRANSACTIONS_PER_CHANNEL * DIRECTION_NUM) * sizeof(CfdpChunkWrapper))); |
There was a problem hiding this comment.
must fix CPP-1 — Multiple bare ::operator new / new[] allocations in Channel constructor
Lines 115-123 contain four distinct dynamic allocations: ::operator new for transactions, ::operator new for chunk wrappers, new History[...], and new Chunk[...]. These use raw heap allocation rather than Fw::MemAllocator. All should be routed through the component's allocator to ensure deterministic memory management. The delete / ::operator delete calls in the destructor would need corresponding changes.
| } | ||
|
|
||
| /** | ||
| * @brief Callback function type for use with CfdpCListTraverse() |
There was a problem hiding this comment.
could fix CPP-10 — reinterpret_cast in container_of_cpp lacks justification comment
This is a well-known C idiom (container_of) ported to C++. The reinterpret_cast is inherently needed for member-to-parent pointer arithmetic and the implementation is correct, but per CPP-10 it requires an inline comment explaining why reinterpret_cast is necessary. A one-line comment noting this is the C++ equivalent of container_of for intrusive list node-to-parent conversion would satisfy the rule.
There was a problem hiding this comment.
Added comment.
| friend class Transaction; | ||
|
|
||
| public: | ||
| // ---------------------------------------------------------------------- |
There was a problem hiding this comment.
suggestion CPP-16 — friend used for inter-component coupling in production code
friend class Engine, friend class Channel, and friend class Transaction grant unrestricted access to CfdpManager internals for production coupling, not just unit-test access. CPP-16 reserves friend for unit-test fixtures only. The same pattern appears in Transaction.hpp (friend Engine, Channel). Consider providing public or protected accessor methods for the specific members these classes need, or restructure ownership so the coupling is explicit through interfaces.
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
| public: | ||
| // ---------------------------------------------------------------------- | ||
| // Class construction and destruction | ||
| // ---------------------------------------------------------------------- |
There was a problem hiding this comment.
suggestion CPP-26 — Unscoped enum Status should be enum class per F Prime style
F Prime style guidelines prefer enum class for type safety to prevent implicit integer conversion and namespace pollution. Several other enums in this PR (e.g., in Types.hpp) also use unscoped enums — this is representative of the pattern.
| // ---------------------------------------------------------------------- | |
| enum class Status { UNINITIALIZED, RUNNING, EXPIRED }; |
There was a problem hiding this comment.
Fixed. This required extensive rework and should be reviewed.
| ((CFDP_MAX_POLLING_DIR_PER_CHAN + CFDP_MAX_COMMANDED_PLAYBACK_DIRECTORIES_PER_CHAN) * \ | ||
| CFDP_NUM_TRANSACTIONS_PER_PLAYBACK)) | ||
|
|
||
| // CFDP File Directive Codes |
There was a problem hiding this comment.
suggestion CPP-8 — #define CFDP_NUM_TRANSACTIONS_PER_CHANNEL should be static constexpr
#define constants have no type, no scope, and no debugger visibility. This computed constant should be a static constexpr expression inside the namespace.
| // CFDP File Directive Codes | |
| static constexpr U32 CFDP_NUM_TRANSACTIONS_PER_CHANNEL = | |
| (CFDP_MAX_COMMANDED_PLAYBACK_FILES_PER_CHAN + CFDP_MAX_SIMULTANEOUS_RX + | |
| ((CFDP_MAX_POLLING_DIR_PER_CHAN + CFDP_MAX_COMMANDED_PLAYBACK_DIRECTORIES_PER_CHAN) * | |
| CFDP_NUM_TRANSACTIONS_PER_PLAYBACK)); |
| } else | ||
| switch (txn->getState()) { | ||
| case TXN_STATE_S1: | ||
| case TXN_STATE_R1: |
There was a problem hiding this comment.
suggestion CPP-6 — Use nullptr instead of NULL
Same as the finding on Engine.cpp — NULL should be nullptr throughout the CFDP component.
| case TXN_STATE_R1: | |
| if (txn == nullptr) { |
|
|
||
| // Check if the poll directory is in use | ||
| pd = m_channels[chanId]->getPollDir(pollId); | ||
| if (pd->enabled == Fw::Enabled::DISABLED) { |
There was a problem hiding this comment.
[must-fix] general-vulnerability · introduced
Inverted condition in stopPollDir prevents stopping active poll directories.
The condition on this line checks pd->enabled == Fw::Enabled::DISABLED and then "clears" an already-disabled slot (including re-setting pd->enabled = DISABLED, which is a no-op). In the else branch (when the directory IS enabled/active), it logs PollDirNotActive and returns ERROR.
This means:
- Ground cannot stop an active polling directory — the command always returns
ERRORwith a misleading "not active" event. - The "clear" path fires on already-disabled slots, doing redundant work.
The condition should be pd->enabled == Fw::Enabled::ENABLED to correctly stop an active poll directory.
This is a loss-of-ground-control issue: once a polling directory is started, it cannot be stopped via the StopPollDirectory command.
There was a problem hiding this comment.
Definite bug, fixed!
|
|
||
| /* store the filenames in transaction - validation already done during deserialization */ | ||
| txn->m_history->fnames.src_filename = md.getSourceFilename(); | ||
| txn->m_history->fnames.dst_filename = md.getDestFilename(); |
There was a problem hiding this comment.
[must-fix] ground-validation-gap · introduced
No path traversal validation on filenames received from remote CFDP entity.
recvMd stores the destination and source filenames from the incoming Metadata PDU directly into the transaction history without any path sanitization. The comment "validation already done during deserialization" refers only to the LV-format length check in MetadataPdu::fromSerialBuffer, not to path content validation.
These filenames are subsequently used to:
- Open/create files in
Transaction::rInit()(TransactionRx.cpp:341) - Rename files in
Transaction::r2RecvMd()(TransactionRx.cpp:1002)
A malicious or compromised remote CFDP entity could craft a Metadata PDU with dst_filename = "../../etc/malicious" or absolute paths like /data/critical_file to write or overwrite files outside the intended receive directory.
Recommended fix: Before storing filenames, validate that:
- The path does not contain
..components - The path is relative (or is confined to the expected receive directory)
- The path does not contain null bytes or other special characters
This is a standard file-boundary risk in any file transfer protocol handler.
There was a problem hiding this comment.
@LeStarch lets discuss this one at the Monday tagup.
| success = false; | ||
| } else { | ||
| // Check that file size is well formed | ||
| FW_ASSERT(this->m_fsize > 0, static_cast<FwAssertArgType>(this->m_fsize)); |
There was a problem hiding this comment.
[suggestion] ground-reachable-assert · introduced
FW_ASSERT on file size > 0 is reachable via ground command if a zero-length file is targeted.
If a ground operator issues a SendFile command targeting a zero-length file (which is a valid filesystem object), m_fd.size() will return 0, and this assert will fire, causing a component-level crash (DoS).
The file size of 0 is a valid condition for empty files on the filesystem. A zero-length CFDP transfer is defined in the protocol (CCSDS 727.0-B-5 allows zero-length file transfers for metadata-only use cases).
Recommended fix: Replace the assert with an error return path that logs a warning and fails the transaction gracefully:
if (this->m_fsize == 0) {
this->m_cfdpManager->log_WARNING_LO_TxZeroLengthFile(...);
success = false;
}There was a problem hiding this comment.
Fixed and verified by UTs.
|
|
||
| // Calculate remaining data size based on header's PDU data length | ||
| U16 pduDataLength = this->m_header.getPduDataLength(); | ||
| this->m_dataSize = static_cast<U16>(pduDataLength - offsetSize); // minus offset size |
There was a problem hiding this comment.
[could-fix] ground-overflow-or-ddos · introduced
Unsigned integer underflow in m_dataSize calculation when pduDataLength < offsetSize.
If a malformed PDU arrives with pduDataLength less than sizeof(FileSize) (4 bytes), the subtraction pduDataLength - offsetSize wraps around to a large U16 value (e.g., if pduDataLength == 0, result is 0xFFFC).
The subsequent check at line 148 (serialBuffer.getDeserializeSizeLeft() < this->m_dataSize) does catch this case and returns FW_DESERIALIZE_SIZE_MISMATCH, so this is not exploitable in the current code. However, the intermediate state where m_dataSize holds a wrapped value is fragile — any future code change that reads m_dataSize before the validation check could introduce a vulnerability.
Recommended fix: Add a defensive check before the subtraction:
if (pduDataLength < offsetSize) {
return Fw::FW_DESERIALIZE_SIZE_MISMATCH;
}
this->m_dataSize = static_cast<U16>(pduDataLength - offsetSize);| sret = this->m_engine->sendAck(this, ACK_TXN_STATUS_ACTIVE, FILE_DIRECTIVE_END_OF_FILE, | ||
| static_cast<ConditionCode>(this->m_state_data.receive.r2.eof_cc), | ||
| this->m_history->peer_eid, this->m_history->seq_num); | ||
| FW_ASSERT(sret != Cfdp::Status::SEND_PDU_ERROR); |
There was a problem hiding this comment.
[suggestion] hardware-reachable-assert · introduced
FW_ASSERT on sendAck return value could fire if the downstream PDU send path encounters an error.
sendAck is called here as part of the RX tick processing to send an EOF ACK. The assert FW_ASSERT(sret != Cfdp::Status::SEND_PDU_ERROR) will crash the component if sendAck returns SEND_PDU_ERROR.
Looking at Engine::sendAck, SEND_PDU_ERROR is returned when getPduBuffer succeeds but toBuffer serialization fails. While unlikely, a serialization failure could occur due to buffer corruption from a hardware fault or an unexpected internal state. In a flight environment, a crash from a serialization failure is disproportionate — the transaction should be failed gracefully instead.
Recommended fix: Replace the assert with error handling that fails the transaction:
if (sret == Cfdp::Status::SEND_PDU_ERROR) {
// handle error, e.g., set error status and finish transaction
}
lestarch-autobot
left a comment
lestarch-autobot
left a comment
There was a problem hiding this comment.
Automated review summary (run 1)
Per-agent results
| Agent | must fix | suggestion | could fix | future work | outstanding | Verdict |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 2 | 2 | 1 | 0 | 5 | No-Go |
| Supply Chain / Runner Safety | 0 | 0 | 0 | 0 | 0 | Go |
| F Prime C/C++ Design | 7 | 6 | 1 | 0 | 14 | No-Go |
| Documentation Currency | 1 | 1 | 2 | 0 | 4 | No-Go |
| Design | 2 | 0 | 0 | 0 | 2 | No-Go |
| Test Quality | 5 | 1 | 1 | 0 | 7 | No-Go |
| CI safety | — | — | — | — | — | Go |
| Totals | 17 | 10 | 5 | 0 | 32 | No-Go |
Supply-chain surfaces
| Surface | Outstanding |
|---|---|
| Dependencies | clean |
| Vendored / submodule | clean |
| Build / test infrastructure | clean |
| Workflows / actions / scripts | clean |
| Generator output | clean |
| Prompt-injection | clean |
| Review-system integrity | clean |
Outstanding must-fix items (17)
Security Vulnerabilities
- Inverted condition in
stopPollDirprevents stopping active poll directories — comment - No path traversal validation on filenames received from remote CFDP entity — comment
F Prime C/C++ Design
- CPP-25/STL:
std::functionbanned in F Prime (Chunk.hpp) — comment - CPP-25/STL:
std::functionbanned in F Prime (Clist.hpp) — comment - CPP-25/STL:
std::functionbanned in F Prime (Types.hpp) — comment - CPP-1: Dynamic
newallocation inEngine::init()— comment - CPP-1: Multiple bare
operator new/new[]inChannelconstructor — comment - CPP-7: Lambda forbidden in F Prime (
Channel.cpp) — comment - CPP-7: Lambda with capture forbidden (
TransactionRx.cpp) — comment
Documentation Currency
- Missing Events section in SDD — 72 ground-facing events undocumented — comment
Design
- Human design adjudication required. 73-file new component requires design-owner sign-off — comment
- FPP–C++ divergence:
Telemetry.fppiclaims "not yet implemented" but telemetry IS implemented — comment
Test Quality
- ~8 of 9 new async commands have no
sendCmd_*/ASSERT_CMD_RESPONSEcoverage — comment - ~53 of 55 new events have no
ASSERT_EVENTS_*coverage — comment - None of 10 new parameters have
paramSet_*/paramGet_*coverage — comment dataReturnInasync port has no test invocation — commentSendFilecommand lacks failure-path coverage — comment
Merge readiness
Merge readiness: No-Go — 5 of 6 reviewers report outstanding must-fix findings (security-review, fprime-code-review, stale-documentation-review, design-review, test-quality-review).
Agents that did not run on this PR
All 6 registered reviewers ran successfully.
Safe satisfying flights begin with thorough reviews — the crew is counting on us. 🚀
Add comprehensive SDD for the FileHandlingCfdp subtopology following the same structure and pattern as the existing FileHandling subtopology documentation. The SDD documents: - Requirements for CFDP file transfer, file management, and parameter management - Component instances (CfdpManager, FileManager, PrmDb) and their purposes - Configuration hooks and required connections (rate groups, communication stack) - Key differences from the FileHandling subtopology (CFDP vs legacy file transfer) - Example topology integration with CFDP PDU wiring - Compile-time and runtime CFDP configuration parameters - Limitations and architectural boundaries This documentation provides integration engineers with a complete reference for using the FileHandlingCfdp subtopology in F' CCSDS deployments with reliable CFDP-based file delivery. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Address three review comments on PR nasa#5138: 1. Add comprehensive Events section documenting all 72 events - Organize events into 7 logical categories: Command/Control, PDU Serialization errors, RX/TX Transaction events, Transfer Complete, Transaction Control, and Miscellaneous/Diagnostic - Include event name, severity, and description for each - Provide context for ground operators to understand event semantics 2. Add missing ResetCounters command to Commands table - Documents the ability to reset telemetry counters per channel - Notes that channelId 0xFF resets all channels 3. Add missing telemetry fields to Telemetry section - Add recvPdu to Receive Counters table - Add sentFileDataBytes and sentPdu to Sent Counters table - Ensures SDD matches ChannelTelemetry struct in Types.fpp Also remove "PROPOSALS" comment from Telemetry.fppi as the telemetry is now implemented and active in the component. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…tions with static wrappers
…emAllocator usage. Needed a new cleanup to avoid duplicate allocator storage
Address CPP-6 and CPP-10 coding standard violations identified in PR review. Changes: - Replace all NULL with nullptr throughout codebase (CPP-6 requirement) - Add justification comments for all const_cast<U8*> usages (CPP-10): "Fw::SerialBuffer requires non-const U8* even for deserialization" - Add justification comment for reinterpret_cast in container_of_cpp (CPP-10): "Required for intrusive list node-to-parent pointer arithmetic" All unit tests pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…, CPP-8) Convert all unscoped enums in CfdpManager to enum class for type safety per F' CPP-26 coding standard: - FileDirective, ConditionCode, AckTxnStatus, FinDeliveryCode, FinFileStatus, ChecksumType - TxnState, TxSubState, RxSubState, Direction, TransactionInitType, CfdpTickType, TxnStatus - PduType, PduDirection, CrcFlag, LargeFileFlag, TlvType Replace #define CFDP_NUM_TRANSACTIONS_PER_CHANNEL with static constexpr per F' CPP-8 standard. All enum references updated with qualified names and static_cast where needed for: - Comparisons with U8/U32 types - Array subscripts - Bitwise operations - FW_ASSERT calls requiring integer types Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update test code to use qualified enum class names and cast comparisons. Unit tests still have compilation errors but production code builds successfully. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixed all remaining unit test compilation errors after enum class conversion: 1. Removed invalid static_cast<U8> from enum class parameters in initialize() calls - FinPdu::initialize() now takes FinDeliveryCode and FinFileStatus directly - Array initializations now use enum class types without casts 2. Fixed misplaced closing parentheses in function calls - Changed `func(..., arg),` → `func(..., arg,` - Changed `func(...))` → `func(...)` 3. Corrected EXPECT_EQ comparisons to compare enum-to-enum - Removed unnecessary static_cast<U8>() from getter results - Direct enum comparisons: `EXPECT_EQ(EnumClass::VALUE, obj.getEnum())` 4. Added missing enum qualifiers for TLV types - TLV_TYPE_MESSAGE_TO_USER → TlvType::TLV_TYPE_MESSAGE_TO_USER - TLV_TYPE_FLOW_LABEL → TlvType::TLV_TYPE_FLOW_LABEL All unit tests now pass: - CfdpManager main tests: 15/15 passed - Types PDU tests: all passed Production code and all unit tests build successfully. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replaced FW_ASSERT with graceful error handling when attempting to transfer zero-length files. This prevents a DoS vulnerability where an operator command could crash the component. Changes: - TransactionTx.cpp: Replace assert with error event and fault counter - Events.fppi: Add TxZeroLengthFile warning event - Add command unit tests verifying both bug fixes: * testSendFileZeroLength: Verifies zero-length file handling (no crash) * testSendFileNonExistent: Verifies file open error handling * testStopPollDirActive: Verifies stopPollDir fix (inverted condition) * testStopPollDirNotActive: Verifies inactive directory handling All tests use ASSERT_EVENTS macros and Os::FileSystem operations. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Added defensive check to prevent unsigned integer underflow when pduDataLength < offsetSize. While the subsequent validation at line 148 already catches malformed PDUs, the intermediate underflow state is fragile and could introduce vulnerabilities in future code changes. The fix validates pduDataLength against offsetSize before performing the subtraction, returning FW_DESERIALIZE_SIZE_MISMATCH for invalid PDUs. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replaced FW_ASSERT calls on sendAck/sendFin return values with proper error handling. The asserts could fire if PDU serialization fails due to hardware fault or unexpected internal state. Changes: - TransactionRx.cpp: Replace asserts with graceful handling of ERROR status - When sendAck returns ERROR: clear retry flag to avoid infinite loop - When sendFin returns ERROR: continue state transition (already logged) - Serialization errors are already logged by FailPduSerialization event This prevents component crashes from serialization failures while maintaining proper error reporting. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…n tests Changes: - Added expectExactCounts parameter to sendAndVerifyClass2Tx helper - Single-transaction tests (testClass2TxNominal, testClass2TxPortBased) now use EXPECT_EQ for exact telemetry counts - Multi-transaction tests and NAK tests continue to use EXPECT_GE for lower bounds - Previously used EXPECT_GE(tlm[...].get_recvPdu(), 3u) which would pass even if count was wrong - With EXPECT_EQ, single-transaction tests will catch regressions that inflate or miscount PDUs Rationale: - In isolated single-transaction tests with fresh tester instance, exact counts are known - EXPECT_EQ catches regressions; EXPECT_GE only validates lower bound - Distinguishes between tests where exact counts are expected vs cumulative/NAK scenarios Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
6 participants
Change Description
This PR adds the CfdpManager component, an F´ implementation of the CCSDS File Delivery Protocol (CFDP) standard ported from NASA's Core Flight System (cFS) CFDP Application v3.0.0. CfdpManager provides both Class 1 (unacknowledged) and Class 2 (acknowledged) file transfer capabilities, designed to replace the standard FileUplink and FileDownlink components with guaranteed file delivery support.
Attribution and License:
Ported components (retain original NASA copyright):
Engine.cpp,Transaction.hpp,TransactionTx.cpp,TransactionRx.cpp)Utils.cpp,Channel.cpp)Chunk.cpp,Clist.cpp)New F´-specific implementations:
SerializableinterfaceSee ATTRIBUTION.md for complete file-by-file attribution breakdown.
Summary of changes:
Rationale
Current F´ file transfer components (FileUplink/FileDownlink) lack reliable delivery guarantees over lossy or intermittent links, automatic retransmission and gap detection, and industry-standard CFDP protocol compliance required for interoperability with ground systems and other spacecraft.
CfdpManager addresses these gaps by implementing the CCSDS CFDP standard, which is specifically designed for space missions with long propagation delays, high error rates, and disruption-tolerant requirements. By porting NASA's proven CF application from cFS, this implementation leverages flight-proven CFDP logic while adapting it to F´'s architecture and component model.
Testing/Review Recommendations
Unit Test Coverage:
Areas to focus on:
dataOut/dataReturnInandbufferAllocate/bufferDeallocate)Future Work
fileInportAI Usage (see policy)
Generative AI (Claude Code) was used for:
Important note: AI was NOT used to generate the core CFDP protocol logic. The core engine, transaction state machines, and protocol logic were ported directly from NASA's CF application v3.0.0 (human-written, flight-proven code). AI assistance was limited to F´-specific integration code, documentation, and port quality improvements.