Skip to content

pthread_setname_np shim should return and int on macOS, but returns void. #2625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
thomcc opened this issue Oct 28, 2022 · 3 comments · Fixed by #2626
Closed

pthread_setname_np shim should return and int on macOS, but returns void. #2625

thomcc opened this issue Oct 28, 2022 · 3 comments · Fixed by #2626
Labels
A-mac Area: Affects only macOS targets E-good-first-issue A good way to start contributing, mentoring is available

Comments

@thomcc
Copy link
Member

thomcc commented Oct 28, 2022

pthread_setname_np on macOS returns an integer success code: https://github.com/apple-oss-distributions/libpthread/blob/cf9e1c7e611440e511af230905be2cfefc5c6121/include/pthread/pthread.h#L509-L510

Unfortunately, the miri shim for it returns void:

"pthread_setname_np" => {
let [name] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let thread = this.pthread_self()?;
let max_len = this.eval_libc("MAXTHREADNAMESIZE")?.to_machine_usize(this)?;
this.pthread_setname_np(
thread,
this.read_scalar(name)?,
max_len.try_into().unwrap(),
)?;
}

This probably happened because the manpage on apple tragically has the same mistake: https://github.com/apple-oss-distributions/libpthread/blob/cf9e1c7e611440e511af230905be2cfefc5c6121/man/pthread_setname_np.3#L35

But the function truly does return int: https://github.com/apple-oss-distributions/libpthread/blob/cf9e1c7e611440e511af230905be2cfefc5c6121/src/pthread.c#L1140-L1163

This leads to false positives for UB in libstd's tests: https://github.com/rust-lang/miri-test-libstd/actions/runs/3342425264/jobs/5534636230#step:5:723

@thomcc
Copy link
Member Author

thomcc commented Oct 28, 2022

The fix for this is probably

diff --git a/src/shims/unix/macos/foreign_items.rs b/src/shims/unix/macos/foreign_items.rs
index 371f56ca..221dc396 100644
--- a/src/shims/unix/macos/foreign_items.rs
+++ b/src/shims/unix/macos/foreign_items.rs
@@ -177,11 +177,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 let [name] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
                 let thread = this.pthread_self()?;
                 let max_len = this.eval_libc("MAXTHREADNAMESIZE")?.to_machine_usize(this)?;
-                this.pthread_setname_np(
+                let res = this.pthread_setname_np(
                     thread,
                     this.read_scalar(name)?,
                     max_len.try_into().unwrap(),
                 )?;
+                // 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] =

but I can't be bothered to check (my miri setup lapsed and this deserves a test anyway).

@oli-obk oli-obk added E-good-first-issue A good way to start contributing, mentoring is available A-mac Area: Affects only macOS targets labels Oct 28, 2022
@JakobDegen
Copy link
Contributor

This would be why we have autogenerated docs 🙃

@thomcc
Copy link
Member Author

thomcc commented Oct 28, 2022

I'll file a radar this weekend to ask them to fix it, but I wouldn't hold my breath.

@bors bors closed this as completed in 395af08 Oct 28, 2022
RalfJung pushed a commit to RalfJung/rust that referenced this issue Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mac Area: Affects only macOS targets E-good-first-issue A good way to start contributing, mentoring is available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants