Description
I had proposed and implemented getrandom_uninit
as a safer alternative to the "raw" API proposed in #279. @josephlr has pointed out that we ended up adding a non-trivial amount of unsafe
code for this, but there really isn't a significant performance benefit, and he suggested we remove it in the next breaking release. We've recently decided not to build other primitives like #281 on top of it within getrandom itself.
The only time there would really be a performance benefit is if one were randomly-initializing an abnormally-large (for cryptography code) buffer.
Off the time of my head, would be in the implementation of RSA (or similar) blinding on non-SIMD 32-bit targets, where we could have (8192 / 32) = 256 words to zero before calling getrandom
if we don't have getrandom_uninit
. Still, the syscall and actual CSPRNG computation cost is going to dominate that, and also blinding implementations typically reuse the buffer for the blinding so that the initial zeroing would only need to be done the very first time (per blinding context), so the zeroing cost is neglibile.
Activity
josephlr commentedon Jun 4, 2024
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 andunsafe
code, but it's unclear if they are worth it. We have benchmarks which compare callinggetrandom_uninit
vs zeroing a buffer and callinggetrandom
. Even with very fast (> 100 MB/sec) rng sources, theuninit
methods aren't consistently faster (and any difference is within the margin of error).On Linux (
cargo bench --jobs=1
):I would be fine removing the
uninit
methods entirely, as it would simplify our implementation in multiple places.briansmith commentedon Jun 7, 2024
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.briansmith commentedon Jun 7, 2024
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 ofMaybeUninit
exactly so I can use memory sanitizer to ensure that theinit()
actually initializes what it is supposed to, instead of leaving some of the zeros untouched. I guess it will likely be the case for othergetrandom
users.newpavlov commentedon Jun 7, 2024
I don't think that
MaybeUninit
adds a lot of complexity. Almost all backends (exceptjs
) 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 theuninit
function.notgull commentedon Jun 7, 2024
+1 to keeping it
briansmith commentedon Jun 7, 2024
I think it is fine to keep it. We have this unsafe code in
getrandom
mostly because stabilizing some libcore features related toMaybeUninit
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 beMaybeUninit
-aware if/when we change that API.josephlr commentedon Jun 8, 2024
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.