read_mp4 can expose uninitialised data to third party code (unsound)

[nox]

read_mp4 calls read_moov which calls read_pssh which calls read_buf which calls allocate_read_buf and passes its result to Read::read.

mp4parse-rust/mp4parse/src/lib.rs

Lines 2320 to 2321 in 26e614b

	if let Ok(mut buf) = allocate_read_buf(size) {
	let r = src.read(&mut buf)?;

But the trait Read is not unsafe, and it is never guaranteed to limit itself to writing to its single argument buf, and the result of allocate_read_buf is a vector of uninitialised bytes (that function should be unsafe, btw).

There were discussions about introducing a new unsafe trait in libstd to signal that a Read implementation doesn't read in the writer it's supposed to write to, I don't know what became of it but I just found out about rust-lang/rust#42788 which seems related.

[kinetiknz]

Thanks for filing this. I wasn't aware of that pitfall for Read, but now that I've re-read the trait's documentation I understand this can happen.

In this particular case:

• Only the "mp4parse_fallible" path of allocate_read_buf can return an uninitialized vec.
• The "mp4parse_fallible" feature is only safe to enable within Gecko due to guaranteed allocator sharing between subsystems.
• Gecko only ever passes in a Read implementation that does not read from the buffer passed in to Read::read.

So this problem can't happen in practice in this code.

However, it's easy to address by initializing the buffer in the "mp4parse_fallible" path and seems worth doing since otherwise allocate_read_buf should be marked unsafe as you mentioned.