google / google/comprehensive-rust

Issues in Unsafe Deep Dive

Open
#3,021 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
33.4k
Forks
2.1k
Avg merge
1d 3h
Merged PRs (30d)
10

Description

Here's a list of issues in the Unsafe Deep Dive chapter which I don't feel confident to fix myself:
- The `may_overflow` [section](https://github.com/google/comprehensive-rust/blob/main/src/unsafe-deep-dive/introduction/may_overflow.md) in the introduction is incorrect. Arithmetic overflow as in `1 + i32::MAX` is [not undefined behavior](https://doc.rust-lang.org/reference/behavior-not-considered-unsafe.html?highlight=overflow-checks#integer-overflow). It is well defined to either panic or wrap-around at the implementation's discretion.
- There seems to be a leftover sentence in the "Arrays of uninit" [section](https://google.github.io/comprehensive-rust/unsafe-deep-dive/initialization/maybeuninit/arrays.html), namely the one about `slice_assume_init_ref`. It says the example uses `&buf[..input.len()]` while it doesn't.
- The [pinning chapter](https://google.github.io/comprehensive-rust/unsafe-deep-dive/pinning.html) contains an empty "details" in the speaker notes.
- The "What pinning is" [section](https://google.github.io/comprehensive-rust/unsafe-deep-dive/pinning/what-pinning-is.html) contains an unfinished sentence before the speaker notes, namely "This is explained in".
- In the same section, the sentence "A “pointer type” wraps every type that implements Deref with a target that implements Unpin." seems wrong. It's too imprecise to actually refute, but Unpin should not be a concern in this sentence.
- The outline in the "Self-Referential Buffer Example" [section](https://google.github.io/comprehensive-rust/unsafe-deep-dive/pinning/self-referential-buffer.html) is wrong. It's the one of the pinning chapter.
- The "With a raw pointer" [section](https://google.github.io/comprehensive-rust/unsafe-deep-dive/pinning/self-referential-buffer/rust-raw-pointers.html) states "`unsafe` blocks lack of safety comments. Therefore, this code is unsound." which is wrong. This Unsafe Deep Dive chapter uses the terminology "not safe" for `unsafe` blocks without safety comments. The terminology "unsound" means it is possible to trigger UB while satisfying safety preconditions.
- In the same section, the answer is insufficient. The fact that the object can move should be addressed. It should be mentioned that in that case, either cursor will point outside the expected data range (for non-overlapping moves) or it will still point inside but at a possibly incorrect position. In both cases, the rest of the function is sound due to this assertion.
- The "With Pin" [section](https://google.github.io/comprehensive-rust/unsafe-deep-dive/pinning/self-referential-buffer/rust-pin.html) feels unsatisfying. The `unsafe` blocks are still very large and the pinning guarantee is not used. The assertions are still there.
- The "Pin and Drop" [section](https://google.github.io/comprehensive-rust/unsafe-deep-dive/pinning/pin-and-drop.html) provides a confusing example for an incorrect drop implementation. I would expect something like `mem::take(&mut self.data)` instead of `ptr::read(&self.data)`. And even then, it's arguable how bad that would be since `self.ptr` is not used afterwards and the whole object is at its lifetime end.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.