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

Posix mutex, rwlock & condvar store identifier index for miri in normal memory. #1649

Closed
JCTyblaidd opened this issue Dec 13, 2020 · 1 comment · Fixed by #3966
Closed

Posix mutex, rwlock & condvar store identifier index for miri in normal memory. #1649

JCTyblaidd opened this issue Dec 13, 2020 · 1 comment · Fixed by #3966
Labels
A-concurrency Area: affects our concurrency (multi-thread) support C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@JCTyblaidd
Copy link
Contributor

The posix mutex, rwlock and condvar store the id miri uses to correspond with the internal implementation and does not prevent any interpreted code from mutating the value.

This can cause unusual behaviour and cause miri to panic.

Although I think this is unlikely to occur in practice it would be good to detect this case, it might be better to store the memory location & associated size for the posix mutex, rwlock and condvar and detect and report any reading and writing of the associated memory as well. I however have not looked at the docs for long enough to be sure of the best solution.

// ignore-windows: No libc on Windows

#![feature(rustc_private)]

extern crate libc;

fn main() {
    unsafe {
        let mut lock = libc::PTHREAD_MUTEX_INITIALIZER;
        assert_eq!(libc::pthread_mutex_lock(&mut lock as *mut _), 0);
        
        let miri_ptr: *mut u32 = &mut lock as *mut _ as *mut u32;
        
        // If false miss undefined behaviour or unsupported operation
        // If true then cause miri to panic.
        const ICE: bool = true;
        
        // This operation edits the index that miri uses to map the mutex to the internal
        // mutex object.
        // If set to 0 makes any pthread_mutex operation assign a new internal mutex.
        // Otherwise can be set to an invalid value and cause miri to panic
        // via loading an invalid mutex index.
        // And should probably be reported as UB or unsupported since
        // it modifies internal mutex state.
        *miri_ptr.add(1) = if ICE { 5 } else { 0 };
        
        
        // If ICE = false then will assign a new lock & not detect any undefined behaviour.
        // If ICE = true then will cause miri to panic.
        assert_eq!(libc::pthread_mutex_lock(&mut lock as *mut _), 0);
    }
}
@RalfJung
Copy link
Member

Yeah, there are ways to "mess" with Miri's internals here in ways that can cause ICEs or might not even be detected as errors. That was a deliberate choice to make the implementation simpler -- detecting all possible misbehavior even for programs that "maliciously" try to mess with Miri was declared a non-goal for now.

Improvements for this are welcome. :) But I do not consider the current behavior a bug.

@RalfJung RalfJung added A-concurrency Area: affects our concurrency (multi-thread) support C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement labels Dec 13, 2020
bors added a commit that referenced this issue Oct 12, 2024
Do not store synchronization primitive IDs in adressable memory

We shouldn't store this in a place where the program can mess with it.

Fixes #1649
bors added a commit that referenced this issue Oct 12, 2024
Do not store synchronization primitive IDs in adressable memory

We shouldn't store this in a place where the program can mess with it.

Fixes #1649
@bors bors closed this as completed in 1362c5f Oct 14, 2024
RalfJung pushed a commit to RalfJung/rust that referenced this issue Oct 15, 2024
Do not store synchronization primitive IDs in adressable memory

We shouldn't store this in a place where the program can mess with it.

Fixes rust-lang/miri#1649

Blocked by rust-lang#131593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: affects our concurrency (multi-thread) support C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants