Skip to content

Commit bcaaffb

Browse files
borsbors
committed
auto merge of #13080 : alexcrichton/rust/possible-osx-deadlock, r=brson
The OSX bots have been deadlocking recently in the rustdoc tests. I have only been able to rarely reproduce the deadlock on my local setup. When reproduced, it looks like the child process is spinning on the malloc mutex, which I presume is locked with no other threads to unlock it. I'm not convinced that this is what's happening, because OSX should protect against this with pthread_atfork by default. Regardless, running as little code as possible in the child after fork() is normally a good idea anyway, so this commit moves all allocation to the parent process to run before the child executes. After running 6k iterations of rustdoc tests, this deadlocked twice before, and after 20k iterations afterwards, it never deadlocked. I draw the conclusion that this is either sweeping the bug under the rug, or it did indeed fix the underlying problem.
2 parents e06348e + 4fdff3e commit bcaaffb

File tree

1 file changed

+105
-107
lines changed

1 file changed

+105
-107
lines changed

‎src/libnative/io/process.rs‎

Lines changed: 105 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -446,132 +446,129 @@ fn spawn_process_os(config: p::ProcessConfig,
446446
assert_eq!(ret, 0);
447447
}
448448

449-
let pipe = os::pipe();
450-
let mut input = file::FileDesc::new(pipe.input, true);
451-
let mut output = file::FileDesc::new(pipe.out, true);
449+
let dirp = dir.map(|p| p.to_c_str());
450+
let dirp = dirp.as_ref().map(|c| c.with_ref(|p| p)).unwrap_or(ptr::null());
451+
452+
with_envp(env, proc(envp) {
453+
with_argv(config.program, config.args, proc(argv) unsafe {
454+
let pipe = os::pipe();
455+
let mut input = file::FileDesc::new(pipe.input, true);
456+
let mut output = file::FileDesc::new(pipe.out, true);
457+
458+
set_cloexec(output.fd());
459+
460+
let pid = fork();
461+
if pid < 0 {
462+
fail!("failure in fork: {}", os::last_os_error());
463+
} else if pid > 0 {
464+
drop(output);
465+
let mut bytes = [0, ..4];
466+
return match input.inner_read(bytes) {
467+
Ok(4) => {
468+
let errno = (bytes[0] << 24) as i32 |
469+
(bytes[1] << 16) as i32 |
470+
(bytes[2] << 8) as i32 |
471+
(bytes[3] << 0) as i32;
472+
Err(super::translate_error(errno, false))
473+
}
474+
Err(e) => {
475+
assert!(e.kind == io::BrokenPipe ||
476+
e.kind == io::EndOfFile,
477+
"unexpected error: {}", e);
478+
Ok(SpawnProcessResult {
479+
pid: pid,
480+
handle: ptr::null()
481+
})
482+
}
483+
Ok(..) => fail!("short read on the cloexec pipe"),
484+
};
485+
}
486+
drop(input);
487+
488+
fn fail(output: &mut file::FileDesc) -> ! {
489+
let errno = os::errno();
490+
let bytes = [
491+
(errno << 24) as u8,
492+
(errno << 16) as u8,
493+
(errno << 8) as u8,
494+
(errno << 0) as u8,
495+
];
496+
assert!(output.inner_write(bytes).is_ok());
497+
unsafe { libc::_exit(1) }
498+
}
452499

453-
unsafe { set_cloexec(output.fd()) };
500+
rustrt::rust_unset_sigprocmask();
454501

455-
unsafe {
456-
let pid = fork();
457-
if pid < 0 {
458-
fail!("failure in fork: {}", os::last_os_error());
459-
} else if pid > 0 {
460-
drop(output);
461-
let mut bytes = [0, ..4];
462-
return match input.inner_read(bytes) {
463-
Ok(4) => {
464-
let errno = (bytes[0] << 24) as i32 |
465-
(bytes[1] << 16) as i32 |
466-
(bytes[2] << 8) as i32 |
467-
(bytes[3] << 0) as i32;
468-
Err(super::translate_error(errno, false))
469-
}
470-
Err(e) => {
471-
assert!(e.kind == io::BrokenPipe ||
472-
e.kind == io::EndOfFile,
473-
"unexpected error: {}", e);
474-
Ok(SpawnProcessResult {
475-
pid: pid,
476-
handle: ptr::null()
477-
})
502+
if in_fd == -1 {
503+
let _ = libc::close(libc::STDIN_FILENO);
504+
} else if retry(|| dup2(in_fd, 0)) == -1 {
505+
fail(&mut output);
506+
}
507+
if out_fd == -1 {
508+
let _ = libc::close(libc::STDOUT_FILENO);
509+
} else if retry(|| dup2(out_fd, 1)) == -1 {
510+
fail(&mut output);
511+
}
512+
if err_fd == -1 {
513+
let _ = libc::close(libc::STDERR_FILENO);
514+
} else if retry(|| dup2(err_fd, 2)) == -1 {
515+
fail(&mut output);
516+
}
517+
// close all other fds
518+
for fd in range(3, getdtablesize()).rev() {
519+
if fd != output.fd() {
520+
let _ = close(fd as c_int);
478521
}
479-
Ok(..) => fail!("short read on the cloexec pipe"),
480-
};
481-
}
482-
drop(input);
483-
484-
fn fail(output: &mut file::FileDesc) -> ! {
485-
let errno = os::errno();
486-
let bytes = [
487-
(errno << 24) as u8,
488-
(errno << 16) as u8,
489-
(errno << 8) as u8,
490-
(errno << 0) as u8,
491-
];
492-
assert!(output.inner_write(bytes).is_ok());
493-
unsafe { libc::_exit(1) }
494-
}
495-
496-
rustrt::rust_unset_sigprocmask();
497-
498-
if in_fd == -1 {
499-
let _ = libc::close(libc::STDIN_FILENO);
500-
} else if retry(|| dup2(in_fd, 0)) == -1 {
501-
fail(&mut output);
502-
}
503-
if out_fd == -1 {
504-
let _ = libc::close(libc::STDOUT_FILENO);
505-
} else if retry(|| dup2(out_fd, 1)) == -1 {
506-
fail(&mut output);
507-
}
508-
if err_fd == -1 {
509-
let _ = libc::close(libc::STDERR_FILENO);
510-
} else if retry(|| dup2(err_fd, 2)) == -1 {
511-
fail(&mut output);
512-
}
513-
// close all other fds
514-
for fd in range(3, getdtablesize()).rev() {
515-
if fd != output.fd() {
516-
let _ = close(fd as c_int);
517522
}
518-
}
519523

520-
match config.gid {
521-
Some(u) => {
522-
if libc::setgid(u as libc::gid_t) != 0 {
523-
fail(&mut output);
524+
match config.gid {
525+
Some(u) => {
526+
if libc::setgid(u as libc::gid_t) != 0 {
527+
fail(&mut output);
528+
}
524529
}
530+
None => {}
525531
}
526-
None => {}
527-
}
528-
match config.uid {
529-
Some(u) => {
530-
// When dropping privileges from root, the `setgroups` call will
531-
// remove any extraneous groups. If we don't call this, then
532-
// even though our uid has dropped, we may still have groups
533-
// that enable us to do super-user things. This will fail if we
534-
// aren't root, so don't bother checking the return value, this
535-
// is just done as an optimistic privilege dropping function.
536-
extern {
537-
fn setgroups(ngroups: libc::c_int,
538-
ptr: *libc::c_void) -> libc::c_int;
539-
}
540-
let _ = setgroups(0, 0 as *libc::c_void);
532+
match config.uid {
533+
Some(u) => {
534+
// When dropping privileges from root, the `setgroups` call will
535+
// remove any extraneous groups. If we don't call this, then
536+
// even though our uid has dropped, we may still have groups
537+
// that enable us to do super-user things. This will fail if we
538+
// aren't root, so don't bother checking the return value, this
539+
// is just done as an optimistic privilege dropping function.
540+
extern {
541+
fn setgroups(ngroups: libc::c_int,
542+
ptr: *libc::c_void) -> libc::c_int;
543+
}
544+
let _ = setgroups(0, 0 as *libc::c_void);
541545

542-
if libc::setuid(u as libc::uid_t) != 0 {
543-
fail(&mut output);
546+
if libc::setuid(u as libc::uid_t) != 0 {
547+
fail(&mut output);
548+
}
544549
}
550+
None => {}
551+
}
552+
if config.detach {
553+
// Don't check the error of setsid because it fails if we're the
554+
// process leader already. We just forked so it shouldn't return
555+
// error, but ignore it anyway.
556+
let _ = libc::setsid();
545557
}
546-
None => {}
547-
}
548-
if config.detach {
549-
// Don't check the error of setsid because it fails if we're the
550-
// process leader already. We just forked so it shouldn't return
551-
// error, but ignore it anyway.
552-
let _ = libc::setsid();
553-
}
554-
555-
with_dirp(dir, |dirp| {
556558
if !dirp.is_null() && chdir(dirp) == -1 {
557559
fail(&mut output);
558560
}
559-
});
560-
561-
with_envp(env, |envp| {
562561
if !envp.is_null() {
563562
set_environ(envp);
564563
}
565-
with_argv(config.program, config.args, |argv| {
566-
let _ = execvp(*argv, argv);
567-
fail(&mut output);
568-
})
564+
let _ = execvp(*argv, argv);
565+
fail(&mut output);
569566
})
570-
}
567+
})
571568
}
572569

573570
#[cfg(unix)]
574-
fn with_argv<T>(prog: &str, args: &[~str], cb: |**libc::c_char| -> T) -> T {
571+
fn with_argv<T>(prog: &str, args: &[~str], cb: proc:(**libc::c_char) -> T) -> T {
575572
use std::slice;
576573

577574
// We can't directly convert `str`s into `*char`s, as someone needs to hold
@@ -597,7 +594,7 @@ fn with_argv<T>(prog: &str, args: &[~str], cb: |**libc::c_char| -> T) -> T {
597594
}
598595

599596
#[cfg(unix)]
600-
fn with_envp<T>(env: Option<~[(~str, ~str)]>, cb: |*c_void| -> T) -> T {
597+
fn with_envp<T>(env: Option<~[(~str, ~str)]>, cb: proc:(*c_void) -> T) -> T {
601598
use std::slice;
602599

603600
// On posixy systems we can pass a char** for envp, which is a
@@ -645,6 +642,7 @@ fn with_envp<T>(env: Option<~[(~str, ~str)]>, cb: |*mut c_void| -> T) -> T {
645642
}
646643
}
647644

645+
#[cfg(windows)]
648646
fn with_dirp<T>(d: Option<&Path>, cb: |*libc::c_char| -> T) -> T {
649647
match d {
650648
Some(dir) => dir.with_c_str(|buf| cb(buf)),

0 commit comments

Comments
 (0)