From 91aeecf7e38e01a9e323683c39649e201a9f726c Mon Sep 17 00:00:00 2001 From: gareth Date: Mon, 22 Apr 2013 20:05:07 +0100 Subject: [PATCH 1/4] Fix issue #5976 - HANDLE leaks and undefined/bad behavour on windows. --- src/libcore/libc.rs | 1 + src/libcore/os.rs | 8 +++- src/libcore/run.rs | 91 +++++++++++++++++++++++++++---------- src/rt/rust_run_program.cpp | 59 ++++++++++++++++++------ 4 files changed, 119 insertions(+), 40 deletions(-) diff --git a/src/libcore/libc.rs b/src/libcore/libc.rs index 945d08323b4e5..44864630f9873 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 0e7131a86f394..0b73eb9dca3ee 100644 --- 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 index f9d7f4a229c42..4c5da132532d4 100644 --- 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,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); } } @@ -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 index 0e8d1d09ee99d..cf4beed1a00c6 100644 --- a/src/rt/rust_run_program.cpp +++ b/src/rt/rust_run_program.cpp @@ -15,6 +15,11 @@ #include #endif +struct RunProgramResult { + pid_t pid; + void* handle; +}; + #if defined(__WIN32__) #include @@ -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; } From 690120d6bc217338afc9b34cb458b086ab3f86d9 Mon Sep 17 00:00:00 2001 From: gareth Date: Mon, 22 Apr 2013 21:58:19 +0100 Subject: [PATCH 2/4] Remove os::waitpid because: - The return value meant different things on different platforms (on windows, it was the exit code, on unix it was the status information returned from waitpid). - It was undocumented. - There also exists run::waitpid, which does much the same thing but has a more consistent return value and also some documentation. --- src/libcore/os.rs | 26 -------------------------- src/libcore/run.rs | 23 +++++++++++++++++++---- 2 files changed, 19 insertions(+), 30 deletions(-) diff --git a/src/libcore/os.rs b/src/libcore/os.rs index 0b73eb9dca3ee..4c660507f15a0 100644 --- a/src/libcore/os.rs +++ b/src/libcore/os.rs @@ -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(pid: c_int) -> c_int; unsafe fn rust_set_exit_status(code: libc::intptr_t); } } @@ -352,31 +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 { - let status = rustrt::rust_process_wait(pid); - if status < 0 { - fail!(fmt!("failure in rust_process_wait: %s", last_os_error())); - } - return status; - } -} - -#[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 4c5da132532d4..701ab1c59df5c 100644 --- a/src/libcore/run.rs +++ b/src/libcore/run.rs @@ -36,6 +36,7 @@ pub mod rustrt { in_fd: c_int, out_fd: c_int, err_fd: c_int) -> run::RunProgramResult; + unsafe fn rust_process_wait(pid: c_int) -> c_int; } } @@ -503,17 +504,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 { @@ -538,7 +549,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 { @@ -584,7 +599,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); From ceeffa1ec662f4c1e214f3762ac116a21886078e Mon Sep 17 00:00:00 2001 From: gareth Date: Tue, 23 Apr 2013 20:02:47 +0100 Subject: [PATCH 3/4] Oops, the should_fail test needs to be ignored on windows. --- src/libcore/run.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libcore/run.rs b/src/libcore/run.rs index 701ab1c59df5c..2ca0449cb59f6 100644 --- a/src/libcore/run.rs +++ b/src/libcore/run.rs @@ -617,6 +617,7 @@ mod tests { #[test] #[should_fail] + #[ignore(cfg(windows))] fn waitpid_non_existant_pid() { run::waitpid(123456789); // assume that this pid doesn't exist } From 62befac51fadbdd64050d84c796e3952789870a1 Mon Sep 17 00:00:00 2001 From: gareth Date: Tue, 23 Apr 2013 21:22:19 +0100 Subject: [PATCH 4/4] Cleanup some mistakes made during rebasing/merging. --- src/libcore/os.rs | 2 +- src/libcore/run.rs | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libcore/os.rs b/src/libcore/os.rs index 4c660507f15a0..fa3ca4577c6d1 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::*; diff --git a/src/libcore/run.rs b/src/libcore/run.rs index 2ca0449cb59f6..2455954ebe128 100644 --- a/src/libcore/run.rs +++ b/src/libcore/run.rs @@ -18,7 +18,6 @@ use option::{Some, None}; use os; use prelude::*; use ptr; -use run; use str; use task; use vec; @@ -377,7 +376,7 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program { fn force_destroy(&mut self) { destroy_repr(&mut self.r, true); } } - let mut repr = ProgRepr { + let repr = ProgRepr { pid: res.pid, handle: res.handle, in_fd: pipe_input.out,