[DRAFT][WIP] feat(session): RDM messages bootstrap #1538
Conversation
Let maintainers know that an action is required on their side
|
There was a problem hiding this comment.
Pull Request Overview
This draft PR introduces support for Remote Desktop Manager (RDM) application lifecycle management through the NOW protocol, including capabilities detection, application launching, and window control operations.
- Adds RDM-specific message handlers for capabilities, app start, and app actions
- Implements registry-based RDM installation detection and version retrieval
- Adds process monitoring and window management functionality for RDM instances
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| devolutions-session/src/main.rs | Adds devolutions_agent_shared import for RDM registry access |
| devolutions-session/src/dvc/task.rs | Implements RDM message handlers, process spawning/monitoring, and window control |
| devolutions-session/Cargo.toml | Updates now-proto-pdu to 0.4.0 and adds devolutions-agent-shared dependency |
| crates/devolutions-agent-shared/src/windows/registry.rs | Adds product version encoding variants and install location retrieval function |
| crates/devolutions-agent-shared/src/windows/mod.rs | Defines RDM_UPDATE_CODE constant for registry lookups |
| crates/devolutions-agent-shared/src/lib.rs | Updates agent version retrieval to use new encoding parameter |
| Cargo.lock | Updates dependency versions including now-proto-pdu 0.4.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let _ = EnumWindows(Some(enum_windows_proc), LPARAM(Box::into_raw(context) as isize)); | ||
| } |
There was a problem hiding this comment.
Memory leak: Similar to send_message_to_all_windows, the context is never properly freed if EnumWindows fails or if the callback is never invoked. The current implementation also has undefined behavior due to incorrect Box::from_raw usage in the callback (see comment on line 1493).
| } | ||
| NowRdmAppAction::MINIMIZE => { | ||
| info!(process_id, "Minimizing RDM application"); | ||
| exec_window_command(process_id, WindowCommand::Minimize) |
There was a problem hiding this comment.
Missing semicolon: This line lacks a semicolon at the end, while similar calls on lines 975 and 979 have semicolons. This will cause the function to return () from the exec_window_command call instead of falling through to Ok(()).
| exec_window_command(process_id, WindowCommand::Minimize) | |
| exec_window_command(process_id, WindowCommand::Minimize); |
| lparam, | ||
| }); | ||
|
|
||
| // SAFETY: Context is a valid pointer to heap memory, threfore call is safe. |
There was a problem hiding this comment.
Typo in comment: "threfore" should be "therefore"
| // SAFETY: Context is a valid pointer to heap memory, threfore call is safe. | |
| // SAFETY: Context is a valid pointer to heap memory, therefore call is safe. |
| fn exec_window_command(process_id: u32, command: WindowCommand) { | ||
| let context = Box::new(WindowCommandEnumContext { process_id, command }); | ||
|
|
||
| // SAFETY: Context is a valid pointer to heap memory, threfore call is safe. |
There was a problem hiding this comment.
Typo in comment: "threfore" should be "therefore"
| // SAFETY: Context is a valid pointer to heap memory, threfore call is safe. | |
| // SAFETY: Context is a valid pointer to heap memory, therefore call is safe. |
| let context = Box::new(SendMessageEnumContext { | ||
| process_id, | ||
| message, | ||
| wparam, | ||
| lparam, | ||
| }); | ||
|
|
||
| // SAFETY: Context is a valid pointer to heap memory, threfore call is safe. | ||
| unsafe { | ||
| let _ = EnumWindows(Some(enum_windows_send_message), LPARAM(Box::into_raw(context) as isize)); | ||
| } | ||
| } | ||
|
|
||
| extern "system" fn enum_windows_send_message(hwnd: HWND, lparam: LPARAM) -> windows::core::BOOL { | ||
| // SAFETY: lparam is a valid pointer to MessageSendContext | ||
| let context = unsafe { Box::from_raw(lparam.0 as *mut SendMessageEnumContext) }; |
There was a problem hiding this comment.
Memory leak: The context created with Box::into_raw is never properly freed. While enum_windows_send_message converts it back using Box::from_raw, this only works correctly if the callback is called exactly once. If EnumWindows fails or the callback is never called, the memory will leak. Additionally, the current implementation causes undefined behavior when the callback is invoked multiple times (see separate comment on line 1453).
Consider wrapping the EnumWindows call in a structure that ensures cleanup in all cases, or use a reference-based approach that doesn't transfer ownership.
| let context = Box::new(SendMessageEnumContext { | |
| process_id, | |
| message, | |
| wparam, | |
| lparam, | |
| }); | |
| // SAFETY: Context is a valid pointer to heap memory, threfore call is safe. | |
| unsafe { | |
| let _ = EnumWindows(Some(enum_windows_send_message), LPARAM(Box::into_raw(context) as isize)); | |
| } | |
| } | |
| extern "system" fn enum_windows_send_message(hwnd: HWND, lparam: LPARAM) -> windows::core::BOOL { | |
| // SAFETY: lparam is a valid pointer to MessageSendContext | |
| let context = unsafe { Box::from_raw(lparam.0 as *mut SendMessageEnumContext) }; | |
| let mut context = SendMessageEnumContext { | |
| process_id, | |
| message, | |
| wparam, | |
| lparam, | |
| }; | |
| // SAFETY: Context is a valid pointer to stack memory and lives for the duration of EnumWindows. | |
| unsafe { | |
| let context_ptr = &mut context as *mut SendMessageEnumContext; | |
| let _ = EnumWindows(Some(enum_windows_send_message), LPARAM(context_ptr as isize)); | |
| } | |
| } | |
| extern "system" fn enum_windows_send_message(hwnd: HWND, lparam: LPARAM) -> windows::core::BOOL { | |
| // SAFETY: lparam is a valid pointer to SendMessageEnumContext for the duration of EnumWindows | |
| let context = unsafe { &mut *(lparam.0 as *mut SendMessageEnumContext) }; |
| Ok(()) | ||
| } | ||
|
|
||
| /// Send RDM app notification message |
There was a problem hiding this comment.
Incorrect documentation: The comment says "Send RDM app notification message" but this is the blocking variant. Should be "Send RDM app notification message (blocking)"
| /// Send RDM app notification message | |
| /// Send RDM app notification message (blocking) |
| } else { | ||
| // Close thread handle as we don't need it | ||
|
|
||
| // SAFETY: It is create owned handle wrapper from created process thread handle |
There was a problem hiding this comment.
Grammar issue in comment: "It is create owned handle wrapper" should be "It is safe to create an owned handle wrapper"
| // SAFETY: It is create owned handle wrapper from created process thread handle | |
| // SAFETY: It is safe to create an owned handle wrapper from the created process thread handle |
| // SAFETY: It is create owned handle wrapper from created process thread handle | ||
| let _ = unsafe { Handle::new(process_info.hThread, true) }; | ||
|
|
||
| // SAFETY: It is safe to create owned handle wrapper from created process handle |
There was a problem hiding this comment.
Grammar issue in comment: Similar to line 906, the phrasing should be improved for clarity
| // SAFETY: It is safe to create owned handle wrapper from created process handle | |
| // SAFETY: It is safe to create an owned handle wrapper from the process handle returned by CreateProcessW. |
| // Context for window enumeration callback | ||
| struct SendMessageEnumContext { | ||
| process_id: u32, | ||
| message: u32, | ||
| wparam: WPARAM, | ||
| lparam: LPARAM, | ||
| } |
There was a problem hiding this comment.
[nitpick] Duplicate code: The find_rdm_pid and is_rdm_running functions (lines 1281-1323 and 1389-1426) contain nearly identical logic for iterating through processes and comparing executable paths. Consider extracting the common logic into a shared helper function that returns Option<u32> and can be used by both functions. The is_rdm_running function could then simply call find_rdm_pid().is_some().
| let short_year = match version_encoding { | ||
| ProductVersionEncoding::Agent => (product_version >> 24) + 2000, | ||
| ProductVersionEncoding::Rdm => (product_version >> 24) + 0x700, |
There was a problem hiding this comment.
Magic number: The constant 0x700 (1792 in decimal) for RDM version encoding is unexplained. This magic number makes the code difficult to understand and maintain. Consider adding a comment explaining what this value represents and why it's used for RDM version calculation, or define it as a named constant with documentation.
No description provided.