Gracefully stop OpenVPN subprocesses by writing to stdin

8 files changed, 129 insertions, 116 deletions

diff --git a/Cargo.lock b/Cargo.lock
index 4f0c148946..de9aa030a5 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1392,6 +1392,7 @@ dependencies = [
  "log 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "notify 4.0.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "openvpn-plugin 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "os_pipe 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "pfctl 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "resolv-conf 0.6.0 (git+https://github.com/tailhook/resolv-conf.git)",
  "shell-escape 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)",
diff --git a/talpid-core/Cargo.toml b/talpid-core/Cargo.toml
index 864368e31f..46c82f15f6 100644
--- a/talpid-core/Cargo.toml
+++ b/talpid-core/Cargo.toml
@@ -13,6 +13,7 @@ jsonrpc-core = { git = "https://github.com/paritytech/jsonrpc", tag = "v8.0.1" }
 jsonrpc-macros = { git = "https://github.com/paritytech/jsonrpc", tag = "v8.0.1" }
 libc = "0.2.20"
 log = "0.4"
+os_pipe = "0.6"
 uuid = { version = "0.6", features = ["v4"] }
 shell-escape = "0.1"
 
diff --git a/talpid-core/src/process/mod.rs b/talpid-core/src/process/mod.rs
index 29fc633a49..1613389e4f 100644
--- a/talpid-core/src/process/mod.rs
+++ b/talpid-core/src/process/mod.rs
@@ -1,9 +1,5 @@
 /// A module for all OpenVPN related process management.
 pub mod openvpn;
-/// A module for OpenVPN process
-pub mod proc_handle;
-
-/// Unix specific process management features.
-#[cfg(unix)]
-pub mod unix;
 
+/// A trait for stopping subprocesses gracefully.
+pub mod stoppable_process;
diff --git a/talpid-core/src/process/openvpn.rs b/talpid-core/src/process/openvpn.rs
index bf7d1ef477..41b6f25244 100644
--- a/talpid-core/src/process/openvpn.rs
+++ b/talpid-core/src/process/openvpn.rs
@@ -1,11 +1,16 @@
-use atty;
 use duct;
+extern crate os_pipe;
 
 use std::ffi::{OsStr, OsString};
 use std::fmt;
 use std::path::{Path, PathBuf};
+use super::stoppable_process::StoppableProcess;
 
+use self::os_pipe::{pipe, PipeWriter};
+use atty;
 use shell_escape;
+use std::io;
+use std::sync::{Arc, Mutex};
 use talpid_types::net;
 
 static BASE_ARGUMENTS: &[&[&str]] = &[
@@ -107,23 +112,7 @@ impl OpenVpnCommand {
     /// Build a runnable expression from the current state of the command.
     pub fn build(&self) -> duct::Expression {
         debug!("Building expression: {}", &self);
-
-        let mut cmd = duct::cmd(&self.openvpn_bin, self.get_arguments()).unchecked();
-
-        // Prevent forwarding the stdio when it's not available.
-        if !atty::is(atty::Stream::Stdin) {
-            cmd = cmd.stdin_null();
-        }
-
-        if !atty::is(atty::Stream::Stdout) {
-            cmd = cmd.stdout_null();
-        }
-
-        if !atty::is(atty::Stream::Stderr) {
-            cmd = cmd.stderr_null();
-        }
-
-        cmd
+        duct::cmd(&self.openvpn_bin, self.get_arguments()).unchecked()
     }
 
     /// Sets extra options
@@ -229,6 +218,62 @@ impl fmt::Display for OpenVpnCommand {
     }
 }
 
+/// Proc handle for an openvpn process
+pub struct OpenVpnProcHandle {
+    /// Duct handle
+    pub inner: duct::Handle,
+    /// Standard input handle
+    pub stdin: Arc<Mutex<PipeWriter>>,
+}
+
+/// Impl for proc handle
+impl OpenVpnProcHandle {
+    const KILL_BYTE: u8 = b'c';
+    /// Constructor for a new openvpn proc handle
+    pub fn new(mut cmd: duct::Expression) -> io::Result<Self> {
+        if atty::is(atty::Stream::Stdout) {
+            cmd = cmd.stdout_null();
+        }
+
+        if atty::is(atty::Stream::Stderr) {
+            cmd = cmd.stderr_null();
+        }
+
+        let (reader, writer) = pipe()?;
+        let proc_handle = cmd.stdin_handle(reader).start()?;
+
+        Ok(Self {
+            inner: proc_handle,
+            stdin: Arc::new(Mutex::new(writer)),
+        })
+    }
+
+}
+
+impl StoppableProcess for OpenVpnProcHandle {
+    /// Writes a byte to the standard input of the OpenVPN process - Mullvad's OpenVPN is modified
+    /// to exit when it reads a particular byte from STDIN.
+    fn stop(&self) -> io::Result<()> {
+        use std::io::Write;
+        let mut writer = self.stdin.lock().unwrap();
+        writer.write_all(&[Self::KILL_BYTE])
+    }
+
+    fn kill(&self) -> io::Result<()> {
+        self.inner.kill()
+    }
+
+    fn has_stopped(&self) -> io::Result<bool> {
+        match self.inner.try_wait() {
+            Ok(None) => Ok(false),
+            Ok(Some(_)) => Ok(true),
+            Err(e) => Err(e),
+        }
+    }
+}
+
+
+
 
 #[cfg(test)]
 mod tests {
diff --git a/talpid-core/src/process/proc_handle.rs b/talpid-core/src/process/proc_handle.rs
deleted file mode 100644
index 0893b85435..0000000000
--- a/talpid-core/src/process/proc_handle.rs
+++ /dev/null
@@ -1,40 +0,0 @@
-/// Some docs
-extern crate duct;
-use std::io;
-
-/// Proc handle for an openvpn process
-pub struct OpenVpnProcHandle {
-    /// Duct handle
-    pub inner: duct::Handle,
-}
-
-/// Impl for proc handle
-impl OpenVpnProcHandle {
-    /// Constructor for a new openvpn proc handle
-    pub fn new(expr: duct::Expression) -> io::Result<Self> {
-        Ok(Self {
-            inner: expr.start()?,
-        })
-    }
-
-    /// Sends a SIGTERM signal to the OpenVPN process. Does not block, no guarantee that the
-    /// process will have exited after this function returns.
-    #[cfg(unix)]
-    pub fn try_stop(&self) -> io::Result<()> {
-        use duct::unix::HandleExt;
-        extern crate libc;
-        self.inner.send_signal(libc::SIGTERM)
-    }
-
-    /// Tries to shut down the OpenVPN process in a nondestructive manner. Does not block, no
-    /// guarantee that the process will have exited after this function returns.
-    #[cfg(windows)]
-    pub fn try_stop(&self) -> io::Result<()> {
-        Ok(())
-    }
-
-    /// Brutally kills the underlinyg process
-    pub fn kill_process(&self) -> io::Result<()> {
-        self.inner.kill()
-    }
-}
diff --git a/talpid-core/src/process/stoppable_process.rs b/talpid-core/src/process/stoppable_process.rs
new file mode 100644
index 0000000000..c2d8304d85
--- /dev/null
+++ b/talpid-core/src/process/stoppable_process.rs
@@ -0,0 +1,60 @@
+extern crate libc;
+
+use std::error::Error;
+use std::io;
+use std::thread;
+use std::time::{Duration, Instant};
+
+static POLL_INTERVAL_MS: Duration = Duration::from_millis(50);
+
+/// A best effort attempt at stopping a subprocess whilst also ensuring that the subprocess is
+/// killed eventually.
+pub trait StoppableProcess
+where
+    Self: Sized,
+{
+    /// Gracefully stops a process.
+    fn stop(&self) -> io::Result<()>;
+
+    /// Kills a process unconditionally. Implementations should strive to never fail.
+    fn kill(&self) -> io::Result<()>;
+
+    /// Check if process is stopped. This method must not block.
+    fn has_stopped(&self) -> io::Result<bool>;
+
+    /// Attempts to stop a process gracefully in the given time period, otherwise kills the
+    /// process.
+    fn nice_kill(&self, timeout: Duration) -> io::Result<()> {
+        trace!("Trying to stop child process gracefully");
+        if let Err(e) = self.stop() {
+            error!(
+                "Failed to stop the child process gracefully: {}",
+                e.description()
+            );
+            return self.kill();
+        };
+
+        if wait_timeout(self, timeout)? {
+            debug!("Child process terminated gracefully");
+            Ok(())
+        } else {
+            warn!("Child process did not terminate gracefully within timeout, forcing termination");
+            self.kill()
+        }
+    }
+}
+/// Wait for a process to die for a maximum of `timeout`. Returns true if the process died within
+/// the timeout.
+fn wait_timeout<T>(process: &T, timeout: Duration) -> io::Result<bool>
+where
+    T: StoppableProcess + Sized,
+{
+    let timer = Instant::now();
+    while timer.elapsed() < timeout {
+        if process.has_stopped()? {
+            return Ok(true);
+        }
+        thread::sleep(POLL_INTERVAL_MS);
+    }
+    Ok(false)
+}
diff --git a/talpid-core/src/process/unix.rs b/talpid-core/src/process/unix.rs
deleted file mode 100644
index 71e96c1246..0000000000
--- a/talpid-core/src/process/unix.rs
+++ /dev/null
@@ -1,46 +0,0 @@
-extern crate libc;
-
-use process::proc_handle::OpenVpnProcHandle;
-
-use duct;
-use std::io;
-use std::thread;
-use std::time::{Duration, Instant};
-
-static POLL_INTERVAL_MS: u64 = 50;
-
-/// Extra methods for terminating `OpenVpnProcHandle` instances.
-pub trait HandleKillExt {
-    /// Kills a process by first sending it the `SIGTERM` signal and then wait up to `timeout`.
-    /// If the process has not died after the timeout has expired it is killed.
-    fn nice_kill(&self, timeout: Duration) -> io::Result<()>;
-}
-
-impl HandleKillExt for OpenVpnProcHandle {
-    fn nice_kill(&self, timeout: Duration) -> io::Result<()> {
-        trace!("Sending SIGTERM to child process");
-        self.try_stop()?;
-
-        if wait_timeout(&self.inner, timeout)? {
-            debug!("Child process exited from SIGTERM");
-            Ok(())
-        } else {
-            warn!("Child process did not exit from SIGTERM, sending SIGKILL");
-            self.kill_process()
-        }
-    }
-}
-
-/// Wait for a process to die for a maximum of `timeout`. Returns true if the process died within
-/// the timeout.
-fn wait_timeout(handle: &duct::Handle, timeout: Duration) -> io::Result<bool> {
-    let timer = Instant::now();
-    while timer.elapsed() < timeout {
-        match handle.try_wait() {
-            Ok(None) => thread::sleep(Duration::from_millis(POLL_INTERVAL_MS)),
-            Ok(Some(_)) => return Ok(true),
-            Err(e) => return Err(e),
-        }
-    }
-    Ok(false)
-}
diff --git a/talpid-core/src/tunnel/openvpn.rs b/talpid-core/src/tunnel/openvpn.rs
index a272164d65..0380f29bce 100644
--- a/talpid-core/src/tunnel/openvpn.rs
+++ b/talpid-core/src/tunnel/openvpn.rs
@@ -1,7 +1,6 @@
-use duct;
 use openvpn_plugin::types::OpenVpnPluginEvent;
-use process::openvpn::OpenVpnCommand;
-use process::proc_handle::OpenVpnProcHandle;
+use process::openvpn::{OpenVpnCommand, OpenVpnProcHandle};
+use process::stoppable_process::StoppableProcess;
 
 use std::collections::HashMap;
 use std::io;
@@ -33,10 +32,8 @@ mod errors {
 pub use self::errors::*;
 
 
-#[cfg(unix)]
 static OPENVPN_DIE_TIMEOUT: Duration = Duration::from_secs(2);
 
-
 /// Struct for monitoring an OpenVPN process.
 #[derive(Debug)]
 pub struct OpenVpnMonitor<C: OpenVpnBuilder = OpenVpnCommand> {
@@ -213,7 +210,6 @@ impl ProcessHandle for OpenVpnProcHandle {
     }
 
     fn kill(&self) -> io::Result<()> {
-        use process::unix::HandleKillExt;
         self.nice_kill(OPENVPN_DIE_TIMEOUT)
     }
 }