Skip to content
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

Thread names are not always valid UTF-8 #123495

Closed
joboet opened this issue Apr 5, 2024 · 4 comments · Fixed by #123505
Closed

Thread names are not always valid UTF-8 #123495

joboet opened this issue Apr 5, 2024 · 4 comments · Fixed by #123505
Labels
A-thread Area: `std::thread` C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue.
Milestone

Comments

@joboet
Copy link
Member

joboet commented Apr 5, 2024

Thread (or well Inner) has the following invariant:

name: Option<CString>, // Guaranteed to be UTF-8

and relies on it for soundness:
self.cname().map(|s| unsafe { str::from_utf8_unchecked(s.to_bytes()) })

Starting with #121666, this invariant can be broken, because the native thread names are not required to be UTF-8. So for example, this code is unsound:

use std::mem::MaybeUninit;
use std::ptr;
use std::thread;

extern "C" fn foreign(_arg: *mut libc::c_void) -> *mut libc::c_void {
    unsafe {
        let id = libc::pthread_self();
        let r = libc::pthread_setname_np(id, [0xff, 0].as_ptr().cast());
        assert_eq!(r, 0);
    }

    let current = thread::current();
    println!("{:?}", current.name().unwrap().chars()); // Whoops!

    ptr::null_mut()
}

fn main() {
    unsafe {
        let mut id = MaybeUninit::uninit();
        let r = libc::pthread_create(id.as_mut_ptr(), ptr::null(), foreign, ptr::null_mut());
        assert_eq!(r, 0);
        let id = id.assume_init();
        let r = libc::pthread_join(id, ptr::null_mut());
        assert_eq!(r, 0);
    }
}

(run with miri and --target x86_64-unknown-linux-gnu)

This can even be exploited in entirely safe code. On platforms with key-based thread locals, the Thread handle is initialized with the system thread name when accessed in TLS destructors. On all UNIXes except macOS, the main thread name is the name of the program. So if you spawn a program with a bad name on these platforms (e.g. Illumos), calling thread::current().name() will cause UB.

We can fix this by checking the thread name in Thread::new, and not using it if it is bad. In the long-term, a function like os_name might be nice and perhaps fix some other issues as well.

CC @ChrisDenton

@joboet joboet added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. A-thread Area: `std::thread` labels Apr 5, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 5, 2024
@ChrisDenton
Copy link
Member

Ooph, yeah. I messed up there by only upholding that it was a CString on *nix platforms. Are you submitting a PR or should I?

In the long-term, a function like os_name might be nice and perhaps fix some other issues as well.

That indeed would be great.

@ChrisDenton
Copy link
Member

I'm actually thinking it might be better to just do a straight up revert. Hm.

@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 5, 2024
@workingjubilee
Copy link
Member

If it's not immediately dawning on us, I think we should revert it and figure out the correct behavior later, yes.

ChrisDenton added a commit to ChrisDenton/rust that referenced this issue Apr 5, 2024
@ChrisDenton
Copy link
Member

Ok, I put up a revert PR.

@ChrisDenton ChrisDenton linked a pull request Apr 5, 2024 that will close this issue
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Apr 5, 2024
…ngjubilee

Revert "Use OS thread name by default"

This reverts rust-lang#121666 (Use the OS thread name by default if `THREAD_INFO` has not been initialized) due to rust-lang#123495 (Thread names are not always valid UTF-8).

It's not a direct revert because there have been other changes since that PR.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Apr 5, 2024
…ngjubilee

Revert "Use OS thread name by default"

This reverts rust-lang#121666 (Use the OS thread name by default if `THREAD_INFO` has not been initialized) due to rust-lang#123495 (Thread names are not always valid UTF-8).

It's not a direct revert because there have been other changes since that PR.
@cuviper cuviper added this to the 1.78.0 milestone Apr 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 6, 2024
Rollup merge of rust-lang#123505 - ChrisDenton:revert-121666, r=workingjubilee

Revert "Use OS thread name by default"

This reverts rust-lang#121666 (Use the OS thread name by default if `THREAD_INFO` has not been initialized) due to rust-lang#123495 (Thread names are not always valid UTF-8).

It's not a direct revert because there have been other changes since that PR.
ChrisDenton added a commit to ChrisDenton/rust that referenced this issue Apr 6, 2024
This reverts rust-lang#121666 due to rust-lang#123495

This has already been done on master but beta needs something that will backport cleanly.
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 6, 2024
cuviper pushed a commit to cuviper/rust that referenced this issue Apr 11, 2024
This reverts rust-lang#121666 due to rust-lang#123495

This has already been done on master but beta needs something that will backport cleanly.

(cherry picked from commit 081ad85)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-thread Area: `std::thread` C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants