Repository: rust-random/getrandom
Pull request #150 (2020-05-28 05:15:06)

cpu: Have "rdrand" feature take precedence over "custom"
Right now, features are always enabled across all targets. This means
that if _any_ Custom RNG is used _anywhere_ in a crate's dependancy
graph on _any_ target. The "custom" feature will be active on all
targets.

This old code made it impossible to have a bare metal target that uses "rdrand" on
x86_64 and a Custom RNG on aarch64. This change fixes this issue, and makes
sure that any implementation `getrandom` itself provides cannot be overridden.

I proposed the old precedence order in https://github.com/rust-random/getrandom/issues/84#issuecomment-587180349, but I think part of that was a mistake, given how Cargo treats features.

This also renames the "cpu" feature to "rdrand". While other CPU architectures have RNG intrinsic, they can behave differently than x86's, so it doesn't make sense to group them.

Signed-off-by: Joe Richey <joerichey@google.com>

josephlr: > Personally I would prefer to move CPU randomness into a custom crate, assuming you'll find the approach proposed [here](https://github.com/rust-random/getrandom/pull/149#issuecomment-635506737) viable.

I agree that CPU-based RNG would ideally just use the custom RNG mechanism. In fact, I think this makes more sense than having the `wasm32-unknown-unknown` stuff use it. However, doing this runs into a bunch of issues in practice (see the #84 thread).

We need the rdrand implementation for SGX, so having a separate crate poses issues regarding out-of-tree modules and code duplication. After thinking about this some more (and incorporating your feedback), I have two proposals:

### Proposal 1: Just rename the feature to `"rdrand"`

This would keep the same functionality, but would make stronger guarantees about forward compatibility. It would only ever do anything on `x86_64` and `x86` targets, preventing a future breaking changes. Going forward, if we support other CPU ISAs, they would be added under different features. This is reasonable, as different ISAs have different rules for using their RNG intrinsics (i.e. ARM's requires that you are in kernel mode), so having all "CPU-based randomness" under one feature might not be wise.

### Proposal 2: Expose a `cpurand` function

We would change the semantics of the `"cpu"` feature to expose a `cpurand` function on platforms that support CPU-based randomness. This wouldn't change _anything_ about how `getrandom` behaves. If a user wants to use CPU-based randomness to back `getrandom`, then then:
  - Enable _both_ "cpu" and "custom" features
  - Put `register_custom_getrandom!(cpurand);` in their root crate

Alternatively, we could just always expose a `cpurand` function on x86.

### Preferred approach

I'm leaning towards Proposal 1, as I don't think the rng instructions on other CPU are similar enough to RDRAND to warrant combing them, but I could go either way.

newpavlov: >Just rename the feature to "rdrand"

Sounds good.
josephlr: > > Just rename the feature to "rdrand"
> 
> Sounds good.

Done, let me know what you think. I updated the PR description. I didn't update the docs, I'm going to do all of that in #135 

Patch:
diff --git a/.travis.yml b/.travis.yml
index a5f74cd..3d3e443 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -145,11 +145,11 @@ jobs:
       script:
         # We test that getrandom builds for all targets
         - echo $STD_TARGETS | xargs -t -n1 cargo build --target
-        - echo $NO_STD_TARGETS | xargs -t -n1 cargo xbuild --features=cpu --target
+        - echo $NO_STD_TARGETS | xargs -t -n1 cargo xbuild --features=rdrand --target
         # also test minimum dependency versions are usable
         - cargo generate-lockfile -Z minimal-versions
         - echo $STD_TARGETS | xargs -t -n1 cargo build --target
-        - echo $NO_STD_TARGETS | xargs -t -n1 cargo xbuild --features=cpu --target
+        - echo $NO_STD_TARGETS | xargs -t -n1 cargo xbuild --features=rdrand --target
 
     # Trust cross-built/emulated targets. We must repeat all non-default values.
     - name: "Linux (MIPS, big-endian)"
diff --git a/Cargo.toml b/Cargo.toml
index 0488cc1..5ace343 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -35,8 +35,8 @@ wasi = "0.9"
 
 [features]
 std = []
-# Feature to enable fallback CPU-based implementation
-cpu = []
+# Feature to enable fallback RDRAND-based implementation
+rdrand = []
 # Feature to enable custom RNG implementations
 custom = []
 # Unstable feature to support being a libstd dependency
diff --git a/src/lib.rs b/src/lib.rs
index 8a03009..b988445 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -183,11 +183,11 @@ cfg_if! {
         #[path = "windows.rs"] mod imp;
     } else if #[cfg(all(target_arch = "x86_64", target_env = "sgx"))] {
         #[path = "rdrand.rs"] mod imp;
-    } else if #[cfg(feature = "custom")] {
-        use custom as imp;
-    } else if #[cfg(all(feature = "cpu",
+    } else if #[cfg(all(feature = "rdrand",
                         any(target_arch = "x86_64", target_arch = "x86")))] {
         #[path = "rdrand.rs"] mod imp;
+    } else if #[cfg(feature = "custom")] {
+        use custom as imp;
     } else {
         compile_error!("\
             target is not supported, for more information see: \
@@ -219,4 +219,4 @@ pub fn getrandom(dest: &mut [u8]) -> Result<(), Error> {
 #[cfg(test)]
 mod test_common;
 #[cfg(test)]
-mod test_cpu;
+mod test_rdrand;


Test patch:
diff --git a/src/test_cpu.rs b/src/test_rdrand.rs
similarity index 71%
rename from src/test_cpu.rs
rename to src/test_rdrand.rs
index 7a9a066..fafa6ac 100644
--- a/src/test_cpu.rs
+++ b/src/test_rdrand.rs
@@ -1,4 +1,4 @@
-// We only test the CPU-based RNG source on supported architectures.
+// We only test the RDRAND-based RNG source on supported architectures.
 #![cfg(any(target_arch = "x86_64", target_arch = "x86"))]
 
 #[path = "rdrand.rs"]