Fix issue #5976 - HANDLE leaks and undefined/bad behavour

gareth · gareth · commit 91aeecf7e38e · 2013-04-23T21:23:15.000+01:00
on windows.
diff --git a/src/libcore/libc.rs b/src/libcore/libc.rs
@@ -1725,6 +1725,7 @@ pub mod funcs {
                                        findFileData: HANDLE)
                     -> BOOL;
                 unsafe fn FindClose(findFile: HANDLE) -> BOOL;
+                unsafe fn CloseHandle(hObject: HANDLE) -> BOOL;
                 unsafe fn TerminateProcess(hProcess: HANDLE, uExitCode: c_uint) -> BOOL;
             }
         }
diff --git a/src/libcore/os.rs b/src/libcore/os.rs
@@ -60,7 +60,7 @@ pub mod rustrt {
         unsafe fn rust_get_argv() -> **c_char;
         unsafe fn rust_path_is_dir(path: *libc::c_char) -> c_int;
         unsafe fn rust_path_exists(path: *libc::c_char) -> c_int;
-        unsafe fn rust_process_wait(handle: c_int) -> c_int;
+        unsafe fn rust_process_wait(pid: c_int) -> c_int;
         unsafe fn rust_set_exit_status(code: libc::intptr_t);
     }
 }
@@ -356,7 +356,11 @@ pub fn fsync_fd(fd: c_int, _l: io::fsync::Level) -> c_int {
 #[cfg(windows)]
 pub fn waitpid(pid: pid_t) -> c_int {
     unsafe {
-        return rustrt::rust_process_wait(pid);
+        let status = rustrt::rust_process_wait(pid);
+        if status < 0 {
+            fail!(fmt!("failure in rust_process_wait: %s", last_os_error()));
+        }
+        return status;
     }
 }
 
diff --git a/src/libcore/run.rs b/src/libcore/run.rs
@@ -24,8 +24,9 @@ use task;
 use vec;
 
 pub mod rustrt {
-    use libc::{c_int, c_void, pid_t};
+    use libc::{c_int, c_void};
     use libc;
+    use run;
 
     #[abi = "cdecl"]
     pub extern {
@@ -34,11 +35,18 @@ pub mod rustrt {
                                    dir: *libc::c_char,
                                    in_fd: c_int,
                                    out_fd: c_int,
-                                   err_fd: c_int)
-                                -> pid_t;
+                                   err_fd: c_int) -> run::RunProgramResult;
     }
 }
 
