Add support for Custom RNGs #109

josephlr · 2019-09-20T08:33:22Z

This is a breaking change for #98, so it's being merged against the 0.2 branch.

This change consists on several significant changes, so reviewing on a per commit basis may be the easiest. The changes (in rough commit order) are:

The getrandom::Error constants are moved to be associated constants, and they are now all public and documented.
The features related to wasm32 implementations (wasm-bindgen and stdweb) are removed from the main crate
Functionality is added (behind the custom Cargo feature) to enable registering and using a custom RNG implementation. This is done by having the custom function be declared with #[no_mangle], and having the function called via extern "C".
To demonstrate the soundness of the Custom RNG approch, the following Custom RNGs are added:
- stdweb-getrandom: Replacing the stdweb feature
- wasm-bindgen-getrandom: Replacing the wasm-bindgen feature
Tests/CI are updated to work with the new WASM Custom RNGs

Fixes #4 and allows for having a getrandom-rdrand Custom RNG (which provides a better solution for #84)

dhardy

Looks like a nice solution to #4 to me.

I get why you built this on top of your CI changes, but it doesn't require your other PR (removing dummy), and I'm not convinced we want that (although I believe the main objection to making wasm32-unknown-unknown a compile failure was that it's a breaking change).

Although it's a nice demonstration of custom RNGs, is there much point in moving the WASM implementations out of the base crate? Dependency reduction?

dhardy · 2019-09-20T09:46:50Z

src/error.rs

-    pub(crate) const STDWEB_NO_RNG: Error = internal_error!(9);
-    pub(crate) const STDWEB_RNG_FAILED: Error = internal_error!(10);
+    #[doc(hidden)]
+    pub const BINDGEN_CRYPTO_UNDEF: Error = internal_error!(7);


Why hide these? They are defined constants either way.

I'll need to go back an document these (this was just to make the CI happy). There's also the separate question of if we should expose all of the Error constants or just those necessary to make the custom RNGs work. What do you think?

There's also the issue of Debug/ Display implementations (error strings) when moving implementations out of the main crate.

I'm not sure what you mean? The error strings would stay in the main getrandom, I guess it means Custom RNGs not in this repo just have to use numbers instead of words. That's not ideal, but fixing it is probably just too hard.

Fixing it is rust-random/rand#837 which it looks like we will not solve. But for these WASM crates we should keep the error stings in getrandom.

Unless we add an auxilliary extern Rust function:

fn __getrandom_error_msg(code: NonZeroU32) -> Option<&'static str>;

I don't believe it's worth it for now though (and it doesn't solve error messages for anything outside getrandom).

dhardy · 2019-09-20T09:50:21Z

src/custom.rs

+pub fn getrandom_inner(dest: &mut [u8]) -> Result<(), Error> {
+    extern "Rust" {
+        #[allow(improper_ctypes)] // See rust-lang/rust#64593
+        fn __getrandom_custom(dest: &mut [u8]) -> Result<(), Error>;


Looks like a nice solution.

The call stack is now potentially getrandom::getrandom → getrandom::getrandom_inner → __getrandom_custom → provider::getrandom_inner, but use of #[inline] in a couple of places reduces this to 2.

I think even without the #[inline] Rust will manage to get rid of all the intermediate function calls except the __getrandom_custom, but I should run a compile test to make sure.

I'm not sure if inlining is automatically available across module boundaries, I know we need #[inline] if we want to do across crate boundaries.

Yes, and I think __getrandom_custom can never be inlined (without LTO), but it shouldn't be a big issue.

josephlr · 2019-09-20T10:09:21Z

I get why you built this on top of your CI changes, but it doesn't require your other PR (removing dummy), and I'm not convinced we want that (although I believe the main objection to making wasm32-unknown-unknown a compile failure was that it's a breaking change).

Ya the previous discussion is why I proposed it. I think having the dummy impl is almost never what a user wants. I think that regardless of if we implement wasm-bindgen/stdweb as Custom RNGs or keep them in the main crate, we should make wasm32-unknown-unknown with no implementation a compile-time error.

Of course, what to do for no-implementation wasm32-unknown-unknown is separate from the issue of removing the dummy Cargo feature. #71 added it to help users who were relying on the 0.1.0 - 0.1.8 behavior where we always used a dummy impl for unsupported targets.

One interesting concept is that a "dummy" impl could be just another Custom RNG

dhardy · 2019-09-20T10:11:24Z

We can always re-add the dummy feature later if need be, so it is safe to remove now. I guess then it makes sense to merge #107 @newpavlov.

josephlr · 2019-09-20T10:14:56Z

Although it's a nice demonstration of custom RNGs, is there much point in moving the WASM implementations out of the base crate? Dependency reduction?

The main reason (other than making the dependency story easier) would be to eliminate the need for crates to keep reexporting the stdweb and wasm-bindgen features.

Thinking of a large Rust application, where getrandom might be at the end of a very long dependency chain. It's the end-user, not the immediate library using getrandom, that's in the best position to determine what type of wasm32-unknown-unknown environment they are in.

I'm not sure how this calculation changes if we are able to detect at build-time if we are in cargo-web vs wasm-bindgen vs neither. I'll have to do some more looking.

dhardy · 2019-09-20T14:42:27Z

The alternative being, as we tell rand users now, that they should depend on getrandom directly just to add the feature flag. However that potentially breaks if any crate in the build depends on a different version of getrandom, whereas with an external crate implementing the feature it doesn't.

The complication is that we have more crate versions to deal with. If we release getrandom 0.3 in the future, we need to make a new release of getrandom-wasm-bindgen just to update the version of getrandom it depends on, since the crate is actually a dependency (assuming the Error type and __getrandom_custom prototype don't change).

josephlr · 2019-09-21T02:37:17Z

The alternative being, as we tell rand users now, that they should depend on getrandom directly just to add the feature flag. However that potentially breaks if any crate in the build depends on a different version of getrandom, whereas with an external crate implementing the feature it doesn't.

External crates have another benefit: it makes custom getrandom implementations look and feel more similar to custom allocators or panic handlers. Intermediate crates can depend on allocation or panicking without knowing exactly how it will be dealt with by the application.

Of course, those have specific lang_items, but our macro-based approach seems like the best thing to do without compiler support for custom lang_items.

The complication is that we have more crate versions to deal with. If we release getrandom 0.3 in the future, we need to make a new release of getrandom-wasm-bindgen just to update the version of getrandom it depends on, since the crate is actually a dependency (assuming the Error type and __getrandom_custom prototype don't change).

So the situation isn't too bad, provided two conditions are met:

__getrandom_custom signature/name doesn't change (i.e. we don't need more arguments)
Error stays a NonZeroU32

For example, let's say we have the following versions of getrandom:

getrandom v0.2.5 - Last version before semver-trick
getrandom v0.2.6 - Shim via semver-trick to v0.3
getrandom v0.3.0 - Backwards incompatible, but not violating the above conditions

Then we can have:

getrandom-wasm-bindgen v0.1.0 depending on getrandom v0.2
getrandom-wasm-bindgen v0.1.1 depending on getrandom v0.3

and nothing should break.

The following use cases all work how you would expect:

A program depends on getrandom v0.2.5 and getrandom-wasm-bindgen v0.1.1
- getrandom v0.3.0 is built, but never called.
- getrandom-wasm-bindgen's function is invoked by getrandom v0.2.5 without issues.
A program depends on getrandom v0.3.0 and getrandom-wasm-bindgen v0.1.0
- Some version of getrandom v0.2 is built, but never called.
- getrandom-wasm-bindgen's function is invoked by getrandom v0.3.0 without issues.
A user has getrandom v0.3.0 and getrandom v0.2.5 and either version getrandom-wasm-bindgen
- Both versions of getrandom will be built and (potentially) called.
- Both versions of getrandom (when built for a custom target) will link against the same function declared by getrandom-wasm-bindgen.
- Thus, the custom RNG will control the behavior for both versions of getrandom.

The only issue would be if there are multiple Custom RNG handlers in a crate's dependency tree, then you'd get a linking error. However, that shouldn't happen as Cargo should only select on of getrandom-wasm-bindgen v0.1.0 or getrandom-wasm-bindgen v0.1.1 as they are semver compatible.

This all works because the extern/#[no_mangle] namespace is global across crates and across versions. It also only needs the representations to be consistent it can't do version checking or anything.

dhardy · 2019-09-21T07:39:05Z

Agreed. Thus we have extra incentive to make sure the API is stable; this looks fine to me but given the scarcity of doc around extern "Rust" it may be worth @RalfJung taking a quick look.

extern "Rust" {
    fn __getrandom_custom(dest: &mut [u8]) -> Result<(), Error>;
}

Once #107 and #108 are ready lets rebase this.

josephlr · 2019-09-22T02:36:26Z

Once #107 and #108 are ready lets rebase this.

Done, I rebased the 0.2 branch onto latest master as well.

josephlr · 2019-09-23T20:58:08Z

Continuing the discussion from @newpavlov and @dhardy's comments in #107

@josephlr
So if I understand correctly you intend to replace the dummy feature with the custom impl crate, right? But I still don't understand how you intend to work around wasm32-unknown-unknown breakage in downstream crates, without a clear plan I don't think we should remove the dummy impl.

I poorly communicated how I wanted to fix the potential wasm32-unknown-unknown breakage in upstream crates. So here's my solution:

First, this PR will add in a getrandom-dummy crate that implements a custom RNG that always fails. I didn't initially think this was necessary, but it makes a lot of the code simpler for downstream crates. This required changing the register_custom_getrandom! macro to always be present if the custom feature is active. This is probably good, as it allows us to add support for targets in later versions of getrandom without breaking back-compat.

Second, we will consider a simplified version of the rand crate. This crate has two features wasm-bindgen and stdweb that (right now) just forward to the getrandom v0.1 features. rand wants to use getrandom v0.2 without making any breaking changes. Specifically (on wasm32-unknown-unknown) it wants to:

If stdweb feature is enabled, use stdweb
If wasm-bindgen feature is enabled, use wasm-bindgen
If both are enabled, use wasm-bindgen
If neither are enabled, use a dummy implementation.

To do this, rand would have the following dependencies in its Cargo.toml:

[dependencies]
getrandom = "0.2"

[target.wasm32-unknown-unknown.dependencies]
wasm-bindgen = { package = "getrandom-wasm-bindgen", version = "0.1", optional = true }
stdweb = { package = "getrandom-stdweb", version = "0.1", optional = true }
getrandom-dummy = "0.1"

and the following code in its src/lib.rs:

#[cfg(all(target_arch = "wasm32", target_os = "unknown"))]
cfg_if! {
    if #[cfg(feature = "wasm-bindgen")] {
        extern crate wasm_bindgen;
    } else if #[cfg(feature = "stdweb")] {
        extern crate stdweb;
    } else {
        extern crate getrandom_dummy;
    }
}

ensuring that for all wasm32-unknown-unknown builds, only a single custom RNG gets linked in.

EDIT: Note that the code above is (about) what would be in the getrandom v0.1 semvar-trick shim to implement the features on top of getrandom v0.2.

josephlr · 2019-09-24T06:25:04Z

EDIT: this comment is wrong, see #109 (comment)

As for rand_core v0.5, it only needs to depend on getrandom::Error not getrandom::getrandom. This means we can use a (on by default) Cargo feature foobar to gate getrandom::getrandom. Then, rand_core v0.5 can just have the following Cargo.toml:

[features]
std = ["getrandom", "getrandom/std"]

[dependencies]
getrandom = { version = "0.2", default-features = false, optional = true }

This allows rand_core v0.5 to compile for any Rust target, as getrandom::Error doesn't depend on anything other than core. It also allows rand_core v0.6 to do:

[features]
std = ["getrandom/std"]

[dependencies]
getrandom = { version = "0.2", default-features = false }

and unconditionally depend on getrandom, solving issues raised in #110.

This imp feature should probably have its own PR, I just wanted to show it would fix these issues.

dhardy · 2019-09-24T07:41:19Z

Edit: the first paragraph is wrong; see below.

@josephlr the use of extern crate does nothing except introduce the crate's API into the build's namespace; in Edition 2018 it does nothing at all except in a few corner cases (standard libraries, #[macro_use]). So there will still be linker conflicts with both stdweb and wasm-bindgen features enabled and I don't know a way of fixing this (again, it's a Cargo limitation).

Maybe we can get away with simply telling people don't enable both; I don't know.

Why add a getrandom-dummy crate over the dummy feature? I see no sense; part of the rartionale for this feature was that you don't have to worry about getting configuration right (when not actually calling getrandom), but with this crate the build would fail if enabling another getrandom-* provider.

Since rand_core 0.5.1 it does use getrandom directly (to implement OsRng).

josephlr · 2019-09-24T08:00:42Z

Since rand_core 0.5.1 it does use getrandom directly (to implement OsRng).

Crap, I forgot about this. Scratch the stuff about rand_core above.

Why add a getrandom-dummy crate over the dummy feature? I see no sense; part of the rartionale for this feature was that you don't have to worry about getting configuration right (when not actually calling getrandom), but with this crate the build would fail if enabling another getrandom-* provider.

This crate would mainly be used to allow crates to migrate to the new system, while exposing their old APIs, while using less code. It also allow you to work around a Cargo limitation where you can't have enable features on a crate based on specific targets. The final reason is that it would allow us to actually remove the dummy feature from the main crate, and tell people to only use getrandom-dummy in specific, limited circumstances.

I would expect rand and rand_core to be the only direct users of the crate.

josephlr · 2019-09-24T09:25:02Z

@josephlr the use of extern crate does nothing except introduce the crate's API into the build's namespace; in Edition 2018 it does nothing at all except in a few corner cases (standard libraries, #[macro_use]). So there will still be linker conflicts with both stdweb and wasm-bindgen features enabled and I don't know a way of fixing this (again, it's a Cargo limitation).

So this isn't true, but I thought the same thing (that extern crate did essentially nothing in Rust 2018). It turns out, if there are no references to a crate (either by using a symbol, a use statement, or a extern crate declaration), then it won't be linked in, regardless of the Cargo dependencies. So the code in #109 (comment) works fine (I tested it).

I also make sure this wasn't a particular linker property. But the wasm-bindgen linker, cargo-web linker, the default Linux linker, etc... all have this property. They will complain about duplicate symbols if two custom implementations are linked in. They will complain about a missing symbol if no custom implementation is linked in.

dhardy · 2019-09-24T09:25:12Z

@josephlr the use of extern crate does nothing except

From testing, it appears that my understanding is wrong on this and that linking only happens if (a) extern crate .. is used or (b) if some symbol is referenced, so your cfg thing may work. However for some reason "multiple definition" errors are not always emitted when both sub-crates are linked. Demonstration code here.

Note that one can also write use getrandom_stdweb as _; which appears to be equivalent to extern crate.

Furthermore, this means that an application adding a dependency on, say, getrandom-wasm-bindgen, is not enough to enable this backend, thus this form of configuration is not as convenient as I had assumed.

The language spec doesn't directly cover this unfortunately. The section on extern crates does talk about linking, but it is not clear whether this has been updated for Edition 2018. The sections on use declarations and linking also do not clear this up.

josephlr · 2019-09-24T09:36:05Z

@josephlr the use of extern crate does nothing except

Looks like we submitted at the exact same time 😁

However for some reason "multiple definition" errors are not always emitted when both sub-crates are linked.

I also ran into this sometimes, apparently the key is to make sure the custom RNG crates are of type dylib, at least that fixed some issues for me. Does it for you?

Furthermore, this means that an application adding a dependency on, say, getrandom-wasm-bindgen, is not enough to enable this backend, thus this form of configuration is not as convenient as I had assumed.

I agree that it could be a little more convenient, but it's not too bad (you just need one line in your lib's src/lib.rs, we will just have to document exactly how to use these crates as well as how to create your own Custom RNGs.

Do you think this is still the way to go for Custom RNGs?

The language spec doesn't directly cover this unfortunately.

I agree, but it does seem like the referencing of a crate (not its declaration as a Cargo dependency) is what controls all aspects of linking.

josephlr · 2019-09-24T10:55:02Z

@dhardy @newpavlov I rebased this, updated the description, and better split the changes into commits. Let me know if you think any of these changes should be in their own PR.

dhardy · 2019-09-24T11:51:38Z

I would prefer to keep anything to-do with dummy out of this PR.

This crate would mainly be used to allow crates to migrate to the new system, while exposing their old APIs, while using less code. It also allow you to work around a Cargo limitation where you can't have enable features on a crate based on specific targets. The final reason is that it would allow us to actually remove the dummy feature from the main crate, and tell people to only use getrandom-dummy in specific, limited circumstances.

The Cargo features issue shouldn't apply here (rand_core would always depend on the dummy feature) and the less-code argument isn't convincing (if even true). It comes down to (a) whether we wish to keep the feature and (b) whether we wish to gracefully handle dummy and another backend without the cfg_if in user-code. So lets leave it until later.

josephlr · 2019-09-24T18:47:55Z

It comes down to (a) whether we wish to keep the feature and (b) whether we wish to gracefully handle dummy and another backend without the cfg_if in user-code. So lets leave it until later.

Make sense, dummy stuff is removed.

josephlr · 2019-10-26T10:01:35Z

I've rebased this onto #120 and improved the commit messages to be more readable. I also renamed the Custom RNG implementations to be wasm-bindgen-getrandom and stdweb-getrandom (to reflect their potential future ownership). I like the idea of (post 0.2 release) moving the Custom RNG crates to their respective owners.

I've addressed all of the outstanding feedback, so I think the only thing left here is to update the module documentation and README.md.

newpavlov · 2019-11-03T16:23:36Z

@josephlr
Can you resolve the conflicts? After that I think we should be good to merge.

josephlr force-pushed the custom branch from 75a5b58 to 21f8a81 Compare September 20, 2019 08:49

dhardy mentioned this pull request Sep 20, 2019

Unnecessary improper_ctypes warning for extern "Rust" rust-lang/rust#64593

Closed

dhardy reviewed Sep 20, 2019

View reviewed changes

dhardy mentioned this pull request Sep 21, 2019

Remove dummy feature #107

Merged

josephlr added this to the 0.2 milestone Sep 21, 2019

josephlr force-pushed the 0.2 branch from 689771b to 05b479a Compare September 21, 2019 20:40

josephlr force-pushed the custom branch from 21f8a81 to f1df844 Compare September 22, 2019 02:33

dhardy mentioned this pull request Sep 24, 2019

Clarify when dependencies are linked rust-lang/rust#64731

Closed

josephlr force-pushed the custom branch from f1df844 to d47c1ba Compare September 24, 2019 10:13

josephlr force-pushed the 0.2 branch from be56f20 to deac126 Compare September 24, 2019 10:14

josephlr force-pushed the custom branch 2 times, most recently from b2367d0 to fe00336 Compare September 24, 2019 10:53

josephlr force-pushed the custom branch from fe00336 to af105eb Compare September 24, 2019 18:45

josephlr force-pushed the custom branch 2 times, most recently from 9d4c68b to b057dfc Compare October 26, 2019 08:07

josephlr mentioned this pull request Oct 26, 2019

0.2: Error and Testing improvements #120

Merged

josephlr force-pushed the custom branch from b057dfc to 37f18a2 Compare October 26, 2019 09:49

josephlr force-pushed the custom branch 2 times, most recently from 6eeba77 to 00d5950 Compare October 26, 2019 10:05

josephlr force-pushed the custom branch from 00d5950 to 434de32 Compare January 3, 2020 02:24

This was referenced Jan 6, 2020

stdweb usage #123

Closed

Tracking issue for 0.2.0 #98

Closed

josephlr force-pushed the custom branch 3 times, most recently from f67987a to e8d6f15 Compare January 9, 2020 11:38

josephlr added 4 commits January 9, 2020 13:28

custom: Add support for Custom RNGs

9db790e

Remove built-in support for wasm32-unknown-unknown

4057b91

custom: Add custom RNG for stdweb

96c731b

custom: Add Custom RNG for wasm-bindgen

651ad03

josephlr force-pushed the custom branch from e8d6f15 to 651ad03 Compare January 9, 2020 21:29

dhardy approved these changes Jan 10, 2020

View reviewed changes

newpavlov merged commit 8365277 into rust-random:0.2 Jan 10, 2020

josephlr deleted the custom branch January 11, 2020 02:26

josephlr mentioned this pull request Feb 16, 2020

Add Cargo feature for CPU randomness #133

Merged

dhardy mentioned this pull request Feb 17, 2020

Feature for using CPU-based RNG #84

Closed

josephlr mentioned this pull request Feb 20, 2020

embedded source compatibility #4

Closed

josephlr mentioned this pull request Apr 27, 2020

Consider using web_sys and unifying wasm32-unknown-unknown targets #63

Closed

ryankurte mentioned this pull request Jul 13, 2020

RNG support in libcore rust-embedded/wg#353

Closed

tkaitchuck mentioned this pull request Nov 7, 2020

Using aHash 0.5 with wasm tkaitchuck/aHash#59

Closed

newpavlov mentioned this pull request Aug 27, 2021

Support for wasm32-unknown-unknown #223

Closed

josephlr mentioned this pull request Mar 9, 2023

Should __getrandom_custom use "Rust" ABI? #345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Custom RNGs #109

Add support for Custom RNGs #109

josephlr commented Sep 20, 2019 •

edited

Loading

dhardy left a comment

dhardy Sep 20, 2019

josephlr Sep 20, 2019

dhardy Sep 20, 2019

josephlr Sep 20, 2019

dhardy Sep 23, 2019

dhardy Sep 20, 2019

josephlr Sep 20, 2019 •

edited

Loading

dhardy Sep 20, 2019 •

edited

Loading

josephlr commented Sep 20, 2019 •

edited

Loading

dhardy commented Sep 20, 2019

josephlr commented Sep 20, 2019

dhardy commented Sep 20, 2019

josephlr commented Sep 21, 2019 •

edited

Loading

dhardy commented Sep 21, 2019

josephlr commented Sep 22, 2019

josephlr commented Sep 23, 2019 •

edited

Loading

josephlr commented Sep 24, 2019 •

edited

Loading

dhardy commented Sep 24, 2019 •

edited

Loading

josephlr commented Sep 24, 2019 •

edited

Loading

josephlr commented Sep 24, 2019

dhardy commented Sep 24, 2019

josephlr commented Sep 24, 2019

josephlr commented Sep 24, 2019

dhardy commented Sep 24, 2019

josephlr commented Sep 24, 2019

josephlr commented Oct 26, 2019

newpavlov commented Nov 3, 2019

Add support for Custom RNGs #109

Add support for Custom RNGs #109

Conversation

josephlr commented Sep 20, 2019 • edited Loading

dhardy left a comment

Choose a reason for hiding this comment

dhardy Sep 20, 2019

Choose a reason for hiding this comment

josephlr Sep 20, 2019

Choose a reason for hiding this comment

dhardy Sep 20, 2019

Choose a reason for hiding this comment

josephlr Sep 20, 2019

Choose a reason for hiding this comment

dhardy Sep 23, 2019

Choose a reason for hiding this comment

dhardy Sep 20, 2019

Choose a reason for hiding this comment

josephlr Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

dhardy Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

josephlr commented Sep 20, 2019 • edited Loading

dhardy commented Sep 20, 2019

josephlr commented Sep 20, 2019

dhardy commented Sep 20, 2019

josephlr commented Sep 21, 2019 • edited Loading

dhardy commented Sep 21, 2019

josephlr commented Sep 22, 2019

josephlr commented Sep 23, 2019 • edited Loading

josephlr commented Sep 24, 2019 • edited Loading

dhardy commented Sep 24, 2019 • edited Loading

josephlr commented Sep 24, 2019 • edited Loading

josephlr commented Sep 24, 2019

dhardy commented Sep 24, 2019

josephlr commented Sep 24, 2019

josephlr commented Sep 24, 2019

dhardy commented Sep 24, 2019

josephlr commented Sep 24, 2019

josephlr commented Oct 26, 2019

newpavlov commented Nov 3, 2019

josephlr commented Sep 20, 2019 •

edited

Loading

josephlr Sep 20, 2019 •

edited

Loading

dhardy Sep 20, 2019 •

edited

Loading

josephlr commented Sep 20, 2019 •

edited

Loading

josephlr commented Sep 21, 2019 •

edited

Loading

josephlr commented Sep 23, 2019 •

edited

Loading

josephlr commented Sep 24, 2019 •

edited

Loading

dhardy commented Sep 24, 2019 •

edited

Loading

josephlr commented Sep 24, 2019 •

edited

Loading