Fixed mirrord-intproxy lingering when executing a non-existent bina…

…ry (#2870)

* added kill_on_drop to intproxy command

* Changelog

* Update mirrord/cli/src/execution.rs

Co-authored-by: Gemma <[email protected]>

---------

Co-authored-by: Gemma <[email protected]>

changelog.d/2869.fixed.md

@@ -0,0 +1 @@
+`mirrord-intproxy` no longer lingers forever when the user tries to execute a non-existent binary.

mirrord/cli/src/container.rs

@@ -384,8 +384,5 @@ pub(crate) async fn container_command(
 
     analytics.set_error(AnalyticsError::BinaryExecuteFailed);
 
-    // Kills the intproxy, freeing the agent.
-    execution_info.stop().await;
-
     Ok(())
 }

mirrord/cli/src/execution.rs

@@ -52,8 +52,11 @@ pub(crate) const INJECTION_ENV_VAR: &str = LINUX_INJECTION_ENV_VAR;
 #[cfg(target_os = "macos")]
 pub(crate) const INJECTION_ENV_VAR: &str = "DYLD_INSERT_LIBRARIES";
 
-/// Struct for holding the execution information
-/// What agent to connect to, what environment variables to set
+/// Struct for holding the execution information.
+///
+/// 1. Environment to set in the user process,
+/// 2. Environment to unset in the user process,
+/// 3. Path to the patched binary to `exec` into (SIP, only on macOS).
 #[derive(Debug, Serialize)]
 pub(crate) struct MirrordExecution {
     pub environment: HashMap<String, String>,
@@ -146,8 +149,19 @@ where
 }
 
 impl MirrordExecution {
-    /// Starts the internal proxy (`intproxy`), and mirrord-layer, even if a bogus binary
-    /// was passed by the user.
+    /// Makes agent connection and starts the internal proxy child process.
+    ///
+    /// # Internal proxy
+    ///
+    /// The internal proxy will be killed as soon as this struct is dropped.
+    /// It **does not** happen when you `exec` into user binary, because Rust destructors are not
+    /// run. The whole process is instantly replaced by the OS.
+    ///
+    /// Therefore, everything should work fine when you create [`MirrordExecution`] with this
+    /// function and then either:
+    /// 1. Drop this struct when an error occurs,
+    /// 2. Successfully `exec`,
+    /// 3. Wait for intproxy exit with [`MirrordExecution::wait`].
     #[tracing::instrument(level = Level::TRACE, skip_all)]
     pub(crate) async fn start<P>(
         config: &LayerConfig,
@@ -219,7 +233,8 @@ impl MirrordExecution {
             .arg("intproxy")
             .stdout(std::process::Stdio::piped())
             .stderr(std::process::Stdio::piped())
-            .stdin(std::process::Stdio::null());
+            .stdin(std::process::Stdio::null())
+            .kill_on_drop(true);
 
         proxy_command.env(
             AGENT_CONNECT_INFO_ENV_KEY,
@@ -537,13 +552,4 @@ impl MirrordExecution {
 
         Ok(())
     }
-
-    /// Kills the child process, stopping the internal proxy, and completing the agent.
-    ///
-    /// Used when mirrord execution fails inside `execvp`.
-    pub async fn stop(self) {
-        let Self { mut child, .. } = self;
-
-        let _ = child.start_kill();
-    }
 }

mirrord/cli/src/main.rs

@@ -151,9 +151,6 @@ where
     error!("Couldn't execute {:?}", errno);
     analytics.set_error(AnalyticsError::BinaryExecuteFailed);
 
-    // Kills the intproxy, freeing the agent.
-    execution_info.stop().await;
-
     #[cfg(all(target_os = "macos", target_arch = "aarch64"))]
     if errno == Errno::from_raw(86) {
         // "Bad CPU type in executable"