Skip to content

Stacked Borrows: not enough UB to justify noalias on Box #376

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

Closed
RalfJung opened this issue Nov 19, 2022 · 3 comments · Fixed by #377
Closed

Stacked Borrows: not enough UB to justify noalias on Box #376

RalfJung opened this issue Nov 19, 2022 · 3 comments · Fixed by #377
Labels
A-aliasing-model Topic: Related to the aliasing model (e.g. Stacked/Tree Borrows)

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 19, 2022

The following example passes Miri, but has UB in LLVM:

unsafe fn test(mut x: Box<i32>, y: *const i32) -> i32 {
    // We will call this in a way that x and y alias.
    *x = 5;
    std::mem::forget(x);
    *y // this invalidates x, but that's fine since Box can be invalidated during the function
}

fn main() { unsafe {
    let mut v = 42;
    let ptr = &mut v as *mut i32;
    test(Box::from_raw(ptr), ptr);
} }

The reason for this is that we allow a Box pointer to be invalidated while test runs (which is necessary because the function might deallocate it), so Stacked Borrows says it is fine to use an aliasing pointer (y) while test runs as long as we don't use x again afterwards.

@RalfJung RalfJung added the A-aliasing-model Topic: Related to the aliasing model (e.g. Stacked/Tree Borrows) label Nov 19, 2022
@saethlin
Copy link
Member

The implementation of Stacked Borrows in Miri deliberately doesn't create a protector for Box. It sounds to me like adding a protector here would be sufficient UB. Is that correct? But we can't add protectors for Box, because that would make drop(Box::new(1)) UB?

If the answers to my questions are yes, I wonder if we could fix this by weakening protectors so that they do not completely forbid deallocation. At least as far as I can understand, what LLVM wants is to insert speculative reads. If deallocating a protected pointer is for sure the last thing that a function does before returning, it seems to me like that would be compatible with inserting speculative reads.

@RalfJung
Copy link
Member Author

On &mut we cannot allow deallocation though since we do emit dereferenceable (and it means "dereferenceable for duration of function call").

If deallocating a protected pointer is for sure the last thing that a function does before returning, it seems to me like that would be compatible with inserting speculative reads.

The speculative read could still be inserted after that last thing. I don't see how this can work. (Leaving aside the question of how to formalize "the last thing a function does before returning".)

I have a pretty clear idea for a fix: we need a weaker kind of protector, that still disallows any action by other pointers that would invalidate this one, but allows this pointer itself to be used for deallocation.

@RalfJung
Copy link
Member Author

#377 describes the fix in some more detail, and rust-lang/miri#2684 implements it in Miri.

bors added a commit to rust-lang/miri that referenced this issue Nov 21, 2022
Stack borrows: weak protectors

This addresses the issue described in rust-lang/unsafe-code-guidelines#376.
RalfJung pushed a commit to RalfJung/rust that referenced this issue Nov 27, 2022
…, r=saethlin

Stack borrows: weak protectors

This addresses the issue described in rust-lang/unsafe-code-guidelines#376.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-aliasing-model Topic: Related to the aliasing model (e.g. Stacked/Tree Borrows)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants