Add optional support for extern_item_impls

[bushrat011899]

Objective

• Experiment with using extern_item_impls in getrandom.

Solution

• Added a new RUSTFLAG, getrandom_extern_item_impls, which enables the nightly-only extern_item_impls feature.
• Refactored backends to use #[eii] on fill_inner, u32_inner, and u64_inner.
  • If a backend is available it will be used as the default implementation for the above externally implementable items.
  • Otherwise, defaults are provided for u32_inner, and u64_inner. fill_inner must be provided to successfully compile a final artifact (will not fail for libraries, etc.)
• Export the attribute macros provided by #[eii] in a new implementation module.
• Updated the READEME.md to provide basic usage information.

Example

With getrandom_extern_item_impls enabled, users can provide (or override) the implementations of the 3 backend functions used by getrandom with a simple attribute macro:

use core::mem::MaybeUninit;

#[getrandom::implementation::fill_uninit]
fn my_fill_uninit_implementation(
    dest: &mut [MaybeUninit<u8>]
) -> Result<(), getrandom::Error> {
    // ...
    Ok(())
}

If an implementation is not provided by the user (or an appropriate library) the compiler will provide a helpful error message during compilation informing the user that they must provide an implementation:

error: `#[fill_uninit]` required, but not found
   --> src\backends.rs:27:53
    |
 27 |         #[cfg_attr(getrandom_extern_item_impls, eii(fill_uninit))]
    |                                                     ^^^^^^^^^^^ expected because `#[fill_uninit]` was declared here in crate `getrandom`
...
205 |                 implementation!();
    |                 ----------------- in this macro invocation
    |
    = help: expected at least one implementation in crate `foo` or any of its dependencies
    = note: this error originates in the macro `implementation` (in Nightly builds, run with -Z macro-backtrace for more info)

If two implementations are provided (e.g., a library and the user) then another error message is provided:

error: multiple implementations of `#[fill_uninit]`
  --> src\lib.rs:81:1
   |
