Deprecate and remove `getrandom_uninit` about getrandom HOT 7 CLOSED

Copying from #439

Is it worth keeping the uninit methods?

This brings up a more important question, does it even make sense to keep the uninit methods at all? They add complexity and unsafe code, but it's unclear if they are worth it. We have benchmarks which compare calling getrandom_uninit vs zeroing a buffer and calling getrandom. Even with very fast (> 100 MB/sec) rng sources, the uninit methods aren't consistently faster (and any difference is within the margin of error).

On Linux (cargo bench --jobs=1):

test aes128::bench_getrandom        ... bench:         409 ns/iter (+/- 20) = 39 MB/s
test aes128::bench_getrandom_uninit ... bench:         413 ns/iter (+/- 37) = 38 MB/s
test p256::bench_getrandom          ... bench:         421 ns/iter (+/- 19) = 76 MB/s
test p256::bench_getrandom_uninit   ... bench:         416 ns/iter (+/- 23) = 76 MB/s
test p384::bench_getrandom          ... bench:         527 ns/iter (+/- 126) = 91 MB/s
test p384::bench_getrandom_uninit   ... bench:         543 ns/iter (+/- 169) = 88 MB/s
test page::bench_getrandom          ... bench:       8,676 ns/iter (+/- 219) = 472 MB/s
test page::bench_getrandom_uninit   ... bench:       8,591 ns/iter (+/- 551) = 476 MB/s

I would be fine removing the uninit methods entirely, as it would simplify our implementation in multiple places.

from getrandom.

When working on Memory Sanitizer support (#463), I realized that the MaybeUninit functionality is actually quite useful for testing, because we can use Memory Sanitizer to ensure that we at least write to the given buffer. We can't __msan_poison a &mut [u8] without trigger UB.

In particular, if/when we update the signature of custom implementations to take MaybeUninit, then memory sanitizer will be able to also test them. So I think it is useful to keep around.

from getrandom.

In ring, currently I do use the pattern let mut x = [0; LEN]; init(&mut x) in many places, but I am planning to replace those with use of MaybeUninit exactly so I can use memory sanitizer to ensure that the init() actually initializes what it is supposed to, instead of leaving some of the zeros untouched. I guess it will likely be the case for other getrandom users.

from getrandom.

I don't think that MaybeUninit adds a lot of complexity. Almost all backends (except js) use raw pointers to call system APIs and there is no difference between casting pointer from &[u8] and &mut [MaybeUninit<u8>]. Buffer zeroization is unnecessary, even if it's performance impact is negligible compared to syscalls. So I am in favor of keeping the uninit function.

from getrandom.

+1 to keeping it

from getrandom.

I think it is fine to keep it. We have this unsafe code in getrandom mostly because stabilizing some libcore features related to MaybeUninit is taking a long time. In the future we'll be able to eliminate the unsafe bits. In the meantime, we've done quite a bit to ensure that the unsafe bits are safe. So I am fine with closing this issue, assuming this also means that we're committing to changing the custom API to be MaybeUninit-aware if/when we change that API.

from getrandom.

Sounds good to me. Personally, I think the only strong argument to keep it is @briansmith's point about sanitizers, but that is a very good point.

from getrandom.