From a605e2f498ce9383d2b9ee44a796e5316c7a5e6c Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Thu, 18 Jul 2024 13:42:11 +0000 Subject: [PATCH 1/4] Safely enforce thread name requirements --- library/std/src/thread/mod.rs | 39 +++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index c8ee365392f85..87d5c2f742c41 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -161,7 +161,7 @@ mod tests; use crate::any::Any; use crate::cell::{OnceCell, UnsafeCell}; use crate::env; -use crate::ffi::{CStr, CString}; +use crate::ffi::CStr; use crate::fmt; use crate::io; use crate::marker::PhantomData; @@ -487,11 +487,7 @@ impl Builder { amt }); - let my_thread = name.map_or_else(Thread::new_unnamed, |name| unsafe { - Thread::new( - CString::new(name).expect("thread name may not contain interior null bytes"), - ) - }); + let my_thread = name.map_or_else(Thread::new_unnamed, |name| Thread::new(name.into())); let their_thread = my_thread.clone(); let my_packet: Arc> = Arc::new(Packet { @@ -1273,10 +1269,34 @@ impl ThreadId { /// The internal representation of a `Thread`'s name. enum ThreadName { Main, - Other(CString), + Other(ThreadNameString), Unnamed, } +// This module ensures private fields are kept private, which is necessary to enforce the safety requirements. +mod thread_name_string { + use crate::ffi::{CStr, CString}; + + /// Like a `String` it's guaranteed UTF-8 and like a `CString` it's null terminated. + pub(crate) struct ThreadNameString { + inner: CString, + } + impl core::ops::Deref for ThreadNameString { + type Target = CStr; + fn deref(&self) -> &CStr { + &self.inner + } + } + impl From for ThreadNameString { + fn from(s: String) -> Self { + Self { + inner: CString::new(s).expect("thread name may not contain interior null bytes"), + } + } + } +} +pub(crate) use thread_name_string::ThreadNameString; + /// The internal representation of a `Thread` handle struct Inner { name: ThreadName, // Guaranteed to be UTF-8 @@ -1316,10 +1336,7 @@ pub struct Thread { impl Thread { /// Used only internally to construct a thread object without spawning. - /// - /// # Safety - /// `name` must be valid UTF-8. - pub(crate) unsafe fn new(name: CString) -> Thread { + pub(crate) fn new(name: ThreadNameString) -> Thread { unsafe { Self::new_inner(ThreadName::Other(name)) } } From 939ee383043dce00c108c22f1e933f1e052f3f3d Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Thu, 18 Jul 2024 17:33:52 +0000 Subject: [PATCH 2/4] Make `Thread::new_inner` a safe function --- library/std/src/thread/mod.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 87d5c2f742c41..13b02cd8d3abe 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -1337,21 +1337,19 @@ pub struct Thread { impl Thread { /// Used only internally to construct a thread object without spawning. pub(crate) fn new(name: ThreadNameString) -> Thread { - unsafe { Self::new_inner(ThreadName::Other(name)) } + Self::new_inner(ThreadName::Other(name)) } pub(crate) fn new_unnamed() -> Thread { - unsafe { Self::new_inner(ThreadName::Unnamed) } + Self::new_inner(ThreadName::Unnamed) } // Used in runtime to construct main thread pub(crate) fn new_main() -> Thread { - unsafe { Self::new_inner(ThreadName::Main) } + Self::new_inner(ThreadName::Main) } - /// # Safety - /// If `name` is `ThreadName::Other(_)`, the contained string must be valid UTF-8. - unsafe fn new_inner(name: ThreadName) -> Thread { + fn new_inner(name: ThreadName) -> Thread { // We have to use `unsafe` here to construct the `Parker` in-place, // which is required for the UNIX implementation. // From 8e4a9205e9fc0a86695799042e6fb70756bbfea2 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Thu, 18 Jul 2024 18:10:36 +0000 Subject: [PATCH 3/4] Style change --- library/std/src/thread/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 13b02cd8d3abe..f3d20026c27b1 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -487,7 +487,7 @@ impl Builder { amt }); - let my_thread = name.map_or_else(Thread::new_unnamed, |name| Thread::new(name.into())); + let my_thread = name.map_or_else(Thread::new_unnamed, Thread::new); let their_thread = my_thread.clone(); let my_packet: Arc> = Arc::new(Packet { @@ -1336,8 +1336,8 @@ pub struct Thread { impl Thread { /// Used only internally to construct a thread object without spawning. - pub(crate) fn new(name: ThreadNameString) -> Thread { - Self::new_inner(ThreadName::Other(name)) + pub(crate) fn new(name: String) -> Thread { + Self::new_inner(ThreadName::Other(name.into())) } pub(crate) fn new_unnamed() -> Thread { From 9432955a01b989d3f4122887875643ef8dbea589 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Thu, 18 Jul 2024 18:13:11 +0000 Subject: [PATCH 4/4] Move ThreadName conversions to &cstr/&str --- library/std/src/thread/mod.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index f3d20026c27b1..0c908a5adae4e 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -1275,7 +1275,9 @@ enum ThreadName { // This module ensures private fields are kept private, which is necessary to enforce the safety requirements. mod thread_name_string { + use super::ThreadName; use crate::ffi::{CStr, CString}; + use core::str; /// Like a `String` it's guaranteed UTF-8 and like a `CString` it's null terminated. pub(crate) struct ThreadNameString { @@ -1294,6 +1296,21 @@ mod thread_name_string { } } } + impl ThreadName { + pub fn as_cstr(&self) -> Option<&CStr> { + match self { + ThreadName::Main => Some(c"main"), + ThreadName::Other(other) => Some(other), + ThreadName::Unnamed => None, + } + } + + pub fn as_str(&self) -> Option<&str> { + // SAFETY: `as_cstr` can only return `Some` for a fixed CStr or a `ThreadNameString`, + // which is guaranteed to be UTF-8. + self.as_cstr().map(|s| unsafe { str::from_utf8_unchecked(s.to_bytes()) }) + } + } } pub(crate) use thread_name_string::ThreadNameString; @@ -1472,15 +1489,11 @@ impl Thread { #[stable(feature = "rust1", since = "1.0.0")] #[must_use] pub fn name(&self) -> Option<&str> { - self.cname().map(|s| unsafe { str::from_utf8_unchecked(s.to_bytes()) }) + self.inner.name.as_str() } fn cname(&self) -> Option<&CStr> { - match &self.inner.name { - ThreadName::Main => Some(c"main"), - ThreadName::Other(other) => Some(&other), - ThreadName::Unnamed => None, - } + self.inner.name.as_cstr() } }