From 8a3994b2cd7a9528131abd361537a9e0e5fcea0d Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 13 Mar 2017 15:25:50 -0700 Subject: [PATCH] Update job object code to match Cargo's This'll help "leak" mspdbsrv which is necessary to help link.exe function correctly on Windows unfortunately. --- Cargo.lock | 11 ++ Cargo.toml | 1 + src/rustup-cli/job.rs | 197 ++++++++++++++++++++++++++++++++--- src/rustup-cli/proxy_mode.rs | 2 +- 4 files changed, 196 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fac4376aeac..e3c102e4e9f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -447,6 +447,15 @@ name = "pkg-config" version = "0.3.8" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "psapi-sys" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "winapi 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi-build 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "rand" version = "0.3.14" @@ -525,6 +534,7 @@ dependencies = [ "lazy_static 0.1.16 (registry+https://github.com/rust-lang/crates.io-index)", "libc 0.2.18 (registry+https://github.com/rust-lang/crates.io-index)", "markdown 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", + "psapi-sys 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "rand 0.3.14 (registry+https://github.com/rust-lang/crates.io-index)", "regex 0.1.73 (registry+https://github.com/rust-lang/crates.io-index)", "rustc-serialize 0.3.19 (registry+https://github.com/rust-lang/crates.io-index)", @@ -997,6 +1007,7 @@ dependencies = [ "checksum openssl-sys 0.9.2 (registry+https://github.com/rust-lang/crates.io-index)" = "58acf9d02ba8903c7c664df3037f4c04935e968d9f3932232400fa60364de62a" "checksum pipeline 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "d15b6607fa632996eb8a17c9041cb6071cb75ac057abd45dece578723ea8c7c0" "checksum pkg-config 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)" = "8cee804ecc7eaf201a4a207241472cc870e825206f6c031e3ee2a72fa425f2fa" +"checksum psapi-sys 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "abcd5d1a07d360e29727f757a9decb3ce8bc6e0efa8969cfaad669a8317a2478" "checksum rand 0.3.14 (registry+https://github.com/rust-lang/crates.io-index)" = "2791d88c6defac799c3f20d74f094ca33b9332612d9aef9078519c82e4fe04a5" "checksum regex 0.1.73 (registry+https://github.com/rust-lang/crates.io-index)" = "56b7ee9f764ecf412c6e2fff779bca4b22980517ae335a21aeaf4e32625a5df2" "checksum regex-syntax 0.3.4 (registry+https://github.com/rust-lang/crates.io-index)" = "31040aad7470ad9d8c46302dcffba337bb4289ca5da2e3cd6e37b64109a85199" diff --git a/Cargo.toml b/Cargo.toml index f588aa61ab9..5b6b0646da1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,6 +58,7 @@ winreg = "0.3.2" user32-sys = "0.1.2" kernel32-sys = "0.2.1" gcc = "0.3.28" +psapi-sys = "0.1" [dev-dependencies] rustup-mock = { path = "src/rustup-mock", version = "1.0.0" } diff --git a/src/rustup-cli/job.rs b/src/rustup-cli/job.rs index 43fef821db9..0c2f85d422a 100644 --- a/src/rustup-cli/job.rs +++ b/src/rustup-cli/job.rs @@ -17,13 +17,18 @@ //! child will be associated with the job object as well. This means if we add //! ourselves to the job object we create then everything will get torn down! -pub fn setup() { +pub use self::imp::Setup; + +pub fn setup() -> Option { unsafe { imp::setup() } } #[cfg(unix)] mod imp { - pub unsafe fn setup() { + pub type Setup = (); + + pub unsafe fn setup() -> Option<()> { + Some(()) } } @@ -31,10 +36,26 @@ mod imp { mod imp { extern crate kernel32; extern crate winapi; + extern crate psapi; + use std::ffi::OsString; + use std::io; use std::mem; + use std::os::windows::prelude::*; + + pub struct Setup { + job: Handle, + } - pub unsafe fn setup() { + pub struct Handle { + inner: winapi::HANDLE, + } + + fn last_err() -> io::Error { + io::Error::last_os_error() + } + + pub unsafe fn setup() -> Option { // Creates a new job object for us to use and then adds ourselves to it. // Note that all errors are basically ignored in this function, // intentionally. Job objects are "relatively new" in Windows, @@ -46,8 +67,9 @@ mod imp { let job = kernel32::CreateJobObjectW(0 as *mut _, 0 as *const _); if job.is_null() { - return + return None } + let job = Handle { inner: job }; // Indicate that when all handles to the job object are gone that all // process in the object should be killed. Note that this includes our @@ -57,27 +79,174 @@ mod imp { info = mem::zeroed(); info.BasicLimitInformation.LimitFlags = winapi::JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; - let r = kernel32::SetInformationJobObject(job, + let r = kernel32::SetInformationJobObject(job.inner, winapi::JobObjectExtendedLimitInformation, &mut info as *mut _ as winapi::LPVOID, mem::size_of_val(&info) as winapi::DWORD); if r == 0 { - kernel32::CloseHandle(job); - return + return None } // Assign our process to this job object, meaning that our children will // now live or die based on our existence. let me = kernel32::GetCurrentProcess(); - let r = kernel32::AssignProcessToJobObject(job, me); + let r = kernel32::AssignProcessToJobObject(job.inner, me); if r == 0 { - kernel32::CloseHandle(job); - return + return None + } + + Some(Setup { job: job }) + } + + impl Drop for Setup { + fn drop(&mut self) { + // This is a litte subtle. By default if we are terminated then all + // processes in our job object are terminated as well, but we + // intentionally want to whitelist some processes to outlive our job + // object (see below). + // + // To allow for this, we manually kill processes instead of letting + // the job object kill them for us. We do this in a loop to handle + // processes spawning other processes. + // + // Finally once this is all done we know that the only remaining + // ones are ourselves and the whitelisted processes. The destructor + // here then configures our job object to *not* kill everything on + // close, then closes the job object. + unsafe { + while self.kill_remaining() { + info!("killed some, going for more"); + } + + let mut info: winapi::JOBOBJECT_EXTENDED_LIMIT_INFORMATION; + info = mem::zeroed(); + let r = kernel32::SetInformationJobObject( + self.job.inner, + winapi::JobObjectExtendedLimitInformation, + &mut info as *mut _ as winapi::LPVOID, + mem::size_of_val(&info) as winapi::DWORD); + if r == 0 { + info!("failed to configure job object to defaults: {}", + last_err()); + } + } } + } + + impl Setup { + unsafe fn kill_remaining(&mut self) -> bool { + #[repr(C)] + struct Jobs { + header: winapi::JOBOBJECT_BASIC_PROCESS_ID_LIST, + list: [winapi::ULONG_PTR; 1024], + } + + let mut jobs: Jobs = mem::zeroed(); + let r = kernel32::QueryInformationJobObject( + self.job.inner, + winapi::JobObjectBasicProcessIdList, + &mut jobs as *mut _ as winapi::LPVOID, + mem::size_of_val(&jobs) as winapi::DWORD, + 0 as *mut _); + if r == 0 { + info!("failed to query job object: {}", last_err()); + return false + } + + let mut killed = false; + let list = &jobs.list[..jobs.header.NumberOfProcessIdsInList as usize]; + assert!(list.len() > 0); + info!("found {} remaining processes", list.len() - 1); + + let list = list.iter().filter(|&&id| { + // let's not kill ourselves + id as winapi::DWORD != kernel32::GetCurrentProcessId() + }).filter_map(|&id| { + // Open the process with the necessary rights, and if this + // fails then we probably raced with the process exiting so we + // ignore the problem. + let flags = winapi::PROCESS_QUERY_INFORMATION | + winapi::PROCESS_TERMINATE | + winapi::SYNCHRONIZE; + let p = kernel32::OpenProcess(flags, + winapi::FALSE, + id as winapi::DWORD); + if p.is_null() { + None + } else { + Some(Handle { inner: p }) + } + }).filter(|p| { + // Test if this process was actually in the job object or not. + // If it's not then we likely raced with something else + // recycling this PID, so we just skip this step. + let mut res = 0; + let r = kernel32::IsProcessInJob(p.inner, self.job.inner, &mut res); + if r == 0 { + info!("failed to test is process in job: {}", last_err()); + return false + } + res == winapi::TRUE + }); + + + for p in list { + // Load the file which this process was spawned from. We then + // later use this for identification purposes. + let mut buf = [0; 1024]; + let r = psapi::GetProcessImageFileNameW(p.inner, + buf.as_mut_ptr(), + buf.len() as winapi::DWORD); + if r == 0 { + info!("failed to get image name: {}", last_err()); + continue + } + let s = OsString::from_wide(&buf[..r as usize]); + info!("found remaining: {:?}", s); - // Intentionally leak the `job` handle here. We've got the only - // reference to this job, so once it's gone we and all our children will - // be killed. This typically won't happen unless Cargo itself is - // ctrl-c'd. + // And here's where we find the whole purpose for this + // function! Currently, our only whitelisted process is + // `mspdbsrv.exe`, and more details about that can be found + // here: + // + // https://github.com/rust-lang/rust/issues/33145 + // + // The gist of it is that all builds on one machine use the + // same `mspdbsrv.exe` instance. If we were to kill this + // instance then we could erroneously cause other builds to + // fail. + if let Some(s) = s.to_str() { + if s.contains("mspdbsrv") { + info!("\toops, this is mspdbsrv"); + continue + } + } + + // Ok, this isn't mspdbsrv, let's kill the process. After we + // kill it we wait on it to ensure that the next time around in + // this function we're not going to see it again. + let r = kernel32::TerminateProcess(p.inner, 1); + if r == 0 { + info!("\tfailed to kill subprocess: {}", last_err()); + info!("\tassuming subprocess is dead..."); + } else { + info!("\tterminated subprocess"); + } + let r = kernel32::WaitForSingleObject(p.inner, winapi::INFINITE); + if r != 0 { + info!("failed to wait for process to die: {}", last_err()); + return false + } + killed = true; + } + + return killed + } + } + + impl Drop for Handle { + fn drop(&mut self) { + unsafe { kernel32::CloseHandle(self.inner); } + } } } diff --git a/src/rustup-cli/proxy_mode.rs b/src/rustup-cli/proxy_mode.rs index a25f30c6a9e..f6943c4bd3d 100644 --- a/src/rustup-cli/proxy_mode.rs +++ b/src/rustup-cli/proxy_mode.rs @@ -11,7 +11,7 @@ use job; pub fn main() -> Result<()> { try!(::self_update::cleanup_self_updater()); - job::setup(); + let _setup = job::setup(); let mut args = env::args();