From e3b1e0e93f8021c4bf6d10aad3023e4cdf5bb331 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 28 Sep 2023 17:54:32 -0700 Subject: [PATCH] Terminate process on timeout in windows for the coverage task (#3529) * Terminate process on timeout in windows for the coverage task * set the timeout before we start the debugger * split the target launch from the debugger initialization wait for the process to finish on a separate thread * fix build * move comments --- src/agent/Cargo.lock | 1 + src/agent/coverage/Cargo.toml | 1 + src/agent/coverage/src/record.rs | 81 ++++++++++++++++++++++-------- src/agent/coverage/src/timer.rs | 1 + src/agent/debugger/src/debugger.rs | 27 ++++++---- 5 files changed, 80 insertions(+), 31 deletions(-) diff --git a/src/agent/Cargo.lock b/src/agent/Cargo.lock index 7a79204631..eb35241201 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 2a1170e3ae..e1ced7050f 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..44faded302 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,32 +120,58 @@ 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()); - 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); + // 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) + } + 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, + }; - let coverage = recorder.coverage; + // 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); + } - Ok(Recorded { coverage, output }) - })? + let coverage = recorder.coverage; + Ok(Recorded { + coverage, + output: output.into(), + }) } } @@ -157,11 +183,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 +208,7 @@ impl From for Output { let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); Self { - status, + status: status.map(Into::into), stdout, stderr, } 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, 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 }