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

Possible atomicity violation in reseeding register_fork_handler #911

Closed
BurtonQin opened this issue Nov 21, 2019 · 8 comments · Fixed by #928 · May be fixed by transparencies/yew#5
Closed

Possible atomicity violation in reseeding register_fork_handler #911

BurtonQin opened this issue Nov 21, 2019 · 8 comments · Fixed by #928 · May be fixed by transparencies/yew#5

Comments

@BurtonQin
Copy link

pub fn register_fork_handler() {
if !FORK_HANDLER_REGISTERED.load(Ordering::Relaxed) {
unsafe { libc::pthread_atfork(None, None, Some(fork_handler)) };
FORK_HANDLER_REGISTERED.store(true, Ordering::Relaxed);
}
}
}

In function register_fork_handler:

  1. load and check the atomic variable FORK_HANDLER_REGISTERED.
  2. if not, call the C library function.
  3. set the atomic variable so that the next call of the same function will not execute the C library function again.

It seems to me that atomic FORK_HANDLER_REGISTERED is utilized to guarantee that libc::pthread_at_fork is only called once.

However, if the function is called by two threads, a possible atomicity violation may happen:

  1. Th1: load and check
  2. Th2: load and check; call C function; set the value
  3. Th1: call C function again

The fix is simple: use one compare_and_swap() to replace the two separate atomic load and store.

@dhardy
Copy link
Member

dhardy commented Nov 21, 2019

Agreed, this doesn't look safe. Since the fix should be straightforward, would you like to make a PR? (Update the CHANGELOG too.)

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 21, 2019

Using compare_and_swap would allow: thread 1 calls ReseedRng::new and runs until just after thr cmpxchg; thread 2 calls ReseedRng::new,thinks it is initialized returns and spawns new process; thread 1 continues running.

Using std::sync::Once should fix this.

@dhardy
Copy link
Member

dhardy commented Nov 21, 2019

@bjorn3 agreed. Unfortunately Once is only available in std. This leaves us with three choices:

  1. Restrict ReseedingRng (and the whole adapter module) to #[cfg(feature="std")]. Since this is likely not of use in no_std and thread_rng is already resticted to std this is probably the best choice, however it necessitates a breaking release of Rand.
  2. Only call the fork handler when on Unix and cfg(feature = "std"). (This combination doesn't technically need to exist, but thanks to the way no_std works is still a valid compile target, thus we shouldn't break it.)
  3. Implement Once ourselves with a spin-lock. This is the worst option IMO.

@vks
Copy link
Collaborator

vks commented Nov 21, 2019

Option 2 seems best, because it is not a breaking change.

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 21, 2019

What about using spin::Once?

@dhardy
Copy link
Member

dhardy commented Nov 21, 2019

My preference is still option 1. @BurtonQin did you encounter this bug in action or find it through analysis? I'm wondering how easy it is to hit in practice (and how much to care about a quick fix and backports).

@BurtonQin
Copy link
Author

Through analysis.

My preference is still option 1. @BurtonQin did you encounter this bug in action or find it through analysis? I'm wondering how easy it is to hit in practice (and how much to care about a quick fix and backports).

@dhardy
Copy link
Member

dhardy commented Nov 22, 2019

Then I suggest we implement (2) now and (1) for Rand 0.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants