Allocator wrapper implementation contains undefined behavior

[complexspaces]

Hey there,

While looking at Bitwarden's implementation of zeroizing dealloc, I noticed that there is some undefined behavior present:

• slice::from_raw_parts on an entire object is invalid. This is because structures and other types may contain padding bytes, which are not initialized during the allocation process. By making a slice from it, you are essentially converting MaybeUninit<u8> to u8 unsoundly. Rust is clear that integers may uninitialized.
• All of the tests, which are disabled on some platforms (out of curiosity, which ones?) are all full of UB. Even if its only in a test, reading memory from a freed heap object's pointer is undefined/unsound in basically every language, not just Rust. IMO the tests should be either deleted or replaced with a custom GlobalAlloc that keeps track of all allocations and can provide a sound API to check if "freed" memory was wiped with zeroes.

We (1Password) made a similar crate wrapper last year for our Core, and we just open sourced it today if you'd like to use it directly or as a reference to improve on this: https://github.com/1Password/zeroizing-alloc

[Hinton]

Thanks for digging into this @complexspaces. We always appreciate outside reviews on these things.

If I understand the concerns around slice::from_raw_parts it's that we're violating the following safety criteria.

data must point to len consecutive properly initialized values of type T.

https://doc.rust-lang.org/std/slice/fn.from_raw_parts_mut.html

Since the padding would be uninitialized?

Looking at the 1password implementation, it uses write_bytes which does not have any requirement of the values being initialized? We'll do some internal testing around this and circle back.

---

Yes the tests are full of UB, I've been meaning to get back and improving them but haven't had the time. Looking at old GH actions runs it seems Ubuntu 22.04 failed, but from what I recall we also ran them on local devices where I believe they did succeed.

[complexspaces]

If I understand the concerns around slice::from_raw_parts it's that we're violating the following safety criteria.
...
Since the padding would be uninitialized?

Yep, that's correct.

it uses write_bytes which does not have any requirement of the values being initialized?

Correct as well 👍. Raw pointers do not have the same initialization/validity requirements that references do, so you can use them to write values to a place with generally less rules. Our implementation is doing something similar to MaybeUninit::write, just without using that type exactly.