From f2769cb6ce536c6b54f32f6a2f95ecd57cc87409 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Thu, 25 Aug 2022 12:30:06 -0700 Subject: [PATCH 1/5] helix-term: send the STOP signal to all processes in the process group From kill(3p): If pid is 0, sig shall be sent to all processes (excluding an unspecified set of system processes) whose process group ID is equal to the process group ID of the sender, and for which the process has permission to send a signal. This fixes the issue of running `git commit`, attempting to suspend helix with ^Z, and then not regaining control over the terminal and having to press ^Z again. --- Cargo.lock | 15 ++++++++++++++- helix-term/Cargo.toml | 1 + helix-term/src/application.rs | 15 ++++++++++----- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a1a9eae4dd27..05abd5a74606 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -854,7 +854,7 @@ checksum = "a20cebf73229debaa82574c4fd20dcaf00fa8d4bfce823a862c4e990d7a0b5b4" dependencies = [ "gix-command", "gix-config-value", - "nix", + "nix 0.26.1", "parking_lot", "thiserror", ] @@ -1178,6 +1178,7 @@ dependencies = [ "ignore", "indoc", "log", + "nix 0.25.1", "once_cell", "pulldown-cmark", "serde", @@ -1508,6 +1509,18 @@ dependencies = [ "windows-sys 0.42.0", ] +[[package]] +name = "nix" +version = "0.25.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f346ff70e7dbfd675fe90590b92d59ef2de15a8779ae305ebcbfd3f0caf59be4" +dependencies = [ + "autocfg", + "bitflags", + "cfg-if", + "libc", +] + [[package]] name = "nix" version = "0.26.1" diff --git a/helix-term/Cargo.toml b/helix-term/Cargo.toml index bca567c28688..5c2611262208 100644 --- a/helix-term/Cargo.toml +++ b/helix-term/Cargo.toml @@ -68,6 +68,7 @@ grep-searcher = "0.1.11" [target.'cfg(not(windows))'.dependencies] # https://github.com/vorner/signal-hook/issues/100 signal-hook-tokio = { version = "0.3", features = ["futures-v0_3"] } +nix = { version = "0.25.0", features = ["signal", "process"], default-features = false } [build-dependencies] helix-loader = { version = "0.6", path = "../helix-loader" } diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index c7e939959ca4..86548a5da243 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -40,10 +40,7 @@ use anyhow::{Context, Error}; use crossterm::{event::Event as CrosstermEvent, tty::IsTty}; #[cfg(not(windows))] -use { - signal_hook::{consts::signal, low_level}, - signal_hook_tokio::Signals, -}; +use {signal_hook::consts::signal, signal_hook_tokio::Signals}; #[cfg(windows)] type Signals = futures_util::stream::Empty<()>; @@ -447,7 +444,15 @@ impl Application { match signal { signal::SIGTSTP => { self.restore_term().unwrap(); - low_level::emulate_default_handler(signal::SIGTSTP).unwrap(); + + // A pid of 0 sends the signal to the entire process group, allowing the user to + // regain control of their terminal if the editor was spawned under another process + // (e.g. when running `git commit`). + nix::sys::signal::kill( + nix::unistd::Pid::from_raw(0), + Some(nix::sys::signal::SIGSTOP), + ) + .unwrap(); } signal::SIGCONT => { self.claim_term().await.unwrap(); From 553e0eaefa856ffc4bc32e367d1354f20acc4702 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Thu, 25 Aug 2022 13:52:23 -0700 Subject: [PATCH 2/5] helix-term: use libc directly to send STOP signal --- Cargo.lock | 16 ++-------------- helix-term/Cargo.toml | 2 +- helix-term/src/application.rs | 19 +++++++++++-------- 3 files changed, 14 insertions(+), 23 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 05abd5a74606..25db822026c1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -854,7 +854,7 @@ checksum = "a20cebf73229debaa82574c4fd20dcaf00fa8d4bfce823a862c4e990d7a0b5b4" dependencies = [ "gix-command", "gix-config-value", - "nix 0.26.1", + "nix", "parking_lot", "thiserror", ] @@ -1177,8 +1177,8 @@ dependencies = [ "helix-view", "ignore", "indoc", + "libc", "log", - "nix 0.25.1", "once_cell", "pulldown-cmark", "serde", @@ -1509,18 +1509,6 @@ dependencies = [ "windows-sys 0.42.0", ] -[[package]] -name = "nix" -version = "0.25.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f346ff70e7dbfd675fe90590b92d59ef2de15a8779ae305ebcbfd3f0caf59be4" -dependencies = [ - "autocfg", - "bitflags", - "cfg-if", - "libc", -] - [[package]] name = "nix" version = "0.26.1" diff --git a/helix-term/Cargo.toml b/helix-term/Cargo.toml index 5c2611262208..279fdcf8ba4e 100644 --- a/helix-term/Cargo.toml +++ b/helix-term/Cargo.toml @@ -68,7 +68,7 @@ grep-searcher = "0.1.11" [target.'cfg(not(windows))'.dependencies] # https://github.com/vorner/signal-hook/issues/100 signal-hook-tokio = { version = "0.3", features = ["futures-v0_3"] } -nix = { version = "0.25.0", features = ["signal", "process"], default-features = false } +libc = "0.2.132" [build-dependencies] helix-loader = { version = "0.6", path = "../helix-loader" } diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 86548a5da243..30985302c934 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -445,14 +445,17 @@ impl Application { signal::SIGTSTP => { self.restore_term().unwrap(); - // A pid of 0 sends the signal to the entire process group, allowing the user to - // regain control of their terminal if the editor was spawned under another process - // (e.g. when running `git commit`). - nix::sys::signal::kill( - nix::unistd::Pid::from_raw(0), - Some(nix::sys::signal::SIGSTOP), - ) - .unwrap(); + let res = unsafe { + // A pid of 0 sends the signal to the entire process group, allowing the user to + // regain control of their terminal if the editor was spawned under another process + // (e.g. when running `git commit`). + libc::kill(0, signal::SIGSTOP) + }; + + if res != 0 { + eprintln!("{}", std::io::Error::from_raw_os_error(res)); + std::process::exit(res); + } } signal::SIGCONT => { self.claim_term().await.unwrap(); From 4a68f1ff6fcc3397b2727cecc77ce348a06fe653 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Fri, 26 Aug 2022 10:15:28 -0700 Subject: [PATCH 3/5] helix-term: document safety of libc::kill --- helix-term/src/application.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 30985302c934..c379a5a9d732 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -445,6 +445,11 @@ impl Application { signal::SIGTSTP => { self.restore_term().unwrap(); + // SAFETY: + // + // - helix must have permissions to send signals to all processes in its signal + // group, either by already having the requisite permission, or by having the + // user's UID / EUID / SUID match that of the receiving process(es). let res = unsafe { // A pid of 0 sends the signal to the entire process group, allowing the user to // regain control of their terminal if the editor was spawned under another process From 0e3b9f259a61512a1a834c61816fc89ba7e3a31e Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Fri, 26 Aug 2022 10:15:47 -0700 Subject: [PATCH 4/5] helix-term: properly handle libc::kill's failure I misread the manpage for POSIX `kill` -- it returns `-1` in the failure case, and sets `errno`, which is retrieved via `std::io::Error::last_os_error()`, has its string representation printed out, and then exits with the matching status code (or 1 if, for whatever reason, there is no matching status code). --- helix-term/src/application.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index c379a5a9d732..f63aa6d0c300 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -458,7 +458,9 @@ impl Application { }; if res != 0 { - eprintln!("{}", std::io::Error::from_raw_os_error(res)); + let err = std::io::Error::last_os_error(); + eprintln!("{}", err); + let res = err.raw_os_error().unwrap_or(1); std::process::exit(res); } } From 6d7b7137e2c955802c45dcc4d0bd3a0ff0afc5cb Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 30 Jan 2023 15:10:16 -0800 Subject: [PATCH 5/5] helix-term: expand upon why we need to SIGSTOP the entire process group Also add a link back to one of the upstream issues. --- helix-term/src/application.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index f63aa6d0c300..281c923c1f84 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -454,6 +454,13 @@ impl Application { // A pid of 0 sends the signal to the entire process group, allowing the user to // regain control of their terminal if the editor was spawned under another process // (e.g. when running `git commit`). + // + // We have to send SIGSTOP (not SIGTSTP) to the entire process group, because, + // as mentioned above, the terminal will get stuck if `helix` was spawned from + // an external process and that process waits for `helix` to complete. This may + // be an issue with signal-hook-tokio, but the author of signal-hook believes it + // could be a tokio issue instead: + // https://github.com/vorner/signal-hook/issues/132 libc::kill(0, signal::SIGSTOP) };