Annotate all unsafe blocks with a safety comment and prevent regressions

[joshlf]

As part of #61, we need to make sure that all unsafe code is proven to be sound. Currently, all unsafe code is either documented with a safety comment (// SAFETY: ...), or is marked with a TODO that references this issue. Our goal is to reach 100% safety comment coverage, and to not regress once we've reached 100%. To that end, we enforce the clippy::undocumented_unsafe_blocks lint to prevent regressions.

A note on linting: It'd be nice to be able to replace the top-level #![deny(clippy::undocumented_unsafe_blocks)] with a forbid once all TODOs are burned down, but unfortunately our safety_comment! macro relies on the ability to use #[allow(clippy::undocumented_unsafe_blocks)], so we have to settle for a deny.

In order to ensure that our soundness is forwards-compatible, safety comments must satisfy the following criteria:

• Safety comments must constitute a (possibly informal) proof that all of Rust's soundness rules are upheld
• Safety comments must not rely on any statements which do not appear in the stable versions of the Rust reference or standard library documentation (ie, the docs for core, alloc, and std); arguments which rely on text from the beta or nightly versions of these documents are not considered complete
• All statements from the reference or standard library documentation which are relied upon for soundness must be quoted in the safety comment. This ensures that there is no ambiguity as to what aspect of the text is being cited. This is especially important in cases where the text of these documents changes in the future. Such changes are of course required to be backwards-compatible, but may change the manner in which a particular guarantee is explained.

Mentoring instructions

• Easy tasks
  • Find any safety comment which links to a version of the Rust reference or stdlib docs that uses a beta or nightly link, and replace it with a link to the stable version of the same docs
• Medium tasks
  • Find any safety comment which cites the reference or stdlib docs without quoting the text it is citing. Fix this by quoting any text on which the safety comment relies for soundness.
• Hard tasks
  • See the list of suggested TODOs at the bottom of this issue, or find any instance of TODO(#429) in a comment; leave a GitHub comment on this issue to claim that instance to avoid duplicated work. Write a safety comment that abides by the requirements listed above.

Feel free to ask for help here if you're stuck or have questions!

List of suggested safety comments

List

This list contains safety comments which are good starter safety comments if you're not already familiar with writing them.

• zerocopy/src/lib.rs

  Lines 757 to 767 in f001cf2

  	// TODO(#429): Add a "SAFETY" comment and remove this `allow`.
  	#[allow(clippy::undocumented_unsafe_blocks)]
  	let ptr = unsafe { alloc::alloc::alloc_zeroed(layout).cast::<Self>() };
  	if ptr.is_null() {
  	alloc::alloc::handle_alloc_error(layout);
  	}
  	// TODO(#429): Add a "SAFETY" comment and remove this `allow`.
  	#[allow(clippy::undocumented_unsafe_blocks)]
  	unsafe {
  	Box::from_raw(ptr)
  	}

• zerocopy/src/lib.rs

  Lines 757 to 767 in f001cf2

  	// TODO(#429): Add a "SAFETY" comment and remove this `allow`.
  	#[allow(clippy::undocumented_unsafe_blocks)]
  	let ptr = unsafe { alloc::alloc::alloc_zeroed(layout).cast::<Self>() };
  	if ptr.is_null() {
  	alloc::alloc::handle_alloc_error(layout);
  	}
  	// TODO(#429): Add a "SAFETY" comment and remove this `allow`.
  	#[allow(clippy::undocumented_unsafe_blocks)]
  	unsafe {
  	Box::from_raw(ptr)
  	}

[ShahRishi]

Hi, is this issue still open?

[jswrenn]

Yes! We have ~20 instances of #[allow(clippy::undocumented_unsafe_blocks)] in our codebase. If you're interested in driving that number down, I recommend you tackle them (at first) one-at-a-time. We have a high bar for safety comments, and it can take some practice to get the hang of. See the comments in ptr.rs for examples of the sort of safety comments we're looking for. Comments should, ideally:

• enumerate the safety preconditions of the function, and explain why each one is satisfied
• whenever possible, cite official Rust documentation regarding guarantees made by the language
• when such guarantees are absent, document the absence of the guarantee and link to an issue in the zerocopy repo tracking getting that guarantee in writing somewhere official.

[ShahRishi]

Yes, I am interested in this, thank you!