Skip to content

Remove cloudabi, winapi, and fuchsia-cprng dependancies #40

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

Merged
merged 6 commits into from
Jun 27, 2019
Merged

Remove cloudabi, winapi, and fuchsia-cprng dependancies #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
12b4f8c
Fix libc dependancies
josephlr Jun 26, 2019
c720a5d
Ony automatically include std impls on platforms using std anyway
josephlr Jun 26, 2019
bddc134
Remove `cloudabi` crate dependancy
josephlr Jun 26, 2019
11931a3
Remove `winapi` crate dependancy
josephlr Jun 26, 2019
68707a5
Remove `fuchsia-cprng` dependancy
josephlr Jun 26, 2019
d54ba39
Cleanup Cargo.toml
josephlr Jun 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ matrix:
- rustup target add x86_64-sun-solaris
- rustup target add x86_64-unknown-cloudabi
- rustup target add x86_64-unknown-freebsd
#- rustup target add x86_64-unknown-fuchsia
- rustup target add x86_64-fuchsia
- rustup target add x86_64-unknown-netbsd
- rustup target add x86_64-unknown-redox
- rustup target add x86_64-fortanix-unknown-sgx
Expand All @@ -113,7 +113,7 @@ matrix:
- cargo build --target=x86_64-sun-solaris --all-features
- cargo build --target=x86_64-unknown-cloudabi --all-features
- cargo build --target=x86_64-unknown-freebsd --all-features
#- cargo build --target=x86_64-unknown-fuchsia --all-features
- cargo build --target=x86_64-fuchsia --all-features
- cargo build --target=x86_64-unknown-netbsd --all-features
- cargo build --target=x86_64-unknown-redox --all-features
- cargo build --target=x86_64-fortanix-unknown-sgx --all-features
Expand All @@ -123,7 +123,7 @@ matrix:
- cargo build --target=x86_64-sun-solaris --all-features
- cargo build --target=x86_64-unknown-cloudabi --all-features
- cargo build --target=x86_64-unknown-freebsd --all-features
#- cargo build --target=x86_64-unknown-fuchsia --all-features
- cargo build --target=x86_64-fuchsia --all-features
- cargo build --target=x86_64-unknown-netbsd --all-features
- cargo build --target=x86_64-unknown-redox --all-features
- cargo build --target=x86_64-fortanix-unknown-sgx --all-features
Expand Down
27 changes: 8 additions & 19 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,32 +20,21 @@ members = ["tests/wasm_bindgen"]
[dependencies]
log = { version = "0.4", optional = true }

[target.'cfg(unix)'.dependencies]
libc = "0.2.34"
lazy_static = "1.3.0"

[target.'cfg(windows)'.dependencies]
winapi = { version = "0.3.6", features = ["minwindef", "ntsecapi", "winnt"] }

[target.'cfg(target_os = "cloudabi")'.dependencies]
cloudabi = "0.0.3"

[target.'cfg(fuchsia)'.dependencies]
fuchsia-cprng = "0.1"
[target.'cfg(any(unix, target_os = "wasi"))'.dependencies]
libc = "0.2.54"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to depend on libc for all targets. Maybe instead construct one big cfg expression with enumeration of targets which use libc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I just used unix || wasi. Although that's not perfect, as we don't use libc on all unix targets (only 7 of them), but writing all that out makes the Cargo.toml way harder to read.

Copy link
Member

Choose a reason for hiding this comment

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

We also could remove libc dependency for WASI as well, libc code is quite straightforward. Not sure if it's worth though.

Copy link
Member Author

Choose a reason for hiding this comment

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

We totally could (Strictly speaking we could do it for any libc declaration), but I think it’s best to not duplicate declarations from the libc, if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we wanted we could not depend on the libc at all for any target, by copying over the 6 or 7 libc declarations needed to make everything link. But I think that would make breakage/instability more likely, not less.

Copy link
Member

