Problem: global random init/deinit breaks existing applications #3001
Conversation
|
@sigiesec this is what I meant - for libsodium, revert to naive behaviour that we had initially before I did the first change that started breaking stuff, without any serialisation |
src/random.cpp
Outdated
| @@ -86,6 +86,13 @@ uint32_t zmq::generate_random () | |||
| // order fiasco, this is done using function-local statics, if the | |||
| // compiler implementation supports thread-safe initialization of those. | |||
| // Otherwise, we fall back to global statics. | |||
| // HOWEVER, this initialisation code adds a race condition when an | |||
There was a problem hiding this comment.
It is not a race condition (this has nothing to do with concurrency). Maybe:
HOWEVER, this initialisation code imposes ordering constraints, which are not obvious to users of libzmq, and may lead to problems if atexit or similar methods are used for cleanup.
|
|
||
| #elif defined(ZMQ_USE_LIBSODIUM) | ||
| if (init) { | ||
| int rc = sodium_init (); |
There was a problem hiding this comment.
Since this does not use the refcount, this means that sodium_init and randombytes_close will be called for each context. Is that what you meant?
There was a problem hiding this comment.
Yes - that is how it worked before my initial change. They were called inline in the Context class constructor and destructor.
In reality I think randombytes_close is mostly a no-op in libsodium, and nobody ever reported a problem with the multiple sodium_init calls either (with libsodium again).
There was a problem hiding this comment.
Adding a clarifying correction for anyone who comes here in the future: this is only a no-op if getrandom is unavailable (already excluded in the above condition). If not, libsodium has all the same threadsafety issues as tweetnacl, so this PR causes crashes because libsodium has all the same requirements as tweetnacl here.
Solution: restrict it only to the original issue zeromq#2632, Tweetnacl on *NIX when using /dev/urandom, ie: without the new Linux getrandom() syscall. Existing applications might use atexit to register cleanup functions (like CZMQ does), and the current change as-is imposes an ordering that did not exist before - the context MUST be created BEFORE registering the cleanup with atexit. This is a backward incompatible change that is reported to cause aborts in some applications. Although libsodium's documentation says that its initialisation APIs is not thread-safe, nobody has ever reported an issue with it, so avoiding the global init/deinit in the libsodium case is the less risky option we have. Tweetnacl users on Windows and on Linux with getrandom (glibc 2.25 and Linux kernel 3.17) are not affected by the original issue. Fixes zeromq#2991
bluca
force-pushed
the
sodium_global_init
branch
from
5fa0f0d to
8f5fc70
Compare
Solution: restrict it only to the original issue #2632, Tweetnacl on
*NIX when using /dev/urandom, ie: without the new Linux getrandom()
syscall.
Existing applications might use atexit to register cleanup functions
(like CZMQ does), and the current change as-is imposes an ordering
that did not exist before - the context MUST be created BEFORE
registering the cleanup with atexit. This is a backward incompatible
change that is reported to cause aborts in some applications.
Although libsodium's documentation says that its initialisation APIs
is not thread-safe, nobody has ever reported an issue with it, so
avoiding the global init/deinit in the libsodium case is the less
risky option we have.
Tweetnacl users on Windows and on Linux with getrandom (glibc 2.25 and
Linux kernel 3.17) are not affected by the original issue.
Fixes #2991