Auto merge of #3957 - YohDeadfall:macos-thread-name, r=RalfJung

Fixed get/set thread name implementations for macOS and FreeBSD

So, the story of fixing `pthread_getname_np` and `pthread_setname_np` continues, but this time I fixed the macOS implementation.

### [`pthread_getname_np`](https://github.com/apple-oss-distributions/libpthread/blob/c032e0b076700a0a47db75528a282b8d3a06531a/src/pthread.c#L1160-L1175)

The function never fails except for an invalid thread. Miri never verifies thread identifiers and uses them as indices when accessing a vector of threads. Therefore, the only possible result is `0` and a possibly trimmed output.

```c
int
pthread_getname_np(pthread_t thread, char *threadname, size_t len)
{
	if (thread == pthread_self()) {
		strlcpy(threadname, thread->pthread_name, len);
		return 0;
	}

	if (!_pthread_validate_thread_and_list_lock(thread)) {
		return ESRCH;
	}

	strlcpy(threadname, thread->pthread_name, len);
	_pthread_lock_unlock(&_pthread_list_lock);
	return 0;
}
```

#### [`strcpy`](https://www.man7.org/linux/man-pages/man7/strlcpy.7.html)

```
strlcpy(3bsd)
strlcat(3bsd)
      Copy and catenate the input string into a destination
      string.  If the destination buffer, limited by its size,
      isn't large enough to hold the copy, the resulting string
      is truncated (but it is guaranteed to be null-terminated).
      They return the length of the total string they tried to
      create.
```

### [`pthread_setname_np`](https://github.com/apple-oss-distributions/libpthread/blob/c032e0b076700a0a47db75528a282b8d3a06531a/src/pthread.c#L1178-L1200)

```c
pthread_setname_np(const char *name)
{
	int res;
	pthread_t self = pthread_self();

	size_t len = 0;
	if (name != NULL) {
		len = strlen(name);
	}

	_pthread_validate_signature(self);

	res = __proc_info(5, getpid(), 2, (uint64_t)0, (void*)name, (int)len);
	if (res == 0) {
		if (len > 0) {
			strlcpy(self->pthread_name, name, MAXTHREADNAMESIZE);
		} else {
			bzero(self->pthread_name, MAXTHREADNAMESIZE);
		}
	}
	return res;

}
```

