Unsoundness in command_helpers.rs

[burakemir]

cc-rs/src/command_helpers.rs

Line 165 in e8b13ba

.read(unsafe { &mut *(buffer.spare_capacity_mut() as *mut _ as *mut [u8]) })

Our unsafe review found that this line constructs a reference that could expose uninitialized memory. The read API requires that initialized (e.g. zeroed) memory be used.

The docs for read say:
"... it is possible that the code that’s supposed to write to the buffer might also read from it. It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit) is not safe, and can lead to undefined behavior."

Now, the safety comment does says that we assume stderr never reads.
Yet, the contract for the read method provides no such guarantee.
Would it make sense to initialize?
Or provide better justification why stderr.read will never ever read the buffer?

[NobodyXu]

It's true that if you take a generic reader: impl Read, then you can't use a MaybeUninit, since the reader could read from the uninitialised buffer.

However we are using the <std::process::Stderr as Read>::read impl, and we know that it indeed doesn't read from it.

It's a bit of an implementation details but I don't think stdlib would break it.

[burakemir]

That makes sense, but note that you also create a reference to uninitialized memory - this is an issue independent of the API: see e.g. a discussion here rust-lang/flate2-rs#220 (comment)

[Manishearth]

https://doc.rust-lang.org/reference/behavior-considered-undefined.html

A reference or Box that is dangling, misaligned, or points to an invalid value (in case of dynamically sized types, using the actual dynamic type of the pointee as determined by the metadata).

Uninitialized memory counts as "invalid value" here. This is insta-UB, even if the created reference was completely unused.

This is a type of UB unique to Rust: so C intuition about UB (which depends on the implementation) does not apply here.

[NobodyXu]

This is a type of UB unique to Rust: so C intuition about UB (which depends on the implementation) does not apply here.

Thanks for explanation, in that case it does make sense for it to be removed.