refactoring: fixed minor comments

Cargo.toml

@@ -3,14 +3,15 @@ resolver = "2"
 members = [
     "crates/*",
     "devolutions-agent",
-    "devolutions-gateway", "devolutions-host",
+    "devolutions-gateway",
+    "devolutions-host",
     "jetsocat",
     "tools/generate-openapi",
 ]
 default-members = [
     "devolutions-agent",
     "devolutions-gateway",
-    "devolutions-agent",
+    "devolutions-host",
     "jetsocat",
 ]
 

crates/win-api-wrappers/src/handle.rs

@@ -1,8 +1,7 @@
 use std::fmt::Debug;
 use std::os::windows::io::{AsRawHandle, BorrowedHandle, IntoRawHandle, OwnedHandle};
 
-use windows::Win32::Foundation::E_HANDLE;
-use windows::Win32::Foundation::{CloseHandle, DuplicateHandle, DUPLICATE_SAME_ACCESS, HANDLE};
+use windows::Win32::Foundation::{CloseHandle, DuplicateHandle, DUPLICATE_SAME_ACCESS, E_HANDLE, HANDLE};
 use windows::Win32::System::Threading::GetCurrentProcess;
 
 // TODO: Use/implement AsHandle and AsRawHandle as appropriate

crates/win-api-wrappers/src/process.rs

@@ -6,18 +6,19 @@ use std::os::windows::ffi::OsStringExt;
 use std::path::{Path, PathBuf};
 use std::{ptr, slice};
 
-use anyhow::{anyhow, bail, Result};
+use anyhow::{bail, Context, Result};
 
 use crate::handle::{Handle, HandleWrapper};
 use crate::security::acl::{RawSecurityAttributes, SecurityAttributes};
 use crate::thread::Thread;
 use crate::token::Token;
 use crate::undoc::{NtQueryInformationProcess, ProcessBasicInformation, RTL_USER_PROCESS_PARAMETERS};
