ClaimOnOom Example's Correctness is Not Yet Definitive

[bjorn3]

The pointer returned by addr_of!() may only be used to read from the pointee. Writing through it is UB. Unfortunately using addr_of_mut!() it still unstable: rust-lang/rust#57349, so there is not a whole lot that can be done for now. One possible solution I can think of is to have a macro which expands to a ClaimOnOom like type, except hard coded for a single specified memory area. This way the address of this memory area only needs to be taken at runtime. If this macro also defines the static for the memory area to claim itself, then this may even allow it to be used entirely without any unsafe code as the macro can guarantee that the memory area is writable and not used for any other purposes.

[SFBdragon]

The pointer returned by addr_of!() may only be used to read from the pointee. Writing through it is UB.

I'm under the impression that this is not the case. I may be wrong, please just point me somewhere that spells this out (addr_of! doesn't mention it). In general, taking a pointer to a memory location is safe in Rust, casting the mutability of a pointer is safe in Rust, and there's nothing forbidden about writing through an originally-const ptr as far as I can tell in here https://doc.rust-lang.org/std/ptr/index.html#safety

Miri thinks the following code is fine:

static mut THING: usize = 1;

fn main() {
    let ptr = unsafe { std::ptr::addr_of!(THING) };
    unsafe {
    	*(ptr as *mut _) = 3;
    }
    let x = unsafe { *ptr };
    println!("The value of x is {}", x);
}

And it prints 3 and all that good stuff. (Of course, changing static mut THING to static THING causes MIRI to freak out, as expected.)

Either way, indeed, not having const_mut_refs makes this initialization code a lot less convenient than it ought to be. (using addr_of made it slightly less awful but it also feels very dirty, UB or not.)

[bjorn3]

Miri doesn't catch this as it currently conservatively assumes it is fine: https://github.com/rust-lang/rust/blob/7c4ac0603e9ee5295bc802c90575391288a69a8a/compiler/rustc_const_eval/src/transform/check_consts/resolver.rs#L100-L102 (AddressOf is what addr_of!() and addr_of_mut!() lower to in MIR) Turns out it isn't decided whether or not this is actually fine: rust-lang/rust#56604

If &THING as *const usize as *mut usize instead of addr_of!(THING) as *mut usize was used, this is definitively not fine though, even if it is turned into a *const pointer and then casted to a *mut pointer later. You can't ever write through a pointer derived from a shared reference without interior mutability.

[SFBdragon]

Thanks. I wasn't aware &THING as *const usize as *mut usize was UB, (or very-thoroughly forgot).

---

So it looks like this particular code is in the grey zone? I'm not hugely concerned by that. Perhaps const_mut_refs will drop before a decision is made on this ^_^ Definitely something I need to keep my eye on though. If addr_of! later disallows mutation through the returned pointer, I'll return to this and think about whether your suggestion is the best way to go about this (it makes sense to me though). Not implementing it now is also mainly a matter of thinking 1. not Talc's job (it's an allocator, where you get the pointers is ideally irrelevant to Talc) and 2. keep it simple. I'm not dead set on this though, if you have a good reason or two to do this now. (Otherwise I'm going to rename this issue and leave it open while rust-lang/rust#56604 is undecided and const_mut_refs isn't stable.)

Please let me know what you think of this path of action.

---

However, this means some of the API surface of Talc is genuinely problematic, particularly the From implementations on Span for &[T] and &[T; N]. Not UB in itself but encourages to an unacceptable degree IMO.

Here, the .into() is doing the legwork in hiding the issue as per the above. MIRI indeed takes issue with it.

fn main() {
    let mut arena = [0u8; 10000];

    unsafe {
        let mut talc = talc::Talc::new(talc::ClaimOnOom::new(arena.as_ref().into()));
        let x = talc.malloc(std::alloc::Layout::new::<[u64; 3]>()).unwrap();
        *x.as_ptr().cast::<[u64; 3]>() = [1, 2, 3];
    }
}

That's a big screw-up. Whoops. These conversion functions should not exist.

So... hurry a V5 out the door and yank everything after V4.2 ...? I don't know if this is reasonable. Will the compiler actually trip up on this yet? I believe it's not super aggressive in the interest of avoiding atypical LLVM execution paths but I stand to be corrected.

I've got quite a lot I want to put into a Talc V5 and don't have the time right now to do it. In a few weeks I'll have completed my exams so I'll have more time thereafter. This is why I'd prefer to delay it. But this stuff can just be a V6 if need be.

I'll go open an issue about this so long. I'm eager to hear your opinion on the severity of this.

[SFBdragon]

Opened #33 for the latter issue.

[bjorn3]

Please let me know what you think of this path of action.

Makes sense. I don't think there is much risk of this miscompiling in the short term.

So... hurry a V5 out the door and yank everything after V4.2 ...? I don't know if this is reasonable.

I don't think yanking is necessary. It is a footgun, but still requires unsafe, so it isn't unsound. Do deprecations work for impls? If so adding a deprecation in a patch release us the best option IMO. Otherwise I would suggest adding a doc comment to the impl to discourge using it and leave it at that until the next breaking release.

In a few weeks I'll have completed my exams so I'll have more time thereafter.

Good luck with your exams!