@newpavlov newpavlov Jun 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's just a bit strange for me to depend on libc crate for a target which, well, does not have libc as a system interface, i.e. it's defined in terms of core functions. (I wonder why WASI target was added to libc crate in the first place, instead of creating a separate crate à la cloudabi or Fuchsia crates)

Copy link
Member Author

Choose a reason for hiding this comment

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

So I think they added it because wasi-libc defines that symbol. However, it's just a macro that links to the wasi_unstable module. From their Github issues, it seems like the wasi_unstable module is an implementation detail and may change in the future.

Given that, linking to the libc seems like the most stable approach, so we are not depending on an unstable components of the WASI API.

Copy link
Member

@newpavlov newpavlov Jun 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In my understanding we shouldn't care about wasi-libc at all. WASI defines Core API and __wasi_random_get is part of it. So we should be able to use it directly without any issues.

cc @alexcrichton @gnzlbg

UPD: Ah, the linked page states that:

The function names are prefixed with "_wasi" to reflect how they are spelled in flat-namespace contexts, however at the wasm module level, they are unprefixed, because they're inside a module namespace (currently "wasi_unstable").

So it looks like random_get (to which __wasi_random_get links) is unstable. I don't quite get this situation, but it seems that using libc will be indeed the best option right now.


[target.'cfg(target_os = "redox")'.dependencies]
# For holding file descriptors
[target.'cfg(any(unix, target_os = "redox"))'.dependencies]
lazy_static = "1.3.0"

# For caching result of CPUID check for RDRAND
[target.'cfg(any(target_env = "sgx", target_os = "uefi"))'.dependencies]
lazy_static = { version = "1.3.0", features = ["spin_no_std"] }

[target.wasm32-unknown-unknown.dependencies]
wasm-bindgen = { version = "0.2.29", optional = true }
stdweb = { version = "0.4.9", optional = true }
lazy_static = "1.3.0"

[target.wasm32-wasi.dependencies]
libc = "0.2.54"

[target.'cfg(any(target_env = "sgx", target_os = "uefi"))'.dependencies]
lazy_static = { version = "1.3.0", features = ["spin_no_std"] }

[features]
std = []
15 changes: 9 additions & 6 deletions src/cloudabi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,17 @@
use crate::Error;
use core::num::NonZeroU32;

extern "C" {
fn cloudabi_sys_random_get(buf: *mut u8, buf_len: usize) -> u16;
}

