From 68bed18a8ae0e82bd3487767bc4ffc304dd4cdf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Wed, 22 May 2024 10:35:43 +0200 Subject: [PATCH 1/5] Use a timeout when calling `setxkbmap` When calling `setxkbmap` in development the `root` user might not have access to the `:0` X display. Maybe there is XDM running or another user is logged in. In that case `setxkbmap` prints Authorization required, but no authorization protocol specified error message in an infinite loop and gets stucked. Additionally the $DISPLAY might be already set so we should honor that value. --- rust/Cargo.lock | 11 ++++ rust/agama-server/Cargo.toml | 1 + rust/agama-server/src/l10n/l10n.rs | 84 ++++++++++++++++++++++++++---- 3 files changed, 85 insertions(+), 11 deletions(-) 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..e8394ec510 100644 --- a/rust/agama-server/src/l10n/l10n.rs +++ b/rust/agama-server/src/l10n/l10n.rs @@ -1,9 +1,13 @@ +use std::env; +use std::ffi::OsStr; 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 +25,60 @@ 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: &[impl AsRef], 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!"); + // 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.len() > 0 { + tracing::warn!("Error output size: {}", err_str.len()); + } + } + + return Ok(out); +} + +// helper function to get the X display name, if not set it returns the ":0" +// default value +fn display() -> String { + env::var("DISPLAY").unwrap_or(String::from(":0")) +} + impl L10n { pub fn new_with_locale(ui_locale: &LocaleId) -> Result { const DEFAULT_TIMEZONE: &str = "Europe/Berlin"; @@ -112,11 +170,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 +203,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); From 8901741397ff4ddd91284ec546946b221f2fa4f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Thu, 23 May 2024 16:49:13 +0200 Subject: [PATCH 2/5] Handle X forwarding, log the command --- rust/agama-server/src/l10n/l10n.rs | 46 ++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/rust/agama-server/src/l10n/l10n.rs b/rust/agama-server/src/l10n/l10n.rs index e8394ec510..9424e05179 100644 --- a/rust/agama-server/src/l10n/l10n.rs +++ b/rust/agama-server/src/l10n/l10n.rs @@ -1,5 +1,4 @@ use std::env; -use std::ffi::OsStr; use std::io; use std::process::Command; use std::time::Duration; @@ -31,7 +30,7 @@ const SETXKBMAP_TIMEOUT: u64 = 3; // helper function which runs a command with timeout and collects it's standard // output -fn run_with_timeout(cmd: &[impl AsRef], timeout: u64) -> Result, PopenError> { +fn run_with_timeout(cmd: &[&String], timeout: u64) -> Result, PopenError> { // start the subprocess let mut process = Popen::create( cmd, @@ -47,7 +46,7 @@ fn run_with_timeout(cmd: &[impl AsRef], timeout: u64) -> Result], timeout: u64) -> Result 0 { + if !err_str.is_empty() { tracing::warn!("Error output size: {}", err_str.len()); } } @@ -73,10 +72,29 @@ fn run_with_timeout(cmd: &[impl AsRef], timeout: u64) -> Result String { + String::from(":0") +} + +// helper function to get the X display name, if not set it returns the default display fn display() -> String { - env::var("DISPLAY").unwrap_or(String::from(":0")) + 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 { @@ -172,7 +190,12 @@ impl L10n { .map_err(LocaleError::Commit)?; let output = run_with_timeout( - &["setxkbmap", "-display", &display(), &keymap], + &[ + &"setxkbmap".to_string(), + &"-display".to_string(), + &display(), + &keymap, + ], SETXKBMAP_TIMEOUT, ); output.map_err(|e| { @@ -204,7 +227,12 @@ impl L10n { fn x11_keymap() -> Result { let output = run_with_timeout( - &["setxkbmap", "-query", "-display", &display()], + &[ + &"setxkbmap".to_string(), + &"-query".to_string(), + &"-display".to_string(), + &display(), + ], SETXKBMAP_TIMEOUT, ); let output = output.map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?; From 504d961b821b86975c21c0418057586e66191447 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Thu, 23 May 2024 16:54:10 +0200 Subject: [PATCH 3/5] Format fix --- rust/agama-server/src/l10n/l10n.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/agama-server/src/l10n/l10n.rs b/rust/agama-server/src/l10n/l10n.rs index 9424e05179..fd5cfeb9f0 100644 --- a/rust/agama-server/src/l10n/l10n.rs +++ b/rust/agama-server/src/l10n/l10n.rs @@ -93,7 +93,7 @@ fn display() -> String { default_display() } } - Err(_) => default_display() + Err(_) => default_display(), } } From e79d318852971bc1f0ed8447183f4f269de83904 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Thu, 23 May 2024 16:19:06 +0100 Subject: [PATCH 4/5] Simplify run_with_timeout signature --- rust/agama-server/src/l10n/l10n.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/rust/agama-server/src/l10n/l10n.rs b/rust/agama-server/src/l10n/l10n.rs index fd5cfeb9f0..e81d805426 100644 --- a/rust/agama-server/src/l10n/l10n.rs +++ b/rust/agama-server/src/l10n/l10n.rs @@ -30,7 +30,7 @@ const SETXKBMAP_TIMEOUT: u64 = 3; // helper function which runs a command with timeout and collects it's standard // output -fn run_with_timeout(cmd: &[&String], timeout: u64) -> Result, PopenError> { +fn run_with_timeout(cmd: &[&str], timeout: u64) -> Result, PopenError> { // start the subprocess let mut process = Popen::create( cmd, @@ -190,12 +190,7 @@ impl L10n { .map_err(LocaleError::Commit)?; let output = run_with_timeout( - &[ - &"setxkbmap".to_string(), - &"-display".to_string(), - &display(), - &keymap, - ], + &["setxkbmap", "-display", &display(), &keymap], SETXKBMAP_TIMEOUT, ); output.map_err(|e| { @@ -227,12 +222,7 @@ impl L10n { fn x11_keymap() -> Result { let output = run_with_timeout( - &[ - &"setxkbmap".to_string(), - &"-query".to_string(), - &"-display".to_string(), - &display(), - ], + &["setxkbmap", "-query", "-display", &display()], SETXKBMAP_TIMEOUT, ); let output = output.map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?; From a2cd2b0d1fa67e86f304e8ceadfcc77e06943b9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Thu, 23 May 2024 17:49:24 +0200 Subject: [PATCH 5/5] Changes --- rust/package/agama.changes | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/rust/package/agama.changes b/rust/package/agama.changes index 19f9f29e81..8bda24edf4 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) + ------------------------------------------------------------------- Fri May 17 09:52:25 UTC 2024 - Imobach Gonzalez Sosa