#235 [Coding Guideline]: Do not create values from uninitialized memory #240
Conversation
✅ Deploy Preview for scrc-coding-guidelines ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I've made some improvement suggestions in a PR here: manhatsu#1 |
Thank you very much. Merged to this branch |
|
Hey @manhatsu 👋 it looks like from the CI that a new tag needs to be added. Could you follow what @rcseacord did in this PR to add the |
manhatsu
force-pushed
the
doc/no-uninit-value
branch
from
d3b29b5 to
57f303a
Compare
PLeVasseur
force-pushed
the
doc/no-uninit-value
branch
from
57f303a to
4b8cbc7
Compare
PLeVasseur
left a comment
PLeVasseur
left a comment
There was a problem hiding this comment.
Hi @manhatsu -- thank you for contributing. Please see the comment I left on how to generate a template.
PLeVasseur
added
chapter: values
coding guideline
There was a problem hiding this comment.
What @inkreasing says is correct. This description is insufficient to reflect the restrictions imposed by MIRI here:
Note this is not UB before line 15.
Bytes remain uninit until written. You may not read uninitialized bytes as any initialized type, period, not even if "all" bitpatterns are considered valid, because uninit is the 257th bitpattern for a byte, effectively: 0xUU. By contrast, u8 is 0x00 through 0xFF, inclusive. We use MaybeUninit<u8> to indicate the final state is possible, and it is valid to read that value (well, from any allocation that has a byte in it, at least).
src/coding-guidelines/values.rst
Outdated
| A program shall not create a value of any type from uninitialized memory, | ||
| except when accessing a field of a union type, | ||
| where such reads are explicitly defined to be permitted even if the bytes of that field are uninitialized. | ||
| It is prohibited to interpret uninitialized memory as a value of any Rust type such as a | ||
| primitive, aggregate, reference, pointer, struct, enum, array, or tuple. |
There was a problem hiding this comment.
This definition does not consider composition of types:
use std::mem::MaybeUninit;
union Uninit32 {
u: u32,
i: i32,
f: f32,
void: (),
}
struct Newtype32(Uninit32);
fn main() {
let x: Newtype32 = unsafe { MaybeUninit::uninit().assume_init() };
}This passes miri because all the bytes in Newtype32 are defined by Uninit32, which is allowed to be uninitialized.
When bytes are read as a type (e.g. by using ptr.read(), *ptr, or mem::transmute), a "typed read" occurs. This asserts the bytes are valid as that type. Uninit32 and MaybeUninit are the same thing here: unions with () as a possibility, which means they must be valid to read from a blob of uninitialized bytes within a valid allocation1. Because Newtype32 is entirely defined by Uninit32, it is also valid to read from uninitialized bytes: the struct wrapper does not impose a novel validity requirement. If this is a mandatory guideline, it should be more exacting about why.
Footnotes
-
Note that this is likely a stronger requirement than the actual rules will be regarding union validity once final details of those are hashed out. I'm just giving an example that is very "in the clear". ↩
| .. non_compliant_example:: | ||
| :id: non_compl_ex_Qb5GqYTP6db1 | ||
| :status: draft | ||
|
|
||
| This noncompliant example creates a value of type ``u32`` from uninitialized memory via | ||
| `assume_init <https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#method.assume_init>`_: | ||
|
|
||
| .. code-block:: rust | ||
| use std::mem::MaybeUninit; | ||
| let x: u32 = unsafe { MaybeUninit::uninit().assume_init() }; // UB |
There was a problem hiding this comment.
You got this right, but the lesson needs to be extended elsewhere: assume_init and the read of a union field at its type are not really different operations in the semantics, so why would u32 be valid to read from a union field?
Updated guidelines on uninitialized memory usage and added examples of compliant and non-compliant code.
Added noncompliant and compliant examples demonstrating safe /unsafe memory initialization in Rust.
manhatsu
force-pushed
the
doc/no-uninit-value
branch
from
9b81ff4 to
0e2776c
Compare
Clarify conditions for accessing union fields and update examples.
Updated examples to clarify compliant and non-compliant behavior with uninitialized padding in Rust.
Clarify conditions for accessing union fields and update non-compliant examples.
| - may violate niche or discriminant validity, | ||
| - may create invalid pointer values, or |
There was a problem hiding this comment.
I believe that these two would benefit of being further developed with their own examples. Going in-depth in the Rationale is fine: your goal is to make it obvious why this guideline is required. Explain as much as you find is needed.
| - creates undefined behavior for most types, | ||
| - may violate niche or discriminant validity, | ||
| - may create invalid pointer values, or | ||
| - may produce values that violate type invariants. |
There was a problem hiding this comment.
Like the first bullet point, I believe the last one needs to point to an official reference or documentation of some kind that explains in full why this is the case.
| :scope: system | ||
| :tags: undefined-behavior, unsafe | ||
|
|
||
| Do not create a typed value from uninitialized memory. |
There was a problem hiding this comment.
I would perhaps divide this guideline into two.
There is a very good guideline in this PR that deals with the creation of typed values from uninitialized memory by using functions like assume_init on memory that has not been fully initialized on the relevant bytes. That transition, from MaybeUninit<T> to T, &mut T and others, is Undefined Behavior.
But there is also another guideline in this PR that deals with how and when it's valid to access fields of unions. I think it's best for the two to be separate, since one of them deals with the creation of typed values and the other deals with the reading of typed values from unions.
Sidenote:
Keep in mind that MaybeUninit<T> is itself a union, so however the guideline that deals with access to fields of unions ends up being written, you will want to make sure you consider it among the exceptions ;)
There was a problem hiding this comment.
You might also want to take a look at these pages:
- The Rustonomicon on working with uninitialized memory from
unsafehttps://doc.rust-lang.org/nomicon/unchecked-uninit.html - The explanation of Initialization Invariant, examples and such, from the docs of
MaybeUninit. - The Language Reference on unions: https://doc.rust-lang.org/reference/items/unions.html
The first two contain most of what I think you'll need to complete this / these Guideline(s). I hope they help.
There was a problem hiding this comment.
I'll have to think on this a bit, as splitting guidelines is a lot of work.
Co-authored-by: Félix Fischer <felix91gr@users.noreply.github.com>
6 participants
Closes #235.