Merge pull request #4004 from YohDeadfall/thread-name-ice-fix

Get/set thread name shims return errors for invalid handles

Author: RalfJung

src/concurrency/thread.rs

@@ -1,7 +1,6 @@
 //! Implements threads.
 
 use std::mem;
-use std::num::TryFromIntError;
 use std::sync::atomic::Ordering::Relaxed;
 use std::task::Poll;
 use std::time::{Duration, SystemTime};
@@ -127,26 +126,6 @@ impl Idx for ThreadId {
     }
 }
 
-impl TryFrom<u64> for ThreadId {
-    type Error = TryFromIntError;
-    fn try_from(id: u64) -> Result<Self, Self::Error> {
-        u32::try_from(id).map(Self)
-    }
-}
-
-impl TryFrom<i128> for ThreadId {
-    type Error = TryFromIntError;
-    fn try_from(id: i128) -> Result<Self, Self::Error> {
-        u32::try_from(id).map(Self)
-    }
-}
-
-impl From<u32> for ThreadId {
-    fn from(id: u32) -> Self {
-        Self(id)
-    }
-}
-
 impl From<ThreadId> for u64 {
     fn from(t: ThreadId) -> Self {
         t.0.into()
@@ -448,6 +427,10 @@ pub enum TimeoutAnchor {
     Absolute,
 }
 
+/// An error signaling that the requested thread doesn't exist.
+#[derive(Debug, Copy, Clone)]
+pub struct ThreadNotFound;
+
 /// A set of threads.
 #[derive(Debug)]
 pub struct ThreadManager<'tcx> {
@@ -509,6 +492,16 @@ impl<'tcx> ThreadManager<'tcx> {
         }
     }
 
+    pub fn thread_id_try_from(&self, id: impl TryInto<u32>) -> Result<ThreadId, ThreadNotFound> {
+        if let Ok(id) = id.try_into()
+            && usize::try_from(id).is_ok_and(|id| id < self.threads.len())
+        {
+            Ok(ThreadId(id))
+        } else {
+            Err(ThreadNotFound)
+        }
+    }
+
     /// Check if we have an allocation for the given thread local static for the
     /// active thread.
     fn get_thread_local_alloc_id(&self, def_id: DefId) -> Option<StrictPointer> {
@@ -534,6 +527,7 @@ impl<'tcx> ThreadManager<'tcx> {
     ) -> &mut Vec<Frame<'tcx, Provenance, FrameExtra<'tcx>>> {
         &mut self.threads[self.active_thread].stack
     }
+
     pub fn all_stacks(
         &self,
     ) -> impl Iterator<Item = (ThreadId, &[Frame<'tcx, Provenance, FrameExtra<'tcx>>])> {
@@ -868,6 +862,11 @@ trait EvalContextPrivExt<'tcx>: MiriInterpCxExt<'tcx> {
 // Public interface to thread management.
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
+    #[inline]
+    fn thread_id_try_from(&self, id: impl TryInto<u32>) -> Result<ThreadId, ThreadNotFound> {
+        self.eval_context_ref().machine.threads.thread_id_try_from(id)
+    }
+
     /// Get a thread-specific allocation id for the given thread-local static.
     /// If needed, allocate a new one.
     fn get_or_create_thread_local_alloc(
@@ -1160,8 +1159,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     /// Set the name of the current thread. The buffer must not include the null terminator.
     #[inline]
     fn set_thread_name(&mut self, thread: ThreadId, new_thread_name: Vec<u8>) {
-        let this = self.eval_context_mut();
-        this.machine.threads.set_thread_name(thread, new_thread_name);
+        self.eval_context_mut().machine.threads.set_thread_name(thread, new_thread_name);
     }
 
     #[inline]

src/shims/unix/android/thread.rs

@@ -2,7 +2,7 @@ use rustc_abi::{ExternAbi, Size};
 use rustc_span::Symbol;
 
 use crate::helpers::check_min_arg_count;
-use crate::shims::unix::thread::EvalContextExt as _;
+use crate::shims::unix::thread::{EvalContextExt as _, ThreadNameResult};
 use crate::*;
 
 const TASK_COMM_LEN: usize = 16;
@@ -32,7 +32,7 @@ pub fn prctl<'tcx>(
             // https://www.man7.org/linux/man-pages/man2/PR_SET_NAME.2const.html
             let res =
                 this.pthread_setname_np(thread, name, TASK_COMM_LEN, /* truncate */ true)?;
-            assert!(res);
+            assert_eq!(res, ThreadNameResult::Ok);
             Scalar::from_u32(0)
         }
         op if op == pr_get_name => {
@@ -46,7 +46,7 @@ pub fn prctl<'tcx>(
                 CheckInAllocMsg::MemoryAccessTest,
             )?;
             let res = this.pthread_getname_np(thread, name, len, /* truncate*/ false)?;
-            assert!(res);
+            assert_eq!(res, ThreadNameResult::Ok);
             Scalar::from_u32(0)
         }
         op => throw_unsup_format!("Miri does not support `prctl` syscall with op={}", op),

src/shims/unix/foreign_items.rs

@@ -603,13 +603,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             }
             "pthread_join" => {
                 let [thread, retval] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?;
-                this.pthread_join(thread, retval)?;
-                this.write_null(dest)?;
+                let res = this.pthread_join(thread, retval)?;
+                this.write_scalar(res, dest)?;
             }
             "pthread_detach" => {
                 let [thread] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?;
-                this.pthread_detach(thread)?;
-                this.write_null(dest)?;
+                let res = this.pthread_detach(thread)?;
+                this.write_scalar(res, dest)?;
             }
             "pthread_self" => {
                 let [] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?;

src/shims/unix/linux/foreign_items.rs

@@ -81,13 +81,17 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             "pthread_setname_np" => {
                 let [thread, name] =
                     this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?;
-                let res = this.pthread_setname_np(
+                let res = match this.pthread_setname_np(
                     this.read_scalar(thread)?,
                     this.read_scalar(name)?,
                     TASK_COMM_LEN,
                     /* truncate */ false,
-                )?;
-                let res = if res { Scalar::from_u32(0) } else { this.eval_libc("ERANGE") };
+                )? {
+                    ThreadNameResult::Ok => Scalar::from_u32(0),
+                    ThreadNameResult::NameTooLong => this.eval_libc("ERANGE"),
+                    // Act like we faild to open `/proc/self/task/$tid/comm`.
+                    ThreadNameResult::ThreadNotFound => this.eval_libc("ENOENT"),
+                };
                 this.write_scalar(res, dest)?;
             }
             "pthread_getname_np" => {
@@ -97,14 +101,18 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // In case of glibc, the length of the output buffer must
                 // be not shorter than TASK_COMM_LEN.
                 let len = this.read_scalar(len)?;
-                let res = if len.to_target_usize(this)? >= TASK_COMM_LEN as u64
-                    && this.pthread_getname_np(
+                let res = if len.to_target_usize(this)? >= TASK_COMM_LEN as u64 {
+                    match this.pthread_getname_np(
                         this.read_scalar(thread)?,
                         this.read_scalar(name)?,
                         len,
                         /* truncate*/ false,
                     )? {
-                    Scalar::from_u32(0)
+                        ThreadNameResult::Ok => Scalar::from_u32(0),
+                        ThreadNameResult::NameTooLong => unreachable!(),
+                        // Act like we faild to open `/proc/self/task/$tid/comm`.
+                        ThreadNameResult::ThreadNotFound => this.eval_libc("ENOENT"),
+                    }
                 } else {
                     this.eval_libc("ERANGE")
                 };

src/shims/unix/macos/foreign_items.rs

@@ -181,18 +181,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // are met, then the name is set and 0 is returned. Otherwise, if
                 // the specified name is lomnger than MAXTHREADNAMESIZE, then
                 // ENAMETOOLONG is returned.
-                //
-                // FIXME: the real implementation maybe returns ESRCH if the thread ID is invalid.
                 let thread = this.pthread_self()?;
-                let res = if this.pthread_setname_np(
+                let res = match this.pthread_setname_np(
                     thread,
                     this.read_scalar(name)?,
                     this.eval_libc("MAXTHREADNAMESIZE").to_target_usize(this)?.try_into().unwrap(),
                     /* truncate */ false,
                 )? {
-                    Scalar::from_u32(0)
-                } else {
-                    this.eval_libc("ENAMETOOLONG")
+                    ThreadNameResult::Ok => Scalar::from_u32(0),
+                    ThreadNameResult::NameTooLong => this.eval_libc("ENAMETOOLONG"),
+                    ThreadNameResult::ThreadNotFound => unreachable!(),
                 };
                 // Contrary to the manpage, `pthread_setname_np` on macOS still
                 // returns an integer indicating success.
@@ -210,15 +208,17 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // https://github.com/apple-oss-distributions/libpthread/blob/c032e0b076700a0a47db75528a282b8d3a06531a/src/pthread.c#L1160-L1175.
                 // The key part is the strlcpy, which truncates the resulting value,
                 // but always null terminates (except for zero sized buffers).
-                //
-                // FIXME: the real implementation returns ESRCH if the thread ID is invalid.
-                let res = Scalar::from_u32(0);
-                this.pthread_getname_np(
+                let res = match this.pthread_getname_np(
                     this.read_scalar(thread)?,
                     this.read_scalar(name)?,
                     this.read_scalar(len)?,
                     /* truncate */ true,
-                )?;
+                )? {
+                    ThreadNameResult::Ok => Scalar::from_u32(0),
+                    // `NameTooLong` is possible when the buffer is zero sized,
+                    ThreadNameResult::NameTooLong => Scalar::from_u32(0),
+                    ThreadNameResult::ThreadNotFound => this.eval_libc("ESRCH"),
+                };
                 this.write_scalar(res, dest)?;
             }
 

src/shims/unix/mod.rs

@@ -21,7 +21,7 @@ pub use self::fs::{DirTable, EvalContextExt as _};
 pub use self::linux::epoll::EpollInterestTable;
 pub use self::mem::EvalContextExt as _;
 pub use self::sync::EvalContextExt as _;
-pub use self::thread::EvalContextExt as _;
+pub use self::thread::{EvalContextExt as _, ThreadNameResult};
 pub use self::unnamed_socket::EvalContextExt as _;
 
 // Make up some constants.

src/shims/unix/solarish/foreign_items.rs

@@ -26,26 +26,33 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // THREAD_NAME_MAX allows a thread name of 31+1 length
                 // https://github.com/illumos/illumos-gate/blob/7671517e13b8123748eda4ef1ee165c6d9dba7fe/usr/src/uts/common/sys/thread.h#L613
                 let max_len = 32;
-                let res = this.pthread_setname_np(
+                // See https://illumos.org/man/3C/pthread_setname_np for the error codes.
+                let res = match this.pthread_setname_np(
                     this.read_scalar(thread)?,
                     this.read_scalar(name)?,
                     max_len,
                     /* truncate */ false,
-                )?;
-                let res = if res { Scalar::from_u32(0) } else { this.eval_libc("ERANGE") };
+                )? {
+                    ThreadNameResult::Ok => Scalar::from_u32(0),
+                    ThreadNameResult::NameTooLong => this.eval_libc("ERANGE"),
+                    ThreadNameResult::ThreadNotFound => this.eval_libc("ESRCH"),
+                };
                 this.write_scalar(res, dest)?;
             }
             "pthread_getname_np" => {
                 let [thread, name, len] =
                     this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?;
-                // https://github.com/illumos/illumos-gate/blob/c56822be04b6c157c8b6f2281e47214c3b86f657/usr/src/lib/libc/port/threads/thr.c#L2449-L2480
-                let res = this.pthread_getname_np(
+                // See https://illumos.org/man/3C/pthread_getname_np for the error codes.
+                let res = match this.pthread_getname_np(
                     this.read_scalar(thread)?,
                     this.read_scalar(name)?,
                     this.read_scalar(len)?,
                     /* truncate */ false,
-                )?;
-                let res = if res { Scalar::from_u32(0) } else { this.eval_libc("ERANGE") };
+                )? {
+                    ThreadNameResult::Ok => Scalar::from_u32(0),
+                    ThreadNameResult::NameTooLong => this.eval_libc("ERANGE"),
+                    ThreadNameResult::ThreadNotFound => this.eval_libc("ESRCH"),
+                };
                 this.write_scalar(res, dest)?;
             }