Add le module, to be moved to rand_core later

[dhardy]

Copied from dhardy/master, minus single-read functions, with extra tests

@pitdicker do you want to switch the seed types to byte-array and update SeedableRng? You did the last changes to this on my branch and it's a little more involved than just a type switch. If you prefer I can just copy the relevant code from that branch rather than try to cherry-pick the commits (especially cc6da7a).

[clarfonthey]

Why not just depend on the byteorder crate, which is already no_std and implements what you're looking for.

[pitdicker]

Nice! You have really paved the road for SeedableRng. Two minor comments.

Yes, I would like to update SeedableRng. It will probably Saturday that I can do it.

[pitdicker]

Shall we try to keep @GabrielMajeri happy 😄? (https vs http)

[pitdicker]

I have not tested it, but the functions are public and this should not be necessary?

[dhardy]

The module's not public, at least not yet (plan is to export from rand-core not rand).

[pitdicker]

@clarcharr True. The code is copied from byteorder, and we use the same names. So it should be easy to drop byteorder in as a dependency.

But we need only very little from byteorder, and are trying to keep the number of dependencies of rand as small as reasonably possible. Any extra dependency of rand would mean an extra dependency for a lot of crates.

[BurntSushi]

byteorder is already a dependency of a lot of crates and it is a pretty small dependency with limited scope.

But yeah, I sympathize with your view. A concern I'd have here is that the code you're copying contains use of unsafe, and it would be better IMO to coalesce on one implementation when it comes to that. A possible alternative would be to implement a slower but safe version, but I don't know if that's appropriate. (Apologies for the drive-by comment without considering the wider context!)

[pitdicker]

We have had the same problem with lazy_static and array_ref (nothing merged yet). Should we depend on them? lazy_static is small and just like byteorder a common dependency.

I don't think the dependency story for rand is completely settled. And I am feeling a bit uneasy with the amount of unsafe code rand is collecting. Some of it is for performance, although the functions in this PR are only used for seeding RNG's and not performance critical. So there you have a good argument.

@BurntSushi What would you do if byteorder where in a situation like this where it could choose between depending on three extra crates, although small and well-polished, or copy ~120 lines of code and comments?

[BurntSushi]

@pitdicker If it were me and I wasn't overly concerned about performance, and it was a tiny amount of code, I'd probably just write the safe variants as small helper functions. If it came to unsafe, then I might draw the line there. But you're right, the bar would be incredibly high for me to add a dependency to byteorder (for example) mostly because today it has none.

It's a tough call either way!

[dhardy]

@clarcharr @BurntSushi finally someone has brought up the problem of crate trust, though I think we're a long way from solving this yet. But it's true, byteorder is a small, widely used crate, so we could depend on it; I'm not sure personally (mostly since we only use a small portion of byteorder anyway).

arrayref would similarly be useful, but it's a little-used crate containing unsafe code from a relatively unknown author, where we only need to copy about 15 lines of code.

lazy_static I just personally don't see the point of unless you need to re-use that particular pattern a lot; the functionality in std::sync is usually sufficient.

[pitdicker]

@dhardy What do you think about merging this now, but creating an issue that we should keep track of the amount of unsafe code in rand? To collect the answer to things like: where do we use unsafe, where does the code come from, how have we checked it is ok, why is it necessary, what are the safe alternatives?

[dhardy]

@pitdicker in my opinion worrying about unsafe is a red-herring (diversion): there are plenty of ways "safe" code can be unsafe as you know, and provided unsafe code has been well-reviewed I just don't think it's worth worrying about at this point. Maybe sometime in the future Rust will have better ways of ensuring unsafe code is well reviewed (e.g. attaching a hash function), but that's a hard problem (because it depends on code outside the unsafe block).

@BurntSushi any other thoughts? Otherwise I'll merge this (because it's not much code) and we can potentially open a tracker issue to think about dependencies.

[BurntSushi]

I don't see worrying about unsafe as a diversion. Is unsafe really needed here? If not, I'd remove it and use a safe implementation.

[dhardy]

Is there an easy safe alternative? Sure, using byteorder avoids us having to write this unsafe code, but since we're using a copy of the same code the end result is the same.

I do believe unsafe is a diversion: look at the very code we are copying. For copy_nonoverlapping to be safe, the length must be correct and the pointers must not overlap. The code is correct but these constraints are checked or enforced outside the unsafe block, thus the unsafe code could be broken e.g. by removing the assert and changing the $size number set outside the macro. IMO the whole macro should be unsafe, but I don't know that Rust supports unsafe macros? (Or otherwise $size should be obtained from size_of instead of passed as a parameter, if possible.)

Anyway, I believe code review, clean interfaces and compiler error-checking are the best tools we have to avoid bugs, and unsafe is just a small part of that.

(This isn't to say we shouldn't use an external dependency, just that I don't believe avoiding the use of unsafe should be an important goal.)

[BurntSushi]

@dhardy I gave you my thoughts. Do what you will with them. :-) Note that using byteorder was not necessarily my implication; it is possible to implement these methods both without using unsafe and without using byteorder. The cost is performance. Is it OK? I don't know.

[dhardy]

Hmm, I suppose it could be implemented with bit-shifts instead, but that also has opportunities for errors in the implementation, and is probably harder to review for correctness. (In other words, performance isn't the only cost.)

So we don't always agree. Perhaps it comes down to worrying more about memory safety or incorrect results; I admit that I often worry more about the latter, especially in something like rand where incorrect results are hard to spot and can lead to significant security issues.

[pitdicker]

With the issue linked above about crate trust on internals my first thought was how rand would do. And we seem to have quite a road to go yet.

• For example, are the CSPRNG's implemented right, do they produce the same results as their reference implementations? Can others verify this without too much effort?
• Do we initialize RNG's correctly and from good entropy sources?
• Do the various wrappers and convenience functions work right, and work they right in combinations with some PRNG?
• Should we have some forms of memory and fork protection?
• Can people have confidence in our use of unsafe code?
• Do we have tests and CI to ensure correctness?
• ...

So I also think unsafe should be just one of the worries if someone where to evaluate how much to trust rand. Splitting off a rand_core and a rand_rngs crates as we planned should help here. And I think a couple of tracking issues would be good too.

To be honest I feel in this case for the slice copy functions we are making more noise than they deserve. About using unsafe code or bitshifts here I am partial. The bigger questions how we deal with dependencies and unsafe are worth thinking about, but we have a lot of other things that seem to deserve more attention first.

[dhardy]

@pitdicker I (finally!) finished merging master back into my branch, so hopefully you can copy a lot of what you need for from_seed and from_rng from there.

[pitdicker]

I completely believe that was no easy merge! Let's hope it doesn't take long before everything is merged here.

[pitdicker]

This is merged into master, but the merge is not visible? Or you did a direct commit?

[dhardy]

I probably rebased and merged without pushing here.