Unsound unsafe usages

[Nugine]

clippy::mut_from_ref

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

clippy::uninit_vec

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

[lucacasonato]

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.

[Nugine]

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?

[aapoalas]

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.

[bartlomieju]

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?

Can you point what is wrong with these comments?

[Nugine]

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

clippy::uninit_vec:

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

[bartlomieju]

Thanks, if you'd like to contribute and fix these problem that would be highly appreciated.

[RalfJung]

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.

[stale[bot]]

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.

[Nugine]

Related PRs:

• https://github.com/denoland/deno/pull/15025
• https://github.com/denoland/rusty_v8/pull/1019
• https://github.com/denoland/deno/pull/16189