From e081c17805203b5e87073e2c766280a84b08e52c Mon Sep 17 00:00:00 2001 From: gareth Date: Sun, 31 Mar 2013 21:24:58 +0100 Subject: [PATCH 1/4] Fix a bug where calling p.destroy() on the result of calling start_program(...) would cause a segfault when p went out of scope due to out_file/err_file being closed twice. --- src/libcore/run.rs | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/libcore/run.rs b/src/libcore/run.rs index 98564beeba901..c6d79bd2ecd6d 100644 --- a/src/libcore/run.rs +++ b/src/libcore/run.rs @@ -172,6 +172,14 @@ fn with_dirp(d: &Option<~str>, } } +/// helper function that closes non-NULL files and then makes them NULL +priv unsafe fn fclose_and_null(f: &mut *libc::FILE) { + if *f != 0 as *libc::FILE { + libc::fclose(*f); + *f = 0 as *libc::FILE; + } +} + /** * Spawns a process and waits for it to terminate * @@ -249,8 +257,8 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program { fn destroy_repr(r: &mut ProgRepr) { unsafe { finish_repr(&mut *r); - libc::fclose(r.out_file); - libc::fclose(r.err_file); + fclose_and_null(&mut r.out_file); + fclose_and_null(&mut r.err_file); } } struct ProgRes { @@ -507,6 +515,19 @@ mod tests { assert!(status == 1); } + #[test] + pub fn test_destroy_once() { + let mut p = run::start_program("echo", []); + p.destroy(); // this shouldn't crash (and nor should the destructor) + } + + #[test] + pub fn test_destroy_twice() { + let mut p = run::start_program("echo", []); + p.destroy(); // this shouldnt crash... + p.destroy(); // ...and nor should this (and nor should the destructor) + } + } // Local Variables: From 622bb6300f8b4e5a644088fe471e63b580b03453 Mon Sep 17 00:00:00 2001 From: gareth Date: Sun, 31 Mar 2013 21:30:33 +0100 Subject: [PATCH 2/4] Update doc-comments to reflect the current year and trait names now being capitalized. --- src/libcore/run.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libcore/run.rs b/src/libcore/run.rs index c6d79bd2ecd6d..1441da01460e7 100644 --- a/src/libcore/run.rs +++ b/src/libcore/run.rs @@ -1,4 +1,4 @@ -// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT // file at the top-level directory of this distribution and at // http://rust-lang.org/COPYRIGHT. // @@ -44,13 +44,13 @@ pub trait Program { /// Returns the process id of the program fn get_id(&mut self) -> pid_t; - /// Returns an io::writer that can be used to write to stdin + /// Returns an io::Writer that can be used to write to stdin fn input(&mut self) -> @io::Writer; - /// Returns an io::reader that can be used to read from stdout + /// Returns an io::Reader that can be used to read from stdout fn output(&mut self) -> @io::Reader; - /// Returns an io::reader that can be used to read from stderr + /// Returns an io::Reader that can be used to read from stderr fn err(&mut self) -> @io::Reader; /// Closes the handle to the child processes standard input @@ -200,9 +200,9 @@ pub fn run_program(prog: &str, args: &[~str]) -> int { } /** - * Spawns a process and returns a program + * Spawns a process and returns a Program * - * The returned value is a boxed class containing a object that can + * The returned value is a boxed class containing a object that can * be used for sending and receiving data over the standard file descriptors. * The class will ensure that file descriptors are closed properly. * From 483e95a35c9f3ab01666de4808134af26fded68c Mon Sep 17 00:00:00 2001 From: gareth Date: Sat, 6 Apr 2013 20:49:52 +0100 Subject: [PATCH 3/4] Change the behaviour of core::run::Program.destroy to forcibly terminate the program (as suggested in issue #5632) --- src/libcore/libc.rs | 16 +++++++++++ src/libcore/run.rs | 66 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/src/libcore/libc.rs b/src/libcore/libc.rs index 95d963a36e3aa..e17a6be3f1f95 100644 --- a/src/libcore/libc.rs +++ b/src/libcore/libc.rs @@ -863,6 +863,7 @@ pub mod consts { pub static F_TEST : int = 3; pub static F_TLOCK : int = 2; pub static F_ULOCK : int = 0; + pub static SIGKILL : int = 9; } pub mod posix01 { } @@ -930,6 +931,7 @@ pub mod consts { pub static F_TEST : int = 3; pub static F_TLOCK : int = 2; pub static F_ULOCK : int = 0; + pub static SIGKILL : int = 9; } pub mod posix01 { } @@ -998,6 +1000,7 @@ pub mod consts { pub static F_TEST : int = 3; pub static F_TLOCK : int = 2; pub static F_ULOCK : int = 0; + pub static SIGKILL : int = 9; } pub mod posix01 { } @@ -1482,6 +1485,17 @@ pub mod funcs { -> ssize_t; } } + + #[nolink] + #[abi = "cdecl"] + pub mod signal { + use libc::types::os::arch::c95::{c_int}; + use libc::types::os::arch::posix88::{pid_t}; + + pub extern { + unsafe fn kill(pid: pid_t, sig: c_int) -> c_int; + } + } } #[cfg(target_os = "linux")] @@ -1623,6 +1637,7 @@ pub mod funcs { pub mod extra { pub mod kernel32 { + use libc::types::os::arch::c95::{c_uint}; use libc::types::os::arch::extra::{BOOL, DWORD, HMODULE}; use libc::types::os::arch::extra::{LPCWSTR, LPWSTR, LPTCH}; use libc::types::os::arch::extra::{LPSECURITY_ATTRIBUTES}; @@ -1663,6 +1678,7 @@ pub mod funcs { findFileData: HANDLE) -> BOOL; unsafe fn FindClose(findFile: HANDLE) -> BOOL; + unsafe fn TerminateProcess(hProcess: HANDLE, uExitCode: c_uint) -> BOOL; } } diff --git a/src/libcore/run.rs b/src/libcore/run.rs index 1441da01460e7..e7602cff4928d 100644 --- a/src/libcore/run.rs +++ b/src/libcore/run.rs @@ -62,7 +62,10 @@ pub trait Program { */ fn finish(&mut self) -> int; - /// Closes open handles + /** + * Forcibly terminate the program. On Posix OSs SIGKILL will be sent + * to the process. On Win32 TerminateProcess(..) will be called. + */ fn destroy(&mut self); } @@ -248,19 +251,43 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program { r.in_fd = invalid_fd; } } + + fn close_repr_outputs(r: &mut ProgRepr) { + unsafe { + fclose_and_null(&mut r.out_file); + fclose_and_null(&mut r.err_file); + } + } + fn finish_repr(r: &mut ProgRepr) -> int { if r.finished { return 0; } r.finished = true; close_repr_input(&mut *r); return waitpid(r.pid); } + fn destroy_repr(r: &mut ProgRepr) { - unsafe { - finish_repr(&mut *r); - fclose_and_null(&mut r.out_file); - fclose_and_null(&mut r.err_file); + killpid(r.pid); + finish_repr(&mut *r); + close_repr_outputs(&mut *r); + + #[cfg(windows)] + fn killpid(pid: pid_t) { + unsafe { + libc::funcs::extra::kernel32::TerminateProcess( + cast::transmute(pid), 1); + } + } + + #[cfg(unix)] + fn killpid(pid: pid_t) { + unsafe { + libc::funcs::posix88::signal::kill( + pid, libc::consts::os::posix88::SIGKILL as c_int); + } } } + struct ProgRes { r: ProgRepr, } @@ -268,8 +295,9 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program { impl Drop for ProgRes { fn finalize(&self) { unsafe { - // FIXME #4943: This is bad. - destroy_repr(cast::transmute(&self.r)); + // FIXME #4943: transmute is bad. + finish_repr(cast::transmute(&self.r)); + close_repr_outputs(cast::transmute(&self.r)); } } } @@ -295,6 +323,7 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program { fn finish(&mut self) -> int { finish_repr(&mut self.r) } fn destroy(&mut self) { destroy_repr(&mut self.r); } } + let mut repr = ProgRepr { pid: pid, in_fd: pipe_input.out, @@ -466,8 +495,10 @@ pub fn waitpid(pid: pid_t) -> int { #[cfg(test)] mod tests { + use libc; use option::None; use os; + use path::Path; use run::{readclose, writeclose}; use run; @@ -528,6 +559,27 @@ mod tests { p.destroy(); // ...and nor should this (and nor should the destructor) } + #[test] + #[cfg(unix)] // there is no way to sleep on windows from inside libcore... + pub fn test_destroy_actually_kills() { + let path = Path("test/core-run-test-destroy-actually-kills.tmp"); + + os::remove_file(&path); + + let cmd = fmt!("sleep 5 && echo MurderDeathKill > %s", path.to_str()); + let mut p = run::start_program("sh", [~"-c", cmd]); + + p.destroy(); // destroy the program before it has a chance to echo its message + + unsafe { + // wait to ensure the program is really destroyed and not just waiting itself + libc::sleep(10); + } + + // the program should not have had chance to echo its message + assert!(!path.exists()); + } + } // Local Variables: From 995d44416b203b5b3ba619250ff8effdcf205049 Mon Sep 17 00:00:00 2001 From: gareth Date: Thu, 11 Apr 2013 21:51:39 +0100 Subject: [PATCH 4/4] Make destroy() send SIGTERM and add a new method called force_destroy() that sends SIGKILL - as suggested by @thestinger. --- src/libcore/libc.rs | 3 +++ src/libcore/run.rs | 53 +++++++++++++++++++++++++++++++++++---------- 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/src/libcore/libc.rs b/src/libcore/libc.rs index e17a6be3f1f95..02c72bcd2f0d0 100644 --- a/src/libcore/libc.rs +++ b/src/libcore/libc.rs @@ -864,6 +864,7 @@ pub mod consts { pub static F_TLOCK : int = 2; pub static F_ULOCK : int = 0; pub static SIGKILL : int = 9; + pub static SIGTERM : int = 15; } pub mod posix01 { } @@ -932,6 +933,7 @@ pub mod consts { pub static F_TLOCK : int = 2; pub static F_ULOCK : int = 0; pub static SIGKILL : int = 9; + pub static SIGTERM : int = 15; } pub mod posix01 { } @@ -1001,6 +1003,7 @@ pub mod consts { pub static F_TLOCK : int = 2; pub static F_ULOCK : int = 0; pub static SIGKILL : int = 9; + pub static SIGTERM : int = 15; } pub mod posix01 { } diff --git a/src/libcore/run.rs b/src/libcore/run.rs index e7602cff4928d..f6f4b9a397d81 100644 --- a/src/libcore/run.rs +++ b/src/libcore/run.rs @@ -63,10 +63,22 @@ pub trait Program { fn finish(&mut self) -> int; /** - * Forcibly terminate the program. On Posix OSs SIGKILL will be sent - * to the process. On Win32 TerminateProcess(..) will be called. + * Terminate the program, giving it a chance to clean itself up if + * this is supported by the operating system. + * + * On Posix OSs SIGTERM will be sent to the process. On Win32 + * TerminateProcess(..) will be called. */ fn destroy(&mut self); + + /** + * Terminate the program as soon as possible without giving it a + * chance to clean itself up. + * + * On Posix OSs SIGKILL will be sent to the process. On Win32 + * TerminateProcess(..) will be called. + */ + fn force_destroy(&mut self); } @@ -266,13 +278,13 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program { return waitpid(r.pid); } - fn destroy_repr(r: &mut ProgRepr) { - killpid(r.pid); + fn destroy_repr(r: &mut ProgRepr, force: bool) { + killpid(r.pid, force); finish_repr(&mut *r); close_repr_outputs(&mut *r); #[cfg(windows)] - fn killpid(pid: pid_t) { + fn killpid(pid: pid_t, _force: bool) { unsafe { libc::funcs::extra::kernel32::TerminateProcess( cast::transmute(pid), 1); @@ -280,10 +292,16 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program { } #[cfg(unix)] - fn killpid(pid: pid_t) { + fn killpid(pid: pid_t, force: bool) { + + let signal = if force { + libc::consts::os::posix88::SIGKILL + } else { + libc::consts::os::posix88::SIGTERM + }; + unsafe { - libc::funcs::posix88::signal::kill( - pid, libc::consts::os::posix88::SIGKILL as c_int); + libc::funcs::posix88::signal::kill(pid, signal as c_int); } } } @@ -321,7 +339,8 @@ pub fn start_program(prog: &str, args: &[~str]) -> @Program { } fn close_input(&mut self) { close_repr_input(&mut self.r); } fn finish(&mut self) -> int { finish_repr(&mut self.r) } - fn destroy(&mut self) { destroy_repr(&mut self.r); } + fn destroy(&mut self) { destroy_repr(&mut self.r, false); } + fn force_destroy(&mut self) { destroy_repr(&mut self.r, true); } } let mut repr = ProgRepr { @@ -559,10 +578,9 @@ mod tests { p.destroy(); // ...and nor should this (and nor should the destructor) } - #[test] #[cfg(unix)] // there is no way to sleep on windows from inside libcore... - pub fn test_destroy_actually_kills() { - let path = Path("test/core-run-test-destroy-actually-kills.tmp"); + pub fn test_destroy_actually_kills(force: bool) { + let path = Path(fmt!("test/core-run-test-destroy-actually-kills-%?.tmp", force)); os::remove_file(&path); @@ -580,6 +598,17 @@ mod tests { assert!(!path.exists()); } + #[test] + #[cfg(unix)] + pub fn test_unforced_destroy_actually_kills() { + test_destroy_actually_kills(false); + } + + #[test] + #[cfg(unix)] + pub fn test_forced_destroy_actually_kills() { + test_destroy_actually_kills(true); + } } // Local Variables: