Stream: t-lang/wg-unsafe-code-guidelines

Topic: miri doesn't detect mutating a hashset value


Jake Goulding (Apr 03 2019 at 19:52, on Zulip):

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=df5174e368210dcab4c40387d858fd5f

use std::borrow::Borrow;
use std::collections::HashSet;
use std::hash::{Hash, Hasher};

type Id = u32;

#[derive(Debug, Eq)]
struct Foo {
    id: Id,
    other_data: u32,
}

impl PartialEq for Foo {
    fn eq(&self, other: &Foo) -> bool {
        self.id == other.id
    }
}

impl Hash for Foo {
    fn hash<H: Hasher>(&self, state: &mut H) {
        self.id.hash(state);
    }
}

impl Borrow<Id> for Foo {
    fn borrow(&self) -> &Id {
        &self.id
    }
}

impl Foo {
    fn set_other_data(set: &mut HashSet<Foo, Fixed>, id: &Id, data: u32) -> bool {
        match set.get(id) {
            Some(x) => {
                let p: *const Foo = x;
                let q: *mut Foo = p as *mut Foo;
                unsafe {
                    (*q).other_data = data;
                }
                return true;
            }
            None => return false,
        }
    }
}

// Miri doesn't "do" random numbers
use std::hash::SipHasher;
#[derive(Default)]
struct Fixed;
impl std::hash::BuildHasher for Fixed {
    type Hasher = SipHasher;
    fn build_hasher(&self) -> Self::Hasher {
        SipHasher::new_with_keys(0, 0)
    }
}

fn main() {
    let mut baz: HashSet<_, Fixed> = HashSet::default();
    baz.insert(Foo {
        id: 1,
        other_data: 2,
    });

    Foo::set_other_data(&mut baz, &1, 3);
    assert_eq!(3, baz.get(&1).unwrap().other_data);
}

Miri gives this a clean bill of health.

Jake Goulding (Apr 03 2019 at 19:54, on Zulip):

but HashSet::get returns an Option<&T> which this code makes into a &mut T — I was under the impression that &T -> &mut T was always invalid — am I incorrect?

Jake Goulding (Apr 04 2019 at 19:13, on Zulip):

Always invalid without some version of UnsafeCell in play, that is.

RalfJ (Apr 07 2019 at 17:40, on Zulip):

@Jake Goulding my guess is that there exists somewhere another raw pointer to this memory (as part of the HashMap impl). When you use q to write, Stacked Borrows assumes that you got it from that other raw pointer. This write invalidates the shared reference, but you are not ever using that again.

RalfJ (Apr 07 2019 at 17:42, on Zulip):

So, it's a bit like this example:

fn main() { unsafe {
    let l = &mut 0;
    let _raw_leak = l as *mut _;
    let s = &*l;
    let c = s as *const _;
    let m = c as *mut _;
    *m = 1; // this line is okay; it is equivalent to `*_raw_leak = 1;`
    let _val = *s; // only this line triggers UB
} }
RalfJ (Apr 07 2019 at 17:49, on Zulip):

So, this is an instance of "all raw pointers to the same location can be used interchangeably"

RalfJ (Apr 07 2019 at 17:50, on Zulip):

that is currently true in Stacked Borrows, but we will likely have to refine that eventually

Jake Goulding (Apr 07 2019 at 18:47, on Zulip):

@RalfJ So, in this case, performing the cast is "ok", but only because of internal implementation details of HashSet::get?

Jake Goulding (Apr 07 2019 at 18:47, on Zulip):

Which isn't something I'd want to bet on, if true.

RalfJ (Apr 07 2019 at 20:19, on Zulip):

this particular program is "ok" according to the current version of Stacked Borrows, and that is intentional

RalfJ (Apr 07 2019 at 20:20, on Zulip):

but yes, this does rely on HashSet implementation details, and also future versions of Stacked Borrows will likely be more strict.

RalfJ (Apr 07 2019 at 20:20, on Zulip):

Also, Miri tests a program, not a library; adding uses of stuff after your small test case (at the end of main) might already be "not ok" according to current Stacked Borrows (like the last line in my example)

RalfJ (Apr 07 2019 at 20:26, on Zulip):

We are tracking the imprecision of Stacked Borrows that you are experiencing here at https://github.com/rust-lang/unsafe-code-guidelines/issues/86

Last update: Nov 19 2019 at 19:00UTC