Where `5` is [`PROC_INFO_CALL_SETCONTROL`](https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/sys/proc_info_private.h#L274), and `2` is [`PROC_INFO_CALL_SETCONTROL`](https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/sys/proc_info.h#L821). And `__proc_info` is a syscall handled by the XNU kernel by [`proc_info_internal`](https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/kern/proc_info.c#L300-L314):

```c
int
proc_info_internal(int callnum, int pid, uint32_t flags, uint64_t ext_id, int flavor, uint64_t arg, user_addr_t buffer, uint32_t  buffersize, int32_t * retval)
{
	switch (callnum) {
	// ...
	case PROC_INFO_CALL_SETCONTROL:
		return proc_setcontrol(pid, flavor, arg, buffer, buffersize, retval);
```

And the actual logic from [`proc_setcontrol`](https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/kern/proc_info.c#L3218-L3227):
```c
	case PROC_SELFSET_THREADNAME: {
		/*
		 * This is a bit ugly, as it copies the name into the kernel, and then
		 * invokes bsd_setthreadname again to copy it into the uthread name
		 * buffer.  Hopefully this isn't such a hot codepath that an additional
		 * MAXTHREADNAMESIZE copy is a big issue.
		 */
		if (buffersize > (MAXTHREADNAMESIZE - 1)) {
			return ENAMETOOLONG;
		}
```

Unrelated to the current pull request, but perhaps, there's a very ugly thing in the kernel/libc because the last thing happening in `PROC_SELFSET_THREADNAME` is `bsd_setthreadname` which sets the name in the user space. But we just saw that `pthread_setname_np` sets the name in the user space too. Guess, I need to open a ticket in one of Apple's repositories at least to clarify that :D

src/shims/unix/freebsd/foreign_items.rs

@@ -34,11 +34,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             "pthread_get_name_np" => {
                 let [thread, name, len] =
                     this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
-                // FreeBSD's pthread_get_name_np does not return anything.
+                // FreeBSD's pthread_get_name_np does not return anything
+                // and uses strlcpy, which truncates the resulting value,
+                // but always adds a null terminator (except for zero-sized buffers).
+                // https://github.com/freebsd/freebsd-src/blob/c2d93a803acef634bd0eede6673aeea59e90c277/lib/libthr/thread/thr_info.c#L119-L144
                 this.pthread_getname_np(
                     this.read_scalar(thread)?,
                     this.read_scalar(name)?,
                     this.read_scalar(len)?,
+                    /* truncate */ true,
                 )?;
             }
 

src/shims/unix/linux/foreign_items.rs

@@ -84,6 +84,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     this.read_scalar(name)?,
                     TASK_COMM_LEN,
                 )?;
+                let res = if res { Scalar::from_u32(0) } else { this.eval_libc("ERANGE") };
                 this.write_scalar(res, dest)?;
             }
             "pthread_getname_np" => {
@@ -93,14 +94,16 @@ 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.eval_libc("ERANGE")
-                } else {
-                    this.pthread_getname_np(
+                let res = if len.to_target_usize(this)? >= TASK_COMM_LEN as u64
+                    && this.pthread_getname_np(
                         this.read_scalar(thread)?,
                         this.read_scalar(name)?,
                         len,
-                    )?
+                        /* truncate*/ false,
+                    )? {
+                    Scalar::from_u32(0)
+                } else {
+                    this.eval_libc("ERANGE")
                 };
                 this.write_scalar(res, dest)?;
             }

src/shims/unix/macos/foreign_items.rs

@@ -164,24 +164,52 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             // Threading
             "pthread_setname_np" => {
                 let [name] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
+
+                // The real implementation has logic in two places:
+                // * in userland at https://github.com/apple-oss-distributions/libpthread/blob/c032e0b076700a0a47db75528a282b8d3a06531a/src/pthread.c#L1178-L1200,
+                // * in kernel at https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/kern/proc_info.c#L3218-L3227.
+                //
+                // The function in libc calls the kernel to validate
+                // the security policies and the input. If all of the requirements
+                // 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 max_len = this.eval_libc("MAXTHREADNAMESIZE").to_target_usize(this)?;
-                let res = this.pthread_setname_np(
+                let res = if this.pthread_setname_np(
                     thread,
                     this.read_scalar(name)?,
-                    max_len.try_into().unwrap(),
-                )?;
+                    this.eval_libc("MAXTHREADNAMESIZE").to_target_usize(this)?.try_into().unwrap(),
+                )? {
+                    Scalar::from_u32(0)
+                } else {
+                    this.eval_libc("ENAMETOOLONG")
+                };
                 // Contrary to the manpage, `pthread_setname_np` on macOS still
                 // returns an integer indicating success.
                 this.write_scalar(res, dest)?;
             }
             "pthread_getname_np" => {
                 let [thread, name, len] =
                     this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
-                let res = this.pthread_getname_np(
+
+                // The function's behavior isn't portable between platforms.
+                // In case of macOS, a truncated name (due to a too small buffer)
+                // does not lead to an error.
+                //
+                // For details, see the implementation at
+                // 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(
                     this.read_scalar(thread)?,
                     this.read_scalar(name)?,
                     this.read_scalar(len)?,
+                    /* truncate */ true,
                 )?;
                 this.write_scalar(res, dest)?;
             }

src/shims/unix/solarish/foreign_items.rs

@@ -31,16 +31,20 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     this.read_scalar(name)?,
                     max_len,
                 )?;
