From 78495d5082f51a2737619824548c9f2407b12a2b Mon Sep 17 00:00:00 2001 From: David Henningsson Date: Fri, 25 Mar 2016 05:46:45 +0100 Subject: [PATCH] Fix unsound behaviour with null characters in thread names (issue #32475) Previously, the thread name (&str) was converted to a CString in the new thread, but outside unwind::try, causing a panic to continue into FFI. This patch changes that behaviour, so that the panic instead happens in the parent thread (where panic infrastructure is properly set up), not the new thread. This could potentially be a breaking change for architectures who don't support thread names. Signed-off-by: David Henningsson --- src/libstd/sys/unix/thread.rs | 27 +++++++++++---------------- src/libstd/sys/windows/thread.rs | 2 +- src/libstd/thread/mod.rs | 21 ++++++++++++++++++--- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/libstd/sys/unix/thread.rs b/src/libstd/sys/unix/thread.rs index 793a2ecae89..6d966a0f694 100644 --- a/src/libstd/sys/unix/thread.rs +++ b/src/libstd/sys/unix/thread.rs @@ -13,7 +13,7 @@ use alloc::boxed::FnBox; use cmp; #[cfg(not(any(target_env = "newlib", target_os = "solaris")))] -use ffi::CString; +use ffi::CStr; use io; use libc; use mem; @@ -84,15 +84,12 @@ pub fn yield_now() { #[cfg(any(target_os = "linux", target_os = "android", target_os = "emscripten"))] - pub fn set_name(name: &str) { + pub fn set_name(name: &CStr) { const PR_SET_NAME: libc::c_int = 15; - let cname = CString::new(name).unwrap_or_else(|_| { - panic!("thread name may not contain interior null bytes") - }); // pthread wrapper only appeared in glibc 2.12, so we use syscall // directly. unsafe { - libc::prctl(PR_SET_NAME, cname.as_ptr() as libc::c_ulong, 0, 0, 0); + libc::prctl(PR_SET_NAME, name.as_ptr() as libc::c_ulong, 0, 0, 0); } } @@ -100,32 +97,30 @@ pub fn set_name(name: &str) { target_os = "dragonfly", target_os = "bitrig", target_os = "openbsd"))] - pub fn set_name(name: &str) { - let cname = CString::new(name).unwrap(); + pub fn set_name(name: &CStr) { unsafe { - libc::pthread_set_name_np(libc::pthread_self(), cname.as_ptr()); + libc::pthread_set_name_np(libc::pthread_self(), name.as_ptr()); } } #[cfg(any(target_os = "macos", target_os = "ios"))] - pub fn set_name(name: &str) { - let cname = CString::new(name).unwrap(); + pub fn set_name(name: &CStr) { unsafe { - libc::pthread_setname_np(cname.as_ptr()); + libc::pthread_setname_np(name.as_ptr()); } } #[cfg(target_os = "netbsd")] - pub fn set_name(name: &str) { + pub fn set_name(name: &CStr) { + use ffi::CString; let cname = CString::new(&b"%s"[..]).unwrap(); - let carg = CString::new(name).unwrap(); unsafe { libc::pthread_setname_np(libc::pthread_self(), cname.as_ptr(), - carg.as_ptr() as *mut libc::c_void); + name.as_ptr() as *mut libc::c_void); } } #[cfg(any(target_env = "newlib", target_os = "solaris"))] - pub fn set_name(_name: &str) { + pub fn set_name(_name: &CStr) { // Newlib and Illumos has no way to set a thread name. } diff --git a/src/libstd/sys/windows/thread.rs b/src/libstd/sys/windows/thread.rs index b18772c0c24..6908775e86f 100644 --- a/src/libstd/sys/windows/thread.rs +++ b/src/libstd/sys/windows/thread.rs @@ -54,7 +54,7 @@ extern "system" fn thread_start(main: *mut c_void) -> c::DWORD { } } - pub fn set_name(_name: &str) { + pub fn set_name(_name: &CStr) { // Windows threads are nameless // The names in MSVC debugger are obtained using a "magic" exception, // which requires a use of MS C++ extensions. diff --git a/src/libstd/thread/mod.rs b/src/libstd/thread/mod.rs index f3139aaf98d..b3549dc1264 100644 --- a/src/libstd/thread/mod.rs +++ b/src/libstd/thread/mod.rs @@ -166,6 +166,8 @@ use cell::UnsafeCell; use fmt; use io; +use str; +use ffi::{CStr, CString}; use sync::{Mutex, Condvar, Arc}; use sys::thread as imp; use sys_common::thread_info; @@ -267,7 +269,7 @@ pub fn spawn(self, f: F) -> io::Result> where let their_packet = my_packet.clone(); let main = move || { - if let Some(name) = their_thread.name() { + if let Some(name) = their_thread.cname() { imp::Thread::set_name(name); } unsafe { @@ -450,7 +452,7 @@ pub fn park_timeout(dur: Duration) { /// The internal representation of a `Thread` handle struct Inner { - name: Option, + name: Option, // Guaranteed to be UTF-8 lock: Mutex, // true when there is a buffered unpark cvar: Condvar, } @@ -465,9 +467,12 @@ pub struct Thread { impl Thread { // Used only internally to construct a thread object without spawning fn new(name: Option) -> Thread { + let cname = name.map(|n| CString::new(n).unwrap_or_else(|_| { + panic!("thread name may not contain interior null bytes") + })); Thread { inner: Arc::new(Inner { - name: name, + name: cname, lock: Mutex::new(false), cvar: Condvar::new(), }) @@ -489,6 +494,10 @@ pub fn unpark(&self) { /// Gets the thread's name. #[stable(feature = "rust1", since = "1.0.0")] pub fn name(&self) -> Option<&str> { + self.cname().map(|s| unsafe { str::from_utf8_unchecked(s.to_bytes()) } ) + } + + fn cname(&self) -> Option<&CStr> { self.inner.name.as_ref().map(|s| &**s) } } @@ -622,6 +631,12 @@ fn test_named_thread() { }).unwrap().join().unwrap(); } + #[test] + #[should_panic] + fn test_invalid_named_thread() { + let _ = Builder::new().name("ada l\0velace".to_string()).spawn(|| {}); + } + #[test] fn test_run_basic() { let (tx, rx) = channel(); -- 2.44.0