deno
deno copied to clipboard
Unsound unsafe usages
https://github.com/denoland/deno/blob/4e92f38d2ced6cbc2c7ed13d79d739dd4ddadb4c/core/ops_metrics.rs#L56-L62
It is not safe to create &mut [u8]
which is pointing to uninitialized memory.
See also https://github.com/rust-lang/unsafe-code-guidelines/issues/71
https://github.com/denoland/deno/blob/4e92f38d2ced6cbc2c7ed13d79d739dd4ddadb4c/ext/net/ops_tls.rs#L378-L382
https://github.com/denoland/deno/blob/4e92f38d2ced6cbc2c7ed13d79d739dd4ddadb4c/serde_v8/de.rs#L679-L692
https://github.com/denoland/deno/blob/4e92f38d2ced6cbc2c7ed13d79d739dd4ddadb4c/serde_v8/de.rs#L708-L720
https://github.com/denoland/deno/blob/4e92f38d2ced6cbc2c7ed13d79d739dd4ddadb4c/serde_v8/magic/bytestring.rs#L52-L66
https://github.com/denoland/deno/blob/4e92f38d2ced6cbc2c7ed13d79d739dd4ddadb4c/serde_v8/magic/u16string.rs#L35-L49
https://github.com/denoland/deno/blob/4e92f38d2ced6cbc2c7ed13d79d739dd4ddadb4c/snapshots/lib.rs#L30-L53
What is this?
https://github.com/denoland/deno/blob/4e92f38d2ced6cbc2c7ed13d79d739dd4ddadb4c/serde_v8/magic/value.rs#L30-L48
Related (maybe):
- https://github.com/denoland/deno/issues/12341
- https://github.com/denoland/deno/issues/12653
- https://github.com/denoland/deno/issues/12680
https://github.com/denoland/deno/issues/12653 is not related. It is related to FFI, which is inherently unsound.
It is not safe to create &mut [u8] which is pointing to uninitialized memory.
Correct, which is why we don't do that. We create a fixed size uninitialized contiguous chunk of memory, then initialize it, and only once it is initialize it treat it as safe. Did you take a look at the SAFETY
comments on top of the relevant code?
Same goes for cases of uninit_vec
.
I am going to close this issue, because it is not actionable. You are pointing out unsafe
blocks that are already marked with safety comments with explanations, or clippy ignores with explanations. If you have actionable feedback for how to turn some of this unsafe code safe, please comment here, or create new issues.
The SAFETY
comments are wrong. They are not safe.
I think this issue should be left open until they are fixed.
Does deno
follow the rules how rust treats unsoundness? Or it has its own rules?
Some seemingly related writings on the serde write_utf8
vec usage (well, similar) and SmallVec::with_capacity
usage:
- https://github.com/rust-lang/rust-clippy/issues/4483
- https://shnatsel.medium.com/how-ive-found-vulnerability-in-a-popular-rust-crate-and-you-can-too-3db081a67fb
I don't know that deeply but I think some of these at least have a sound
MaybeUninit
implementation in existence.
The
SAFETY
comments are wrong. They are not safe. I think this issue should be left open until they are fixed. Doesdeno
follow the rules how rust treats unsoundness? Or it has its own rules?
Can you point what is wrong with these comments?
Can you point what is wrong with these comments?
It is not safe to create &mut [u8]
which is pointing to uninitialized memory.
See also:
- https://doc.rust-lang.org/nomicon/uninitialized.html
- https://doc.rust-lang.org/stable/std/ptr/macro.addr_of_mut.html
- https://github.com/rust-lang/unsafe-code-guidelines/issues/71
It creates a Vec with uninitialized data, which leads to undefined behavior with most safe operations. Notably, uninitialized Vec
must not be used with generic Read. Moreover, calling set_len() on a Vec created with new() or default() creates out-of-bound values that lead to heap memory corruption when used.
And the last one:
// SAFETY: not fully safe, since lifetimes are detached from original scope
Thanks, if you'd like to contribute and fix these problem that would be highly appreciated.
FWIW https://github.com/rust-lang/unsafe-code-guidelines/issues/71 is about by-value integers/floats. This here also involves references, so https://github.com/rust-lang/unsafe-code-guidelines/issues/77 is relevant, too.
Indeed &mut [u8]
pointing to uninit data (as it is created e.g. here, I think) is wrong according to the Reference. However Miri accepts such code because it does not "look behind" an &mut
when checking whether everything is properly initialized. It would still be better to avoid this situation, but this is much less critical than having a by-value uninitialized integer/float.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.
Related PRs:
- https://github.com/denoland/deno/pull/15025
- https://github.com/denoland/rusty_v8/pull/1019
- https://github.com/denoland/deno/pull/16189