+                let res = if res { Scalar::from_u32(0) } else { this.eval_libc("ERANGE") };
                 this.write_scalar(res, dest)?;
             }
             "pthread_getname_np" => {
                 let [thread, name, len] =
                     this.check_shim(abi, Abi::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(
                     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") };
                 this.write_scalar(res, dest)?;
             }
 

src/shims/unix/thread.rs

@@ -63,38 +63,41 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         interp_ok(Scalar::from_uint(thread_id.to_u32(), this.libc_ty_layout("pthread_t").size))
     }
 
-    /// Set the name of the current thread. `max_name_len` is the maximal length of the name
-    /// including the null terminator.
+    /// Set the name of the specified thread. If the name including the null terminator
+    /// is longer than `name_max_len`, then `false` is returned.
     fn pthread_setname_np(
         &mut self,
         thread: Scalar,
         name: Scalar,
-        max_name_len: usize,
-    ) -> InterpResult<'tcx, Scalar> {
+        name_max_len: usize,
+    ) -> InterpResult<'tcx, bool> {
         let this = self.eval_context_mut();
 
         let thread = thread.to_int(this.libc_ty_layout("pthread_t").size)?;
         let thread = ThreadId::try_from(thread).unwrap();
         let name = name.to_pointer(this)?;
-
         let name = this.read_c_str(name)?.to_owned();
 
         // Comparing with `>=` to account for null terminator.
-        if name.len() >= max_name_len {
-            return interp_ok(this.eval_libc("ERANGE"));
+        if name.len() >= name_max_len {
+            return interp_ok(false);
         }
 
         this.set_thread_name(thread, name);
 
-        interp_ok(Scalar::from_u32(0))
+        interp_ok(true)
     }
 
+    /// Get the name of the specified thread. If the thread name doesn't fit
+    /// the buffer, then if `truncate` is set the truncated name is written out,
+    /// otherwise `false` is returned.
     fn pthread_getname_np(
         &mut self,
         thread: Scalar,
         name_out: Scalar,
         len: Scalar,
-    ) -> InterpResult<'tcx, Scalar> {
+        truncate: bool,
+    ) -> InterpResult<'tcx, bool> {
         let this = self.eval_context_mut();
 
         let thread = thread.to_int(this.libc_ty_layout("pthread_t").size)?;
@@ -104,9 +107,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         // FIXME: we should use the program name if the thread name is not set
         let name = this.get_thread_name(thread).unwrap_or(b"<unnamed>").to_owned();
-        let (success, _written) = this.write_c_str(&name, name_out, len)?;
+        let name = match truncate {
+            true => &name[..name.len().min(len.try_into().unwrap_or(usize::MAX).saturating_sub(1))],
+            false => &name,
+        };
+
+        let (success, _written) = this.write_c_str(name, name_out, len)?;
 
-        interp_ok(if success { Scalar::from_u32(0) } else { this.eval_libc("ERANGE") })
+        interp_ok(success)
     }
 
     fn sched_yield(&mut self) -> InterpResult<'tcx, ()> {

tests/pass-dep/libc/pthread-threadname.rs

@@ -1,9 +1,23 @@
 //@ignore-target: windows # No pthreads on Windows
-use std::ffi::CStr;
-#[cfg(not(target_os = "freebsd"))]
-use std::ffi::CString;
+use std::ffi::{CStr, CString};
 use std::thread;
 
