Audit rand - Githubissues

TyPR124 commented 5 years ago

Currently the most downloaded crate on crates.io.

Contains quite a few unsafe

Functions  Expressions  Impls  Traits  Methods  Dependency

0/0        37/95        0/0    0/0     0/0      !  rand 0.7.2
0/4        4/80         0/0    0/0     0/1      !  ├── getrandom 0.1.13
0/0        0/0          0/0    0/0     0/0      ?  │   └── cfg-if 0.1.10
0/0        0/0          0/0    0/0     0/0      ?  ├── rand_chacha 0.2.1
0/0        0/0          0/0    0/0     0/0      ?  │   ├── c2-chacha 0.2.3
2/2        469/500      0/0    0/0     14/22    !  │   │   └── ppv-lite86 0.2.6
0/0        22/22        0/0    0/0     0/0      !  │   └── rand_core 0.5.1
0/4        4/80         0/0    0/0     0/1      !  │       └── getrandom 0.1.13
0/0        22/22        0/0    0/0     0/0      !  ├── rand_core 0.5.1
0/0        0/0          0/0    0/0     0/0      ?  └── rand_pcg 0.2.1
0/0        22/22        0/0    0/0     0/0      !      └── rand_core 0.5.1

Lokathor commented 5 years ago

I audited some of it in the past and sent in a few small fixes.

Unfortunately, having them take on extra dependencies is less likely because of how central the crate is, but there is probably still space to improve.

Phlosioneer commented 4 years ago

This task should probably be broken up into the sub-crates. A lot of the unsafe code is mostly to use OS functions while in no_std mode, from what I've gathered. The first step is to figure where nontrivial unsafe code is.

dhardy commented 4 years ago

Extracted from a reddit post:

[..] inspired me to have a quick look at uses of unsafe in the Rand crate. It would seem that uses can be categorised under:

Type conversions. For example, accessing an [i16] as &mut [u8] is unsafe only in that interpretation of the values is not so well defined (in practice, one must byte-swap on Big or Little Endian to get consistent results). What does this mean in practice? (a) that the type prover cannot constrain the output values [which it couldn't anyway in this case], and (b) that results may be platform dependent. So this is not memory safety, but still unsafe.
The reverse type conversion (e.g. [u8; 8] to u64). This does come with a memory safety issue: alignment, and thus we use ptr::read_unaligned.
ptr::copy_nonoverlapping: in our uses the borrow checker should (in theory) be able to prove that the source and target do not alias, that both regions are valid, and that the target values are valid (since the target is an integer array which does not have invalid values). So it may be viable for the type system to validate this in the future.
Calling core::ptr::NonNull::as_mut on a thread-local object. As far as I understand, the unsafety comes from the inability of the borrow checker to guard against concurrent mutation. We could instead use Rc and rely on the std lib's more complex abstraction over unsafe, but is that an improvement?
Accessing some intrinsics to support SIMD types. This is currently feature-gated and nightly only.
Constructing a char sampled from a fixed range. I guess this comes down to a choice of guaranteeing performance over safety, and may be the wrong choice in this instance (feel free to open a PR).
FFI to access platform functionality not available through std
To avoid a performance penalty associated with bounds checks.

So in my view, unsafe is a big hammer where often a much smaller, more specialised tool could do the job. I have in the past found memory safety issues in unsafe code which had nothing to do with the motivation for using unsafe, but which were nevertheless hidden by use of it. Better tools could go a long way to improving this situation, e.g. things like unsafe_assertion(i < len) or unsafe(concurrent_access).

It looks like this repository is focussed on memory safety, so I'd just like to quickly mention that Rand has a few other safety concerns: that generated keys/values are filled with random data, that RNGs are correctly initialised, that CSPRNG state is not inadvertently leaked, that CSPRNGs correspond to published test vectors, and a few other bits like fork detection.

alex commented 4 years ago

To point number two, could those be replaced with u64::from_ne_bytes()?

dhardy commented 4 years ago

@alex ~no, but see this post to avoid redundant discussion.~

Possibly yes actually, though I think it requires a more recent compiler than our current MSRV of 1.32.

TyPR124 commented 4 years ago

u64::from_ne_bytes() is available on 1.32 so it should be doable while keeping MSRV 1.32, unless there's something else that would be needed.