core::ptr::replace implementation is unsound since Rust 1.80

[orlp]

Consider the following program:

fn main() {
    use core::ptr::*;
    unsafe { dbg!(replace(null_mut(), ())); }
}

This was made a legal program in Rust 1.80, the preconditions state:

• dst must be valid for both reads and writes.
• dst must be properly aligned.
• dst must point to a properly initialized value of type T.

This is because in Rust 1.80 the following line was added to valid:

For operations of size zero, every pointer is valid, including the null pointer. The following points are only concerned with non-zero-sized accesses.

Nevertheless, running the above program results in undefined behavior:

error: Undefined Behavior: constructing invalid value: encountered a null reference
    --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:1227:22
     |
1227 |         mem::replace(&mut *dst, src)
     |                      ^^^^^^^^^ constructing invalid value: encountered a null reference
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: BACKTRACE:
     = note: inside `std::ptr::replace::<()>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:1227:22: 1227:31
note: inside `main`
    --> src/main.rs:3:19
     |
3    |     unsafe { dbg!(replace(null_mut(), ())); }
     |                   ^^^^^^^^^^^^^^^^^^^^^^^

---

Rather than changing the implementation of core::ptr::replace, or an isolated change to the requirements of core::ptr::replace, I would like to see the change to the definition of a valid pointer for ZSTs reverted, I've expanded more about my thoughts and provided more arguments in this T-opsem zulip thread. In short, I believe that unsoundness similar to the case here in core::ptr::replace will keep happening as long as "dereferencing a pointer" and "reborrowing a pointer" have different requirements (which they do since Rust 1.80).

[RalfJung]

Good catch, thanks! That is indeed an unintended consequence of the decision to allow zero-sized reads/writes from the null pointer.

Cc @rust-lang/opsem @rust-lang/lang

[RalfJung]

In short, I believe that unsoundness similar to the case here in core::ptr::replace will keep happening as long as "dereferencing a pointer" and "reborrowing a pointer" have different requirements (which they do since Rust 1.80).

To be fully precise, this is about the following two things not being equivalent any more:

1. The pointer is valid for reads of size_of::<T>() many bytes, and it is aligned to align_of::<T>(), and reading those bytes produces a valid representation of T
2. The pointer can be converted to a reference of type &T

[zhi-ping]

@rustbot claim

[RalfJung]

@zhi-ping How did you pick this issue? I would strongly recommend that you follow the advice we give in the dev guide and start with am issue that's explicitly listed as being suitable for beginners. It's not a good idea to pick something at random. This issue here involves discussing potential changes to the language semantics, it's not suited for someone new to the project.

[RalfJung]

@rust-lang/opsem so, what do we think about this? I entirely lost track of this issue and it seems the same goes for everyone else.

The discussion on Zulip was not fully conclusive.

1. Some people argued that "readable" and "convertible to a reference" should be the same property, i.e., we should make reading (and writing) a null pointer UB again even if the read has size zero. However, the team members that replied generally didn't agree: adding more UB is not, or at least not always, the right way to make Rust more teachable; we can also teach people a weaker "layer of the tower" where they avoid reading from null even though doing so would be "fine". Or, as Jubilee put it:

   the entire motivation for the change was not "suddenly, it's a good idea to shove null ZST pointers everywhere!" but rather "we agreed that the blender should only turn on when it has something to actually blend, if you happen to put your foot in it and the blender does not automatically activate then you should consider yourself fortunate rather than continuing to find ways to tempt fate".

   On top of that, while "readable" and "convertible to a reference" being different indicates a loss of symmetry, there is other symmetry we gained: "dereferenceable for N bytes" and "can be inbounds-offset for N bytes" are now the same thing.

2. If we don't change what is UB, this becomes more of a @rust-lang/libs-api question, though functions like ptr::read/write are so fundamental that we may want to keep the lang team in the loop as well. Specifically, we could resolve this by changing the definition that the library uses for "valid for reads" to again require "is not null" (and similar for writes, which I won't keep repeating). Doing so would make null_ptr.read() be library UB even when it is not language UB.

3. The third option is to keep the notion of "valid for reads" unchanged, and instead change ptr::replace to simply return early doing nothing when T is a ZST -- that's known at compile-time, so it has no overhead. However, we should then audit all other functions that use "valid for reads/writes" as their precondition to ensure they behave correctly when used for ZST accesses on a null pointer.

Personally I prefer option 3. I don't like the idea of ptr.read() being UB when *ptr is not.