feat(rdpdr): redirect a printer on Linux and macOS - #2017
Benoît Cortier (CBenoit) merged 3 commits into
Conversation
GitHub Actions
Error
github-actions
Bot
added
needs-review
AKolenda
changed the title
github-actions
Bot
added
kind/protocol
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds nix printer redirection: a factory announcing drives plus one virtual printer, and a PrinterSpooler answering printer create/write/close IRPs, spooling jobs privately (mkdtemp 0700, exclusive 0600 files) before handing them to lp or saving them. Core spooling is sound. Published findings: failure completions echo the requested write length and stamp FILE_OPENED on failed creates; the no-printer fallback answers writes with a close-response PDU; the factory does no device-ID validation so misconfiguration aborts the channel mid-connection; lp runs without timeout on the IRP path; the lp-failure fallback writes remote jobs into the user's home without caps; spooler state ignores the reset contract; PrintTarget::parse is unused API; and a failed spool unlink can delete an already-saved job.
- [skeptical] Printer spooler state survives the RDPDR reset sequence — low 🟡 — crates/ironrdp-rdpdr-native/src/nix/backend.rs
RdpdrBackend::reset's contract requires stateful backends to discard state when a new announcement sequence starts, but NixRdpdrBackend relies on the no-op default while now holding open jobs, poisoned handles, and spool files in /tmp. On a reconnect sequence the server never completes the old file ids, so stale spool files linger until the backend is dropped. Bounded leak cleaned at session end, but the trait contract is unmet.
| Err(error) => { | ||
| warn!(%error, "Could not open a spool file for a print job"); | ||
| DeviceCreateResponse { | ||
| device_io_reply: DeviceIoResponse::new(create.device_io_request, NtStatus::UNSUCCESSFUL), | ||
| file_id: 0, | ||
| information: Information::FILE_OPENED, | ||
| } | ||
| } | ||
| }; | ||
| Ok(vec![SvcMessage::from(RdpdrPdu::DeviceCreateResponse(response))]) | ||
| } | ||
| PrinterIoRequest::Write(write) => { | ||
| let file_id = write.device_io_request.file_id; | ||
| let length = u32::try_from(write.write_data.len()).unwrap_or(u32::MAX); | ||
| let status = match self.jobs.get_mut(&file_id) { | ||
| Some(job) => match job.file.write_all(&write.write_data) { | ||
| Ok(()) => { | ||
| job.bytes = job.bytes.saturating_add(u64::from(length)); | ||
| NtStatus::SUCCESS | ||
| } | ||
| Err(error) => { | ||
| warn!(%error, file_id, "Could not spool print data"); | ||
| NtStatus::UNSUCCESSFUL | ||
| } | ||
| }, | ||
| None => NtStatus::UNSUCCESSFUL, | ||
| }; | ||
| Ok(vec![SvcMessage::from(RdpdrPdu::DeviceWriteResponse( | ||
| DeviceWriteResponse { | ||
| device_io_reply: DeviceIoResponse::new(write.device_io_request, status), | ||
| length, | ||
| }, | ||
| ))]) | ||
| } |
There was a problem hiding this comment.
[protocol + skeptical + code-compressor] Failed printer completions echo requested length and stamp FILE_OPENED — low 🟡 — A failed spool write still replies with DR_WRITE_RSP Length set to the full request length instead of zero (MS-RDPEPC 3.2.5.1.12 requires bytes written successfully), and a failed create response sets Information::FILE_OPENED, which is only meaningful on success. The Windows sibling sends length 0 and Information::empty() on failure. Servers that use these fields for flow control or accounting can be misled, e.g. assuming a failed write's data was consumed.
There was a problem hiding this comment.
Fixed in 23b3805. A failed write now reports a length of 0, and a failed create carries file ID 0 and no FILE_OPENED, as in the Windows backend. Covered by failed_requests_carry_no_handle_or_length.
| fn handle_printer_io_request(&mut self, req: PrinterIoRequest) -> PduResult<Vec<SvcMessage>> { | ||
| match self.printer.as_mut() { | ||
| Some(spooler) => spooler.handle(req), | ||
| None => Ok(vec![SvcMessage::from(RdpdrPdu::DeviceCloseResponse( | ||
| DeviceCloseResponse { | ||
| device_io_response: DeviceIoResponse::new(req.into_device_io_request(), NtStatus::NOT_SUPPORTED), | ||
| }, | ||
| ))]), | ||
| } | ||
| } |
There was a problem hiding this comment.
[protocol] No-printer fallback completes printer IRPs with a close-response PDU — low 🟡 — When no spooler is configured, every printer IRP — including IRP_MJ_WRITE — is answered with a DeviceCloseResponse, which is not the DR_PRN_WRITE_RSP shape a server expects for a write completion, so the Length parse fails or truncates. Unreachable through NixRdpdrBackendFactory (which configures the product printer and the backend spooler together) but visible when integrations assemble the backend manually; it also mirrors the trait's default handler.
There was a problem hiding this comment.
Fixed in 23b3805. Without a printer, a create, write or close is answered with its own response type and NOT_SUPPORTED, and an oversized write gets a write response instead of an error. Covered by unsupported_requests_are_answered_with_their_own_response_type. The default RdpdrBackend::handle_printer_io_request answers every request with a close response too. I left that alone because it is outside this PR.
| /// Builds a [`NixRdpdrBackend`] for every RDPDR channel lifetime. | ||
| /// | ||
| /// `drives` are announced as redirected folders; `printer` announces a virtual | ||
| /// printer named after the client, whose jobs go to the given target. | ||
| #[derive(Debug, Clone)] | ||
| pub struct NixRdpdrBackendFactory { | ||
| file_base: String, | ||
| drives: Vec<(u32, String)>, | ||
| printer: Option<(String, PrintTarget)>, | ||
| } | ||
|
|
||
| impl NixRdpdrBackendFactory { | ||
| pub fn new(file_base: String) -> Self { | ||
| Self { | ||
| file_base, | ||
| drives: Vec::new(), | ||
| printer: None, | ||
| } | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub fn with_drive(mut self, device_id: u32, name: String) -> Self { | ||
| self.drives.push((device_id, name)); | ||
| self | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub fn with_printer(mut self, name: String, target: PrintTarget) -> Self { | ||
| self.printer = Some((name, target)); | ||
| self | ||
| } | ||
| } | ||
|
|
||
| /// Device id of the virtual printer; drives use the ids given to [`NixRdpdrBackendFactory::with_drive`]. | ||
| pub const PRINTER_DEVICE_ID: u32 = 0x0001_0000; | ||
|
|
||
| impl RdpdrBackendFactory for NixRdpdrBackendFactory { | ||
| fn build_rdpdr_backend(&self) -> RdpdrBackendFactoryResult<RdpdrBackendProduct> { | ||
| let mut backend = NixRdpdrBackend::new(self.file_base.clone()); | ||
| if let Some((_, target)) = &self.printer { | ||
| backend = backend.with_printer(target.clone()); | ||
| } | ||
| let drives = self | ||
| .drives | ||
| .iter() | ||
| .map(|(id, name)| RdpdrDrive::new(*id, name.clone())) | ||
| .collect(); | ||
| let mut product = RdpdrBackendProduct::new(Box::new(backend), drives); | ||
| if let Some((name, _)) = &self.printer { | ||
| product = product.with_printer(RdpdrPrinter::new( | ||
| PRINTER_DEVICE_ID, | ||
| name.clone(), | ||
| DEFAULT_PRINTER_DRIVER_NAME.to_owned(), | ||
| )); | ||
| } | ||
| Ok(product) | ||
| } | ||
| } |
There was a problem hiding this comment.
[skeptical] NixRdpdrBackendFactory performs no device-ID validation — medium 🟠 — with_drive accepts duplicate ids, 0, and ids colliding with the hardcoded PRINTER_DEVICE_ID. Duplicate announced ids make Rdpdr::announce_devices fail mid-connection ('device was announced more than once before server acknowledgement'), aborting the channel for what is a configuration error. The Windows sibling factory rejects duplicate ids up front and walks to a collision-free printer id; direct consumers of this exported factory bypass the client builder's collision checks.
There was a problem hiding this comment.
Fixed in 23b3805. build_rdpdr_backend now returns a NixRdpdrBackendFactoryError for drive ID 0 or a duplicate drive ID, before the channel starts. The printer no longer uses a fixed ID. It takes the highest device ID that no drive uses, starting at u32::MAX - 1 as in the Windows factory. Covered by the_factory_rejects_reserved_and_duplicate_drive_ids and the_printer_takes_an_id_no_drive_uses.
| fn submit(&self, spool: &Path, bytes: u64) { | ||
| if bytes == 0 { | ||
| debug!(?spool, "Empty print job discarded"); | ||
| let _ = std::fs::remove_file(spool); | ||
| return; | ||
| } | ||
| let mut command = Command::new("lp"); | ||
| match &self.target { | ||
| PrintTarget::DefaultPrinter => {} | ||
| PrintTarget::Printer(name) => { | ||
| command.arg("-d").arg(name); | ||
| } | ||
| PrintTarget::Folder(dir) => { | ||
| self.keep(spool, dir); | ||
| return; | ||
| } | ||
| } | ||
| command.arg("-t").arg("RDP print job").arg(spool); | ||
| match command.output() { | ||
| Ok(output) if output.status.success() => { | ||
| info!( | ||
| bytes, | ||
| "Print job handed to lp: {}", | ||
| String::from_utf8_lossy(&output.stdout).trim() | ||
| ); | ||
| let _ = std::fs::remove_file(spool); | ||
| } | ||
| Ok(output) => { | ||
| warn!( | ||
| "lp refused the print job ({}); keeping it as a file instead", | ||
| String::from_utf8_lossy(&output.stderr).trim() | ||
| ); | ||
| self.keep(spool, &self.fallback_dir.clone()); | ||
| } | ||
| Err(error) => { | ||
| warn!(%error, "lp is not available; keeping the print job as a file instead"); | ||
| self.keep(spool, &self.fallback_dir.clone()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[skeptical] Print submission performs unbounded blocking work inside the IRP handler — medium 🟠 — submit() spawns lp and waits via Command::output with no timeout, and keep() copies the entire spooled job synchronously; both run inside handle_printer_io_request, which the portable channel calls from its SVC processing loop. A hung CUPS socket or a slow folder destination stalls processing of all static channels. The Windows printer backend in this crate deliberately runs printer commands on a dedicated worker thread, so the asymmetry is a concrete design gap.
There was a problem hiding this comment.
Fixed in 23b3805. A closed job is queued for a worker thread, which runs lp or saves the file, and the close is answered right away. The queue holds 16 jobs. lp is killed if it has not accepted the job within 60 seconds. I checked this by hand with a stand-in lp that hangs, with the timeout shortened to one second: the close was answered at once and lp was killed on time.
| Ok(output) => { | ||
| warn!( | ||
| "lp refused the print job ({}); keeping it as a file instead", | ||
| String::from_utf8_lossy(&output.stderr).trim() | ||
| ); | ||
| self.keep(spool, &self.fallback_dir.clone()); | ||
| } | ||
| Err(error) => { | ||
| warn!(%error, "lp is not available; keeping the print job as a file instead"); | ||
| self.keep(spool, &self.fallback_dir.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
[skeptical] lp-failure fallback silently persists remote-driven jobs in the user's home without caps — medium 🟠 — When lp is missing (typical on CUPS-less systems) or refuses a job, the remote-rendered document is written into $HOME/Downloads or $HOME even though the user configured a printer target, not a folder. There is no limit on job count or total bytes (the channel only caps a single write at 16 MiB), so a misbehaving server can fill the disk with files that outlive the session. Explicit folder targets are opt-in; the implicit home fallback plus absent resource caps is a remote-driven disk-consumption path.
There was a problem hiding this comment.
Fixed in 23b3805. The implicit fallback is gone. A job that lp cannot take is discarded with a warning, so only an explicit folder target writes files. As in the Windows backend, a job is also limited to 128 MiB, and at most 16 jobs are open at once.
| impl PrintTarget { | ||
| /// Parses a target description: `default` (or an empty string), `folder:<dir>`, or a CUPS | ||
| /// destination name. | ||
| pub fn parse(value: &str) -> Self { | ||
| let value = value.trim(); | ||
| if value.is_empty() || value.eq_ignore_ascii_case("default") { | ||
| Self::DefaultPrinter | ||
| } else if let Some(dir) = value.strip_prefix("folder:") { | ||
| Self::Folder(PathBuf::from(dir)) | ||
| } else { | ||
| Self::Printer(value.to_owned()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[code-compressor] PrintTarget::parse is unused speculative public API — low 🟡 — No caller exists in the head tree; the only references are the method and its test. Consumers configure targets as PrintTarget values via the factory, so the 'default | folder:<dir> | name' grammar is a CLI format invented ahead of any CLI, and it accepts an empty folder: path unvalidated. Removing it shrinks the public API with zero behavior loss; it can return with its consumer.
| let moved = File::open(spool) | ||
| .and_then(|mut input| std::io::copy(&mut input, &mut output)) | ||
| .and_then(|_| output.flush()) | ||
| .and_then(|()| std::fs::remove_file(spool)); | ||
| match moved { | ||
| Ok(()) => info!(?destination, "Print job saved as a PostScript file"), | ||
| Err(error) => { | ||
| drop(output); | ||
| let _ = std::fs::remove_file(&destination); | ||
| warn!(%error, ?destination, "Could not save the print job"); | ||
| } | ||
| } |
There was a problem hiding this comment.
[code-compressor] Failed spool unlink after a successful save deletes the saved job — low 🟡 — remove_file(spool) is chained into the success pipeline, so a copy and flush that fully succeeded but whose spool unlink failed falls into the Err arm that removes the destination and logs 'Could not save the print job' — destroying a document that was saved. Cleanup is best-effort everywhere else in the file; performing the unlink after the save succeeds (ignoring its error) keeps the destination rollback for real copy/flush failures only.
There was a problem hiding this comment.
Fixed in 23b3805. Saving no longer removes the spool file. The worker removes it after the save, whatever the outcome, so a failed unlink cannot delete a saved job.
…#2007) Windows lists every dynamic channel it intends to move in its Soft-Sync request, including the ones the client declined with NO_LISTENER. Against a Windows 11 host the request lists channels 2, 6, 7, 8, 9, 10, 11 and 12 (CoreInput, MouseCursor, Graphics, Video, Geometry, ...), and only channel 7, the graphics pipeline, is open. `process_soft_sync_request` dropped a whole channel list as soon as one ID in it was not open. The tunnel was then never switched, and the channels the client had opened stayed on TCP while the server was already sending them on the tunnel (MS-RDPEDYC 3.2.5.3.1). Unopened channels are now skipped one by one, and the tunnel is switched for the rest. ## Testing - New `dvc::client::soft_sync_skips_channels_the_client_did_not_open` in `ironrdp-testsuite-core`. - Live, against a Windows 11 host over RDP-UDP version 2, with the viewer built from a branch that also carries the tunnel and client PRs of this series: the Soft-Sync request above now switches the tunnel, and the graphics pipeline moves onto it. ## Checks - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warnings` - `cargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra`, plus the lib tests of the crates touched here - `cargo test --workspace --locked` on a branch that merges this PR with the other Windows interop PRs from this series - `typos` on the changed files ## Series These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on `master` and can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order. - #2007 fix(dvc): Soft-Sync tunnel with declined channels - #2008 fix(session)!: channels and graphics on the tunnel - #2009 fix(rdpeudp): auto-detect on the tunnel - #2010 fix(graphics)!: SRL streams from Windows - #2011 fix(egfx): bitmap cache across ResetGraphics - #2012 feat(session): bandwidth measurements during the session - #2013 feat(client): graphics pipeline and RDP-UDP version options - #2014 fix(client): resize reconnects on the graphics pipeline - #2015 feat(client): transport event - #2016 feat(cliprdr): Linux clipboard backend - #2017 feat(rdpdr): printer on Linux and macOS Co-authored-by: AKolenda <testedemail2222@gmail.com>
The RDPDR printer support added for Windows has no counterpart in the `nix` backend, so Linux and macOS clients cannot offer a printer. - `NixRdpdrBackendFactory` builds a `NixRdpdrBackend` per channel and announces redirected drives and, optionally, one virtual printer that uses the default PostScript driver. - `nix::printer::PrinterSpooler` handles the printer's create, write and close requests. A job is spooled while the server streams it, then handed to CUPS with `lp` (the default or a named destination), or saved as a PostScript file in a folder. When `lp` is missing or refuses the job, the file is kept in the user's Downloads folder (or home) instead. - Spooling is private: jobs go into a directory created with `mkdtemp` (0700) as files created exclusively with mode 0600, saved files never replace or follow an existing path, a job abandoned after an oversized write is discarded on close, and dropping the backend deletes unfinished jobs. - `PrintTarget::parse` reads a target from a string: `default`, `folder:<dir>`, or a CUPS destination name.
Address the review of the Linux and macOS printer backend: - A closed job is queued for a worker thread, which hands it to `lp` or saves it in the configured folder. `lp` gets 60 seconds to accept the job before it is killed, so a hung print system no longer stalls the static channels. - A job `lp` cannot take is discarded instead of being saved in the user's home folder, so only an explicit folder target writes files. - A job is limited to 128 MiB and at most 16 jobs are open at once, as in the Windows backend. A job that fails to spool is abandoned rather than submitted in part. - A failed write reports a length of 0 (MS-RDPEPC 3.2.5.1.12), and a failed create carries no file handle and no FILE_OPENED. - Without a printer, each request is answered with its own response type instead of a close response. - The factory rejects drive ID 0 and duplicate drive IDs, and the printer takes the highest device ID no drive uses. - `reset` discards open jobs and their spool files. - A saved job is kept when removing its spool file fails. - Remove the unused `PrintTarget::parse`.
06ad5ae to
23b3805
Compare
GitHub Actions
Error
|
On the review's first finding, that the spooler state survives the RDPDR reset sequence: fixed in 23b3805. |
github-actions
Bot
added
needs-review
There was a problem hiding this comment.
PR #2017 adds MS-RDPEPC printer redirection to the nix RDPDR backend: NixRdpdrBackendFactory validates drive device IDs, picks a collision-free printer ID, and a new PrinterSpooler spools create/write/close print jobs into a private mkdtemp directory, submitting closed jobs via lp or a folder target on a worker thread. Core state machine, limits, reset/drop cleanup, and response layouts check out. Three low-severity issues stand: close for unknown FileIds returns SUCCESS where the Windows backend and MS-RDPEFS error rules indicate failure; the factory accepts empty or NUL-containing device names that every other configuration path rejects; and print() drains lp's pipes only after wait, so a chatty lp can stall the serial submission queue until the 60 s timeout.
Reduced coverage: optional reviewer code-compressor was unavailable.
| fn close(&mut self, request: DeviceIoRequest) -> RdpdrPdu { | ||
| let file_id = request.file_id; | ||
| if !self.abandoned.remove(&file_id) | ||
| && let Some(Job { spool, file, bytes }) = self.jobs.remove(&file_id) | ||
| { | ||
| drop(file); | ||
| if bytes == 0 { | ||
| debug!(?spool, "Empty print job discarded"); | ||
| let _ = std::fs::remove_file(spool); | ||
| } else { | ||
| self.queue_submission(FinishedJob { spool, bytes }); | ||
| } | ||
| } | ||
| close_response(request, NtStatus::SUCCESS) | ||
| } |
There was a problem hiding this comment.
[protocol] Close for unknown printer FileId answered with STATUS_SUCCESS — low 🟡 — PrinterSpooler::close returns SUCCESS without checking whether the FileId names a job it created and has not yet closed, so stale, duplicated, or retransmitted closes complete successfully. MS-RDPEFS directs implementations to complete requests whose FileId is not a valid outstanding create with STATUS_UNSUCCESSFUL, and the Windows spooler answers INVALID_HANDLE here, so this is both a conformance gap and a platform inconsistency. Normal sequenced closes behave correctly.
There was a problem hiding this comment.
Fixed in 1625d3a. A close for a FileId that no open job owns, including a second close of the same job, is now answered with STATUS_UNSUCCESSFUL, as MS-RDPEFS 3.1.5.2 says. Closing an abandoned job still succeeds, since that FileId was opened. Covered by closing_a_file_id_that_is_not_open_fails.
| pub fn with_drive(mut self, device_id: u32, name: String) -> Self { | ||
| self.drives.push((device_id, name)); | ||
| self | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub fn with_printer(mut self, name: String, target: PrintTarget) -> Self { | ||
| self.printer = Some((name, target)); | ||
| self | ||
| } |
There was a problem hiding this comment.
[skeptical] Factory accepts drive/printer names the Windows factory and dynamic-drive path reject — low 🟡 — with_drive and with_printer store names verbatim. Drive names are encoded as NUL-terminated UTF-16 with a truncated or DRIVE-fallback PreferredDosName, so an empty name announces an empty device and an embedded NUL truncates it server-side; the printer PrintName is NUL-terminated too. WindowsRdpdrBackendFactory rejects empty/NUL names and Rdpdr::add_dynamic_drive errors on both, leaving this new public factory as the only configuration path that lets a malformed announce through.
There was a problem hiding this comment.
Fixed in 1625d3a. build_rdpdr_backend now returns InvalidDriveName or InvalidPrinterName for an empty name or one with an embedded NUL, like the Windows factory and Rdpdr::add_dynamic_drive. Covered by the_factory_rejects_empty_and_nul_names.
| .stdin(Stdio::null()) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()); | ||
| let mut child = match command.spawn() { | ||
| Ok(child) => child, | ||
| Err(error) => { | ||
| warn!(%error, "lp is not available; the print job is discarded"); | ||
| return; | ||
| } | ||
| }; | ||
| let deadline = Instant::now() + LP_TIMEOUT; | ||
| let status = loop { | ||
| match child.try_wait() { | ||
| Ok(Some(status)) => break status, | ||
| Ok(None) if Instant::now() < deadline => std::thread::sleep(Duration::from_millis(50)), | ||
| Ok(None) => { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| warn!(timeout = ?LP_TIMEOUT, "lp did not accept the print job in time; the job is discarded"); | ||
| return; | ||
| } | ||
| Err(error) => { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| warn!(%error, "Could not wait for lp; the print job is discarded"); | ||
| return; | ||
| } | ||
| } | ||
| }; | ||
| let mut stdout = String::new(); | ||
| let mut stderr = String::new(); | ||
| if let Some(mut pipe) = child.stdout.take() { | ||
| let _ = pipe.read_to_string(&mut stdout); | ||
| } | ||
| if let Some(mut pipe) = child.stderr.take() { | ||
| let _ = pipe.read_to_string(&mut stderr); | ||
| } |
There was a problem hiding this comment.
[skeptical] print() drains lp's piped output only after wait, so a chatty lp stalls the submission queue — low 🟡 — lp runs with piped stdout/stderr while the worker spins in try_wait; the pipes are read only after the child exits. A child writing more than the pipe buffer (~64 KiB) blocks on write until the 60 s timeout kills it. submit_jobs is strictly serial and the queue holds 16 jobs, so each stalled job delays every later print by up to 60 s, after which further closed jobs are discarded with a warning. Real lp emits one line, so this is rare; draining the pipes concurrently removes it.
There was a problem hiding this comment.
Fixed in 1625d3a. stdout and stderr are now read on their own threads while lp runs, and joined after it exits or is killed. I checked it with a stand-in lp that writes 200 KiB to each pipe: the job finished in about 50 ms instead of blocking.
github-actions
Bot
added
ai-reviewed/2
Address the second review of the Linux and macOS printer backend: - Closing a FileId that no open job owns, such as one already closed, is answered with STATUS_UNSUCCESSFUL (MS-RDPEFS 3.1.5.2) instead of success. - The factory rejects an empty drive or printer name and one with an embedded NUL, since both are announced as NUL-terminated strings, as the Windows factory and the dynamic-drive path already do. - `lp`'s output is read on threads while it runs, so output beyond the pipe buffer cannot block it until the timeout and stall the queue.
github-actions
Bot
added
the
needs-review
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you! LGTM
d839ad3
into
Devolutions:master
The RDPDR printer support added for Windows (#1876) has no counterpart in the
nixbackend, so Linux and macOS clients cannot offer a printer.NixRdpdrBackendFactorybuilds aNixRdpdrBackendfor every RDPDR channel. It announces redirected drives and, optionally, one virtual printer that uses the default PostScript driver. It rejects drive ID 0, duplicate drive IDs, and empty device names or ones with an embedded NUL before the channel starts, and the printer takes the highest device ID no drive uses, as in the Windows factory.nix::printer::PrinterSpoolerhandles the printer's create, write and close requests. A job is spooled while the server streams it. Once it is closed, a worker thread:lp, either to the default destination or to a named one; orThe channel never waits for either.
lpgets 60 seconds to accept a job before it is killed, and its output is read while it runs. A job thatlpcannot take is discarded with a warning, so only an explicit folder target writes files.Limits, as in the Windows backend: a job is limited to 128 MiB, and at most 16 jobs are open at once. A job that fails to spool is abandoned rather than submitted in part. A failed write reports a length of 0 (MS-RDPEPC 3.2.5.1.12), and a failed create carries no file handle and no
FILE_OPENED. A close for a FileId that is not open is answered with STATUS_UNSUCCESSFUL (MS-RDPEFS 3.1.5.2).Private spooling:
mkdtemp(0700), as files created exclusively with mode 0600.resetand dropping the backend delete unfinished jobs. Jobs that were already closed are still submitted, and the worker thread then removes the spool directory.nixgains itsfeaturefeature, whichunistd::mkdtemprequires.Testing
Twelve unit tests:
resetdiscarding open jobs;By hand, with a stand-in
lpscript onPATH: the job reacheslpwith-d <name>, and the worker removes the spool directory afterwards. With the timeout shortened to one second, alpthat hangs is killed on time, and the close is answered without waiting for it. A stand-inlpthat writes 200 KiB to each pipe no longer blocks.Checks
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warningscargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra, plus the lib tests of the crates touched herecargo test --workspace --lockedon a branch that merges this PR with the other Windows interop PRs from this seriestyposon the changed filesSeries
These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on
masterand can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order.