Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

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