Use a timeout when calling setxkbmap (#1249)

## Problem

- 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 stuck.
- Additionally the $DISPLAY might be already set so we should honor that
value (e.g. when you manually start the web server from terminal)

## Solution

- Use the [subprocess](https://crates.io/crates/subprocess) crate to
limit the execution time
- Use the current `DISPLAY` value when already set

### SSH X Forwarding Problem

We need to set the display for the `setxkbmap` call when running on the
Live ISO because when running Agama from systemd service the `DISPLAY`
is not set but we need to access the locally running web browser so we
have to explicitly set the `:0` value.

When the web server is started manually from a terminal we should keep
the current value if it is already set. But using the current `DISPLAY`
value might behave a bit unexpectedly when you login to the system via
SSH with X forwarding enabled. In that case the `setxkbmap` call will
query and set (!) keyboard of the remote X server. That might be quite
confusing.

Sure, that is a corner case, but if you select some exotic layout with
complete different mapping like Dvorak then it might not be trivial to
restore your original layout. When using X forwarding the `DISPLAY` is
set to something like `localhost:10.0`, so we should use it only
when it starts with `:` character which means the local X server.

## Testing

- Tested manually

---------

Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>

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>