From 5f0f3fca76faf00892b07171ddc3ed5cdc30c78d Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 26 Sep 2023 13:03:16 -0700 Subject: [PATCH 1/5] Terminate process on timeout in windows for the coverage task --- src/agent/Cargo.lock | 1 + src/agent/coverage/Cargo.toml | 1 + src/agent/coverage/src/record.rs | 60 ++++++++++++++++++++------------ 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/agent/Cargo.lock b/src/agent/Cargo.lock index 65c2967ec0..e72bd17b8b 100644 --- a/src/agent/Cargo.lock +++ b/src/agent/Cargo.lock @@ -572,6 +572,7 @@ dependencies = [ "nix", "pete", "pretty_assertions", + "process_control", "procfs", "regex", "symbolic", diff --git a/src/agent/coverage/Cargo.toml b/src/agent/coverage/Cargo.toml index 70a55cd07f..29c756048b 100644 --- a/src/agent/coverage/Cargo.toml +++ b/src/agent/coverage/Cargo.toml @@ -20,6 +20,7 @@ symbolic = { version = "12.3", features = [ "symcache", ] } thiserror = "1.0" +process_control = "4.0" [target.'cfg(target_os = "windows")'.dependencies] debugger = { path = "../debugger" } diff --git a/src/agent/coverage/src/record.rs b/src/agent/coverage/src/record.rs index 534d1d4d63..359046cd6e 100644 --- a/src/agent/coverage/src/record.rs +++ b/src/agent/coverage/src/record.rs @@ -121,31 +121,34 @@ impl CoverageRecorder { #[cfg(target_os = "windows")] pub fn record(self) -> Result { use debugger::Debugger; + use process_control::{ChildExt, Control}; use windows::WindowsRecorder; let loader = self.loader.clone(); + let mut recorder = + WindowsRecorder::new(&loader, self.module_allowlist, self.cache.as_ref()); + let (mut dbg, child) = Debugger::init(self.cmd, &mut recorder)?; + dbg.run(&mut recorder)?; + + // If the debugger callbacks fail, this may return with a spurious clean exit. + let output = child + .controlled_with_output() + .time_limit(self.timeout) + .terminate_for_timeout() + .wait()? + .ok_or_else(|| crate::timer::TimerError::Timeout(self.timeout))?.into(); + + // Check if debugging was stopped due to a callback error. + // + // If so, the debugger terminated the target, and the recorded coverage and + // output are both invalid. + if let Some(err) = recorder.stop_error { + return Err(err); + } - crate::timer::timed(self.timeout, move || { - let mut recorder = - WindowsRecorder::new(&loader, self.module_allowlist, self.cache.as_ref()); - let (mut dbg, child) = Debugger::init(self.cmd, &mut recorder)?; - dbg.run(&mut recorder)?; - - // If the debugger callbacks fail, this may return with a spurious clean exit. - let output = child.wait_with_output()?.into(); - - // Check if debugging was stopped due to a callback error. - // - // If so, the debugger terminated the target, and the recorded coverage and - // output are both invalid. - if let Some(err) = recorder.stop_error { - return Err(err); - } - - let coverage = recorder.coverage; + let coverage = recorder.coverage; - Ok(Recorded { coverage, output }) - })? + Ok(Recorded { coverage, output }) } } @@ -157,11 +160,24 @@ pub struct Recorded { #[derive(Clone, Debug, Default)] pub struct Output { - pub status: Option, + pub status: Option, pub stderr: String, pub stdout: String, } +impl From for Output { + fn from(output: process_control::Output) -> Self { + let status = Some(output.status); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + Self { + status, + stdout, + stderr, + } + } +} + impl From for Output { fn from(output: std::process::Output) -> Self { let status = Some(output.status); @@ -169,7 +185,7 @@ impl From for Output { let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); Self { - status, + status: status.map(Into::into), stdout, stderr, } From 9c4c8aec0a828144a5f3a5212b5055d4d31a6b71 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 26 Sep 2023 14:24:31 -0700 Subject: [PATCH 2/5] set the timeout before we start the debugger --- src/agent/coverage/src/record.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/agent/coverage/src/record.rs b/src/agent/coverage/src/record.rs index 359046cd6e..077f3e6bb6 100644 --- a/src/agent/coverage/src/record.rs +++ b/src/agent/coverage/src/record.rs @@ -128,15 +128,19 @@ impl CoverageRecorder { let mut recorder = WindowsRecorder::new(&loader, self.module_allowlist, self.cache.as_ref()); let (mut dbg, child) = Debugger::init(self.cmd, &mut recorder)?; + + let controlled_child = child + .controlled_with_output() + .time_limit(self.timeout) + .terminate_for_timeout(); + dbg.run(&mut recorder)?; // If the debugger callbacks fail, this may return with a spurious clean exit. - let output = child - .controlled_with_output() - .time_limit(self.timeout) - .terminate_for_timeout() - .wait()? - .ok_or_else(|| crate::timer::TimerError::Timeout(self.timeout))?.into(); + let output = controlled_child + .wait()? + .ok_or_else(|| crate::timer::TimerError::Timeout(self.timeout))? + .into(); // Check if debugging was stopped due to a callback error. // From 838a5fa67d2e5fac2f6d1c806dfd2afe1bec9f79 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 27 Sep 2023 12:56:06 -0700 Subject: [PATCH 3/5] split the target launch from the debugger initialization wait for the process to finish on a separate thread --- src/agent/coverage/src/record.rs | 46 +++++++++++++++++++++--------- src/agent/debugger/src/debugger.rs | 27 +++++++++++------- 2 files changed, 50 insertions(+), 23 deletions(-) diff --git a/src/agent/coverage/src/record.rs b/src/agent/coverage/src/record.rs index 077f3e6bb6..6c1d620186 100644 --- a/src/agent/coverage/src/record.rs +++ b/src/agent/coverage/src/record.rs @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use std::process::{Command, ExitStatus, Stdio}; +use std::process::{Command, Stdio}; use std::sync::Arc; use std::time::Duration; @@ -120,27 +120,45 @@ impl CoverageRecorder { #[cfg(target_os = "windows")] pub fn record(self) -> Result { + use anyhow::bail; use debugger::Debugger; use process_control::{ChildExt, Control}; use windows::WindowsRecorder; + let child = Debugger::create_child(self.cmd)?; + + // Spawn a thread to wait for the target process to exit. + let taget_process = std::thread::spawn(move || { + let output = child + .controlled_with_output() + .time_limit(self.timeout) + .terminate_for_timeout() + .wait(); + output + }); + let loader = self.loader.clone(); let mut recorder = WindowsRecorder::new(&loader, self.module_allowlist, self.cache.as_ref()); - let (mut dbg, child) = Debugger::init(self.cmd, &mut recorder)?; - - let controlled_child = child - .controlled_with_output() - .time_limit(self.timeout) - .terminate_for_timeout(); + // the debugger is initialized in the same thread that created the target process to be able to receive the debug events + let mut dbg = Debugger::init_debugger(&mut recorder)?; dbg.run(&mut recorder)?; + let output = match taget_process.join() { + Err(err) => { + bail!("failed to launch target thread: {:?}", err) + } + Ok(Err(err)) => { + bail!("failed to launch target process: {:?}", err) + } + Ok(Ok(None)) => { + bail!(crate::timer::TimerError::Timeout(self.timeout)) + } + Ok(Ok(Some(output))) => output, + }; + // If the debugger callbacks fail, this may return with a spurious clean exit. - let output = controlled_child - .wait()? - .ok_or_else(|| crate::timer::TimerError::Timeout(self.timeout))? - .into(); // Check if debugging was stopped due to a callback error. // @@ -151,8 +169,10 @@ impl CoverageRecorder { } let coverage = recorder.coverage; - - Ok(Recorded { coverage, output }) + Ok(Recorded { + coverage, + output: output.into(), + }) } } diff --git a/src/agent/debugger/src/debugger.rs b/src/agent/debugger/src/debugger.rs index d7c0f1ba5e..ae67f66fed 100644 --- a/src/agent/debugger/src/debugger.rs +++ b/src/agent/debugger/src/debugger.rs @@ -134,15 +134,7 @@ pub struct Debugger { } impl Debugger { - pub fn init( - mut command: Command, - callbacks: &mut impl DebugEventHandler, - ) -> Result<(Self, Child)> { - let child = command - .creation_flags(DEBUG_ONLY_THIS_PROCESS.0) - .spawn() - .context("debugee failed to start")?; - + pub fn init_debugger(callbacks: &mut impl DebugEventHandler) -> Result { unsafe { DebugSetProcessKillOnExit(TRUE) } .ok() .context("Setting DebugSetProcessKillOnExit to TRUE")?; @@ -186,12 +178,27 @@ impl Debugger { return Err(last_os_error()); } - Ok((debugger, child)) + Ok(debugger) } else { anyhow::bail!("Unexpected event: {}", de) } } + pub fn create_child(mut command: Command) -> Result { + let child = command + .creation_flags(DEBUG_ONLY_THIS_PROCESS.0) + .spawn() + .context("debugee failed to start")?; + + Ok(child) + } + + pub fn init(command: Command, callbacks: &mut impl DebugEventHandler) -> Result<(Self, Child)> { + let child = Self::create_child(command)?; + let debugger = Self::init_debugger(callbacks)?; + Ok((debugger, child)) + } + pub fn target(&mut self) -> &mut Target { &mut self.target } From 5a0ffc11899d133d5fbef3f440ecfcb75fc29084 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 27 Sep 2023 13:15:12 -0700 Subject: [PATCH 4/5] fix build --- src/agent/coverage/src/timer.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/agent/coverage/src/timer.rs b/src/agent/coverage/src/timer.rs index 760e453b28..3271666d67 100644 --- a/src/agent/coverage/src/timer.rs +++ b/src/agent/coverage/src/timer.rs @@ -7,6 +7,7 @@ use std::time::Duration; use thiserror::Error; +#[allow(dead_code)] pub fn timed(timeout: Duration, function: F) -> Result where T: Send + 'static, From b42067559ca5744cc9896367c1e12039122df43d Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 27 Sep 2023 15:09:25 -0700 Subject: [PATCH 5/5] move comments --- src/agent/coverage/src/record.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/agent/coverage/src/record.rs b/src/agent/coverage/src/record.rs index 6c1d620186..44faded302 100644 --- a/src/agent/coverage/src/record.rs +++ b/src/agent/coverage/src/record.rs @@ -141,10 +141,11 @@ impl CoverageRecorder { let mut recorder = WindowsRecorder::new(&loader, self.module_allowlist, self.cache.as_ref()); - // the debugger is initialized in the same thread that created the target process to be able to receive the debug events + // The debugger is initialized in the same thread that created the target process to be able to receive the debug events let mut dbg = Debugger::init_debugger(&mut recorder)?; dbg.run(&mut recorder)?; + // If the debugger callbacks fail, this may return with a spurious clean exit. let output = match taget_process.join() { Err(err) => { bail!("failed to launch target thread: {:?}", err) @@ -158,8 +159,6 @@ impl CoverageRecorder { Ok(Ok(Some(output))) => output, }; - // If the debugger callbacks fail, this may return with a spurious clean exit. - // Check if debugging was stopped due to a callback error. // // If so, the debugger terminated the target, and the recorded coverage and