mobilecoinfoundation / sgx

SGX Libraries and Utilities
Apache License 2.0
9 stars 7 forks source link

getrandom custom target fails for target_os=none #499

Open addd45 opened 1 week ago

addd45 commented 1 week ago

I am unable to compile my library project which depends on mc-sgx-core-types when using a build target where os = none.

in mc-sgx-core-types lib.rs file, there is a call to register a custom getrandom method -

// For targets that don't have a random number source we force it to always
// fail.
// Per https://docs.rs/getrandom/latest/getrandom/macro.register_custom_getrandom.html
// this function will *only* be used if getrandom doesn't know of a native
// secure spng
#[cfg(target_os = "none")]
use getrandom::register_custom_getrandom;

#[cfg(target_os = "none")]
register_custom_getrandom!(always_fail);

#[cfg(target_os = "none")]
fn always_fail(_buf: &mut [u8]) -> Result<(), getrandom::Error> {
    Err(getrandom::Error::UNSUPPORTED)
}

This is not the correct way to register a custom getrandom per the documentation for register_custom_getrandom, per the doc (and bolding relevant part):

Registering a custom getrandom implementation

Functions can only be registered in the root binary crate. Attempting to register a function in a non-root crate will result in a linker error. This is similar to #[panic_handler] or #[global_allocator], where helper crates define handlers/allocators but only the binary crate actually uses the functionality.

Thus, when adding mc-sgx-core-sys-types as a dependency and compiling with a target where os=none indeed a linker error occurs.

error[E0432]: unresolved import `getrandom::register_custom_getrandom`
  --> /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/mc-sgx-core-types-0.11.0/src/lib.rs:44:5
   |
44 | use getrandom::register_custom_getrandom;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `register_custom_getrandom` in the root

By reading the comments I understand the goal is to default to an error scenario if no suitable get random implementation is available, but this should be something that is handled "down the line" where it is up to the users of the library to decide how they want to use getrandom. For example, my use case is to use rdrand which is something that can just be enabled with the "rdrand" feature for getrandom, but since mc-sgx-core-types enforces the "custom" feature (and the issues mentioned already), I am not able to use this library with my current build target

nick-mobilecoin commented 1 week ago

It does seem like this was implemented incorrectly :(.

To set expectations, we probably won't be prioritizing any fixes or improvements in this repo until we have a need to update the SGX bindings.

addd45 commented 1 week ago

Fair enough :).

If you prefer, and if I get some time, I could open up a PR to take out the registration part of code. I presume it would not break anything but want to be safe.

nick-mobilecoin commented 1 week ago

I'll need to spin back up on the what and why that I did there.

It seems like it should fail to link if one doesn't specify a get random function so doesn't seem like the force failure should be needed

So yes if you happen to get to creating a PR prior to I would be keen to reviewing it