auto merge of #6010 : Dretch/rust/run-windows, r=brson

This fixes #5976.

It also removes `os::waitpid` in favour of (the existing) `run::waitpid`. I included this change because I figured it is kind of related.

r?

4 files changed, 133 insertions, 65 deletions

diff --git a/src/libcore/libc.rs b/src/libcore/libc.rs
index 945d08323b4..44864630f98 100644
--- 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
index 0e7131a86f3..fa3ca4577c6 100644
--- a/src/libcore/os.rs
+++ b/src/libcore/os.rs
@@ -30,7 +30,7 @@ use cast;
 use io;
 use libc;
 use libc::{c_char, c_void, c_int, size_t};
-use libc::{mode_t, pid_t, FILE};
+use libc::{mode_t, FILE};
 use option;
 use option::{Some, None};
 use prelude::*;
@@ -60,7 +60,6 @@ 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_set_exit_status(code: libc::intptr_t);
     }
 }
@@ -352,27 +351,6 @@ 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);
-    }
-}
-
-#[cfg(unix)]
-pub fn waitpid(pid: pid_t) -> c_int {
-    unsafe {
-        use libc::funcs::posix01::wait::*;
-        let mut status = 0 as c_int;
-
-        assert!((waitpid(pid, &mut status, 0 as c_int) !=
-                     (-1 as c_int)));
-        return status;
-    }
-}
-
-
 pub struct Pipe { mut in: c_int, mut out: c_int }
 
 #[cfg(unix)]
diff --git a/src/libcore/run.rs b/src/libcore/run.rs
index f9d7f4a229c..2455954ebe1 100644
--- a/src/libcore/run.rs
+++ b/src/libcore/run.rs
@@ -18,14 +18,14 @@ use option::{Some, None};
 use os;
 use prelude::*;
 use ptr;
-use run;
 use str;
 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 +34,19 @@ 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;
+        unsafe fn rust_process_wait(pid: c_int) -> c_int;
     }
 }
 
+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,13 +265,13 @@ 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);
@@ -248,6 +279,7 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program {
 
     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);
         }
     }
 
@@ -344,7 +377,8 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program {
     }
 
     let repr = ProgRepr {
-        pid: pid,
+        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;
@@ -466,17 +503,27 @@ pub fn readclose(fd: c_int) -> ~str {
     }
 }
 
-/// Waits for a process to exit and returns the exit code
+/**
+ * Waits for a process to exit and returns the exit code, failing
+ * if there is no process with the specified id.
+ */
 pub fn waitpid(pid: pid_t) -> int {
     return waitpid_os(pid);
 
     #[cfg(windows)]
     fn waitpid_os(pid: pid_t) -> int {
-        os::waitpid(pid) as int
+        let status = unsafe { rustrt::rust_process_wait(pid) };
+        if status < 0 {
+            fail!(fmt!("failure in rust_process_wait: %s", os::last_os_error()));
+        }
+        return status as int;
     }
 
     #[cfg(unix)]
     fn waitpid_os(pid: pid_t) -> int {
+
+        use libc::funcs::posix01::wait::*;
+
         #[cfg(target_os = "linux")]
         #[cfg(target_os = "android")]
         fn WIFEXITED(status: i32) -> bool {
@@ -501,7 +548,11 @@ pub fn waitpid(pid: pid_t) -> int {
             status >> 8i32
         }
 
-        let status = os::waitpid(pid);
+        let mut status = 0 as c_int;
+        if unsafe { waitpid(pid, &mut status, 0) } == -1 {
+            fail!(fmt!("failure in waitpid: %s", os::last_os_error()));
+        }
+
         return if WIFEXITED(status) {
             WEXITSTATUS(status) as int
         } else {
@@ -547,7 +598,7 @@ mod tests {
         writeclose(pipe_in.out, copy expected);
         let actual = readclose(pipe_out.in);
         readclose(pipe_err.in);
-        os::waitpid(pid);
+        run::waitpid(pid);
 
         debug!(copy expected);
         debug!(copy actual);
@@ -564,6 +615,13 @@ mod tests {
     }
 
     #[test]
+    #[should_fail]
+    #[ignore(cfg(windows))]
+    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", []);
         p.destroy(); // this shouldn't crash (and nor should the destructor)
diff --git a/src/rt/rust_run_program.cpp b/src/rt/rust_run_program.cpp
index 0e8d1d09ee9..cf4beed1a00 100644
--- 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;
 }