+const MAX_THREAD_NAME_LEN: usize = {
+    cfg_if::cfg_if! {
+        if #[cfg(any(target_os = "linux"))] {
+            16
+        } else if #[cfg(any(target_os = "illumos", target_os = "solaris"))] {
+            32
+        } else if #[cfg(target_os = "macos")] {
+            libc::MAXTHREADNAMESIZE // 64, at the time of writing
+        } else if #[cfg(target_os = "freebsd")] {
+            usize::MAX // as far as I can tell
+        } else {
+            panic!()
+        }
+    }
+};
+
 fn main() {
     // The short name should be shorter than 16 bytes which POSIX promises
     // for thread names. The length includes a null terminator.
@@ -52,61 +66,117 @@ fn main() {
     }
 
     thread::Builder::new()
-        .name(short_name.clone())
         .spawn(move || {
-            // Rust remembers the full thread name itself.
-            assert_eq!(thread::current().name(), Some(short_name.as_str()));
+            // Set short thread name.
+            let cstr = CString::new(short_name.clone()).unwrap();
+            assert!(cstr.to_bytes_with_nul().len() <= MAX_THREAD_NAME_LEN); // this should fit
+            assert_eq!(set_thread_name(&cstr), 0);
 
-            // Note that glibc requires 15 bytes long buffer exculding a null terminator.
-            // Otherwise, `pthread_getname_np` returns an error.
+            // Now get it again, in various ways.
+
+            // POSIX seems to promise at least 15 chars excluding a null terminator.
             let mut buf = vec![0u8; short_name.len().max(15) + 1];
             assert_eq!(get_thread_name(&mut buf), 0);
-
             let cstr = CStr::from_bytes_until_nul(&buf).unwrap();
-            // POSIX seems to promise at least 15 chars excluding a null terminator.
-            assert_eq!(short_name.as_bytes(), cstr.to_bytes());
+            assert_eq!(cstr.to_bytes(), short_name.as_bytes());
 
-            // Also test directly calling pthread_setname to check its return value.
-            assert_eq!(set_thread_name(&cstr), 0);
+            // Test what happens when the buffer is shorter than 16, but still long enough.
+            let res = get_thread_name(&mut buf[..15]);
+            cfg_if::cfg_if! {
+                if #[cfg(target_os = "linux")] {
+                    // For glibc used by linux-gnu there should be a failue,
+                    // if a shorter than 16 bytes buffer is provided, even if that would be
+                    // large enough for the thread name.
+                    assert_eq!(res, libc::ERANGE);
+                } else {
+                    // Everywhere else, this should work.
+                    assert_eq!(res, 0);
+                    // POSIX seems to promise at least 15 chars excluding a null terminator.
+                    let cstr = CStr::from_bytes_until_nul(&buf).unwrap();
+                    assert_eq!(short_name.as_bytes(), cstr.to_bytes());
+                }
+            }
+
+            // Test what happens when the buffer is too short even for the short name.
+            let res = get_thread_name(&mut buf[..4]);
+            cfg_if::cfg_if! {
+                if #[cfg(any(target_os = "freebsd", target_os = "macos"))] {
+                    // On macOS and FreeBSD it's not an error for the buffer to be
+                    // too short for the thread name -- they truncate instead.
+                    assert_eq!(res, 0);
+                    let cstr = CStr::from_bytes_until_nul(&buf).unwrap();
+                    assert_eq!(cstr.to_bytes_with_nul().len(), 4);
+                    assert!(short_name.as_bytes().starts_with(cstr.to_bytes()));
+                } else {
+                    // The rest should give an error.
+                    assert_eq!(res, libc::ERANGE);
+                }
+            }
 
-            // For glibc used by linux-gnu there should be a failue,
-            // if a shorter than 16 bytes buffer is provided, even if that would be
-            // large enough for the thread name.
-            #[cfg(target_os = "linux")]
-            assert_eq!(get_thread_name(&mut buf[..15]), libc::ERANGE);
+            // Test zero-sized buffer.
+            let res = get_thread_name(&mut []);
+            cfg_if::cfg_if! {
+                if #[cfg(any(target_os = "freebsd", target_os = "macos"))] {
+                    // On macOS and FreeBSD it's not an error for the buffer to be
+                    // too short for the thread name -- even with size 0.
+                    assert_eq!(res, 0);
+                } else {
+                    // The rest should give an error.
+                    assert_eq!(res, libc::ERANGE);
+                }
+            }
         })
         .unwrap()
         .join()
         .unwrap();
 
     thread::Builder::new()
