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

Should drop_in_place imply dereferencability? #373

Open
JakobDegen opened this issue Nov 2, 2022 · 6 comments
Open

Should drop_in_place imply dereferencability? #373

JakobDegen opened this issue Nov 2, 2022 · 6 comments
Labels
C-open-question Category: An open question that we should revisit

Comments

@JakobDegen
Copy link
Contributor

JakobDegen commented Nov 2, 2022

The following code is currently DB as reported by Miri:

#[repr(transparent)]
struct LoudDrop(u8);

impl Drop for LoudDrop {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}

#[repr(C)]
struct PartiallyDrop(LoudDrop, u8);

pub fn main() {
    let mut x = LoudDrop(0);
    let p = std::ptr::addr_of_mut!(x).cast::<PartiallyDrop>();
    
    unsafe {
        core::ptr::drop_in_place(p);   
    }
}

Should it be?

@JakobDegen
Copy link
Contributor Author

(I've checked and the LLVM IR we currently emit for this does not have UB)

@RalfJung
Copy link
Member

RalfJung commented Nov 2, 2022

If the example had comments, not every reader would have to reverse engineer what the question is about. ;)
The point is that at the drop_in_place, p points to x, a LoudDrop, but at type PartiallyDrop it is (partially) dangling beyond the end of x.

I think @pcwalton also recently asked to treat the drop_in_place argument more like a reference for aliasing purposes, and I am inclined to agree. Probably the generated drop_in_place shim should start with a reborrow (and retag) of the argument as a mutable reference?

@RalfJung RalfJung added the C-open-question Category: An open question that we should revisit label Nov 2, 2022
@pcwalton
Copy link

pcwalton commented Nov 2, 2022

Treating drop_in_place as &mut would be great. BTW I plan to add dereferenceable and align attributes to it, which are needed to trigger argument promotion, which helps a lot with removing memcpys.

@pcwalton
Copy link

pcwalton commented Nov 3, 2022

By the way, https://doc.rust-lang.org/std/ptr/fn.drop_in_place.html#safety already says that to_drop must be valid or UB will result, which seems to imply dereferenceable, no? (It doesn't say valid for how many bytes, but I assume it has to mean valid for the entire size of the type, or else that isn't a very useful definition of "valid", as it otherwise would only mean the 1 byte the pointer is pointing at is valid…)

@JakobDegen
Copy link
Contributor Author

That seems like a reasonable reading of the documentation. We'll still want to check how much of the ecosystem actually complies with this though (sorry, didn't get the branch up yesterday)

@RalfJung
Copy link
Member

RalfJung commented Nov 3, 2022

Yeah, on the library API front we are probably good, we just haven't made this into language UB yet, at least not in Miri.

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 22, 2022
Retag as FnEntry on `drop_in_place`

This commit changes the mir drop shim to always retag its argument as if it were a `&mut`.

cc rust-lang/unsafe-code-guidelines#373
RalfJung pushed a commit to RalfJung/miri that referenced this issue Dec 24, 2022
Retag as FnEntry on `drop_in_place`

This commit changes the mir drop shim to always retag its argument as if it were a `&mut`.

cc rust-lang/unsafe-code-guidelines#373
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-open-question Category: An open question that we should revisit
Projects
None yet
Development

No branches or pull requests

3 participants