Skip to content
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.

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

Implement own xorshift PRNG to avoid rand dependency #144

Merged
merged 3 commits into from
May 30, 2019

Conversation

faern
Copy link
Collaborator

@faern faern commented May 30, 2019

Trying to solve #138

The PRNG implementation is stolen from https://github.com/rust-lang/rust/blob/c28084ac16af4ab594b6860958df140e7c876a13/src/libcore/slice/sort.rs#L486-L493 as pointed out in the issue.

If we are happy with the PRNG algorithm, the only question is the seed. I simply used a pointer readily available when creating the FairTimeout. We could potentially also use the nanos offset from the elapsed timeout as a seed. Something like:

let seed = now.duration_since(self.timeout).as_nanos() as u32;
let nanos = Self::gen_u32_with_seed(seed) % 1_000_000;

@faern
Copy link
Collaborator Author

faern commented May 30, 2019

A seed of zero will generate the output zero. If that ever happened we would be stuck forever on zero. Luckily the only input that gives zero as output is zero. So we must make sure the initial seed is non-zero and we are fine.

EDIT: The pointer should never be zero (null). But casting a pointer to u32 could very well result in zero.

}

impl FairTimeout {
#[inline]
fn new(timeout: Instant) -> FairTimeout {
FairTimeout { timeout, rng: SmallRng::from_entropy() }
FairTimeout { timeout, seed: &timeout as *const _ as usize as u32 }
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a big fan of using memory addresses as seeds since they could in theory be used to leak memory layout information to attackers and defeat address space randomization. Could we maybe use timeout.nanos() + 1 as the initial seed instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. I just now realized other downsides with the address approach as well. Fixing 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, Instant has no nanos method. We would need to compute a Duration for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could just take the seed as argument and have HashTable::new just have an incrementing counter it sends. So first FairTimeout has seed 1 etc.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, an incrementing counter would work fine.

@faern
Copy link
Collaborator Author

faern commented May 30, 2019

I removed the word "random" from "Random time between 0 and 1ms.". This is now very deterministic so I did not want to use the word in the wrong way.

@faern
Copy link
Collaborator Author

faern commented May 30, 2019

I will be offline from now until Monday.

@Amanieu Amanieu merged commit 6087747 into Amanieu:master May 30, 2019
@faern faern deleted the vendor-own-prng branch June 3, 2019 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants