Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add getrandom::value function based on zerocopy::FromBytes #381

Closed
wants to merge 10 commits into from
Closed

Add getrandom::value function based on zerocopy::FromBytes #381

wants to merge 10 commits into from

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Nov 7, 2023 •
edited

This PR is an alternative to #293 and closes #281.

It introduces function with the folllowing signature:

#[cfg(feature = "zerocopy")]
#[inline]
pub fn value<T: zerocopy::FromBytes + Sized>() -> Result<T, Error> { ... }

It allows to generate a random value of any type which implements the FromBytes trait, including nested arrays.

The main disadvantage is an additional dependency, but since it's optional and rand started to unconditionally pull zerocopy starting from the rand_core level, I think it's a reasonable cost. Enabling the zerocopy feature bumps MSRV to 1.60. Notably, zerocopy has a conservative MSRV policy, so it shouldn't cause any MSRV breakage in future.

cc @briansmith @joshlf

@josephlr
Copy link
Member

josephlr commented Nov 7, 2023

I really like this. It seems low-cost, and I think we could add it in a non-breaking release of getrandom (as it's an optional feature).

@newpavlov newpavlov marked this pull request as ready for review November 7, 2023 06:33
@newpavlov newpavlov mentioned this pull request Nov 8, 2023
Closed
@joshlf
Copy link

joshlf commented Nov 16, 2023

Sounds cool! Out of curiosity, do you have preferences on how to handle different versions of zerocopy? Ie, if/when we release a 0.8, will you wait for a breaking version of getrandom to upgrade your dependency, will you provide multiple versions of the feature (ie, zerocopy_0_7, zerocopy_0_8, etc), or something else? We're currently working to better support use cases like these (google/zerocopy#619), so knowing what your preferences are will help our planning.

@newpavlov
Copy link
Member Author

No, I did not plan to. Updating to a breaking release of zerocopy would need a breaking release of getrandom.

If you think that zerocopy will have breaking releases more frequently than getrandom, then it may be better to reverse things a bit and add an optional getrandom support to zerocopy, not vice versa.

@joshlf
Copy link

joshlf commented Nov 16, 2023

We have short-term plans to release two breaking changes (0.8 and 0.9), but then things should be stable for the foreseeable future after that. Maybe it'd make sense to just wait for 0.9 and then land this? Both should be released on the order of a few weeks to a few months.

briansmith
briansmith reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@briansmith briansmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I do not think it is worth adding the dependency. First, the vast majority of uses should be in seeding a userspace PRNG, where I believe the current APIs are good enough, even without this or even #293, because doing "construct an all-zeros array and then overwrite it" has more than good enough performance characteristics for that. And, in general, "construct an all-zeros array and then overwrite it" is almost always going to be good enough because the syscall overhead is going to dwarf the zeroing, and in the extremely rare cases where that isn't true, we have the _uninit version now.

You can see that between the time you submitted this PR and now, everything that depended on zerocopy had to deal with google/zerocopy#716 due to their security monitoring (dependeabot, etc.) going off, and because it was yanked. I think that's likely to happen again. It would be very frustrating to me to be affected by it.

If the user wants to use zerocopy then it is easy for them to do so, since zerocopy does (or should) make it easy.

Cargo.toml Outdated Show resolved Hide resolved
src/error_impls.rs Outdated Show resolved Hide resolved
src/custom.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@newpavlov
Copy link
Member Author

If the user wants to use zerocopy then it is easy for them to do so, since zerocopy does (or should) make it easy.

The goal is to eliminate the need for users to use unsafe and slightly improve ergonomics. As you correctly note, the main use-case of getrandom is generation of seeds. let seed: u64 = getrandom::value(); is short and good at conveying the intent compared to the small dance around zeroized arrays and from_ne_bytes.

Your concerns about zerocopy itself are valid, but note that I do not plan to make the zerocopy feature enabled by default (even if forget about the MSRV mismatch), so users would need to explicitly opt-in for it.

As I mentioned earlier, a potential alternative to this PR would be to add this functionality to the zerocopy crate.

@newpavlov newpavlov mentioned this pull request Jan 24, 2024
Open
@briansmith
Copy link
Contributor

The goal is to eliminate the need for users to use unsafe and slightly improve ergonomics. As you correctly note, the main use-case of getrandom is generation of seeds. let seed: u64 = getrandom::value(); is short and good at conveying the intent compared to the small dance around zeroized arrays and from_ne_bytes.

As a user, I would prefer the "small dance" over adding a dependency to the dependency tree.

I still think that PR #293 had the best compromise to get to a "no unsafe" usage. The usage would be (as documented in that PR) like let random_u32s: [u32; 4] = getrandom::array()?.map(u32::from_ne_bytes); which is concise and clear and efficient. Since seeds for a CSPRNG are relatively small even if some copying would be involved, it would have virtually no performance impact.

In contrast, I feel like the usability improvement between this PR and PR #293 is very small which makes taking on a dependency not worthwhile.

@newpavlov
Copy link
Member Author

I am going to close this PR in favor of the zerocopy PR which hopefully will land, if not, we may revive the getrandom::array PR.

@newpavlov newpavlov closed this Feb 21, 2024
@newpavlov newpavlov deleted the zerocopy branch February 21, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briansmith briansmith briansmith left review comments

@ChrisDenton ChrisDenton ChrisDenton left review comments

@josephlr josephlr Awaiting requested review from josephlr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add function which returns random array?
5 participants
@newpavlov @josephlr @joshlf @briansmith @ChrisDenton