Add Unix fork protection #466

pitdicker · 2018-05-20T13:01:35Z

This adds a simple kind of fork protection to ReseedingRng using pthread_atfork and a global static.
It is not optimal in two ways:

it only works on Unix (is it even necessary on Windows), but I think it does not get completely optimized out on Windows.
The check only happens when a new block of values has to be generated.

Let's first see how bad the CI thinks it is...

pitdicker · 2018-05-20T16:31:54Z

We could consider using __register_atfork, like LibreSSL does to prevent linking with pthread, but I don't know if there is any advantage in that. libressl/portable@32d9eee

dhardy

Good work!

CI passes, so it seems this is not broken anywhere (I guess Unix implies libc or a compatible replacement)
Presumably performance overhead is minimal

So probably fine, but I'll take a closer look later. Also this will wait until after the 0.5 release.

dhardy · 2018-05-21T07:05:18Z

src/rngs/adapter/reseeding.rs

@@ -161,11 +166,14 @@ where R: BlockRngCore + SeedableRng,
    /// * `reseeder`: the RNG to use for reseeding.
    pub fn new(rng: R, threshold: u64, reseeder: Rsdr) -> Self {
        assert!(threshold <= ::core::i64::MAX as u64);
+        register_fork_handler();


You call this for every new reseeding RNG...

I thought about this also. We can use RESEEDING_RNG_FORK_COUNTER to see if it has already been registered, by setting it to 0 the first time (and something like u64::MAX otherwise).

Not unless you're careful to avoid reseed_counter ever being set to MAX. Obviously it shouldn't, but seems more fragile regarding code adjustment, so better to initialise to 0 then update to 1 maybe?

I don't think it matters much. But I would like to see someone forking a process u64::MAX times 😄.

dhardy · 2018-05-21T07:07:48Z

src/rngs/adapter/reseeding.rs

+#[cfg(all(feature="std", unix))]
+fn register_fork_handler() {
+    unsafe {
+        libc::pthread_atfork(None, None, Some(forkhandler));


... to register a parameterless function updating a static variable. I don't think this approach is wrong, but it will probably update the counter more than necessary as soon as multiple threads/reseeding RNGs are used.

So use std::sync::Once to register forkhandler maybe?

pitdicker · 2018-05-21T11:30:57Z

Forgot about emscripten, which is sort-of Unix but without threads/forking etc.

Also this will wait until after the 0.5 release.

That is completely the intention 😄.

pitdicker · 2018-05-21T11:34:34Z

Before:

test thread_rng_u32        ... bench:       2,959 ns/iter (+/- 15) = 1351 MB/s
test thread_rng_u64        ... bench:       4,274 ns/iter (+/- 116) = 1871 MB/s

After:

test thread_rng_u32        ... bench:       2,886 ns/iter (+/- 10) = 1386 MB/s
test thread_rng_u64        ... bench:       4,317 ns/iter (+/- 11) = 1853 MB/s

(the u32 test remains a bit variable with HC-128)

pitdicker · 2018-05-21T12:39:25Z

The very simple program I used for testing:

extern crate rand;
extern crate libc;
extern crate env_logger;

use rand::prelude::*;

fn main() {
    env_logger::init();
    println!("{}", thread_rng().gen::<u32>());

    unsafe {
        libc::fork();
    }
    for i in 0..16 {
        println!("{}, {}", i, thread_rng().gen::<u32>());
    }
}

Output:

DEBUG 2018-05-21T12:37:06Z: rand::rngs::os::imp: OsRng: testing getrandom
 INFO 2018-05-21T12:37:06Z: rand::rngs::os::imp: OsRng: using getrandom
DEBUG 2018-05-21T12:37:06Z: rand::rngs::entropy: EntropyRng: using OsRng
1412561879
0, 1339319797
1, 3354836293
2, 142399467
3, 4164526720
4, 2601913822
5, 2804858658
6, 2621357469
7, 3127216253
8, 3752501422
9, 3323828289
10, 4152097158
11, 1302825427
12, 1759720918
13, 1106126016
14, 2406812502
15, 362280914
0, 1339319797
1, 3354836293
2, 142399467
3, 4164526720
4, 2601913822
5, 2804858658
6, 2621357469
7, 3127216253
8, 3752501422
9, 3323828289
10, 4152097158
11, 1302825427
12, 1759720918
13, 1106126016
14, 2406812502
 WARN 2018-05-21T12:37:06Z: rand::rngs::adapter::reseeding: Fork detected, reseeding RNG
15, 4196445698

At round nr 15 the fork is detected, the RNG of the forked process gets reseeded, and the generated values are different.

dhardy · 2018-05-21T12:51:40Z

Well, it should be detected after 16 outputs, right? Though looks like 15 above (including initial number); maybe something else used thread_rng. But confirms it works.

Forgot about emscripten, which is sort-of Unix but without threads/forking etc.

Well presumably it still implements the pthread API but doesn't do anything? Looks like there is experimental pthread support in fact.

pitdicker · 2018-05-22T05:22:06Z

Well, it should be detected after 16 outputs, right? Though looks like 15 above (including initial number); maybe something else used thread_rng.

My example is probably not the clearest. It uses one value from thread_rng to make sure it is initialized, not much to fork otherwise. Nr. 0-14 are the remaining 15 values. Nr. 15 is the start of a new block.

Well presumably it still implements the pthread API but doesn't do anything? Looks like there is experimental pthread support in fact.

Ah, didn't know that it is being experimented with. The example code didn't compile without removing emscripten support, but I could re-check that...

dhardy

Also document the fork protection on ReseedingRng?

dhardy · 2018-05-22T13:12:02Z

src/rngs/adapter/reseeding.rs

-        let threshold = if let Err(e) = self.reseed()  {
+        let fork_counter = get_fork_counter();
+        if self.fork_counter < fork_counter {
+            warn!("Fork detected, reseeding RNG");


Probably info or trace is more appropriate. It's not actually a "problem". In a way it would be nice to use the same level for both.

dhardy · 2018-05-22T13:18:11Z

src/rngs/adapter/reseeding.rs

-        if self.bytes_until_reseed <= 0 {
-            // We get better performance by not calling only `auto_reseed` here
+        if self.bytes_until_reseed <= 0 ||
+           self.fork_counter < get_fork_counter() {


I wonder if we'd get better performance by using binary OR (|) to allow calculation of both conditions simultaneously? Normally both would be false.

dhardy · 2018-05-22T13:21:33Z

src/rngs/adapter/reseeding.rs

+            let num_bytes =
+                results.as_ref().len() * size_of::<<R as BlockRngCore>::Item>();
+            self.fork_counter = fork_counter;
+            self.threshold - num_bytes as i64


num_bytes should be small relative to threshold and we aren't exact with its handling anyway, so is there any point subtracting it? You've already adjusted the should_retry behaviour here.

Actually, why make this change at all (i.e. why not revert the num_bytes subtraction to how it was)?

Forgot (and rediscovered) why I changed things here, the old core was slightly confusing for me.
threshold is set to 0 on ErrorKind::Transient. Then bytes_until_reseed was threshold - num_bytes. Does the case Transient work correctly?

I have partly undone the changes, and only set the delay caused by ErrorKind::Transient to num_bytes, to get the right value when logging.

dhardy · 2018-05-22T13:33:44Z

src/rngs/adapter/reseeding.rs

+fn register_fork_handler() {}
+
+#[cfg(all(feature="std", unix, not(target_os="emscripten")))]
+fn get_fork_counter() -> u64 { unsafe { RESEEDING_RNG_FORK_COUNTER } }


Do you know why reading mutable static data is unsafe? Because it's not even guaranteed that the value read will equal the old or new value if concurrent with a write.

So can get_fork_counter() and forkhandler() be called concurrently? If so we should instead use std::sync::atomic::AtomicUsize (with relaxed ordering).

dhardy · 2018-05-22T13:33:56Z

src/rngs/adapter/reseeding.rs

+static mut RESEEDING_RNG_FORK_COUNTER: u64 = ::core::u64::MAX;
+
+#[cfg(all(feature="std", unix, not(target_os="emscripten")))]
+extern fn forkhandler() {


fork_handler for consistency?

pitdicker · 2018-05-23T18:02:05Z

I thought it didn't matter if checking/setting the static was a bit racy, but that was wrong. It will probably not be before the weekend that I can change update this.

dhardy

This doesn't need to be postponed any longer. Do you want to update it?

dhardy · 2018-06-08T10:33:01Z

src/rngs/adapter/reseeding.rs

+            let num_bytes =
+                results.as_ref().len() * size_of::<<R as BlockRngCore>::Item>();
+            self.fork_counter = fork_counter;
+            self.threshold - num_bytes as i64


Actually, why make this change at all (i.e. why not revert the num_bytes subtraction to how it was)?

pitdicker · 2018-06-15T11:21:32Z

Updated, but only tested on Windows. And that doesn't do much good for Unix fork protection 😄. I'll test it this evening or tomorrow.

dhardy · 2018-06-15T11:54:20Z

Thanks. I tested successfully:

extern crate rand;
extern crate libc;
extern crate env_logger;

use rand::prelude::*;

fn main() {
    env_logger::init();
    println!("10 random numbers: {:?}",
        (0..10).map(|_| thread_rng().gen::<u32>()).collect::<Vec<_>>());

    println!("Forking");
    unsafe {
        libc::fork();
    }
    for i in 0..10 {
        println!("{}\t{}", i, thread_rng().gen::<u32>());
    }
}

output:

DEBUG 2018-06-15T11:53:05Z: rand::rngs::os::imp: OsRng: testing getrandom
 INFO 2018-06-15T11:53:05Z: rand::rngs::os::imp: OsRng: using getrandom
TRACE 2018-06-15T11:53:05Z: rand::rngs::os::imp: OsRng: reading 32 bytes via getrandom
DEBUG 2018-06-15T11:53:05Z: rand::rngs::entropy: EntropyRng: using OsRng
10 random numbers: [1963114428, 767501265, 2135083893, 4131317456, 2836868865, 3043376375, 1105308048, 2162474411, 3928815976, 1138765833]
Forking
0       1978668678
1       1237056761
2       2838703247
3       1355971655
4       2047927564
5       2348773195
6       807637979
7       1752572411
8       2057889876
9       2016292138
0       1978668678
1       1237056761
2       2838703247
3       1355971655
4       2047927564
5       2348773195
 INFO 2018-06-15T11:53:05Z: rand::rngs::adapter::reseeding: Fork detected, reseeding RNG
TRACE 2018-06-15T11:53:05Z: rand::rngs::os::imp: OsRng: reading 32 bytes via getrandom
6       1002371784
7       2880221289
8       3982627430
9       3694601920

pitdicker · 2018-06-15T12:08:49Z

Thank you for testing 👍.

Just remembered the documentation still needs a little work. Now that ReseedingRng reseeds on clone() and forks, it is starting to look like something useful instead of mostly a historical leftover.

dhardy · 2018-06-15T12:04:37Z

src/rngs/adapter/reseeding.rs

+        } else {
+            // FIXME: the amount logged here may not be correct if the intial
+            // `bytes_until_reseed` was set by a delay instead of `threshold`.
+            trace!("Reseeding RNG after {} generated bytes",


We don't need to include the byte count here at all. In fact since it can be incorrect, it might be better not to.

dhardy · 2018-06-15T12:06:49Z

src/rngs/adapter/reseeding.rs

+// `RESEEDING_RNG_FORK_COUNTER`, it is time to reseed this RNG.
+//
+// If reseeding fails, we don't deal with this by setting a delay, but just
+// don't update `fork_counter`, so a reseed is attempted a soon as possible.


dhardy · 2018-06-15T12:09:40Z

src/rngs/adapter/reseeding.rs

+// If reseeding fails, we don't deal with this by setting a delay, but just
+// don't update `fork_counter`, so a reseed is attempted a soon as possible.
+#[cfg(all(feature="std", unix, not(target_os="emscripten")))]
+static RESEEDING_RNG_FORK_COUNTER: AtomicUsize = ATOMIC_USIZE_INIT;


You could use #[cfg(..)] mod fork { .. } so that the cfg stuff doesn't need to be repeated so much. Not important.

Much cleaner 😄

dhardy · 2018-06-15T15:55:59Z

src/rngs/adapter/reseeding.rs

+/// - On a manual call to [`reseed()`].
+/// - After `clone()`, the clone will be reseeded on first use.
+/// - After a process is forked, the RNG in the child process is reseeded on
+///   first use. Several values may be returned before the fork is detected.


After a process is forked, the RNG in the child process is reseeded within the next few generated values, depending on the block size of the underlying PRNG. For ChaChaRng and Hc128Rng this is a maximum of 15 u32 values before reseeding.

Your doc isn't quite accurate, and also not very precise.

Yes, better 👍

dhardy · 2018-06-15T16:03:21Z

src/rngs/adapter/reseeding.rs

-    /// * `threshold`: the number of generated bytes after which to reseed the RNG.
-    /// * `reseeder`: the RNG to use for reseeding.
+    /// `threshold` sets the number of generated bytes after which to reseed the
+    /// PRNG. Set it to zero to never reseed based on the number of renerated


dhardy · 2018-06-15T16:03:44Z

src/rngs/adapter/reseeding.rs

+    // `RESEEDING_RNG_FORK_COUNTER`, it is time to reseed this RNG.
+    //
+    // If reseeding fails, we don't deal with this by setting a delay, but just
+    // don't update `fork_counter`, so a reseed is attempted as soon a


as soon as

Third time lucky? 😆

And support setting no threshold with zero

dhardy reviewed May 21, 2018

View reviewed changes

pitdicker force-pushed the fork_protection branch from acdd6d7 to 4caf86b Compare May 21, 2018 11:28

dhardy reviewed May 22, 2018

View reviewed changes

dhardy added the P-postpone Waiting on something else label May 30, 2018

dhardy added V-0.6 and removed V-0.6 P-postpone Waiting on something else labels Jun 7, 2018

dhardy reviewed Jun 8, 2018

View reviewed changes

dhardy added the D-changes Do: changes requested label Jun 8, 2018

pitdicker force-pushed the fork_protection branch 2 times, most recently from fc48dce to e96b616 Compare June 15, 2018 11:40

dhardy approved these changes Jun 15, 2018

View reviewed changes

dhardy reviewed Jun 15, 2018

View reviewed changes

pitdicker added 4 commits June 15, 2018 18:14

Add Unix fork protection

cf84ba6

Use atomic fork counter

bccefbc

Create fork module

e9013ef

Update ReseedingRng documentation

5edec75

And support setting no threshold with zero

pitdicker force-pushed the fork_protection branch from b84861f to 5edec75 Compare June 15, 2018 16:16

dhardy added T-entropy and removed D-changes Do: changes requested labels Jun 15, 2018

dhardy merged commit bd3a806 into rust-random:master Jun 15, 2018

pitdicker deleted the fork_protection branch June 15, 2018 17:11

This was referenced Jun 15, 2018

Fork protection / reseeding / pub-priv generators #314

Closed

Tracker: 0.6 release #520

Closed

dhardy added this to the 0.6 release milestone Aug 23, 2018

sync-by-unito bot mentioned this pull request Dec 15, 2020

Bump rand_core from 0.5.1 to 0.6.0 ProvableHQ/leo#503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Unix fork protection #466

Add Unix fork protection #466

pitdicker commented May 20, 2018

pitdicker commented May 20, 2018

dhardy left a comment

dhardy May 21, 2018

pitdicker May 21, 2018

dhardy May 21, 2018

pitdicker May 21, 2018

dhardy May 21, 2018

pitdicker commented May 21, 2018

pitdicker commented May 21, 2018

pitdicker commented May 21, 2018

dhardy commented May 21, 2018

pitdicker commented May 22, 2018

dhardy left a comment

dhardy May 22, 2018

dhardy May 22, 2018

dhardy May 22, 2018

dhardy Jun 8, 2018

pitdicker Jun 15, 2018 •

edited

Loading

dhardy May 22, 2018

dhardy May 22, 2018

pitdicker commented May 23, 2018

dhardy left a comment

dhardy Jun 8, 2018

pitdicker commented Jun 15, 2018

dhardy commented Jun 15, 2018

pitdicker commented Jun 15, 2018

dhardy Jun 15, 2018

dhardy Jun 15, 2018

dhardy Jun 15, 2018

pitdicker Jun 15, 2018

dhardy Jun 15, 2018

pitdicker Jun 15, 2018

dhardy Jun 15, 2018

dhardy Jun 15, 2018

Add Unix fork protection #466

Add Unix fork protection #466

Conversation

pitdicker commented May 20, 2018

pitdicker commented May 20, 2018

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented May 21, 2018

pitdicker commented May 21, 2018

pitdicker commented May 21, 2018

dhardy commented May 21, 2018

pitdicker commented May 22, 2018

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented May 23, 2018

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented Jun 15, 2018

dhardy commented Jun 15, 2018

pitdicker commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker Jun 15, 2018 •

edited

Loading