pub fn getrandom_inner(dest: &mut [u8]) -> Result<(), Error> {
let errno = unsafe { cloudabi::random_get(dest) };
if errno == cloudabi::errno::SUCCESS {
Ok(())
} else {
let code = NonZeroU32::new(errno as u32).unwrap();
error!("cloudabi::random_get syscall failed with code {}", code);
let errno = unsafe { cloudabi_sys_random_get(dest.as_mut_ptr(), dest.len()) };
if let Some(code) = NonZeroU32::new(errno as u32) {
error!("cloudabi_sys_random_get failed with code {}", code);
Err(Error::from(code))
} else {
Ok(()) // Zero means success for CloudABI
}
}

Expand Down
7 changes: 6 additions & 1 deletion src/fuchsia.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,13 @@
use crate::Error;
use core::num::NonZeroU32;

#[link(name = "zircon")]
extern "C" {
fn zx_cprng_draw(buffer: *mut u8, length: usize);
}

pub fn getrandom_inner(dest: &mut [u8]) -> Result<(), Error> {
fuchsia_cprng::cprng_draw(dest);
unsafe { zx_cprng_draw(dest.as_mut_ptr(), dest.len()) }
Ok(())
}

Expand Down
10 changes: 5 additions & 5 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
//! | Solaris, illumos | [`getrandom`][9] system call if available, otherwise [`/dev/random`][10]
//! | Fuchsia OS | [`cprng_draw`][11]
//! | Redox | [`rand:`][12]
//! | CloudABI | [`random_get`][13]
//! | CloudABI | [`cloudabi_sys_random_get`][13]
//! | Haiku | `/dev/random` (identical to `/dev/urandom`)
//! | SGX, UEFI | [RDRAND][18]
//! | Web browsers | [`Crypto.getRandomValues`][14] (see [Support for WebAssembly and ams.js][14])
Expand Down Expand Up @@ -99,17 +99,17 @@
//!
//! [1]: http://man7.org/linux/man-pages/man2/getrandom.2.html
//! [2]: http://man7.org/linux/man-pages/man4/urandom.4.html
//! [3]: https://msdn.microsoft.com/en-us/library/windows/desktop/aa387694.aspx
//! [3]: https://docs.microsoft.com/en-us/windows/desktop/api/ntsecapi/nf-ntsecapi-rtlgenrandom
//! [4]: https://developer.apple.com/documentation/security/1399291-secrandomcopybytes?language=objc
//! [5]: https://www.freebsd.org/cgi/man.cgi?query=random&sektion=4
//! [6]: https://man.openbsd.org/getentropy.2
//! [7]: http://netbsd.gw.com/cgi-bin/man-cgi?random+4+NetBSD-current
//! [8]: https://leaf.dragonflybsd.org/cgi/web-man?command=random&section=4
//! [9]: https://docs.oracle.com/cd/E88353_01/html/E37841/getrandom-2.html
//! [10]: https://docs.oracle.com/cd/E86824_01/html/E54777/random-7d.html
//! [11]: https://fuchsia.googlesource.com/zircon/+/HEAD/docs/syscalls/cprng_draw.md
//! [11]: https://fuchsia.googlesource.com/fuchsia/+/master/zircon/docs/syscalls/cprng_draw.md
//! [12]: https://github.com/redox-os/randd/blob/master/src/main.rs
//! [13]: https://github.com/NuxiNL/cloudabi/blob/v0.20/cloudabi.txt#L1826
//! [13]: https://github.com/nuxinl/cloudabi#random_get
//! [14]: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues
//! [15]: https://nodejs.org/api/crypto.html#crypto_crypto_randombytes_size_callback
//! [16]: #support-for-webassembly-and-amsjs
Expand Down Expand Up @@ -153,11 +153,11 @@ macro_rules! mod_use {
};
}

// These targets use std anyway, so we use the std declarations.
#[cfg(any(
feature = "std",
windows,
unix,
target_os = "cloudabi",
target_os = "redox",
target_arch = "wasm32",
))]
Expand Down
2 changes: 1 addition & 1 deletion src/macos.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ struct SecRandom([u8; 0]);
extern "C" {
static kSecRandomDefault: *const SecRandom;

fn SecRandomCopyBytes(rnd: *const SecRandom, count: usize, bytes: *mut u8) -> libc::c_int;
fn SecRandomCopyBytes(rnd: *const SecRandom, count: usize, bytes: *mut u8) -> i32;
}

pub fn getrandom_inner(dest: &mut [u8]) -> Result<(), Error> {
Expand Down
14 changes: 8 additions & 6 deletions src/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,16 @@ extern crate std;
use crate::Error;
use core::num::NonZeroU32;
use std::io;
use winapi::shared::minwindef::ULONG;
use winapi::um::ntsecapi::RtlGenRandom;
use winapi::um::winnt::PVOID;

extern "system" {
#[link_name = "SystemFunction036"]
fn RtlGenRandom(RandomBuffer: *mut u8, RandomBufferLength: u32) -> u8;
}

pub fn getrandom_inner(dest: &mut [u8]) -> Result<(), Error> {
// Prevent overflow of ULONG
for chunk in dest.chunks_mut(ULONG::max_value() as usize) {
let ret = unsafe { RtlGenRandom(chunk.as_mut_ptr() as PVOID, chunk.len() as ULONG) };
// Prevent overflow of u32
for chunk in dest.chunks_mut(u32::max_value() as usize) {
let ret = unsafe { RtlGenRandom(chunk.as_mut_ptr(), chunk.len() as u32) };
if ret == 0 {
error!("RtlGenRandom call failed");
return Err(io::Error::last_os_error().into());
Expand Down