-use crate::utils::{serialize_environment, Allocation, AnsiString, CommandLine, WideString};
+use crate::utils::{Allocation, AnsiString, CommandLine, WideString};
 use crate::Error;
 use windows::core::PCWSTR;
 use windows::Win32::Foundation::{
-    FreeLibrary, ERROR_INCORRECT_SIZE, HANDLE, HMODULE, MAX_PATH, WAIT_EVENT, WAIT_FAILED,
+    FreeLibrary, ERROR_INCORRECT_SIZE, ERROR_NO_MORE_FILES, E_INVALIDARG, HANDLE, HMODULE, MAX_PATH, WAIT_EVENT,
+    WAIT_FAILED,
 };
 use windows::Win32::Security::{
     SE_ASSIGNPRIMARYTOKEN_NAME, SE_INCREASE_QUOTA_NAME, SE_TCB_NAME, TOKEN_ACCESS_MASK, TOKEN_ADJUST_PRIVILEGES,
@@ -566,9 +567,14 @@ pub struct ProcessEntry32Iterator {
 
 impl ProcessEntry32Iterator {
     pub fn new() -> Result<Self> {
-        // SAFETY: `CreateToolhelp32Snapshot` call is always safe and `Owned` wrapper ensures
-        // that the handle is properly closed when the iterator is dropped.
-        let snapshot_handle = unsafe { Handle::new(CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0)?, true) };
+        // SAFETY: `CreateToolhelp32Snapshot` call is always safe to call and returns a
+        // valid handle on success.
+        let raw_handle = unsafe { CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0) }?;
+
+        // SAFETY: `Handle::new` is safe to call because the `raw_handle` is valid.
+        let snapshot_handle = unsafe {
+            Handle::new(raw_handle, true).expect("BUG: handle should be valid after CreateToolhelp32Snapshot call")
+        };
 
         // SAFETY: It is safe to zero out the structure as it is a simple POD type.
         let mut process_entry: PROCESSENTRY32W = unsafe { mem::zeroed() };
@@ -590,7 +596,7 @@ impl Iterator for ProcessEntry32Iterator {
     fn next(&mut self) -> Option<Self::Item> {
         let result = if self.first {
             // SAFETY: `windows` library ensures that `snapshot` handle is correct on creation,
-            // therefore it is safe to call Process32First
+            // therefore it is safe to call Process32First.
             unsafe { Process32FirstW(self.snapshot_handle.raw(), &mut self.process_entry as *mut _) }
         } else {
             // SAFETY: `Process32Next` is safe to call because the `snapshot_handle` is valid while it
@@ -599,9 +605,9 @@ impl Iterator for ProcessEntry32Iterator {
         };
 
         match result {
-            Err(err) if err.code() == ERROR_NO_MORE_FILES.to_hresult() => None,
-            Err(err) => {
-                error!(%err, "Failed to iterate over processes");
+            Err(error) if error.code() == ERROR_NO_MORE_FILES.to_hresult() => None,
+            Err(error) => {
+                error!(%error, "Failed to iterate over processes");
                 None
             }
             Ok(()) => {
@@ -627,10 +633,10 @@ impl ProcessEntry {
             .szExeFile
             .iter()
             .position(|&c| c == 0)
-            .ok_or_else(|| anyhow!("Executable name null terminator not found"))?;
+            .context("executable name null terminator not found")?;
 
         let name = String::from_utf16(&self.0.szExeFile[..exe_name_length])
-            .map_err(|_| anyhow!("Invalid executable name UTF16 encoding"))?;
+            .context("invalid executable name UTF16 encoding")?;
 
         Ok(name)
     }
@@ -653,11 +659,12 @@ impl ProcessEnvironment {
 impl Drop for ProcessEnvironment {
     fn drop(&mut self) {
         if let ProcessEnvironment::OsDefined(block) = self {
-            // SAFETY: `block` is checked to be valid before free.
+            // SAFETY: `ProcessEnvironment` is a private enum, and we ensured that `block` will only
+            // ever hold pointers returned by `CreateEnvironmentBlock` in the current module.
             unsafe {
                 if !block.is_null() {
-                    if let Err(err) = DestroyEnvironmentBlock(*block) {
-                        warn!(%err, "Failed to destroy environment block");
+                    if let Err(error) = DestroyEnvironmentBlock(*block) {
+                        warn!(%error, "Failed to destroy environment block");
                     }
                 }
             };
@@ -688,7 +695,13 @@ pub fn create_process_as_user(
         let mut environment: *mut c_void = ptr::null_mut();
 
         if let Some(token) = token {
-            // SAFETY: Allocated memory will be freed by `ProcessEnvironment` destructor.
+            // SAFETY: As per `CreateEnvironmentBlock` documentation: We must specify
+            // `CREATE_UNICODE_ENVIRONMENT` and call `DestroyEnvironmentBlock` after
+            // `CreateProcessAsUser` call.
+            // - `CREATE_UNICODE_ENVIRONMENT` is always set unconditionaly.
+            // - `DestroyEnvironmentBlock` is called in the `ProcessEnvironment` destructor.
+            //
+            // Therefore, all preconditions are met to safely call `CreateEnvironmentBlock`.
             unsafe { CreateEnvironmentBlock(&mut environment, token.handle().raw(), false) }?;
         }
 
@@ -741,6 +754,25 @@ pub fn create_process_as_user(
     })
 }
 
+fn serialize_environment(environment: &HashMap<String, String>) -> Result<Vec<u16>> {
+    let mut serialized = Vec::new();
+
+    for (k, v) in environment.iter() {
+        if k.contains('=') {
+            bail!(Error::from_hresult(E_INVALIDARG));
+        }
+
+        serialized.extend(k.encode_utf16());
+        serialized.extend("=".encode_utf16());
+        serialized.extend(v.encode_utf16());
+        serialized.push(0);
+    }
+
+    serialized.push(0);
+
+    Ok(serialized)
+}
+
 /// Starts new process in the specified session. Note that this function requires the current
 /// process to have `SYSTEM`-level permissions. Use with caution.
 #[allow(clippy::too_many_arguments)]
@@ -844,16 +876,16 @@ fn terminate_process_by_name_impl(process_name: &str, session_id: Option<u32>) -
                 Ok(process) => {
                     // SAFETY: `OpenProcess` ensures that the handle is valid.
                     unsafe {
-                        if let Err(err) = TerminateProcess(process, 1) {
-                            warn!(process_name, session_id, %err, "TerminateProcess failed");
+                        if let Err(error) = TerminateProcess(process, 1) {
+                            warn!(process_name, session_id, %error, "TerminateProcess failed");
                             return Ok(false);
                         }
                     }
 
                     return Ok(true);
                 }
-                Err(err) => {
-                    warn!(process_name, session_id, %err, "OpenProcess failed");
+                Err(error) => {
+                    warn!(process_name, session_id, %error, "OpenProcess failed");
                     continue;
                 }
             }

crates/win-api-wrappers/src/security/privilege.rs

@@ -156,11 +156,11 @@ impl<'a> ScopedPrivileges<'a> {
 
 impl Drop for ScopedPrivileges<'_> {
     fn drop(&mut self) {
-        if let Err(err) = self
+        if let Err(error) = self
             .token
             .adjust_privileges(&TokenPrivilegesAdjustment::Disable(self.token_privileges.clone()))
         {
-            error!(%err, "Failed to disable ScopedPrivileges({})", self.description);
+            error!(%error, "Failed to disable ScopedPrivileges({})", self.description);
         }
     }
 }

crates/win-api-wrappers/src/session.rs

@@ -2,7 +2,7 @@ use super::handle::Handle;
 use super::process::Process;
 use super::security::privilege::ScopedPrivileges;
 use anyhow::Result;
-use windows::Win32::Foundation::{ERROR_NO_TOKEN, HANDLE};
+use windows::Win32::Foundation::{CloseHandle, ERROR_NO_TOKEN, HANDLE};
 use windows::Win32::Security::{SE_TCB_NAME, TOKEN_ADJUST_PRIVILEGES, TOKEN_QUERY};
 use windows::Win32::System::RemoteDesktop::WTSQueryUserToken;
 
@@ -20,7 +20,8 @@ pub fn session_has_logged_in_user(session_id: u32) -> Result<bool> {
         Err(err) => Err(err.into()),
         Ok(()) => {
             // Close handle immediately.
-            let _handle: Handle = Handle::new(handle, true);
+            // SAFETY: `CloseHandle` is safe to call with a valid handle.
+            unsafe { CloseHandle(handle).expect("BUG: WTSQueryUserToken should return a valid handle") };
             Ok(true)
         }
     }

crates/win-api-wrappers/src/token.rs

@@ -24,7 +24,7 @@ use crate::undoc::{
     TOKEN_SECURITY_ATTRIBUTE_TYPE_OCTET_STRING, TOKEN_SECURITY_ATTRIBUTE_TYPE_STRING,
     TOKEN_SECURITY_ATTRIBUTE_TYPE_UINT64, TOKEN_SECURITY_ATTRIBUTE_V1, TOKEN_SECURITY_ATTRIBUTE_V1_VALUE,
 };
-use crate::utils::WideString;
+use crate::utils::{size_of_u32, WideString};
 use crate::{create_impersonation_context, Error};
 use windows::Win32::Foundation::{
     ERROR_ALREADY_EXISTS, ERROR_INVALID_SECURITY_DESCR, ERROR_INVALID_VARIANT, ERROR_NO_TOKEN, ERROR_SUCCESS, HANDLE,
@@ -42,10 +42,8 @@ use windows::Win32::Security::{
     TOKEN_INFORMATION_CLASS, TOKEN_MANDATORY_POLICY, TOKEN_MANDATORY_POLICY_ID, TOKEN_OWNER, TOKEN_PRIMARY_GROUP,
     TOKEN_PRIVILEGES, TOKEN_PRIVILEGES_ATTRIBUTES, TOKEN_QUERY, TOKEN_SOURCE, TOKEN_STATISTICS, TOKEN_TYPE, TOKEN_USER,
 };
-use windows::Win32::System::SystemServices::SE_GROUP_LOGON_ID;
-
-use super::utils::size_of_u32;
 use windows::Win32::System::RemoteDesktop::WTSQueryUserToken;
+use windows::Win32::System::SystemServices::SE_GROUP_LOGON_ID;
 
 #[derive(Debug)]
 pub struct Token {

crates/win-api-wrappers/src/utils.rs

@@ -343,25 +343,6 @@ pub unsafe fn set_memory_protection(
     Ok(old_prot)
 }
 
-pub fn serialize_environment(environment: &HashMap<String, String>) -> Result<Vec<u16>> {
-    let mut serialized = Vec::new();
-
-    for (k, v) in environment.iter() {
-        if k.contains('=') {
-            bail!(Error::from_hresult(E_INVALIDARG));
-        }
-
-        serialized.extend(k.encode_utf16());
-        serialized.extend("=".encode_utf16());
-        serialized.extend(v.encode_utf16());
-        serialized.push(0);
-    }
-
-    serialized.push(0);
-
-    Ok(serialized)
-}
-
 pub fn environment_block(token: Option<&Token>, inherit: bool) -> Result<HashMap<String, String>> {
     let mut blocks = Vec::new();
 

devolutions-agent/src/main.rs

@@ -60,6 +60,7 @@ fn agent_service_main(
 
             match control_code {
                 AgentServiceEvent::Stop => {
+                    service.stop();
                     break;
                 }
                 AgentServiceEvent::SessionConnect(_)
@@ -69,10 +70,10 @@ fn agent_service_main(
                 | AgentServiceEvent::SessionLogon(_)
                 | AgentServiceEvent::SessionLogoff(_) => {
                     if let Some(tx) = service_event_tx.as_mut() {
-                        match tx.send(control_code) {
+                        match tx.blocking_send(control_code) {
                             Ok(()) => {}
-                            Err(err) => {
-                                error!(%err, "Failed to send event to session manager");
+                            Err(error) => {
+                                error!(%error, "Failed to send event to session manager");
                                 service_event_tx = None;
                             }
                         }

devolutions-agent/src/service.rs

@@ -24,7 +24,7 @@ struct TasksCtx {
     /// Spawned service tasks
     tasks: Tasks,
     /// Sender side of the service event channel (Used for session manager module)
-    service_event_tx: Option<mpsc::UnboundedSender<AgentServiceEvent>>,
+    service_event_tx: Option<mpsc::Sender<AgentServiceEvent>>,
 }
 
 #[allow(clippy::large_enum_variant)] // `Running` variant is bigger than `Stopped` but we don't care
@@ -40,7 +40,7 @@ pub struct AgentService {
     conf_handle: ConfHandle,
     state: AgentState,
     _logger_guard: LoggerGuard,
-    service_event_tx: Option<mpsc::UnboundedSender<AgentServiceEvent>>,
+    service_event_tx: Option<mpsc::Sender<AgentServiceEvent>>,
 }
 
 impl AgentService {
@@ -165,7 +165,7 @@ impl AgentService {
         }
     }
 
-    pub fn service_event_tx(&self) -> Option<mpsc::UnboundedSender<AgentServiceEvent>> {
+    pub fn service_event_tx(&self) -> Option<mpsc::Sender<AgentServiceEvent>> {
         self.service_event_tx.clone()
     }
 }