81 | / fn my_first_fill_uninit_implementation(
82 | |     _dest: &mut [MaybeUninit<u8>]
83 | | ) -> Result<(), Error> {
   | |______________________^ first implemented here in crate `foo`
...
88 | / fn my_second_fill_uninit_implementation(
89 | |     _dest: &mut [MaybeUninit<u8>]
90 | | ) -> Result<(), Error> {
   | |______________________- also implemented here in crate `bar`
   |
   = help: an "externally implementable item" can only have a single implementation in the final artifact. When multiple implementations are found, also in different crates, they conflict

Open Questions

• Should this be merged even in a nightly-only fashion? No new syntax is introduced by the feature, so even if the feature is removed from nightly entirely, getrandom will continue to function correctly as long as the Rust flag isn't used. Noting as well that the efi_rng backend also relies on a nightly feature, uefi_std.
• If this was stable, should it be the default?
• If there wasn't a Rust flag required to enable this functionality (e.g., on by default or through a Cargo feature), should there be a way for the user to defensively disable it for security reasons? For example, to prevent a malicious dependency changing the source of randomness.

---

Notes

• Because the current #[eii] attribute doesn't provide a way to conditionally provide a default implementation for an externally implementable item, I've added a macro to avoid repetition in the backends module. Not super happy with that solution, but I think it's preferable to the added verbosity of repeatedly defining the fill_inner function within the cfg_if statement.
• Offering this PR largely as a hypothetical, since I'm uncertain if/when extern_item_impls will be stabilised. I hope relatively soon, since even the current implementation seems vastly cleaner than #[unsafe(no_mangle)] extern "Rust" fn ....
• I personally have very little availability to maintain this feature (our daughter just hit 6 months old and work is a slog!), so if this is to be merged, please keep that in mind. I don't want to push a feature onto a project if the current maintainers don't want to maintain it.

[newpavlov]

extern_item_impls certainly looks very interesting! But I would prefer if it was implemented as a new opt-in backend.

Should this be merged even in a nightly-only fashion?

Yes, it's fine.

If this was stable, should it be the default?

I think yes, assuming it satisfies all our needs. But we probably should introduce such change only in a new breaking release.

If there wasn't a Rust flag required to enable this functionality, should there be a way for the user to defensively disable it for security reasons?

I am not sure... On one hand, I would prefer if it was impossible to silently overwrite default source without downstream user knowledge, but on the other hand we have #[global_allocator] which allows a similar thing...

[dhardy]

This would indeed be a nice feature to have.

I would suggest using an externally-implementable trait. Certainly the default impls for u32 and u64 should be implemented over fill_inner and not over the default backend. Also, we shouldn't allow crate A to provide fill_inner and crate B to provide inner_u32; at most one crate should supply the custom backend.

Yes, it's fine.

Do we have a real use-case for this yet? Once stable it should likely replace the current custom backend system; in the mean-time is there any actual reason to support two methods?

but on the other hand we have #[global_allocator] which allows a similar thing

I'm not sure that has the same security implications. Personally I prefer that custom backends be off-by-default with only the binary being able to enable the functionality (which is approximately what rust flags achieve).

[newpavlov]

I would suggest using an externally-implementable trait.

Can it work with the rand_core::TryRng trait? We could make rand_core non-optional dependency in a future breaking release.

in the mean-time is there any actual reason to support two methods?

I think it's worth to test extern_item_impls as an opt-in backend to find any potential issues. Additionally, for some users it may be a worthwhile alternative to the custom opt-in backend.

[dhardy]

Can it work with the rand_core::TryRng trait?

I don't think so... in any case, the Error type is fixed here and next_u32 / next_u64 should have default impls (assuming most custom backends must implement fill_bytes).

[bushrat011899]

extern_item_impls certainly looks very interesting! But I would prefer if it was implemented as a new opt-in backend.

Basically a copy of how the custom backend, just using the nightly feature instead of an extern function?

Should this be merged even in a nightly-only fashion?

Yes, it's fine.

I'll mark this PR as ready for review then!

If this was stable, should it be the default?

I think yes, assuming it satisfies all our needs. But we probably should introduce such change only in a new breaking release.

I think so too. This feels like a big enough change to warrant a new breaking release, especially since it makes the custom backend redundant (if users want the same functionality they can just define an extern function and call it in the eii method themselves).

If there wasn't a Rust flag required to enable this functionality, should there be a way for the user to defensively disable it for security reasons?

I am not sure... On one hand, I would prefer if it was impossible to silently overwrite default source without downstream user knowledge, but on the other hand we have #[global_allocator] which allows a similar thing...

Another option is we could publicly export whatever the default backend is. So users who want to ensure the default backend is used can just call the default in their own eii function.

[newpavlov]

Basically a copy of how the custom backend, just using the nightly feature instead of an extern function?

Yes. It also may require all 3 functions instead of just fill_bytes.

[bushrat011899]

I would suggest using an externally-implementable trait.

I do think that's a better fit, but I'm not sure how far along that feature is? It appears to me at least that externally implementable functions will land relatively soon.

Also, we shouldn't allow crate A to provide fill_inner and crate B to provide inner_u32; at most one crate should supply the custom backend.

I'm not sure if we can prevent that with the nightly feature as-is. We definitely could with extern traits though. We could also just advise libraries/users to override all 3 as best practice (maybe export inner_u32/64 so they can explicitly use the defaults).

Yes, it's fine.

Do we have a real use-case for this yet? Once stable it should likely replace the current custom backend system; in the mean-time is there any actual reason to support two methods?

My thinking is it's be good to allow nightly users to battle test the feature before eventually (maybe) relying on it more heavily once stable. It does have the benefit of allowing the override of the u32 and u64 methods, which the current custom backend cannot though.

but on the other hand we have #[global_allocator] which allows a similar thing

I'm not sure that has the same security implications. Personally I prefer that custom backends be off-by-default with only the binary being able to enable the functionality (which is approximately what rust flags achieve).

Thankfully this question can be kicked down the road for now, since it's a nightly feature and will require a flag until stabilised. Personally, I think this should be on by default in a future breaking release. If it was, the experience of using a 3rd party library as a backend would be as simple as including it as a dependency; no RUSTFLAGS shenanigans, and no confusing linker failure like with the current custom backend.