Creating a slice of uninit memory is unsound (equivalent to mem::uninitialized)

[Shnatsel]

flate2 creates slices of uninitialized memory in several places. The only two places where it happens when using the Rust backend are here and here.

This is equivalent to the use of the now-deprecated mem::uninitialized. Instead, either a slice of MaybeUninit<T> should be constructed and passed to the backends, or the backends should receive a structure that does not expose uninitialized memory by design such as Vec or a Vec-like fixed-capacity view of memory.

If the backend does not overwrite the entire slice, this can become an exploitable security vulnerability.

[fintelia]

What is the status of this? Are these concerns purely hypothetical, or is there potentially unsoundness in the library?

[Manishearth]

This is potential unsoundness; the current Rust memory model (and the current LLVM memory model) treats uninits specially.

[Shnatsel]

You can find more details about the way unitialized memory is treated by C and Rust here: https://www.ralfj.de/blog/2019/07/14/uninit.html

flate2 itself does not read from the backing slice, but the compiler is allowed to insert spurious reads for optimization purposes. Some examples (although for different kinds of UB) can be found here.

The nightly-only BorrowedCursor would make fixing this easier, but alas, it's nightly-only. The reasonable fix I can see is calling .assume_init() only after the slice is filled and only on the filled parts for C libraries, and using miniz_oxide's output to Vec to avoid going through unsafe in the pure-rust codepath.

[oyvindln]

For the rust back-end we shouldn't really need to be using unsafe stuff unless there is some very good reason for it. Not sure if say avoiding zero-initializing a vector is worth it. miniz_oxide already avoids using unsafe without any drastic performance implications, so not sure why flate2 would need to either.

[anforowicz]

FWIW I've attempted to address this by switching to naive safe code (using vec.resize(capacity, 0) to fill the spare capacity with zeros). See the PR here: #373

Going via &mut [MaybeUninit<u8>] may also be possible, but it would require some extra considerations:

• Vec::spare_capacity_mut has been introduced in Rust 1.60.0. Thankfully this seems compatible with this crate's MSRV policy
• MaybeUninit<u8> support would have to be plumbed into the following places which currently write their output to &mut [u8]:
  • Compress::compress which passes output to DeflateBackend::compress which has the following impls:
    • C-based DeflateBackend::compress
    • Rusty DeflateBackend::compress which calls into
      • deflate in miniz_oxide
  • Decompress::decompress
    • C-based InflateBackend::decompress
    • Rusty InflateBackend::decompress which calls into
      • inflate in miniz_oxide

[Shnatsel]

If you are creating a vector from scratch, using vec![0; capacity] is faster than vec.resize(capacity, 0) because it requests already-zeroed memory from the OS.

[anforowicz]

If you are creating a vector from scratch, using vec![0; capacity] is faster than vec.resize(capacity, 0) because it requests already-zeroed memory from the OS.

Ack.

Here (e.g. in compress_vec) we are not creating a Vec from scratch, but instead we are working with the extra space in the spare capacity of an existing Vec. (OTOH, I guess that the caller of compress_vec may very well call Vec::with_capacity to crate a Vec from scratch.)

Still, maybe using vec.resize(capacity, 0) is unnecessarily pessimizing the performance here. One alternative would be to use Vec::spare_capacity_mut and then (somehow... waving hands...) use unsafe code to fill the spare capacity with zeros and treat it as &mut [u8].

• I think that slice::fill doesn't help, because it requires an already-initialized &mut [u8] as input
• FWIW https://users.rust-lang.org/t/zeroing-a-slice-of-integers/33825 mentions
  • safemem::write_bytes - takes &mut [u8] as input and therefore doesn't help
  • zeroize - also doesn't work with MaybeUninit
  • ptr::write_bytes - maybe that's what we have to use (if Vec::resize is a no-go):

So, maybe I should change the PR to use a little bit of unsafe as follows?:

fn write_to_spare_capacity_of_vec<T>(
    output: &mut Vec<u8>,
    writer: impl FnOnce(&mut [u8]) -> (usize, T),
) -> T {
    let zeroed_spare_space: &mut [u8] = {
        let uninited_spare_space = output.spare_capacity_mut();
        let ptr = uninited_spare_space.as_mut_ptr() as *mut u8;
        let len = uninited_spare_space.len();
        unsafe {
            // Safety of `write_bytes` and `from_raw_parts_mut`:
            // * `ptr` is non-null and points to `len` bytes within a single allocated object
            //   (guaranteed by `spare_capacity_mut` returning `&mut [MaybeUninit<u8>]`).
            // * Alignment of `u8` is 1 (so, not a concern)
            core::ptr::write_bytes(ptr, 0, len);

            // Safety of `from_raw_parts_mut`:
            // * `ptr points to `len` consecutive properly initialized values of type T
            //   (guaranteed by our call to `ptr::write_bytes`).
            // * The memory referenced by the returned slice must not be accessed through any other
            //   pointer (not derived from the return value) for the duration of lifetime 'a
            //   (guaranteed by exclusivity of `&mut [T]` returned by `spare_capacity_mut`).       
            core::slice::from_raw_parts_mut(ptr, len)
        }
    };

    let (bytes_written, ret) = writer(zeroed_spare_space);

    // Safety: Usage of `core::cmp::min` sanitizes `bytes_written` (making `set_len` safe even if
    // `writer` misbehaves and returns an arbitrary `bytes_written`).  Note that above we have
    // already made sure that all spare-capacity-bytes are initialized via `ptr::write_bytes`).
    let new_len = core::cmp::min(output.len() + bytes_written, output.capacity());
    unsafe { output.set_len(new_len); }

    ret
}

WDYT?

[Shnatsel]

I don't see why any of those solutions would be better than vec.resize(capacity, 0).

We could avoid re-initializing the same part over and over by keeping track what part of the Vec has already been initialized in previous iterations, but that relies on no other code running mem::replace on our Vec and also the Vec never reallocating, which is very fragile.

BorrowedBuf + BorrowedCursor could help here, but that's nightly-only.

[Shnatsel]

The approach of writing to the spare capacity without zeroing it first sounds good, provided that the C library wrappers are adjusted to write to a &mut [MaybeUninit<u8>].

[anforowicz]

The approach of writing to the spare capacity without zeroing it first sounds good, provided that the C library wrappers are adjusted to write to a &mut [MaybeUninit<u8>].

Yes, that would work for the C backend.

What about the Rust backend though? The Rust backend would also need to be adjusted to write to a &mut [MaybeUninit<u8>], right? If so, then a prerequisite for this approach would be to also change the miniz_oxide crate as outlined in #220 (comment). Do you think that we should pursue such changes to the miniz_oxide crate?

[Shnatsel]

miniz_oxide always zeroes the output buffer for safety even when writing to a Vec: https://docs.rs/miniz_oxide/0.7.1/src/miniz_oxide/deflate/mod.rs.html#121-157

I think it's best to just zero the buffer when using miniz_oxide instead of revamping its internals to use MaybeUninit.