Increase dialog exit status parsing safety in the locker

This also stops using -1 as auth failed, and moves all failure statuses
to positive numbers.  It looks like Exiting with -1 ends up setting the
status to 255 on exit, but then in the locker, it sees this as 255 and
not -1, since things get coerced into 32-bit integers.

dialog-gtk3/src/main.rs

@@ -6,7 +6,7 @@ use gtk::{prelude::*, Button, Entry, Label, Plug, Window};
 use log::{debug, error, warn};
 use std::{io::{self, Write}, thread, time::Duration};
 
-use bscreensaver_util::{init_logging, DialogExitStatus, settings::Configuration, desktop::NewLoginCommand};
+use bscreensaver_util::{init_logging, dialog::DialogExitStatus, settings::Configuration, desktop::NewLoginCommand};
 
 const DIALOG_UPDATE_INTERVAL: Duration = Duration::from_millis(100);
 const DIALOG_TIMEOUT: Duration = Duration::from_secs(60);

locker/src/screensaver.rs

@@ -1,7 +1,6 @@
 use log::{debug, error, info, trace, warn};
 use std::{
     io::Read,
-    os::unix::process::ExitStatusExt,
     process::{Child, Command, Stdio},
     time::Instant, path::{PathBuf, Path},
 };
@@ -9,7 +8,7 @@ use xcb::{randr, x, Xid, XidNew};
 use xcb_xembed::embedder::Embedder;
 
 use bscreensaver_command::{BCommand, create_command_window};
-use bscreensaver_util::{*, settings::Configuration};
+use bscreensaver_util::{*, dialog::DialogExitStatus, settings::Configuration};
 
 use crate::{
     monitor::*,
@@ -308,17 +307,24 @@ impl<'a> Screensaver<'a> {
                     warn!("Failed to check unlock dialog's status: {}", err);
                     self.unlock_dialog = Some(unlock_dialog);
                 },
-                Ok(Some(status)) if status.success() => {
+                Ok(Some(status)) => {
-                    info!("Authentication succeeded");
+                    let ok = match DialogExitStatus::try_from(status) {
-                    self.unlock_screen()?;
+                        Ok(result) => {
-                }
+                            info!("{}", result);
-                Ok(Some(status)) if status.signal().is_some() => {
+                            result == DialogExitStatus::AuthSucceeded
-                    if let Some(signum) = status.signal() {
+                        },
-                        warn!("Unlock dialog crashed with signal {}", signum);
+                        Err(error) => {
-                    }
+                            warn!("{}", error);
-                    self.hide_cursor();
+                            false
+                        },
+                    };
+
+                    if ok {
+                        self.unlock_screen()?;
+                    } else {
+                        self.hide_cursor();
+                    }
                 },
-                Ok(Some(_)) => self.hide_cursor(),  // auth failed, dialog has quit, do nothing
                 Ok(None) => self.unlock_dialog = Some(unlock_dialog),  // dialog still running
             }
         }

util/src/dialog.rs

@@ -0,0 +1,68 @@
+use std::{error, fmt, process::ExitStatus, os::unix::prelude::ExitStatusExt};
+
+#[derive(Debug, Clone, Copy)]
+pub enum DialogExitStatusError {
+    NoStatus,
+    Crashed(i32),
+    InvalidStatus(i32),
+}
+
+impl fmt::Display for DialogExitStatusError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        use DialogExitStatusError::*;
+        match self {
+            InvalidStatus(other) => write!(f, "Unlock dialog exited with unexpected status {}", other),
+            Crashed(signum) => write!(f, "Unlock dialog crashed with signal {}", signum),
+            NoStatus => write!(f, "Unlock dialog exited unexpectedly"),
+        }
+    }
+}
+
+impl error::Error for DialogExitStatusError {}
+
+#[repr(i32)]
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub enum DialogExitStatus {
+    AuthSucceeded = 0,
+    AuthFailed = 1,
+    AuthCanceled = 2,
+    AuthTimedOut = 3,
+    OtherError = 4,
+}
+
+impl DialogExitStatus {
+    pub fn exit(&self) -> ! {
+        std::process::exit(*self as i32);
+    }
+}
+
+impl fmt::Display for DialogExitStatus {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        use DialogExitStatus::*;
+        match self {
+            AuthSucceeded => write!(f, "Authentication succeeded"),
+            AuthFailed => write!(f, "Authentication failed"),
+            AuthCanceled => write!(f, "Authentication canceled"),
+            AuthTimedOut => write!(f, "Authentication timed out"),
+            OtherError => write!(f, "An unexpected error occurred with the unlock dialog"),
+        }
+    }
+}
+
+impl TryFrom<ExitStatus> for DialogExitStatus {
+    type Error = DialogExitStatusError;
+    fn try_from(value: ExitStatus) -> Result<Self, Self::Error> {
+        match value.code() {
+            Some(0) => Ok(DialogExitStatus::AuthSucceeded),
+            Some(1) => Ok(DialogExitStatus::AuthFailed),
+            Some(2) => Ok(DialogExitStatus::AuthCanceled),
+            Some(3) => Ok(DialogExitStatus::AuthTimedOut),
+            Some(4) => Ok(DialogExitStatus::OtherError),
+            Some(other) => Err(DialogExitStatusError::InvalidStatus(other)),
+            None => match value.signal() {
+                Some(signum) => Err(DialogExitStatusError::Crashed(signum)),
+                None => Err(DialogExitStatusError::NoStatus),
+            },
+        }
+    }
+}

util/src/lib.rs

@@ -2,26 +2,11 @@ use std::{ffi::CStr, io};
 use xcb::x;
 
 pub mod desktop;
+pub mod dialog;
 pub mod settings;
 
 pub const BSCREENSAVER_WM_CLASS: &[u8] = b"bscreensaver\0Bscreensaver\0";
 
-#[repr(i32)]
-#[derive(Debug, Clone, Copy, PartialEq)]
-pub enum DialogExitStatus {
-    AuthFailed = -1,
-    AuthSucceeded = 0,
-    AuthCanceled = 1,
-    AuthTimedOut = 2,
-    OtherError = 3,
-}
-
-impl DialogExitStatus {
-    pub fn exit(&self) -> ! {
-        std::process::exit(*self as i32);
-    }
-}
-
 pub fn init_logging(env_name: &str) {
     env_logger::builder()
         .format_timestamp_millis()