Skip to content

Commit

Permalink
std: Lift out Windows' CreateProcess lock a bit
Browse files Browse the repository at this point in the history
The function `CreateProcess` is not itself unsafe to call from many threads, the
article in question is pointing out that handles can be inherited by unintended
child processes. This is basically the same race as the standard Unix
open-then-set-cloexec race.

Since the intention of the lock is to protect children from inheriting
unintended handles, the lock is now lifted out to before the creation of the
child I/O handles (which will all be inheritable). This will ensure that we only
have one process in Rust at least creating inheritable handles at a time,
preventing unintended inheritance to children.
  • Loading branch information
alexcrichton committed Feb 10, 2016
1 parent b8bd8f3 commit 18f9a79
Showing 1 changed file with 20 additions and 13 deletions.
33 changes: 20 additions & 13 deletions src/libstd/sys/windows/process.rs
Expand Up @@ -159,14 +159,6 @@ impl Process {
si.cb = mem::size_of::<c::STARTUPINFO>() as c::DWORD;
si.dwFlags = c::STARTF_USESTDHANDLES;

let stdin = try!(in_handle.to_handle(c::STD_INPUT_HANDLE));
let stdout = try!(out_handle.to_handle(c::STD_OUTPUT_HANDLE));
let stderr = try!(err_handle.to_handle(c::STD_ERROR_HANDLE));

si.hStdInput = stdin.raw();
si.hStdOutput = stdout.raw();
si.hStdError = stderr.raw();

let program = program.as_ref().unwrap_or(&cfg.program);
let mut cmd_str = try!(make_command_line(program, &cfg.args));
cmd_str.push(0); // add null terminator
Expand All @@ -180,12 +172,27 @@ impl Process {
let (envp, _data) = try!(make_envp(cfg.env.as_ref()));
let (dirp, _data) = try!(make_dirp(cfg.cwd.as_ref()));
let mut pi = zeroed_process_information();
try!(unsafe {
// `CreateProcess` is racy!
// http://support.microsoft.com/kb/315939
static CREATE_PROCESS_LOCK: StaticMutex = StaticMutex::new();
let _lock = CREATE_PROCESS_LOCK.lock();

// Prepare all stdio handles to be inherited by the child. This
// currently involves duplicating any existing ones with the ability to
// be inherited by child processes. Note, however, that once an
// inheritable handle is created, *any* spawned child will inherit that
// handle. We only want our own child to inherit this handle, so we wrap
// the remaining portion of this spawn in a mutex.
//
// For more information, msdn also has an article about this race:
// http://support.microsoft.com/kb/315939
static CREATE_PROCESS_LOCK: StaticMutex = StaticMutex::new();
let _lock = CREATE_PROCESS_LOCK.lock();

let stdin = try!(in_handle.to_handle(c::STD_INPUT_HANDLE));
let stdout = try!(out_handle.to_handle(c::STD_OUTPUT_HANDLE));
let stderr = try!(err_handle.to_handle(c::STD_ERROR_HANDLE));
si.hStdInput = stdin.raw();
si.hStdOutput = stdout.raw();
si.hStdError = stderr.raw();

try!(unsafe {
cvt(c::CreateProcessW(ptr::null(),
cmd_str.as_mut_ptr(),
ptr::null_mut(),
Expand Down

0 comments on commit 18f9a79

Please sign in to comment.