Merge pull request #1383 from Nael-Nathanael/fix/control-port-1373

Fix #1373: --control without a valid port stops the desktop before it starts

apps/vectorcraft/src/automation_args.rs

@@ -1,6 +1,8 @@
-//! Parse automation roots before starting any desktop services. A malformed flag must not
-//! silently remove a root and leave the control channel unconfined.
+//! Parse the control port and automation roots before starting any desktop services. A malformed
+//! flag must not silently remove a root and leave the control channel unconfined, or leave the app
+//! running without the control channel that was asked for.
 
+use std::ffi::OsString;
 use std::path::PathBuf;
 
 pub struct AutomationArgs {
@@ -46,6 +48,50 @@ impl AutomationArgs {
     }
 }
 
+/// `--control <port>` or `--control=<port>`, given at most once.
+#[derive(Default)]
+pub struct ControlArg {
+    pub port: Option<u16>,
+}
+
+impl ControlArg {
+    /// Returns whether this argument is `--control`. The next argument is its port whatever it
+    /// is, so `--control --automation-read-root` reports the flag rather than reading it as a root.
+    pub fn consume(&mut self, arg: &str, args: &mut impl Iterator<Item = String>) -> Result<bool, String> {
+        let value = match arg.strip_prefix("--control") {
+            Some("") => args.next(),
+            Some(rest) => match rest.strip_prefix('=') {
+                Some(value) => Some(value.to_owned()),
+                None => return Ok(false),
+            },
+            None => return Ok(false),
+        };
+        if self.port.is_some() {
+            return Err("--control given twice".into());
+        }
+        self.port = Some(port("--control", value.as_deref())?);
+        Ok(true)
+    }
+}
+
+/// `VECTORCRAFT_CONTROL_PORT`, for when `--control` isn't given: unset or empty is no control
+/// channel, anything else must be a port.
+pub fn env_control_port(value: Option<OsString>) -> Result<Option<u16>, String> {
+    match value {
+        Some(v) if !v.is_empty() => port("VECTORCRAFT_CONTROL_PORT", Some(&v.to_string_lossy())).map(Some),
+        _ => Ok(None),
+    }
+}
+
+/// A port from 1 to 65535 (the server would listen on 0's random port without saying which).
+fn port(name: &str, value: Option<&str>) -> Result<u16, String> {
+    let need = format!("{name} needs a port number from 1 to 65535");
+    match value {
+        None => Err(need),
+        Some(v) => v.parse().ok().filter(|&p| p != 0).ok_or(format!("{need}, not `{v}`")),
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -87,4 +133,53 @@ mod tests {
         assert_eq!(roots.read_root, Some("-folder".into()));
         assert_eq!(roots.write_root, Some("./-folder".into()));
     }
+
+    fn control(args: &[&str]) -> Result<Option<u16>, String> {
+        let mut control = ControlArg::default();
+        let mut args = args.iter().map(|a| a.to_string());
+        while let Some(a) = args.next() {
+            assert!(control.consume(&a, &mut args)?, "{a}");
+        }
+        Ok(control.port)
+    }
+
+    #[test]
+    fn control_needs_a_port_from_1_to_65535() {
+        let need = "--control needs a port number from 1 to 65535";
+        assert_eq!(control(&["--control"]), Err(need.into()));
+        assert_eq!(control(&["--control", "--automation-read-root"]), Err(format!("{need}, not `--automation-read-root`")));
+        for bad in ["abc", "0", "70000", "-1", ""] {
+            assert_eq!(control(&["--control", bad]), Err(format!("{need}, not `{bad}`")));
+            assert_eq!(control(&[&format!("--control={bad}")]), Err(format!("{need}, not `{bad}`")));
+        }
+        assert_eq!(control(&["--control", "7979"]), Ok(Some(7979)));
+        assert_eq!(control(&["--control=65535"]), Ok(Some(65535)));
+        assert_eq!(control(&["--control", "7979", "--control=7980"]), Err("--control given twice".into()));
+    }
+
+    #[test]
+    fn other_arguments_are_not_control() {
+        let mut control = ControlArg::default();
+        let mut args = ["7979".to_owned()].into_iter();
+        for other in ["--controls", "--control-port", "drawing.svg", "--in-window-menus"] {
+            assert!(!control.consume(other, &mut args).unwrap(), "{other}");
+        }
+        assert_eq!(args.next().as_deref(), Some("7979"));
+        assert_eq!(control.port, None);
+    }
+
+    #[test]
+    fn control_port_variable_is_a_port_or_unset() {
+        assert_eq!(env_control_port(None), Ok(None));
+        assert_eq!(env_control_port(Some("".into())), Ok(None));
+        assert_eq!(env_control_port(Some("7979".into())), Ok(Some(7979)));
+        let need = "VECTORCRAFT_CONTROL_PORT needs a port number from 1 to 65535";
+        assert_eq!(env_control_port(Some("abc".into())), Err(format!("{need}, not `abc`")));
+        assert_eq!(env_control_port(Some("0".into())), Err(format!("{need}, not `0`")));
+        #[cfg(unix)]
+        {
+            use std::os::unix::ffi::OsStringExt;
+            assert_eq!(env_control_port(Some(OsString::from_vec(vec![0x39, 0xff]))), Err(format!("{need}, not `9\u{fffd}`")));
+        }
+    }
 }

apps/vectorcraft/src/main.rs

@@ -6,7 +6,8 @@
 //! `--in-window-menus` (or `VECTORCRAFT_IN_WINDOW_MENUS=1`) keeps the menus inside the window on
 //! macOS instead of the macOS menu bar (`mac_menu`).
 //!
-//! `--control <port>` (or `VECTORCRAFT_CONTROL_PORT`) starts a localhost JSON-lines control server:
+//! `--control <port>` (or `VECTORCRAFT_CONTROL_PORT`; a port from 1 to 65535, or the app doesn't
+//! start) starts a localhost JSON-lines control server:
 //! `{"id":1,"method":"ui.inspect","params":{}}` → `{"id":1,"ok":true,"result":…}`.
 //! See `vectorcraft_ui_egui::control` for the methods. `--automation-read-root` and
 //! `--automation-write-root` (or `VECTORCRAFT_AUTOMATION_READ_ROOT` / `_WRITE_ROOT`) confine the
@@ -605,7 +606,7 @@ fn native_options(custom_titlebar: bool) -> eframe::NativeOptions {
 fn main() -> std::process::ExitCode {
     // Validate roots before logging, font scans, preferences or the window: a missing value
     // must not clear an environment root and start the control channel without confinement.
-    let mut control_port: Option<u16> = std::env::var("VECTORCRAFT_CONTROL_PORT").ok().and_then(|p| p.parse().ok());
+    let mut control = automation_args::ControlArg::default();
     let root_env = |name: &str| std::env::var_os(name).filter(|v| !v.is_empty()).map(std::path::PathBuf::from);
     let mut roots_args =
         automation_args::AutomationArgs::new(root_env("VECTORCRAFT_AUTOMATION_READ_ROOT"), root_env("VECTORCRAFT_AUTOMATION_WRITE_ROOT"));
@@ -613,7 +614,7 @@ fn main() -> std::process::ExitCode {
     let mut in_window_menus = std::env::var_os("VECTORCRAFT_IN_WINDOW_MENUS").is_some_and(|v| !v.is_empty() && v != "0");
     let mut args = std::env::args().skip(1);
     while let Some(a) = args.next() {
-        match roots_args.consume(&a, &mut args) {
+        match roots_args.consume(&a, &mut args).and_then(|root| if root { Ok(true) } else { control.consume(&a, &mut args) }) {
             Ok(true) => continue,
             Ok(false) => {}
             Err(e) => {
@@ -622,7 +623,6 @@ fn main() -> std::process::ExitCode {
             }
         }
         match a.as_str() {
-            "--control" => control_port = args.next().and_then(|p| p.parse().ok()),
             "--in-window-menus" => in_window_menus = true,
             "--version" => {
                 println!("vectorcraft {}", env!("CARGO_PKG_VERSION"));
@@ -632,6 +632,17 @@ fn main() -> std::process::ExitCode {
         }
     }
     let automation_args::AutomationArgs { read_root, write_root, .. } = roots_args;
+    // After the arguments: `--control` and `--version` win over a bad environment value.
+    let control_port = match control.port {
+        Some(port) => Some(port),
+        None => match automation_args::env_control_port(std::env::var_os("VECTORCRAFT_CONTROL_PORT")) {
+            Ok(port) => port,
+            Err(e) => {
+                eprintln!("vectorcraft: {e}");
+                return std::process::ExitCode::FAILURE;
+            }
+        },
+    };
     // Before desktop services, so every start-up warning is recorded (`logging`).
     let logger = logging::install();
     vectorcraft_ui_egui::i18n::detect_system_lang_in_background();

apps/vectorcraft/tests/automation_args.rs

@@ -9,12 +9,17 @@ const BIN: &str = env!("CARGO_BIN_EXE_vectorcraft");
 /// Invalid roots must exit before desktop initialization on every platform, including a CI
 /// machine with no display. A regression must fail rather than hang with a window open.
 fn rejects(args: &[&str], message: &str) {
+    rejects_with_env(args, &[], message);
+}
+
+fn rejects_with_env(args: &[&str], env: &[(&str, &str)], message: &str) {
     let mut child = Command::new(BIN)
         .args(args)
         .env_remove("VECTORCRAFT_CONTROL_PORT")
         .env_remove("VECTORCRAFT_AUTOMATION_READ_ROOT")
         .env_remove("VECTORCRAFT_AUTOMATION_WRITE_ROOT")
         .env("VECTORCRAFT_NO_PREFS", "1")
+        .envs(env.iter().copied())
         .stdin(Stdio::null())
         .stdout(Stdio::piped())
         .stderr(Stdio::piped())
@@ -68,3 +73,24 @@ fn explicit_dash_folder_does_not_consume_version() {
     assert_eq!(String::from_utf8(out.stdout).unwrap(), format!("vectorcraft {}\n", env!("CARGO_PKG_VERSION")));
     assert!(out.stderr.is_empty());
 }
+
+/// `--control` without a port no longer starts the app without a control server, and the
+/// argument after it isn't reinterpreted (#1373).
+#[test]
+fn control_without_a_valid_port_fails_before_startup() {
+    let need = "--control needs a port number from 1 to 65535";
+    rejects(&["--control", "--automation-read-root", ".", "--automation-write-root", "."], &format!("{need}, not `--automation-read-root`"));
+    rejects(&["--control"], need);
+    rejects(&["--control", "abc"], &format!("{need}, not `abc`"));
+    rejects(&["--control=0"], &format!("{need}, not `0`"));
+    rejects(&["--control="], &format!("{need}, not ``"));
+    rejects(&["--control", "7979", "--control", "7980"], "--control given twice");
+    rejects_with_env(&[], &[("VECTORCRAFT_CONTROL_PORT", "abc")], "VECTORCRAFT_CONTROL_PORT needs a port number from 1 to 65535, not `abc`");
+}
+
+#[test]
+fn version_wins_over_a_bad_control_port_variable() {
+    let out = Command::new(BIN).arg("--version").env("VECTORCRAFT_CONTROL_PORT", "abc").output().unwrap();
+    assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr));
+    assert_eq!(String::from_utf8(out.stdout).unwrap(), format!("vectorcraft {}\n", env!("CARGO_PKG_VERSION")));
+}

contributors/people.toml

@@ -86,3 +86,6 @@ sync_github_name = false
 real_name = "Aaron Guo"
 display_name = "Aaron Guo"
 sync_github_name = false
+
+[people.Nael-Nathanael]
+sync_github_name = true

docs/control-protocol.md

@@ -1,6 +1,8 @@
 # Control protocol
 
-`vectorcraft --control <port>` listens on `127.0.0.1:<port>` (loopback only). One JSON request per line:
+`vectorcraft --control <port>` listens on `127.0.0.1:<port>` (loopback only). The port must be a number
+from 1 to 65535 (`--control=<port>` works too), given once; otherwise the app prints an error and exits
+before it starts. One JSON request per line:
 `{"id": 1, "method": "ui.inspect", "params": {}}` → `{"id": 1, "ok": true, "result": {...}}` or `{"id":1,"ok":false,"error":"..."}`.
 
 Set `VECTORCRAFT_CONFIG_DIR` to a dedicated directory for an isolated native test profile. Preferences,

docs/development.md

@@ -78,7 +78,7 @@ By default VectorCraft's own crates log at `info` and everything else at `warn`.
 
 | Variable | Effect |
 |---|---|
-| `VECTORCRAFT_CONTROL_PORT` | Same as `--control <port>` |
+| `VECTORCRAFT_CONTROL_PORT` | Same as `--control <port>`, which overrides it; a value that isn't a port from 1 to 65535 stops the app |
 | `VECTORCRAFT_IN_WINDOW_MENUS` | Any value but empty or `0`: same as `--in-window-menus` (see [macOS: the menu bar](#macos-the-menu-bar)) |
 | `VECTORCRAFT_NO_PREFS` | No preferences read or written, no default Data Recovery folder and no log file (agents' test runs) |
 | `WGPU_POWER_PREF` | Graphics adapter: `low`, `high` or `none` (see [Desktop graphics processor](#desktop-graphics-processor)) |