Use a timeout when calling setxkbmap

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"

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<Option<String>, 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<Self, Error> {
         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<String, io::Error> {
-        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);

rust/package/agama.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Thu May 23 15:47:28 UTC 2024 - Ladislav Slezák <lslezak@suse.com>
+
+- Avoid deadlock when "setxkbmap" call gets stucked, use a timeout
+  (gh#openSUSE/agama#1249)
+
 -------------------------------------------------------------------
 Wed May 22 12:31:25 UTC 2024 - Josef Reidinger <jreidinger@suse.com>