Skip to content

Expose API interface for MaybeUninit<u8> slice #226

Closed
@bdbai

Description

@bdbai

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.

Activity

dhardy

dhardy commented on Sep 25, 2021

@dhardy
Member

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

briansmith commented on Sep 30, 2021

@briansmith
Contributor

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>].

briansmith

briansmith commented on Sep 30, 2021

@briansmith
Contributor

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

josephlr commented on Oct 15, 2021

@josephlr
Member

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.

josephlr

josephlr commented on Oct 15, 2021

@josephlr
Member

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:

pub fn getrandom_readbuf(dest: &mut ReadBuf<'_>) -> 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.

gilescope

gilescope commented on Oct 15, 2021

@gilescope

Hopefully we could see readbuf hit nightly soon.

briansmith

briansmith commented on Oct 20, 2021

@briansmith
Contributor

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

briansmith commented on Oct 20, 2021

@briansmith
Contributor

@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.

josephlr

josephlr commented on Oct 20, 2021

@josephlr
Member

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.

briansmith

briansmith commented on Oct 6, 2022

@briansmith
Contributor

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.

briansmith

briansmith commented on Oct 7, 2022

@briansmith
Contributor

@bdbai This issue shouldn't be closed yet, since PR #279 hasn't been approved/merged yet, and might not be.

reopened this on Oct 7, 2022
added a commit that references this issue on Oct 13, 2022
9fdc800
added 3 commits that reference this issue on Oct 13, 2022
a9a6061
b7df3bc
7dad584
added a commit that references this issue on Oct 20, 2022
2bb9a7d
added a commit that references this issue on Oct 21, 2022
47a59dd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @briansmith@dhardy@gilescope@bdbai@josephlr

      Issue actions

        Expose API interface for MaybeUninit<u8> slice · Issue #226 · rust-random/getrandom