feat(NowAgent): refactoring after review

Author: pacmancoder

Cargo.lock

@@ -25,9 +25,7 @@ tracing = { version = "0.1", features = ["log"] }
 
 
 [target.'cfg(windows)'.dependencies]
-
 widestring = "1"
-smallvec = "1"
 windows = { version = "0.61", features = [
     "Win32_Foundation",
     "Win32_Security",

crates/ironrdp-dvc-pipe-proxy/Cargo.toml

@@ -1,5 +1,15 @@
 # IronRDP DVC pipe proxy
 
-Generic DVC handler which makes IronRDP connect to specific DVC channel and create a named pipe
-server, which will be used for proxying DVC messages to/from user-defined DVC logic
-implemented as named pipe clients (either in the same process or in a different process).
+This crate provides a Device Virtual Channel (DVC) handler for IronRDP, enabling proxying of RDP DVC
+traffic over a named pipe.
+
+It was originally designed to simplify custom DVC integration within Devolutions Remote Desktop
+Manager (RDM). By implementing a thin pipe proxy for target RDP clients (such as IronRDP, FreeRDP,
+mstsc, etc.), the main client logic can be centralized and reused across all supported clients via a
+named pipe.
+
+This approach allows you to implement your DVC logic in one place, making it easier to support
+multiple RDP clients without duplicating code.
+
+Additionally, this crate can be used for other scenarios, such as testing your own custom DVC
+channel client, without needing to patch or rebuild IronRDP itself.

crates/ironrdp-dvc-pipe-proxy/README.md

@@ -45,7 +45,7 @@ pub(crate) fn worker_thread_func(worker_ctx: WorkerCtx) -> Result<(), DvcPipePro
         if !connect_ctx.overlapped_connect()? {
             const EVENT_ID_ABORT: usize = 0;
             let events = [abort_event.borrow(), connect_ctx.borrow_event()];
-            let wait_result = match wait_any_with_timeout(events, PIPE_CONNECT_TIMEOUT_SECS) {
+            let wait_result = match wait_any_with_timeout(&events, PIPE_CONNECT_TIMEOUT_SECS) {
                 Ok(idx) => idx,
                 Err(WindowsError::WaitForMultipleObjectsTimeout) => {
                     warn!(%channel_name, %pipe_name, "DVC pipe proxy connection timed out");
@@ -83,7 +83,7 @@ pub(crate) fn worker_thread_func(worker_ctx: WorkerCtx) -> Result<(), DvcPipePro
             read_ctx.borrow_event(),
             to_pipe_semaphore.borrow(),
         ];
-        let wait_result = wait_any(events)?;
+        let wait_result = wait_any(&events)?;
 
         if wait_result == EVENT_ID_ABORT {
             info!(%channel_name, %pipe_name, "DVC pipe proxy connection has been aborted");
@@ -132,7 +132,7 @@ pub(crate) fn worker_thread_func(worker_ctx: WorkerCtx) -> Result<(), DvcPipePro
             overlapped_write.overlapped_write()?;
 
             let events = [abort_event.borrow(), overlapped_write.borrow_event()];
-            let wait_result = wait_any_with_timeout(events, PIPE_WRITE_TIMEOUT_SECS)?;
+            let wait_result = wait_any_with_timeout(&events, PIPE_WRITE_TIMEOUT_SECS)?;
 
             if wait_result == EVENT_ID_ABORT {
                 info!(%channel_name, %pipe_name, "DVC pipe proxy write aborted");

crates/ironrdp-dvc-pipe-proxy/src/platform/windows/worker.rs

@@ -12,7 +12,6 @@ pub(crate) use event::Event;
 pub(crate) use pipe::MessagePipeServer;
 pub(crate) use semaphore::Semaphore;
 
-use smallvec::SmallVec;
 use windows::Win32::Foundation::{
     ERROR_IO_PENDING, HANDLE, WAIT_ABANDONED_0, WAIT_EVENT, WAIT_FAILED, WAIT_OBJECT_0, WAIT_TIMEOUT,
 };
@@ -21,17 +20,19 @@ use windows::Win32::System::Threading::{WaitForMultipleObjects, INFINITE};
 /// Thin wrapper around borrowed `windows` crate `HANDLE` reference.
 /// This is used to ensure handle lifetime when passing it to FFI functions
 /// (see `wait_any_with_timeout` for example).
+#[repr(transparent)]
 pub(crate) struct BorrowedHandle<'a>(&'a HANDLE);
 
 /// Safe wrapper around `WaitForMultipleObjects`.
-pub(crate) fn wait_any_with_timeout<'a, T>(handles: T, timeout: u32) -> Result<usize, WindowsError>
-where
-    T: IntoIterator<Item = BorrowedHandle<'a>>,
-{
-    let handles: SmallVec<[HANDLE; 8]> = handles.into_iter().map(|h| *h.0).collect();
+pub(crate) fn wait_any_with_timeout(handles: &[BorrowedHandle<'_>], timeout: u32) -> Result<usize, WindowsError> {
+    let handles = cast_handles(handles);
 
-    // SAFETY: FFI call with no outstanding preconditions.
-    let result = unsafe { WaitForMultipleObjects(&handles, false, timeout) };
+    // SAFETY:
+    // - BorrowedHandle alongside with rust type system ensures that the HANDLEs are valid for
+    // the duration of the call.
+    // - All handles in this module have SYNCHRONIZE access rights.
+    // - cast_handles ensures no handle duplicates.
+    let result = unsafe { WaitForMultipleObjects(handles, false, timeout) };
 
     match result {
         WAIT_FAILED => Err(WindowsError::WaitForMultipleObjectsFailed(
@@ -47,17 +48,45 @@ where
 }
 
 /// Safe `WaitForMultipleObjects` wrapper with infinite timeout.
-pub(crate) fn wait_any<'a, T>(handles: T) -> Result<usize, WindowsError>
-where
-    T: IntoIterator<Item = BorrowedHandle<'a>>,
-{
+pub(crate) fn wait_any(handles: &[BorrowedHandle<'_>]) -> Result<usize, WindowsError> {
     // Standard generic syntax is used instead if `impl` because of the following lint:
     // > warning: lifetime parameter `'a` only used once
     //
     // Fixing this lint (use of '_ lifetime) produces compiler error.
     wait_any_with_timeout(handles, INFINITE)
 }
 
+fn cast_handles<'a>(handles: &'a [BorrowedHandle<'a>]) -> &'a [HANDLE] {
+    // Very basic sanity checks to ensure that the handles are valid
+    // and there are no duplicates.
+    // This is only done in debug builds to avoid performance overhead in release builds, while
+    // still catching undefined behavior early in development.
+    #[cfg(debug_assertions)]
+    {
+        // Ensure that there are no duplicate handles without hash.
+        for (i, handle) in handles.iter().enumerate() {
+            for other_handle in &handles[i + 1..] {
+                if handle.0 == other_handle.0 {
+                    panic!("Duplicate handle found in wait_any_with_timeout");
+                }
+            }
+        }
+    }
+
+    for handle in handles {
+        // Ensure that the handle is valid.
+        if handle.0.is_invalid() {
+            panic!("Invalid handle in wait_any_with_timeout");
+        }
+    }
+
+    // SAFETY:
+    // - BorrowedHandle is #[repr(transparent)] over *const c_void, and so is HANDLE,
+    //   so the layout is the same.
+    // - We ensure the lifetime is preserved.
+    unsafe { core::slice::from_raw_parts(handles.as_ptr() as *const HANDLE, handles.len()) }
+}
+
 /// Maps ERROR_IO_PENDING to Ok(()) and returns other errors as is.
 fn ensure_overlapped_io_result(result: windows::core::Result<()>) -> Result<windows::core::Result<()>, WindowsError> {
     if let Err(error) = &result {

crates/ironrdp-dvc-pipe-proxy/src/windows/mod.rs

@@ -186,8 +186,10 @@ impl<'a> OverlappedPipeReadCtx<'a> {
     }
 
     pub(crate) fn overlapped_read(&mut self) -> Result<(), WindowsError> {
-        // SAFETY: hfile is a valid handle to a named pipe; lpBuffer
-        // is a valid pointer which should be alive until the operation is completed;
+        // SAFETY: self.pipe.raw() returns a valid handle. The read buffer pointer returned
+        // by self.buffer.as_mut_slice() is valid and remains alive for the entire duration
+        // of the overlapped I/O operation. The OVERLAPPED structure is pinned and not moved
+        // in memory, ensuring its address remains stable until the operation completes.
         let result = unsafe {
             ReadFile(
                 self.pipe.raw(),
@@ -251,8 +253,10 @@ impl<'a> OverlappedWriteCtx<'a> {
     }
 
     pub(crate) fn overlapped_write(&mut self) -> Result<(), WindowsError> {
-        // SAFETY: hfile is a valid handle to a named pipe; lpBuffer
-        // is a valid pointer which should be alive until the operation is completed;
+        // SAFETY: self.pipe.raw() returns a valid handle. The write buffer pointer (&self.data) is valid
+        // and remains alive for the entire duration of the overlapped I/O operation. The OVERLAPPED
+        // structure is pinned and not moved in memory, ensuring its address remains stable until the
+        // operation completes.
         let result = unsafe {
             WriteFile(
                 self.pipe.raw(),
@@ -271,8 +275,7 @@ impl<'a> OverlappedWriteCtx<'a> {
 
     pub(crate) fn get_result(&mut self) -> Result<u32, WindowsError> {
         let mut bytes_written = 0u32;
-
-        // SAFETY: The handle is valid and we are the owner of the handle.
+        // SAFETY: The pipe handle is valid and we are the owner of the handle.
         unsafe {
             GetOverlappedResult(
                 self.pipe.raw(),