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

Setting a thread name on Linux doesn't work with tracked raw pointers #1717

Closed
Kestrer opened this issue Feb 21, 2021 · 6 comments · Fixed by #2055
Closed

Setting a thread name on Linux doesn't work with tracked raw pointers #1717

Kestrer opened this issue Feb 21, 2021 · 6 comments · Fixed by #2055
Labels
A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows)

Comments

@Kestrer
Copy link

Kestrer commented Feb 21, 2021

Spawning a thread with a custom name triggers a stacked borrows violation when -Zmiri-track-raw-pointers is enabled. The root cause of the issue is that prctl only accepts integers as parameters, so Rust casts the C string pointer to an integer before passing it in but of course this doesn't work with tracked raw pointers.

Minimum reproducible example:

fn main() {
    let name = std::ffi::CString::new("abc").unwrap();
    unsafe { libc::prctl(libc::PR_SET_NAME, name.as_ptr() as libc::c_ulong, 0, 0, 0) };
}

Output:

error: Undefined Behavior: no item granting read access to tag <untagged> at alloc1423 found in borrow stack.
 --> src/main.rs:3:14
  |
3 |     unsafe { libc::prctl(libc::PR_SET_NAME, name.as_ptr() as libc::c_ulong, 0, 0, 0) };
  |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no item granting read access to tag <untagged> at alloc1423 found in borrow stack.
  |
  = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
  = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

  = note: inside `main` at src/main.rs:3:14
  = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
  = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:125:18
  = note: inside closure at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:66:18
  = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:259:13
  = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:379:40
  = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:343:19
  = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:431:14
  = note: inside `std::rt::lang_start_internal` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:51:25
  = note: inside `std::rt::lang_start::<()>` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:65:5

Would it be possible to special-case that function for now?

@RalfJung
Copy link
Member

Would it be possible to special-case that function for now?

That wouldn't help... as stated in the README, raw-ptr tracking does not support ptr-int-casts. When prctl gets called, it's already too late as the cast already happened.

The only hack I can imagine here is, under cfg(miri), to transmute the ptr to an int instead of casting it.

@RalfJung RalfJung added the A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) label Jan 4, 2022
bors bot pushed a commit to bevyengine/bevy that referenced this issue Mar 25, 2022
# Objective

Fixes #1529
Run bevy_ecs in miri

## Solution

- Don't set thread names when running in miri rust-lang/miri/issues/1717
- Update `event-listener` to `2.5.2` as previous versions have UB that is detected by miri: [event-listener commit](smol-rs/event-listener@1fa31c5)
- Ignore memory leaks when running in miri as they are impossible to track down rust-lang/miri/issues/1481
- Make `table_add_remove_many` test less "many" because miri is really quite slow :)
- Make CI run `RUSTFLAGS="-Zrandomize-layout" MIRIFLAGS="-Zmiri-ignore-leaks -Zmiri-tag-raw-pointers -Zmiri-disable-isolation" cargo +nightly miri test -p bevy_ecs`
@saethlin
Copy link
Member

saethlin commented Apr 2, 2022

The above suggestion to transmute instead of casting is no longer an option for hardmode since #2040.

But we could still do this:

diff --git a/library/std/src/sys/unix/thread.rs b/library/std/src/sys/unix/thread.rs
index bb4e4ee9aa7..88bfbbfe2c3 100644
--- a/library/std/src/sys/unix/thread.rs
+++ b/library/std/src/sys/unix/thread.rs
@@ -116,6 +116,7 @@ pub fn yield_now() {
         debug_assert_eq!(ret, 0);
     }
 
+    #[cfg(not(miri))]
     #[cfg(any(target_os = "linux", target_os = "android"))]
     pub fn set_name(name: &CStr) {
         const PR_SET_NAME: libc::c_int = 15;
@@ -126,6 +127,26 @@ pub fn set_name(name: &CStr) {
         }
     }
 
