add safety sections for three functions in core::mem

[hxuhack]

View all comments

This pull request adds Safety sections for three library APIs in the core::mem module:

• mem::uninitialized
• mem::zeroed
• mem::transmute_copy

r? @Mark-Simulacrum

[Copilot]

Pull request overview

This PR improves the API documentation for unsafe/legacy core::mem APIs by adding explicit # Safety sections, helping users understand the invariants required to call these functions soundly.

Changes:

• Added a # Safety section to mem::zeroed describing the “all-zero must be valid for T” requirement.
• Added a # Safety section to deprecated mem::uninitialized to discourage use.
• Added a # Safety section to mem::transmute_copy listing key caller obligations.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

View changes since this review

[bjorn3]

These safety sections are duplicating what is already said below. They also cause text that was previously not part of any section to be part of the safety section.

Also would you mind disabling the option to automatically request copilot reviews in your user settings? These copilot reviews are still shown to people like me who have disabled every copilot related option that github offers.

[hxuhack]

@bjorn3 I wasn't aware that Copilot had permission to provide automatic reviews for this repository. I have disabled it now. Sorry about that.

Also would you mind disabling the option to automatically request copilot reviews in your user settings? These copilot reviews are still shown to people like me who have disabled every copilot related option that github offers.

[hxuhack]

@bjorn3

These safety sections are duplicating what is already said below.

In the first commit, I tried to preserve the original text, although I also noticed the duplication.
I have now removed such content that was duplicated elsewhere in the Safety section.

They also cause text that was previously not part of any section to be part of the safety section.

I have reorganized the Safety section so that all text before the Example section is now relevant to safety.
This issue should be resolved with these changes.

Does this version work for you?

[hxuhack]

The case about mem::transmute_copy is still under discussion on zulip.

[hxuhack]

@RalfJung

As you mentioned on zulip, it seems incorrect to state that

users must ensure that creating the returned value does not violate Rust's aliasing rules

for mem::transmute_copy.

What would you suggest should be documented here instead as the precondition?

[RalfJung]

You seem to have misunderstood what I said. "Rust's aliasing rules" refers to two different things, depending on context:

• The language-level rules assumed by the compiler (Stacked/Tree Borrows) -- at least in t-opsem, this is what we mean most of the time. These must always be followed everywhere, or else we have UB.
• The type-system-level rules -- this is what you seem to mean. These can be temporarily violated within a module, but cannot be violated across modules as that would be unsound and could indirectly lead to UB.

This is the same dichotomy as in my old blog post. It is a fundamental aspect of working with unsafe Rust that your model and PR do not seem to properly capture.

[hxuhack]

Ok, thanks, Ralf, for the clarification. This operates at the type-system level.
I assume it is appropriate to refer to the aliasing rules described in Breaking the pointer aliasing rules, since they apply at both the language and type-system levels.

[Mark-Simulacrum]

I don't think this should be in a safety section. The function is already deprecated (signaling it shouldn't be used).

The safety requirement here is ultimately equivalent to MaybeUninit::assume_init's, everything else here is describing that being hard to ensure without the 'initialization' phase MaybeUninit provides.

[hxuhack]

They are different.
mem::uninitialized directly produces an invalid value, which leads to language-level UB. For example, if the value implements Drop and the program panics after creating it, the program may end up with a dropping uninitialised value during unwinding, which is UB. That is why this API has been deprecated.
MaybeUninit::assume_init is different. You can create a value of type MaybeUninit<T>, initialize it step by step, and finally call MaybeUninit::assume_init. This approach is safe even if T implements Drop and the program panics at any point during initialization.

There are many issues in the past, e.g., CVE-2019-15552, CVE-2019-15553.

That's why the doc of mem::uninitialized says "The reason for deprecation is that the function basically cannot be used correctly"

[Mark-Simulacrum]

I'm not disputing that it's much harder to use mem::uninitialized correctly. But ultimately the conditions on calling MaybeUninit::assume_init and mem::uninitialized are exactly the same: the returned value must be a valid value at the type. It's just that with mem::uninitialized you don't have an opportunity to do anything before hand, so most usage can't be correct.

[hxuhack]

Do you think we should add a Safety section here? If you don’t have a strong preference either way, then perhaps it would be reasonable to add one? One extra benefit is that Clippy already supports checking for missing safety documentation via the missing_safety_doc lint. So adding a Safety section would also align with Clippy’s expectations well.

[Mark-Simulacrum]

I think a Safety section is a good idea. But I don't think a Safety section should say "reserved for legacy code only", that doesn't seem appropriate. If you left this section to cover the actual safety requirement (discussed somewhat below after the newly added line), that seems fine to me.

[hxuhack]

To avoid duplication, I moved the last sentence of the original doc to the Safety section.

/// # Safety
///
/// It is immediate undefined behavior to call this function on nearly all types,
/// including integer types and arrays of integer types, and even if the result is unused.

Does this look good to you?

[Mark-Simulacrum]

This feels confusing to me. Half the point of using transmute_copy is when the two types don't have the same size (or at least not provably so to the compiler) -- otherwise you could just transmute in most cases.

I'm wondering if this should just defer to the Safety documentation of read_unaligned, which is what this boils down to AFAICT.

[hxuhack]

This feels confusing to me. Half the point of using transmute_copy is when the two types don't have the same size (or at least not provably so to the compiler) -- otherwise you could just transmute in most cases.

I copied this sentence from the original documentation without modification, although I also find it odd that it highlights “highly encouraged to only invoke this function where Src and Dst have the same size.” I think we should remove this sentence.

[hxuhack]

I also considered referring to the safety requirements of the callee, read_unaligned. However, I give up the choice for three reasons:

• The current safety requirements documented for read_unaligned focus on pointer validity, which is not a concern for this function..
• This function has an additional requirement regarding the sizes of Src and Dst.
• The current description of the library-level UB that may be introduced by read_unaligned is misleading. It states that it is unsafe if T is not Copy, which makes readers falsely think it would be safe if it T is Copy, as discussed in zulip.

[Mark-Simulacrum]

This function has an additional requirement regarding the sizes of Src and Dst.

I don't think this function has any additional safety requirement on those sizes? The requirement seems essentially the same to me: you must be able to read an value of size Dst from &Src.

[hxuhack]

Oh, right. There is already an assertion in the implementation, so it will panic instead of causing UB in this case.

rust/library/core/src/mem/mod.rs

Lines 1072 to 1076 in 5f36a7f

	pub const unsafe fn transmute_copy<Src, Dst>(src: &Src) -> Dst {
	assert!(
	size_of::<Src>() >= size_of::<Dst>(),
	"cannot transmute_copy if Dst is larger than Src"
	);

[hxuhack]

I’ve updated the documentation by removing the size requirement from the Safety section and instead adding a note about the panic behavior right after the API description:

The function will panic if [size_of::<Src>][size_of] is less than [size_of::<Dst>][size_of].

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[hxuhack]

@rustbot ready