Unsound unsafe usages

[Nugine]

clippy::mut_from_ref

deno/core/ops_metrics.rs

Lines 56 to 62 in 4e92f38

	#[allow(clippy::mut_from_ref)]
	#[inline]
	fn ops_mut(&self) -> &mut Vec<OpMetrics> {
	// SAFETY: `OpsTracker` is created after registering ops so it is guaranteed
	// that that `ops` will be initialized.
	unsafe { &mut *self.ops.get() }
	}

It is not safe to create &mut [u8] which is pointing to uninitialized memory.
See also rust-lang/unsafe-code-guidelines#71

deno/ext/net/ops_tls.rs

Lines 378 to 382 in 4e92f38

	// TODO(bartlomieju):
	#[allow(clippy::undocumented_unsafe_blocks)]
	let buf_slice =
	unsafe { &mut *(buf.unfilled_mut() as *mut [_] as *mut [u8]) };
	let bytes_read = self.tls.reader().read(buf_slice)?;

deno/serde_v8/de.rs

Lines 679 to 692 in 4e92f38

	let mut buf = Vec::with_capacity(capacity);
	let mut nchars = 0;
	let data = buf.as_mut_ptr();
	let length = s.write_utf8(
	scope,
	// SAFETY: we're essentially providing the raw internal slice/buffer owned by the Vec
	// which fulfills all of from_raw_parts_mut's safety requirements besides "initialization"
	// and since we're operating on a [u8] not [T] we can safely assume the slice's values
	// are sufficiently "initialized" for writes
	unsafe { std::slice::from_raw_parts_mut(data, capacity) },
	Some(&mut nchars),
	v8::WriteOptions::NO_NULL_TERMINATION
	| v8::WriteOptions::REPLACE_INVALID_UTF8,
	);

deno/serde_v8/de.rs

Lines 708 to 720 in 4e92f38

	let mut buf = Vec::with_capacity(capacity);
	let data = buf.as_mut_ptr();
	let length = s.write_utf8(
	scope,
	// SAFETY: we're essentially providing the raw internal slice/buffer owned by the Vec
	// which fulfills all of from_raw_parts_mut's safety requirements besides "initialization"
	// and since we're operating on a [u8] not [T] we can safely assume the slice's values
	// are sufficiently "initialized" for writes
	unsafe { std::slice::from_raw_parts_mut(data, capacity) },
	None,
	v8::WriteOptions::NO_NULL_TERMINATION
	| v8::WriteOptions::REPLACE_INVALID_UTF8,
	);

clippy::uninit_vec

deno/serde_v8/magic/bytestring.rs

Lines 52 to 66 in 4e92f38

	let len = v8str.length();
	let mut buffer = SmallVec::with_capacity(len);
	// SAFETY: we set length == capacity (see previous line),
	// before immediately writing into that buffer and sanity check with an assert
	#[allow(clippy::uninit_vec)]
	unsafe {
	buffer.set_len(len);
	let written = v8str.write_one_byte(
	scope,
	&mut buffer,
	0,
	v8::WriteOptions::NO_NULL_TERMINATION,
	);
	assert!(written == len);
	}

deno/serde_v8/magic/u16string.rs

Lines 35 to 49 in 4e92f38

	let len = v8str.length();
	let mut buffer = Vec::with_capacity(len);
	// SAFETY: we set length == capacity (see previous line),
	// before immediately writing into that buffer and sanity check with an assert
	#[allow(clippy::uninit_vec)]
	unsafe {
	buffer.set_len(len);
	let written = v8str.write(
	scope,
	&mut buffer,
	0,
	v8::WriteOptions::NO_NULL_TERMINATION,
	);
	assert!(written == len);
	}

deno/snapshots/lib.rs

Lines 30 to 53 in 4e92f38

	static CLI_SNAPSHOT: Lazy<Box<[u8]>> = Lazy::new(
	#[allow(clippy::uninit_vec)]
	#[cold]
	#[inline(never)]
	|| {
	static COMPRESSED_CLI_SNAPSHOT: &[u8] =
	include_bytes!(concat!(env!("OUT_DIR"), "/CLI_SNAPSHOT.bin"));
	let size =
	u32::from_le_bytes(COMPRESSED_CLI_SNAPSHOT[0..4].try_into().unwrap())
	as usize;
	let mut vec = Vec::with_capacity(size);
	// SAFETY: vec is allocated with exact snapshot size (+ alignment)
	// SAFETY: non zeroed bytes are overwritten with decompressed snapshot
	unsafe {
	vec.set_len(size);
	}
	lzzzz::lz4::decompress(&COMPRESSED_CLI_SNAPSHOT[4..], &mut vec).unwrap();
	vec.into_boxed_slice()
	},
	);

What is this?

deno/serde_v8/magic/value.rs

Lines 30 to 48 in 4e92f38

	impl ToV8 for Value<'_> {
	fn to_v8<'a>(
	&self,
	_scope: &mut v8::HandleScope<'a>,
	) -> Result<v8::Local<'a, v8::Value>, crate::Error> {
	// SAFETY: not fully safe, since lifetimes are detached from original scope
	Ok(unsafe { transmute(self.v8_value) })
	}
	}
	impl FromV8 for Value<'_> {
	fn from_v8(
	_scope: &mut v8::HandleScope,
	value: v8::Local<v8::Value>,
	) -> Result<Self, crate::Error> {
	// SAFETY: not fully safe, since lifetimes are detached from original scope
	Ok(unsafe { transmute::<Value, Value>(value.into()) })
	}
	}

Related (maybe):

• Using buffers with non-blocking FFI calls is unsound #12341
• Segmentation fault at exit #12653
• ZeroCopyBuf and MagicBuffer should not be Send or Sync deno_core#129

[lucacasonato]

#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:

• New lint: dangerous use of vec.set_len() rust-lang/rust-clippy#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
• Validity of integers and floating point rust-lang/unsafe-code-guidelines#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 rust-lang/unsafe-code-guidelines#71 is about by-value integers/floats.
This here also involves references, so rust-lang/unsafe-code-guidelines#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]

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.