You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
It is inefficient to zero-fill a large byte buffer before actually being initialized with random bytes. Thus, it would be better if getrandom exposes a function that takes a slice of MaybeUninit<u8> so that the user will not have to initialize the buffer.
I've seen this type of request a couple of times before, with the rand project, but have never been convinced that this optimisation would be worth the effort. This is even more true of getrandom where a slow system call is required to get the random bytes.
If a crate is depending on getrandom() and it wants to expose an interface where it fills a MaybeUninit<u8> slice then it would be good to be able to fill the part of that slice that needs the random bytes using getrandom directly, instead of needing to fill a temporary &[u8] and then copying that temporary &[u8] to the &[MaybeUninit<u8>].
In particular, for security reasons, one may wish to ensure (to the extent Rust allows) that the generated random bytes are never copied anywhere except the target buffer.
It is inefficient to zero-fill a large byte buffer before actually being initialized with random bytes.
@bdbai do you have any benchmarks on this? Do we know if there's a performance difference between:
Zeroing a buffer then calling getrandom
Calling getrandom on uninitialized bytes
It would be fine to do a comparison in C if getting benchmarks in Rust might be overly complex.
instead of needing to fill a temporary &[u8] and then copying that temporary &[u8] to the &[MaybeUninit<u8>].
In particular, for security reasons, one may wish to ensure (to the extent Rust allows) that the generated random bytes are never copied anywhere except the target buffer.
@briansmith I'm more sensitive to the security concern here (we definitely want it to be possible to initialize random buffers in-place). However, why wouldn't it be possible to just zero the &mut [MaybeUninit<u8>], get a &mut [u8] to the same buffer, and then pass the buffer to getrandom? That should avoid copying secret data or needing a temporary buffer.
In general, I wouldn't be totally opposed to adding such an API, but we would probably want to wait for rust-lang/rfcs#2930 to be implemented (tracked in rust-lang/rust#78485), before adding an API.
I don't want to invent our own API for dealing with uninitialized buffers. If we add this, we would want to use the ReadBuf type and something like:
pubfngetrandom_readbuf(dest:&mutReadBuf<'_>) -> Result<(),Error>{// Our implementation dealing with [MaybeUninit<u8>]}
Initially, this would need to be behind an unstable feature gate (like "read_buf") until the RFC is stabilized.
One advantage of this sort of API is that it might allow for partial reads from the underlying RNG to not be wasted. However, this is a very slight benefit as if the RNG succeeds once, it virtually always succeeds for all future calls.
I don't want to invent our own API for dealing with uninitialized buffers. If we add this, we would want to use the ReadBuf type and something like:
ReadBuf is planned to be in std::io so it can't be used by getrandom in general, right? Perhaps we should provide feedback that it should be moved to libcore and avoid using std::io::Error.
However, I think ReadBuf is more than what is needed, because it tries to keep track of the intermediate state of a partially-filled/initialized buffer, which isn't applicable here.
@briansmith I'm more sensitive to the security concern here (we definitely want it to be possible to initialize random buffers in-place). However, why wouldn't it be possible to just zero the &mut [MaybeUninit<u8>], get a &mut [u8] to the same buffer, and then pass the buffer to getrandom? That should avoid copying secret data or needing a temporary buffer.
Of course that does work. OTOH, if that's going to be a common pattern then why not encapsulate it within getrandom by adding such an API? And then we could (later) optimize the implementation to avoid the initial zeroing.
ReadBuf is planned to be in std::io so it can't be used by getrandom in general, right? Perhaps we should provide feedback that it should be moved to libcore and avoid using std::io::Error.
getrandom has a "std" feature, so we could just gate getrandom_readbuf behind that feature. Looking at the API for ReadBuf, it seems that nothing in its API requires it to be in libstd, so it could be in libcore. I would encourage people to raise this point in the ReadBuf tracking issue.
OTOH, if that's going to be a common pattern then why not encapsulate it within getrandom by adding such an API?
It's mostly due a desire to keep the functionality of this crate minimal to reduce our maintenance burden, so I would actually want to know that this is a common pattern across many crates before we commit to supporting it in the long-term. However, if adding this API increases ergonomics for folks, I would support adding it. This would be true even if the only benefit is ergonomics (e.g. if there is no significant performance difference).
However, I think ReadBuf is more than what is needed, because it tries to keep track of the intermediate state of a partially-filled/initialized buffer, which isn't applicable here.
If the main reason for adding this change is ergonomics (i.e. handling common patterns), I would want getrandom to integrate well with those common patterns. It looks like ReadBuf is going to be "standard way" to express this pattern in Rust, so we would probably want to use that rather than inventing our own API.
The latest revision of the PR #279 implements this.
Regarding the ReadBuf idea, I think that should be a separate proposal with its own issue. It looks like ReadBuf is dead, based on rust-lang/rust#97015, and it looks like it might be a while before all that stuff is resolved. And, it isn't clear the resolution will be no_std-compatible. A lot of things need/want a no_std-compatible interface and a std::io::*-based one is a non-starter.
Activity
dhardy commentedon Sep 25, 2021
I've seen this type of request a couple of times before, with the rand project, but have never been convinced that this optimisation would be worth the effort. This is even more true of
getrandom
where a slow system call is required to get the random bytes.briansmith commentedon Sep 30, 2021
If a crate is depending on
getrandom()
and it wants to expose an interface where it fills aMaybeUninit<u8>
slice then it would be good to be able to fill the part of that slice that needs the random bytes usinggetrandom
directly, instead of needing to fill a temporary&[u8]
and then copying that temporary&[u8]
to the&[MaybeUninit<u8>]
.briansmith commentedon Sep 30, 2021
In particular, for security reasons, one may wish to ensure (to the extent Rust allows) that the generated random bytes are never copied anywhere except the target buffer.
josephlr commentedon Oct 15, 2021
@bdbai do you have any benchmarks on this? Do we know if there's a performance difference between:
getrandom
getrandom
on uninitialized bytesIt would be fine to do a comparison in C if getting benchmarks in Rust might be overly complex.
@briansmith I'm more sensitive to the security concern here (we definitely want it to be possible to initialize random buffers in-place). However, why wouldn't it be possible to just zero the
&mut [MaybeUninit<u8>]
, get a&mut [u8]
to the same buffer, and then pass the buffer togetrandom
? That should avoid copying secret data or needing a temporary buffer.josephlr commentedon Oct 15, 2021
In general, I wouldn't be totally opposed to adding such an API, but we would probably want to wait for rust-lang/rfcs#2930 to be implemented (tracked in rust-lang/rust#78485), before adding an API.
I don't want to invent our own API for dealing with uninitialized buffers. If we add this, we would want to use the
ReadBuf
type and something like:Initially, this would need to be behind an unstable feature gate (like
"read_buf"
) until the RFC is stabilized.One advantage of this sort of API is that it might allow for partial reads from the underlying RNG to not be wasted. However, this is a very slight benefit as if the RNG succeeds once, it virtually always succeeds for all future calls.
gilescope commentedon Oct 15, 2021
Hopefully we could see readbuf hit nightly soon.
briansmith commentedon Oct 20, 2021
ReadBuf
is planned to be instd::io
so it can't be used by getrandom in general, right? Perhaps we should provide feedback that it should be moved to libcore and avoid usingstd::io::Error
.However, I think
ReadBuf
is more than what is needed, because it tries to keep track of the intermediate state of a partially-filled/initialized buffer, which isn't applicable here.briansmith commentedon Oct 20, 2021
Of course that does work. OTOH, if that's going to be a common pattern then why not encapsulate it within
getrandom
by adding such an API? And then we could (later) optimize the implementation to avoid the initial zeroing.josephlr commentedon Oct 20, 2021
getrandom
has a"std"
feature, so we could just gategetrandom_readbuf
behind that feature. Looking at the API forReadBuf
, it seems that nothing in its API requires it to be inlibstd
, so it could be inlibcore
. I would encourage people to raise this point in theReadBuf
tracking issue.It's mostly due a desire to keep the functionality of this crate minimal to reduce our maintenance burden, so I would actually want to know that this is a common pattern across many crates before we commit to supporting it in the long-term. However, if adding this API increases ergonomics for folks, I would support adding it. This would be true even if the only benefit is ergonomics (e.g. if there is no significant performance difference).
If the main reason for adding this change is ergonomics (i.e. handling common patterns), I would want
getrandom
to integrate well with those common patterns. It looks likeReadBuf
is going to be "standard way" to express this pattern in Rust, so we would probably want to use that rather than inventing our own API.briansmith commentedon Oct 6, 2022
The latest revision of the PR #279 implements this.
Regarding the
ReadBuf
idea, I think that should be a separate proposal with its own issue. It looks likeReadBuf
is dead, based on rust-lang/rust#97015, and it looks like it might be a while before all that stuff is resolved. And, it isn't clear the resolution will be no_std-compatible. A lot of things need/want a no_std-compatible interface and astd::io::*
-based one is a non-starter.briansmith commentedon Oct 7, 2022
@bdbai This issue shouldn't be closed yet, since PR #279 hasn't been approved/merged yet, and might not be.
Add `getrandom_uninit_slice(dest: &mut [MaybeUninit<u8>]) -> ...`.
getrandom_uninit(dest: &mut [MaybeUninit<u8>]) -> ...
. #291Add `getrandom_uninit_slice(dest: &mut [MaybeUninit<u8>]) -> ...`.
Add `getrandom_uninit_slice(dest: &mut [MaybeUninit<u8>]) -> ...`.
Add `getrandom_uninit(dest: &mut [MaybeUninit<u8>]) -> ...`.
Add `getrandom_uninit(dest: &mut [MaybeUninit<u8>]) -> ...`.
Add `getrandom_uninit_slice(dest: &mut [MaybeUninit<u8>]) -> ...`. (#291