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

Stacked Borrows: too much raw pointer retagging hurts #1608

Closed
RalfJung opened this issue Oct 29, 2020 · 4 comments · Fixed by rust-lang/rust#78597
Closed

Stacked Borrows: too much raw pointer retagging hurts #1608

RalfJung opened this issue Oct 29, 2020 · 4 comments · Fixed by rust-lang/rust#78597
Labels
A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows)

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 29, 2020

Consider the following example (due to @ssomers):

#![feature(raw_ref_macros)]
use std::ptr;

struct Part {
    _lame: i32,
}

#[repr(C)]
struct Whole {
    part: Part,
    extra: i32,
}

fn main() {
    let it = Box::new(Whole { part: Part { _lame: 0 }, extra: 42 });
    let whole = ptr::raw_mut!(*Box::leak(it));
    let part = unsafe { ptr::raw_mut!((*whole).part) };
    let typed = unsafe { &mut *(part as *mut Whole) };
    assert!(typed.extra == 42);
}

Arguably, this is using raw_mut! correctly, but Miri flags this code as UB when using -Zmiri-track-raw-pointers. The problem is that at ptr::raw_mut!((*whole).part), a raw retag happens, which generates a fresh tag for part that is only valid for the memory of the part field, not for the extra field.

@RalfJung RalfJung added the A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) label Oct 29, 2020
@RalfJung
Copy link
Member Author

A possible fix might be to not retag after ptr::raw_mut!, but then ptr::raw_mut!(*x) with x: &mut _ would be different from x as *mut _.

On the other hand, right now ptr::raw_mut!(*x) with x: *mut _ is different from just x, which is also not what we want.

We could detect if the dereferenced pointer is raw or a reference... that seems to work for these cases, but it strikes me as rather ugly.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 1, 2020

So, to summarize, there are a bunch of alternatives, and we need to move away from the status quo.

currently

x: &mut T x: *mut T
x as *mut T retag no action
raw_mut!(*x) retag retag

your PR

x: &mut T x: *mut T
x as *mut T retag no action
raw_mut!(*x) retag no action

not retag

x: &mut T x: *mut T
x as *mut T retag no action
raw_mut!(*x) no action no action

unmentioned "alternative"

x: &mut T x: *mut T
x as *mut T no action no action
raw_mut!(*x) no action no action

I'm not sure what the implications of the unmentioned alternative are, but we should probaby reject it explicitly

@RalfJung
Copy link
Member Author

RalfJung commented Nov 1, 2020

The unmentioned alternative means that raw pointers maintain the tag of their parent reference. That has some far-reaching consequences. For example, it means after doing raw_mut!(x.field), I can use the resulting raw pointer to access all fields, not just the one the pointer was taken to.

Worse, it means that when I do

let ptr = raw_mut!(x.field);
x.field = 13;

the raw pointer is still active, as it has the same tag as x! That seems very much not like what we want. Or at least, it is a stark departure from the strategy so far, which is to consider raw ptrs as being "unsafe reborrows" of the reference. Your proposal makes them aliases that are entirely equivalent to the original reference.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 2, 2020

Thanks for the explanation! That makes sense

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Nov 3, 2020
Retagging: do not retag 'raw reborrows'

When doing `&raw const (*raw_ptr).field`, we do not want any retagging; the original provenance should be fully preserved.

Fixes rust-lang/miri#1608
Test added by rust-lang/miri#1614

Not sure whom to ask for review on this... `@oli-obk` can you have a look? Or maybe highfive makes a good choice.^^
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.

2 participants