Auto merge of #3162 - alexcrichton:ugh-mspdbsrv, r=brson

Leak mspdbsrv.exe processes on Windows

Instead of having our job object tear them down, instead leak them intentionally
if everything succeeded.

Closes #3161

Author: bors

Cargo.lock

@@ -27,14 +27,15 @@ filetime = "0.1"
 flate2 = "0.2"
 fs2 = "0.2"
 git2 = "0.4"
-libgit2-sys = "0.4"
 git2-curl = "0.5"
 glob = "0.2"
 kernel32-sys = "0.2"
 libc = "0.2"
+libgit2-sys = "0.4"
 log = "0.3"
 miow = "0.1"
 num_cpus = "1.0"
+psapi-sys = "0.1"
 regex = "0.1"
 rustc-serialize = "0.3"
 semver = "0.2.3"

Cargo.toml

@@ -123,7 +123,7 @@ fn execute(flags: Flags, config: &Config) -> CliResult<Option<()>> {
                           flags.flag_locked));
 
     init_git_transports(config);
-    cargo::util::job::setup();
+    let _token = cargo::util::job::setup();
 
     if flags.flag_version {
         println!("{}", cargo::version());

src/bin/cargo.rs

@@ -15,7 +15,9 @@
 //! 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<Setup> {
     unsafe { imp::setup() }
 }
 
@@ -24,25 +26,44 @@ mod imp {
     use std::env;
     use libc;
 
-    pub unsafe fn setup() {
+    pub type Setup = ();
+
+    pub unsafe fn setup() -> Option<()> {
         // There's a test case for the behavior of
         // when-cargo-is-killed-subprocesses-are-also-killed, but that requires
         // one cargo spawned to become its own session leader, so we do that
         // here.
         if env::var("__CARGO_TEST_SETSID_PLEASE_DONT_USE_ELSEWHERE").is_ok() {
             libc::setsid();
         }
+        Some(())
     }
 }
 
 #[cfg(windows)]
 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<Setup> {
         // 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,
@@ -54,8 +75,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
@@ -65,27 +87,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); }
+        }
     }
 }