+pub struct RunProgramResult {
+    // the process id of the program, or -1 if in case of errors
+    pid: pid_t,
+    // a handle to the process - on unix this will always be NULL, but on windows it will be a
+    // HANDLE to the process, which will prevent the pid being re-used until the handle is closed.
+    handle: *(),
+}
+
 /// A value representing a child process
 pub trait Program {
     /// Returns the process id of the program
@@ -100,16 +108,24 @@ pub trait Program {
  * The process id of the spawned process
  */
 pub fn spawn_process(prog: &str, args: &[~str],
-                 env: &Option<~[(~str,~str)]>,
-                 dir: &Option<~str>,
-                 in_fd: c_int, out_fd: c_int, err_fd: c_int)
-              -> pid_t {
+                     env: &Option<~[(~str,~str)]>,
+                     dir: &Option<~str>,
+                     in_fd: c_int, out_fd: c_int, err_fd: c_int) -> pid_t {
+
+    let res = spawn_process_internal(prog, args, env, dir, in_fd, out_fd, err_fd);
+    free_handle(res.handle);
+    return res.pid;
+}
+
+fn spawn_process_internal(prog: &str, args: &[~str],
+                          env: &Option<~[(~str,~str)]>,
+                          dir: &Option<~str>,
+                          in_fd: c_int, out_fd: c_int, err_fd: c_int) -> RunProgramResult {
     unsafe {
         do with_argv(prog, args) |argv| {
             do with_envp(env) |envp| {
                 do with_dirp(dir) |dirp| {
-                    rustrt::rust_run_program(argv, envp, dirp,
-                                             in_fd, out_fd, err_fd)
+                    rustrt::rust_run_program(argv, envp, dirp, in_fd, out_fd, err_fd)
                 }
             }
         }
@@ -195,6 +211,18 @@ priv unsafe fn fclose_and_null(f: &mut *libc::FILE) {
     }
 }
 
+#[cfg(windows)]
+priv fn free_handle(handle: *()) {
+    unsafe {
+        libc::funcs::extra::kernel32::CloseHandle(cast::transmute(handle));
+    }
+}
+
+#[cfg(unix)]
+priv fn free_handle(_handle: *()) {
+    // unix has no process handle object, just a pid
+}
+
 /**
  * Spawns a process and waits for it to terminate
  *
@@ -208,10 +236,13 @@ priv unsafe fn fclose_and_null(f: &mut *libc::FILE) {
  * The process's exit code
  */
 pub fn run_program(prog: &str, args: &[~str]) -> int {
-    let pid = spawn_process(prog, args, &None, &None,
-                            0i32, 0i32, 0i32);
-    if pid == -1 as pid_t { fail!(); }
-    return waitpid(pid);
+    let res = spawn_process_internal(prog, args, &None, &None,
+                                     0i32, 0i32, 0i32);
+    if res.pid == -1 as pid_t { fail!(); }
+
+    let code = waitpid(res.pid);
+    free_handle(res.handle);
+    return code;
 }
 
 /**
@@ -234,20 +265,21 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program {
     let pipe_input = os::pipe();
     let pipe_output = os::pipe();
     let pipe_err = os::pipe();
-    let pid =
-        spawn_process(prog, args, &None, &None,
-                      pipe_input.in, pipe_output.out,
-                      pipe_err.out);
+    let res =
+        spawn_process_internal(prog, args, &None, &None,
+                               pipe_input.in, pipe_output.out,
+                               pipe_err.out);
 
     unsafe {
-        if pid == -1 as pid_t { fail!(); }
+        if res.pid == -1 as pid_t { fail!(); }
         libc::close(pipe_input.in);
         libc::close(pipe_output.out);
         libc::close(pipe_err.out);
     }
 
     struct ProgRepr {
         pid: pid_t,
+        handle: *(),
         in_fd: c_int,
         out_file: *libc::FILE,
         err_file: *libc::FILE,
@@ -317,6 +349,7 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program {
                 finish_repr(cast::transmute(&self.r));
                 close_repr_outputs(cast::transmute(&self.r));
             }
+            free_handle(self.r.handle);
         }
     }
 
@@ -343,8 +376,9 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program {
         fn force_destroy(&mut self) { destroy_repr(&mut self.r, true); }
     }
 
-    let repr = ProgRepr {
-        pid: pid,
+    let mut repr = ProgRepr {
+        pid: res.pid,
+        handle: res.handle,
         in_fd: pipe_input.out,
         out_file: os::fdopen(pipe_output.in),
         err_file: os::fdopen(pipe_err.in),
@@ -385,13 +419,13 @@ pub fn program_output(prog: &str, args: &[~str]) -> ProgramOutput {
     let pipe_in = os::pipe();
     let pipe_out = os::pipe();
     let pipe_err = os::pipe();
-    let pid = spawn_process(prog, args, &None, &None,
-                            pipe_in.in, pipe_out.out, pipe_err.out);
+    let res = spawn_process_internal(prog, args, &None, &None,
+                                     pipe_in.in, pipe_out.out, pipe_err.out);
 
     os::close(pipe_in.in);
     os::close(pipe_out.out);
     os::close(pipe_err.out);
-    if pid == -1i32 {
+    if res.pid == -1i32 {
         os::close(pipe_in.out);
         os::close(pipe_out.in);
         os::close(pipe_err.in);
@@ -415,7 +449,10 @@ pub fn program_output(prog: &str, args: &[~str]) -> ProgramOutput {
         let output = readclose(pipe_out.in);
         ch_clone.send((1, output));
     };
-    let status = run::waitpid(pid);
+
+    let status = waitpid(res.pid);
+    free_handle(res.handle);
+
     let mut errs = ~"";
     let mut outs = ~"";
     let mut count = 2;
@@ -563,6 +600,12 @@ mod tests {
         assert!(status == 1);
     }
 
+    #[test]
+    #[should_fail]
+    fn waitpid_non_existant_pid() {
+        run::waitpid(123456789); // assume that this pid doesn't exist
+    }
+
     #[test]
     fn test_destroy_once() {
         let mut p = run::start_program("echo", []);
diff --git a/src/rt/rust_run_program.cpp b/src/rt/rust_run_program.cpp
@@ -15,6 +15,11 @@
 #include <crt_externs.h>
 #endif
 
+struct RunProgramResult {
+    pid_t pid;
+    void* handle;
+};
+
 #if defined(__WIN32__)
 
 #include <process.h>
@@ -78,7 +83,7 @@ void append_arg(char *& buf, char const *arg, bool last) {
     }
 }
 
-extern "C" CDECL int
+extern "C" CDECL RunProgramResult
 rust_run_program(const char* argv[],
                  void* envp,
                  const char* dir,
@@ -88,19 +93,21 @@ rust_run_program(const char* argv[],
     si.cb = sizeof(STARTUPINFO);
     si.dwFlags = STARTF_USESTDHANDLES;
 
+    RunProgramResult result = {-1, NULL};
+
     HANDLE curproc = GetCurrentProcess();
     HANDLE origStdin = (HANDLE)_get_osfhandle(in_fd ? in_fd : 0);
     if (!DuplicateHandle(curproc, origStdin,
         curproc, &si.hStdInput, 0, 1, DUPLICATE_SAME_ACCESS))
-        return -1;
+        return result;
     HANDLE origStdout = (HANDLE)_get_osfhandle(out_fd ? out_fd : 1);
     if (!DuplicateHandle(curproc, origStdout,
         curproc, &si.hStdOutput, 0, 1, DUPLICATE_SAME_ACCESS))
-        return -1;
+        return result;
     HANDLE origStderr = (HANDLE)_get_osfhandle(err_fd ? err_fd : 2);
     if (!DuplicateHandle(curproc, origStderr,
         curproc, &si.hStdError, 0, 1, DUPLICATE_SAME_ACCESS))
-        return -1;
+        return result;
 
     size_t cmd_len = 0;
     for (const char** arg = argv; *arg; arg++) {
@@ -124,18 +131,39 @@ rust_run_program(const char* argv[],
     CloseHandle(si.hStdError);
     free(cmd);
 
-    if (!created) return -1;
-    return (int)pi.hProcess;
+    if (!created) {
+        return result;
+    }
+
+    // We close the thread handle because we don't care about keeping the thread id valid,
+    // and we aren't keeping the thread handle around to be able to close it later. We don't
+    // close the process handle however because we want the process id to stay valid at least
+    // until the calling rust code closes the process handle.
+    CloseHandle(pi.hThread);
+    result.pid = pi.dwProcessId;
+    result.handle = pi.hProcess;
+    return result;
 }
 
 extern "C" CDECL int
-rust_process_wait(int proc) {
+rust_process_wait(int pid) {
+
+    HANDLE proc = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION, FALSE, pid);
+    if (proc == NULL) {
+        return -1;
+    }
+
     DWORD status;
     while (true) {
-        if (GetExitCodeProcess((HANDLE)proc, &status) &&
-            status != STILL_ACTIVE)
-            return (int)status;
-        WaitForSingleObject((HANDLE)proc, INFINITE);
+        if (!GetExitCodeProcess(proc, &status)) {
+            CloseHandle(proc);
+            return -1;
+        }
+        if (status != STILL_ACTIVE) {
+            CloseHandle(proc);
+            return (int) status;
+        }
+        WaitForSingleObject(proc, INFINITE);
     }
 }
 
@@ -151,13 +179,16 @@ rust_process_wait(int proc) {
 extern char **environ;
 #endif
 
-extern "C" CDECL int
+extern "C" CDECL RunProgramResult
 rust_run_program(const char* argv[],
                  void* envp,
                  const char* dir,
                  int in_fd, int out_fd, int err_fd) {
     int pid = fork();
-    if (pid != 0) return pid;
+    if (pid != 0) {
+        RunProgramResult result = {pid, NULL};
+        return result;
+    }
 
     sigset_t sset;
     sigemptyset(&sset);
@@ -187,7 +218,7 @@ rust_run_program(const char* argv[],
 }
 
 extern "C" CDECL int
-rust_process_wait(int proc) {
+rust_process_wait(int pid) {
     // FIXME: stub; exists to placate linker. (#2692)
     return 0;
 }

Original file line number	Diff line number	Diff line change
`@@ -1725,6 +1725,7 @@ pub mod funcs {`
`1725`	`1725`	`findFileData: HANDLE)`
`1726`	`1726`	`-> BOOL;`
`1727`	`1727`	`unsafe fn FindClose(findFile: HANDLE) -> BOOL;`
	`1728`	`+ unsafe fn CloseHandle(hObject: HANDLE) -> BOOL;`
`1728`	`1729`	`unsafe fn TerminateProcess(hProcess: HANDLE, uExitCode: c_uint) -> BOOL;`
`1729`	`1730`	`}`
`1730`	`1731`	`}`
Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,7 @@ pub mod rustrt {`
`60`	`60`	`unsafe fn rust_get_argv() -> **c_char;`
`61`	`61`	`unsafe fn rust_path_is_dir(path: *libc::c_char) -> c_int;`
`62`	`62`	`unsafe fn rust_path_exists(path: *libc::c_char) -> c_int;`
`63`		`- unsafe fn rust_process_wait(handle: c_int) -> c_int;`
	`63`	`+ unsafe fn rust_process_wait(pid: c_int) -> c_int;`
`64`	`64`	`unsafe fn rust_set_exit_status(code: libc::intptr_t);`
`65`	`65`	`}`
`66`	`66`	`}`
`@@ -356,7 +356,11 @@ pub fn fsync_fd(fd: c_int, _l: io::fsync::Level) -> c_int {`
`356`	`356`	`#[cfg(windows)]`
`357`	`357`	`pub fn waitpid(pid: pid_t) -> c_int {`
`358`	`358`	`unsafe {`
`359`		`- return rustrt::rust_process_wait(pid);`
	`359`	`+ let status = rustrt::rust_process_wait(pid);`
	`360`	`+ if status < 0 {`
	`361`	`+ fail!(fmt!("failure in rust_process_wait: %s", last_os_error()));`
	`362`	`+ }`
	`363`	`+ return status;`
`360`	`364`	`}`
`361`	`365`	`}`
`362`	`366`