diff --git a/rust/Cargo.lock b/rust/Cargo.lock index f0606a143a..dba9fcb2f4 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -124,6 +124,7 @@ dependencies = [ "serde_json", "serde_with", "serde_yaml", + "subprocess", "thiserror", "tokio", "tokio-openssl", @@ -3283,6 +3284,16 @@ version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5ee073c9e4cd00e28217186dbe12796d692868f432bf2e97ee73bed0c56dfa01" +[[package]] +name = "subprocess" +version = "0.2.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0c2e86926081dda636c546d8c5e641661049d7562a68f5488be4a1f7f66f6086" +dependencies = [ + "libc", + "winapi", +] + [[package]] name = "subtle" version = "2.5.0" diff --git a/rust/agama-server/Cargo.toml b/rust/agama-server/Cargo.toml index ab26dcff19..f39e0766d3 100644 --- a/rust/agama-server/Cargo.toml +++ b/rust/agama-server/Cargo.toml @@ -55,6 +55,7 @@ futures-util = { version = "0.3.30", default-features = false, features = [ "alloc", ] } libsystemd = "0.7.0" +subprocess = "0.2.9" [[bin]] name = "agama-dbus-server" diff --git a/rust/agama-server/src/l10n/l10n.rs b/rust/agama-server/src/l10n/l10n.rs index 5c70cce1df..e81d805426 100644 --- a/rust/agama-server/src/l10n/l10n.rs +++ b/rust/agama-server/src/l10n/l10n.rs @@ -1,9 +1,12 @@ +use std::env; use std::io; use std::process::Command; +use std::time::Duration; use crate::error::Error; use agama_locale_data::{KeymapId, LocaleId}; use regex::Regex; +use subprocess::{Popen, PopenConfig, PopenError, Redirection}; use super::keyboard::KeymapsDatabase; use super::locale::LocalesDatabase; @@ -21,6 +24,79 @@ pub struct L10n { pub ui_keymap: KeymapId, } +// timeout for the setxkbmap call (in seconds), when there is an authentication +// problem when accessing the X server then it enters an infinite loop +const SETXKBMAP_TIMEOUT: u64 = 3; + +// helper function which runs a command with timeout and collects it's standard +// output +fn run_with_timeout(cmd: &[&str], timeout: u64) -> Result, PopenError> { + // start the subprocess + let mut process = Popen::create( + cmd, + PopenConfig { + stdout: Redirection::Pipe, + stderr: Redirection::Pipe, + ..Default::default() + }, + )?; + + // wait for it to finish or until the timeout is reached + if process + .wait_timeout(Duration::from_secs(timeout))? + .is_none() + { + tracing::warn!("Command {:?} timed out!", cmd); + // if the process is still running after the timeout then terminate it, + // ignore errors, there is another attempt later to kill the process + let _ = process.terminate(); + + // give the process some time to react to SIGTERM + if process.wait_timeout(Duration::from_secs(1))?.is_none() { + // process still running, kill it with SIGKILL + process.kill()?; + } + + return Err(PopenError::LogicError("Timeout reached")); + } + + // get the collected stdout/stderr + let (out, err) = process.communicate(None)?; + + if let Some(err_str) = err { + if !err_str.is_empty() { + tracing::warn!("Error output size: {}", err_str.len()); + } + } + + return Ok(out); +} + +// the default X display to use if not configured or when X forwarding is used +fn default_display() -> String { + String::from(":0") +} + +// helper function to get the X display name, if not set it returns the default display +fn display() -> String { + let display = env::var("DISPLAY"); + + match display { + Ok(display) => { + // use the $DISPLAY value only when it is a local X server + if display.starts_with(":") { + display + } else { + // when using SSH X forwarding (e.g. "localhost:10.0") we could + // accidentally change the configuration of the remote X server, + // in that case try using the local X server if it is available + default_display() + } + } + Err(_) => default_display(), + } +} + impl L10n { pub fn new_with_locale(ui_locale: &LocaleId) -> Result { const DEFAULT_TIMEZONE: &str = "Europe/Berlin"; @@ -112,11 +188,15 @@ impl L10n { .args(["set-x11-keymap", &keymap]) .output() .map_err(LocaleError::Commit)?; - Command::new("/usr/bin/setxkbmap") - .arg(keymap) - .env("DISPLAY", ":0") - .output() - .map_err(LocaleError::Commit)?; + + let output = run_with_timeout( + &["setxkbmap", "-display", &display(), &keymap], + SETXKBMAP_TIMEOUT, + ); + output.map_err(|e| { + LocaleError::Commit(io::Error::new(io::ErrorKind::Other, e.to_string())) + })?; + Ok(()) } @@ -141,12 +221,12 @@ impl L10n { } fn x11_keymap() -> Result { - let output = Command::new("setxkbmap") - .arg("-query") - .env("DISPLAY", ":0") - .output()?; - let output = String::from_utf8(output.stdout) - .map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?; + let output = run_with_timeout( + &["setxkbmap", "-query", "-display", &display()], + SETXKBMAP_TIMEOUT, + ); + let output = output.map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?; + let output = output.unwrap_or(String::new()); let keymap_regexp = Regex::new(r"(?m)^layout: (.+)$").unwrap(); let captures = keymap_regexp.captures(&output); diff --git a/rust/package/agama.changes b/rust/package/agama.changes index 1da2d2f86c..0c5cd8d029 100644 --- a/rust/package/agama.changes +++ b/rust/package/agama.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Thu May 23 15:47:28 UTC 2024 - Ladislav Slezák + +- Avoid deadlock when "setxkbmap" call gets stucked, use a timeout + (gh#openSUSE/agama#1249) + ------------------------------------------------------------------- Wed May 22 12:31:25 UTC 2024 - Josef Reidinger