Avoid memory leaks when destroying TLS values on MacOS X

src/libstd/panicking.rs

@@ -12,19 +12,25 @@ use prelude::v1::*;
 use io::prelude::*;
 
 use any::Any;
+use cell::Cell;
 use cell::RefCell;
+use intrinsics;
 use sys::stdio::Stderr;
 use sys_common::backtrace;
 use sys_common::thread_info;
-use sys_common::unwind;
+use sys_common::util;
+use thread::LocalKeyState;
+
+thread_local! { pub static PANIC_COUNT: Cell<usize> = Cell::new(0) }
 
 thread_local! {
     pub static LOCAL_STDERR: RefCell<Option<Box<Write + Send>>> = {
         RefCell::new(None)
     }
 }
 
-pub fn on_panic(obj: &(Any+Send), file: &'static str, line: u32) {
+fn log_panic(obj: &(Any+Send), file: &'static str, line: u32,
+             log_backtrace: bool) {
     let msg = match obj.downcast_ref::<&'static str>() {
         Some(s) => *s,
         None => match obj.downcast_ref::<String>() {
@@ -35,37 +41,63 @@ pub fn on_panic(obj: &(Any+Send), file: &'static str, line: u32) {
     let mut err = Stderr::new().ok();
     let thread = thread_info::current_thread();
     let name = thread.as_ref().and_then(|t| t.name()).unwrap_or("<unnamed>");
-    let prev = LOCAL_STDERR.with(|s| s.borrow_mut().take());
+
+    let write = |err: &mut ::io::Write| {
+        let _ = writeln!(err, "thread '{}' panicked at '{}', {}:{}",
+                         name, msg, file, line);
+        if log_backtrace {
+            let _ = backtrace::write(err);
+        }
+    };
+
+    let prev = if LOCAL_STDERR.state() == LocalKeyState::Destroyed {
+        None
+    } else {
+        LOCAL_STDERR.with(|s| s.borrow_mut().take())
+    };
     match (prev, err.as_mut()) {
         (Some(mut stderr), _) => {
-            // FIXME: what to do when the thread printing panics?
-            let _ = writeln!(stderr,
-                             "thread '{}' panicked at '{}', {}:{}\n",
-                             name, msg, file, line);
-            if backtrace::log_enabled() {
-                let _ = backtrace::write(&mut *stderr);
-            }
+            write(&mut *stderr);
             let mut s = Some(stderr);
             LOCAL_STDERR.with(|slot| {
                 *slot.borrow_mut() = s.take();
             });
         }
-        (None, Some(ref mut err)) => {
-            let _ = writeln!(err, "thread '{}' panicked at '{}', {}:{}",
-                             name, msg, file, line);
-            if backtrace::log_enabled() {
-                let _ = backtrace::write(err);
-            }
-        }
+        (None, Some(ref mut err)) => { write(err) }
         _ => {}
     }
+}
 
-    // If this is a double panic, make sure that we printed a backtrace
-    // for this panic.
-    match err {
-        Some(ref mut err) if unwind::panicking() && !backtrace::log_enabled() => {
-            let _ = backtrace::write(err);
-        }
-        _ => {}
+pub fn on_panic(obj: &(Any+Send), file: &'static str, line: u32) {
+    let panics = PANIC_COUNT.with(|s| {
+        let count = s.get() + 1;
+        s.set(count);
+        count
+    });
+
+    // If this is the third nested call, on_panic triggered the last panic,
+    // otherwise the double-panic check would have aborted the process.
+    // Even if it is likely that on_panic was unable to log the backtrace,
+    // abort immediately to avoid infinite recursion, so that attaching a
+    // debugger provides a useable stacktrace.
+    if panics >= 3 {
+        util::dumb_print(format_args!("thread panicked while processing \
+                                       panic. aborting."));
+        unsafe { intrinsics::abort() }
+    }
+
+    // If this is a double panic, make sure that we print a backtrace
+    // for this panic. Otherwise only print it if logging is enabled.
+    let log_backtrace = panics >= 2 || backtrace::log_enabled();
+    log_panic(obj, file, line, log_backtrace);
+
+    if panics >= 2 {
+        // If a thread panics while it's already unwinding then we
+        // have limited options. Currently our preference is to
+        // just abort. In the future we may consider resuming
+        // unwinding or otherwise exiting the thread cleanly.
+        util::dumb_print(format_args!("thread panicked while panicking. \
+                                       aborting."));
+        unsafe { intrinsics::abort() }
     }
 }

src/libstd/sys/common/unwind/mod.rs

@@ -64,9 +64,8 @@ use prelude::v1::*;
 
 use any::Any;
 use boxed;
-use cell::Cell;
 use cmp;
-use panicking;
+use panicking::{self,PANIC_COUNT};
 use fmt;
 use intrinsics;
 use mem;
@@ -92,8 +91,6 @@ pub mod imp;
 #[path = "gcc.rs"] #[doc(hidden)]
 pub mod imp;
 
-thread_local! { static PANICKING: Cell<bool> = Cell::new(false) }
-
 /// Invoke a closure, capturing the cause of panic if one occurs.
 ///
 /// This function will return `Ok(())` if the closure did not panic, and will
@@ -131,9 +128,9 @@ pub unsafe fn try<F: FnOnce()>(f: F) -> Result<(), Box<Any + Send>> {
     // care of exposing correctly.
     unsafe fn inner_try(f: fn(*mut u8), data: *mut u8)
                         -> Result<(), Box<Any + Send>> {
-        PANICKING.with(|s| {
+        PANIC_COUNT.with(|s| {
             let prev = s.get();
-            s.set(false);
+            s.set(0);
             let ep = intrinsics::try(f, data);
             s.set(prev);
             if ep.is_null() {
@@ -161,7 +158,7 @@ pub unsafe fn try<F: FnOnce()>(f: F) -> Result<(), Box<Any + Send>> {
 
 /// Determines whether the current thread is unwinding because of panic.
 pub fn panicking() -> bool {
-    PANICKING.with(|s| s.get())
+    PANIC_COUNT.with(|s| s.get() != 0)
 }
 
 // An uninlined, unmangled function upon which to slap yer breakpoints
@@ -234,17 +231,6 @@ fn begin_unwind_inner(msg: Box<Any + Send>,
     // First, invoke the default panic handler.
     panicking::on_panic(&*msg, file, line);
 
-    if panicking() {
-        // If a thread panics while it's already unwinding then we
-        // have limited options. Currently our preference is to
-        // just abort. In the future we may consider resuming
-        // unwinding or otherwise exiting the thread cleanly.
-        super::util::dumb_print(format_args!("thread panicked while panicking. \
-                                              aborting."));
-        unsafe { intrinsics::abort() }
-    }
-    PANICKING.with(|s| s.set(true));
-
     // Finally, perform the unwinding.
     rust_panic(msg);
 }

src/libstd/thread/local.rs

@@ -277,13 +277,14 @@ mod imp {
     use intrinsics;
     use ptr;
 
+    thread_local!(static DTOR_RUNNING: Cell<bool> = Cell::new(false));
+
     pub struct Key<T> {
         inner: UnsafeCell<Option<T>>,
 
         // Metadata to keep track of the state of the destructor. Remember that
         // these variables are thread-local, not global.
         dtor_registered: Cell<bool>,
-        dtor_running: Cell<bool>,
     }
 
     unsafe impl<T> ::marker::Sync for Key<T> { }
@@ -293,13 +294,14 @@ mod imp {
             Key {
                 inner: UnsafeCell::new(None),
                 dtor_registered: Cell::new(false),
-                dtor_running: Cell::new(false)
             }
         }
 
         pub unsafe fn get(&'static self) -> Option<&'static UnsafeCell<Option<T>>> {
-            if intrinsics::needs_drop::<T>() && self.dtor_running.get() {
-                return None
+            if intrinsics::needs_drop::<T>() {
+                if DTOR_RUNNING.with(|running| { running.get() }) {
+                    return None
+                }
             }
             self.register_dtor();
             Some(&self.inner)
@@ -389,12 +391,12 @@ mod imp {
         _tlv_atexit(dtor, t);
     }
 
-    pub unsafe extern fn destroy_value<T>(ptr: *mut u8) {
+    unsafe extern fn destroy_value<T>(ptr: *mut u8) {
         let ptr = ptr as *mut Key<T>;
         // Right before we run the user destructor be sure to flag the
         // destructor as running for this thread so calls to `get` will return
         // `None`.
-        (*ptr).dtor_running.set(true);
+        DTOR_RUNNING.with(|running| running.set(true));
 
         // The OSX implementation of TLS apparently had an odd aspect to it
         // where the pointer we have may be overwritten while this destructor
@@ -470,7 +472,7 @@ mod imp {
         }
     }
 
-    pub unsafe extern fn destroy_value<T: 'static>(ptr: *mut u8) {
+    unsafe extern fn destroy_value<T: 'static>(ptr: *mut u8) {
         // The OS TLS ensures that this key contains a NULL value when this
         // destructor starts to run. We set it back to a sentinel value of 1 to
         // ensure that any future calls to `get` for this thread will return