-        .name(long_name.clone())
         .spawn(move || {
-            // Rust remembers the full thread name itself.
-            assert_eq!(thread::current().name(), Some(long_name.as_str()));
+            // Set full thread name.
+            let cstr = CString::new(long_name.clone()).unwrap();
+            let res = set_thread_name(&cstr);
+            cfg_if::cfg_if! {
+                if #[cfg(target_os = "freebsd")] {
+                    // Names of all size are supported.
+                    assert!(cstr.to_bytes_with_nul().len() <= MAX_THREAD_NAME_LEN);
+                    assert_eq!(res, 0);
+                } else if #[cfg(target_os = "macos")] {
+                    // Name is too long.
+                    assert!(cstr.to_bytes_with_nul().len() > MAX_THREAD_NAME_LEN);
+                    assert_eq!(res, libc::ENAMETOOLONG);
+                } else {
+                    // Name is too long.
+                    assert!(cstr.to_bytes_with_nul().len() > MAX_THREAD_NAME_LEN);
+                    assert_eq!(res, libc::ERANGE);
+                }
+            }
+            // Set the longest name we can.
+            let truncated_name = &long_name[..long_name.len().min(MAX_THREAD_NAME_LEN - 1)];
+            let cstr = CString::new(truncated_name).unwrap();
+            assert_eq!(set_thread_name(&cstr), 0);
+
+            // Now get it again, in various ways.
 
-            // But the system is limited -- make sure we successfully set a truncation.
-            // Note that there's no specific to glibc buffer requirement, since the value
-            // `long_name` is longer than 16 bytes including a null terminator.
+            // This name should round-trip properly.
             let mut buf = vec![0u8; long_name.len() + 1];
             assert_eq!(get_thread_name(&mut buf), 0);
-
             let cstr = CStr::from_bytes_until_nul(&buf).unwrap();
-            // POSIX seems to promise at least 15 chars excluding a null terminator.
-            assert!(
-                cstr.to_bytes().len() >= 15,
-                "name is too short: len={}",
-                cstr.to_bytes().len()
-            );
-            assert!(long_name.as_bytes().starts_with(cstr.to_bytes()));
-
-            // Also test directly calling pthread_setname to check its return value.
-            assert_eq!(set_thread_name(&cstr), 0);
+            assert_eq!(cstr.to_bytes(), truncated_name.as_bytes());
 
-            // But with a too long name it should fail (except on FreeBSD where the
-            // function has no return, hence cannot indicate failure).
-            #[cfg(not(target_os = "freebsd"))]
-            assert_ne!(set_thread_name(&CString::new(long_name).unwrap()), 0);
+            // Test what happens when our buffer is just one byte too small.
+            let res = get_thread_name(&mut buf[..truncated_name.len()]);
+            cfg_if::cfg_if! {
+                if #[cfg(any(target_os = "freebsd", target_os = "macos"))] {
+                    // On macOS and FreeBSD it's not an error for the buffer to be
+                    // too short for the thread name -- they truncate instead.
+                    assert_eq!(res, 0);
+                    let cstr = CStr::from_bytes_until_nul(&buf).unwrap();
+                    assert_eq!(cstr.to_bytes(), &truncated_name.as_bytes()[..(truncated_name.len() - 1)]);
+                } else {
+                    // The rest should give an error.
+                    assert_eq!(res, libc::ERANGE);
+                }
+            }
         })
         .unwrap()
         .join()

tests/pass/concurrency/threadname.rs

@@ -16,6 +16,19 @@ fn main() {
         .join()
         .unwrap();
 
+    // Long thread name.
+    let long_name = std::iter::once("test_named_thread_truncation")
+        .chain(std::iter::repeat(" long").take(100))
+        .collect::<String>();
+    thread::Builder::new()
+        .name(long_name.clone())
+        .spawn(move || {
+            assert_eq!(thread::current().name().unwrap(), long_name);
+        })
+        .unwrap()
+        .join()
+        .unwrap();
+
     // Also check main thread name.
     assert_eq!(thread::current().name().unwrap(), "main");
 }