+    #[cfg(miri)]
+    #[cfg(any(target_os = "linux", target_os = "android"))]
+    pub fn set_name(name: &CStr) {
+        extern "C" {
+            fn prctl(option: libc::c_int, option: *const i8, ...) -> libc::c_int;
+        }
+        const PR_SET_NAME: libc::c_int = 15;
+        // pthread wrapper only appeared in glibc 2.12, so we use syscall
+        // directly.
+        unsafe {
+            prctl(
+                PR_SET_NAME,
+                name.as_ptr(),
+                ptr::null::<i8>(),
+                ptr::null::<i8>(),
+                ptr::null::<i8>(),
+            );
+        }
+    }
+
     #[cfg(any(target_os = "freebsd", target_os = "dragonfly", target_os = "openbsd"))]
     pub fn set_name(name: &CStr) {
         unsafe {

... or is that too silly?

@RalfJung
Copy link
Member

RalfJung commented Apr 2, 2022

If we do cfg(miri) we might as well add a dedicated Miri API...

@saethlin
Copy link
Member

saethlin commented Apr 2, 2022

What would that look like? If we're adding something specially for Miri it doesn't even need an implementation... Right?

@RalfJung
Copy link
Member

RalfJung commented Apr 2, 2022

It would look like these functions.

But I'm not necessarily a fan of doing that. The easiest solution is to simply do nothing at all on Miri... or to use the approach you suggested even outside Miri, relying on the fact that declaring prctl with a pointer type instead of c_ulong is fine.

@saethlin
Copy link
Member

saethlin commented Apr 3, 2022

I think we can just delete the cast to an integer type: rust-lang/rust#95626

@RalfJung RalfJung mentioned this issue Apr 8, 2022
bors added a commit that referenced this issue Apr 8, 2022
bors added a commit that referenced this issue Apr 8, 2022
@bors bors closed this as completed in be72564 Apr 8, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this issue Jun 7, 2022
# Objective

Fixes bevyengine#1529
Run bevy_ecs in miri

## Solution

- Don't set thread names when running in miri rust-lang/miri/issues/1717
- Update `event-listener` to `2.5.2` as previous versions have UB that is detected by miri: [event-listener commit](smol-rs/event-listener@1fa31c5)
- Ignore memory leaks when running in miri as they are impossible to track down rust-lang/miri/issues/1481
- Make `table_add_remove_many` test less "many" because miri is really quite slow :)
- Make CI run `RUSTFLAGS="-Zrandomize-layout" MIRIFLAGS="-Zmiri-ignore-leaks -Zmiri-tag-raw-pointers -Zmiri-disable-isolation" cargo +nightly miri test -p bevy_ecs`
bors bot pushed a commit to bevyengine/bevy that referenced this issue Jun 26, 2022
# Objective

rust-lang/miri#1717 has been fixed so we can set thread names in Miri now.

## Solution

We set thread names in Miri.
inodentry pushed a commit to IyesGames/bevy that referenced this issue Aug 8, 2022
# Objective

rust-lang/miri#1717 has been fixed so we can set thread names in Miri now.

## Solution

We set thread names in Miri.
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
# Objective

rust-lang/miri#1717 has been fixed so we can set thread names in Miri now.

## Solution

We set thread names in Miri.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

rust-lang/miri#1717 has been fixed so we can set thread names in Miri now.

## Solution

We set thread names in Miri.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

Fixes bevyengine#1529
Run bevy_ecs in miri

## Solution

- Don't set thread names when running in miri rust-lang/miri/issues/1717
- Update `event-listener` to `2.5.2` as previous versions have UB that is detected by miri: [event-listener commit](smol-rs/event-listener@1fa31c5)
- Ignore memory leaks when running in miri as they are impossible to track down rust-lang/miri/issues/1481
- Make `table_add_remove_many` test less "many" because miri is really quite slow :)
- Make CI run `RUSTFLAGS="-Zrandomize-layout" MIRIFLAGS="-Zmiri-ignore-leaks -Zmiri-tag-raw-pointers -Zmiri-disable-isolation" cargo +nightly miri test -p bevy_ecs`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants