Change as_ptr to as_mut_ptr to fix Miri error

[jturner314]

Before, the example in the docs for ByteOrder::from_slice_i32 caused Miri to error with the message, "error: Undefined Behavior: trying to reborrow for Unique at alloc1389, but parent tag does not have an appropriate item in the borrow stack". Now it runs without errors (tested locally by creating an example and running it with cargo +nightly miri run --example the_example).

(This is the example in the Rust Playground. You can run it with Miri by selecting "Miri" from the "Tools" menu.)

Fwiw, I'm not sure if the original code really has undefined behavior or not, but this PR is a simple change, and the new code is a little clearer anyway.

[BurntSushi]

Thanks!

[BurntSushi]

This PR is on crates.io in byteorder 1.4.3.

[BurntSushi]

@RalfJung Would you be so kind as to help me understand why this is UB? Of course the change in and of itself makes the code clearer, and I trust that Miri is correct here. So I am not trying to argue this. But rather, I just don't understand. My understanding of const vs mut raw pointers is that their only difference is in variance, but one can otherwise freely change between them. Perhaps my mental model requires revision? :-)

[RalfJung]

My understanding of const vs mut raw pointers is that their only difference is in variance, but one can otherwise freely change between them. Perhaps my mental model requires revision? :-)

At least the way Miri is currently implemented, this is only almost correct. You can indeed freely cast between them to no effect, but casting a reference to a const ptr is different from casting a reference to a mut ptr. IOW, the kind of raw ptr matters at the transition point from the safe world of references to the unsafe world of raw pointers; it never matters again later in the "life of a raw pointer". If a raw pointer is "born" as *const T, then it is read-only (with the usual exception for UnsafeCell).

Note that long-term, this is not necessarily the model we will end up with, but due to "interesting" behavior of the borrow checker and Stacked Borrows limitations, this is currently the best we can do. Also see:

• Differences between *const T and *mut T. Initially *const T pointers are forever read-only? rust-lang/unsafe-code-guidelines#257
• Cast-to-raw: inherit permission from parent pointer? rust-lang/unsafe-code-guidelines#227
• Coercing &mut to *const should not create a shared reference rust-lang/rust#56604

[RalfJung]

Is the as ... even still needed? I think you can make the code slightly more type-safe by removing the casts. :D

(Raw-ptr-casts always make me nervous as they are so incredibly unchecked.)

[BurntSushi]

Looks like it, unfortunately. Because src is a slice of i16, but we want a slice of u16. (The endianness conversions are implemented on &[u16], and we want to reuse that implementation for a &[i16].)

[RalfJung]

Ah, right... If you like, you could use the cast method which makes sure that only the pointee type is changed (but mutability is not).

[BurntSushi]

@RalfJung Oh excellent, thank you so much for fixing my mental model. Appreciate it!

[jturner314]

Fwiw, one thing that surprised me is that running the tests through Miri (with cargo +nightly miri test) did not catch this, even though the tests call from_slice_i32 (via read_i32_into).

[RalfJung]

Possibly catching this as part of the tests requires -Zmiri-track-raw-pointers. Without that option, Miri might "confuse" the new read-only raw pointer with previously created read-write raw pointers, and hence grant read-write permission. However, with that option, int-to-ptr-casts are not properly supported.

[jturner314]

Thanks for pointing out that option; I didn't notice it in the README. I just tried running MIRIFLAGS='-Zmiri-track-raw-pointers' cargo +nightly miri test with byteorder 5d9d038 (the commit before this PR, which causes Miri to error on the example), but the tests didn't error in this case either. (Note: If anyone reading this wants to try it, make sure to have plenty of RAM; it took ~45 minutes and used ~16 GiB of memory.) It is puzzling. I'll try updating to the latest nightly. (I've been using 2021-02-10.)

[jturner314]

There is no change with the latest nightly (rustc 2021-03-09, miri 2021-03-02). Oh, well. I don't have time to pursue this further, but I thought it was interesting to point out.

[RalfJung]

Hm, so something must be different about how the test suite calls this than about how you called it in the code that triggered this PR...

[RalfJung]

@jturner314

even though the tests call from_slice_i32 (via read_i32_into).

read_i32_into calls read_u32_into, I do not see a call to from_slice_i32 there. Am I looking at the wrong thing?

[jturner314]

Oh, you're right. I'm sorry for the confusion. I misread the testing code. I was looking at ReadBytesExt::read_i32_into in io.rs, rather than ByteOrder::read_i32_into in lib.rs. ReadBytesExt::read_i32_into does call from_slice_i32, but the non-doc-tests use ByteOrder::read_i32_into, which does not. The only tests which ultimately call from_slice_i32 are the doc tests for from_slice_i32 and ReadBytesExt::read_i32_into, which Miri doesn't run.