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

Tracking Issue for box_as_ptr #129090

Open
1 of 4 tasks
RalfJung opened this issue Aug 14, 2024 · 2 comments
Open
1 of 4 tasks

Tracking Issue for box_as_ptr #129090

RalfJung opened this issue Aug 14, 2024 · 2 comments
Labels
A-box Area: Our favorite opsem complication C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Aug 14, 2024

Feature gate: #![feature(box_as_ptr)]

This is a tracking issue for Box::as_ptr and Box::as_mut_ptr.

Public API

impl<T: ?Sized, A: Allocator> Box<T, A> {
    pub fn as_mut_ptr(b: &mut Self) -> *mut T;
    pub fn as_ptr(b: &Self) -> *const T;
}

Steps / History

Unresolved Questions

  • None yet.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@RalfJung RalfJung added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 14, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 25, 2024
add Box::as_ptr and Box::as_mut_ptr methods

Unstably implements rust-lang/libs-team#355. Tracking issue: rust-lang#129090.

r? libs-api
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 25, 2024
add Box::as_ptr and Box::as_mut_ptr methods

Unstably implements rust-lang/libs-team#355. Tracking issue: rust-lang#129090.

r? libs-api
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 25, 2024
Rollup merge of rust-lang#129091 - RalfJung:box_as_ptr, r=Amanieu

add Box::as_ptr and Box::as_mut_ptr methods

Unstably implements rust-lang/libs-team#355. Tracking issue: rust-lang#129090.

r? libs-api
@workingjubilee workingjubilee added the A-box Area: Our favorite opsem complication label Oct 1, 2024
@orlp
Copy link
Contributor

orlp commented Oct 6, 2024

The current implementation of as_mut_ptr using addr_of_mut still suffers from the fact that moving the Box invalidates the pointer, which seems like a pretty big defect:

#![feature(box_as_ptr)]

fn main() {
    let mut b = Box::new(42);
    let p = Box::as_mut_ptr(&mut b);
    let _b2 = b;
    unsafe { *p = 0; };
}
error: Undefined Behavior: attempting deallocation using <1988> at alloc1024, but that tag does not exist in the borrow stack for this location
    --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1679:17
     |
1679 |                 self.1.deallocate(From::from(ptr.cast()), layout);
     |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempting deallocation using <1988> at alloc1024, but that tag does not exist in the borrow stack for this location
     |
     = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
     = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <1988> was created by a Unique retag at offsets [0x0..0x4]
    --> src/main.rs:6:15
     |
6    |     let _b2 = b;
     |               ^
help: <1988> was later invalidated at offsets [0x0..0x4] by a write access
    --> src/main.rs:7:14
     |
7    |     unsafe { *p = 0; };
     |              ^^^^^^
     = note: BACKTRACE (of the first span):
     = note: inside `<std::boxed::Box<i32> as std::ops::Drop>::drop` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1679:17: 1679:66
     = note: inside `std::ptr::drop_in_place::<std::boxed::Box<i32>> - shim(Some(std::boxed::Box<i32>))` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:574:1: 574:56
note: inside `main`
    --> src/main.rs:8:1
     |
8    | }
     | ^

@RalfJung
Copy link
Member Author

RalfJung commented Oct 6, 2024

Yes, that is currently expected. Moving the Box asserts full uniqueness of the Box, just like a mutable reference.

Also see the discussion at rust-lang/unsafe-code-guidelines#326.

apljungquist added a commit to AxisCommunications/acap-rs that referenced this issue Oct 23, 2024
If we like the deferred pattern and keep it we may want to align with
axevent where it is implemented like below:

```rust
struct Deferred {
    func: Option<Box<dyn FnOnce()>>,
}

impl Drop for Deferred {
    fn drop(&mut self) {
        let func = self.func.take().unwrap();
        func();
    }
}

impl Deferred {
    pub fn new<F>(func: F) -> Self
    where
        F: FnOnce() + 'static,
    {
        Self {
            func: Some(Box::new(func)),
        }
    }
}
```

But I think I forgot to reason about the safety because now that I look
at it I think we need to take care that F is Send.

Another implementation that I experimented with is storing the callback
as `Option<Box<dyn Any>>` and using `ptr::addr_of_mut!` to get a
pointer for the FFI call. This feels nice because there is less
indirection but:
- It still doesn't capture the Send requirement.
- I'm not sure if moving the Box makes it ub[^1]

[^1]: rust-lang/rust#129090
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-box Area: Our favorite opsem complication C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants