feat(cliprdr): add a chunked file-contents fetch primitive - #1742
feat(cliprdr): add a chunked file-contents fetch primitive#1742Greg Lamberson (glamberson) wants to merge 2 commits into
Conversation
MS-RDPECLIP's file-contents protocol is receiver-driven: fetching a file means sending a sequence of byte-range FileContentsRequests and reassembling the FileContentsResponses. Nothing in this crate helps with that sequencing, so every consumer that wants a whole file ends up writing its own request/response loop by hand. Add ChunkedFetch: a small state machine that produces the next request to send and consumes each response, tracking offset and buffered bytes so the caller doesn't have to. The size phase is optional: construct with a known size (the common case, since it's usually already available from an earlier FileGroupDescriptorW exchange) to skip straight to range requests, or without one to query it first. It does not touch any of Cliprdr's internals. request_file_contents already validates every request against negotiated capabilities and the remote file list, and already renews the active lock's activity timestamp on every call that carries a data_id, including one it auto-fills from the current lock, so there is nothing left for a higher-level helper to manage on that front; ChunkedFetch just calls it the same way any other caller would. Two defensive cases beyond the happy path: an empty range response before the file is complete fails the fetch rather than re-requesting the same range forever, since the protocol has no "not ready yet" signal that would legitimately produce one, and a response longer than what's left is clamped rather than trusted, since accepting it as-is would grow the buffer past the file's total size. Tests live in ironrdp-testsuite-core since this crate sets [lib] test = false.
There was a problem hiding this comment.
New, self-contained ChunkedFetch state machine in ironrdp-cliprdr for sequencing MS-RDPECLIP file-contents SIZE/RANGE requests and reassembling responses. It touches no existing Cliprdr internals, reuses existing validated pdu types and request_file_contents() validation, and adds only new public items plus tests, matching the PR's stated no-API-break scope. SIZE requests are built with the mandatory literal fields (requested_size=8, position=0) and RANGE requests derive position/size from tracked offset and remaining bytes, both consistent with MS-RDPECLIP 2.2.5.3. Error/malformed/empty-before-completion responses are all mapped to a terminal Failed state to avoid infinite re-request loops. The one substantive issue (RANGE responses accepted up to total_size without checking against the request's own cbRequested) is a pre-existing pattern already present in Cliprdr::process_svc_message's response handling, not a regression introduced by this change, so it does not block merge.
Protocol analysis: partially_accepted — The flagged conflict (RANGE responses clamped to total_size, not cbRequested) is factually accurate, but the handoff itself notes MS-RDPECLIP has no normative requirement here, and crates/ironrdp-cliprdr/src/lib.rs shows the pre-existing Cliprdr::process_svc_message response handling already never validates RANGE length against requested_size (only SIZE responses get an exact-length check). So ChunkedFetch matches, rather than regresses, the crate's existing posture. I downgrade this from 'conflicts/medium' to a non-blocking doc/robustness suggestion. The handoff's other conformance mappings (SIZE request field literals, error and malformed-SIZE handling) are independently confirmed correct.
Cliprdr::process_svc_message's response dispatch validates SIZE responses against an exact 8-byte length but forwards RANGE responses unchecked, and FileTransferState never stored requested_size in the first place, so there was nothing to check a RANGE response's length against even if the dispatch wanted to. ChunkedFetch inherited that gap: a peer answering with more bytes than the request's own requested_size, but still within the file's total size, was silently folded into the buffer. Track last_requested_size, set when next_request builds a RANGE request, and treat a response exceeding it as a protocol violation rather than extra data to accept. The concern is not just the byte count: accepting unrequested bytes means trusting content that was never validated against the specific range this fetch actually asked for, on wire input this crate has to treat as untrusted. The existing total_size-based clamp stays as a defensive backstop for the case where on_response is called without a prior next_request, a caller-side misuse rather than the normal path. Updates the existing "oversized response is clamped" test, which now correctly expects failure since that response exceeds cbRequested, and adds a new test isolating the backstop clamp on its own.
Marc-André Moreau (mamoreau-devolutions)
left a comment
There was a problem hiding this comment.
Requesting changes for two correctness issues: each fetch must remain bound to its original clipboard-lock snapshot across FormatList changes, and remote-controlled file sizes must not drive unbounded aggregate memory growth. Details are inline.
Note
LLM-assisted content (no human feedback).
| flags: FileContentsFlags::RANGE, | ||
| position: self.next_offset, | ||
| requested_size, | ||
| data_id: None, |
There was a problem hiding this comment.
Please preserve this fetch's clipDataId instead of leaving every request unset. request_file_contents fills None from current_lock_id, but a later FormatList expires the old lock and clears or replaces current_lock_id while deliberately keeping the old lock alive for in-flight downloads. If the clipboard changes between chunks, the next request can target a different snapshot and silently assemble bytes from two files. ChunkedFetch should capture the clip_data_id passed to on_remote_file_list and use it for both SIZE and RANGE requests.
Note
LLM-assisted content (no human feedback).
| // Infallible: just clamped to `data.len()` via `data_len`/`min()` above. | ||
| let take = usize::try_from(remaining).unwrap_or(data.len()); | ||
|
|
||
| self.received.extend_from_slice(&data[..take]); |
There was a problem hiding this comment.
Please add an aggregate size limit or a streaming boundary before appending here. new_with_size_query accepts any remote-supplied u64, and every response is retained in this Vec; a peer can report u64::MAX and keep satisfying chunk requests until allocation failure terminates the process. Chunking bounds each response, not total memory. Require a caller-provided maximum and fail before RANGE fetching when it is exceeded, or yield chunks to the caller instead of retaining the whole file.
Note
LLM-assisted content (no human feedback).
Summary
file means sending a sequence of byte-range FileContentsRequests and
reassembling the FileContentsResponses. Nothing in this crate helps
with that sequencing, so every consumer that wants a whole file ends
up writing its own request/response loop by hand.
request to send and consumes each response, tracking offset and
buffered bytes so the caller doesn't have to. The size phase is
optional: construct with a known size (the common case, since it's
usually already available from an earlier FileGroupDescriptorW
exchange) to skip straight to range requests, or without one to
query it first.
already validates every request against negotiated capabilities and
the remote file list, and already renews the active lock's activity
timestamp on every call that carries a data_id, including one it
auto-fills from the current lock, so there is nothing left for a
higher-level helper to manage on that front; ChunkedFetch just calls
it the same way any other caller would.
before the file is complete fails the fetch rather than
re-requesting the same range forever, since the protocol has no
"not ready yet" signal that would legitimately produce one, and a
response longer than what's left is clamped rather than trusted,
since accepting it as-is would grow the buffer past the file's
total size.
Validation
cargo xtask check fmt/lints/tests/typos/locksall pass. Tests livein ironrdp-testsuite-core since this crate sets [lib] test = false.
Notes
No public API break: this is a new module with entirely new public
types, no existing signature changes.