Skip to content

Commit

Permalink
Problem: global random init/deinit breaks existing applications
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bluca committed Mar 19, 2018
1 parent 4d9fc80 commit 5fa0f0d
Showing 1 changed file with 19 additions and 6 deletions.
25 changes: 19 additions & 6 deletions src/random.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
// application uses atexit or similar methods for cleanup callbacks.
// In that case, a strict ordering is imposed whereas the contexts MUST
// be initialised BEFORE registering the cleanup with atexit. CZMQ is an
// example. Hence we make the choice to restrict this global transition
// mechanism ONLY to Tweenacl + *NIX (when using /dev/urandom) as it is
// the less risky option.

// TODO if there is some other user of libsodium besides libzmq, this must
// be synchronized by the application. This should probably also be
Expand All @@ -105,18 +112,16 @@ uint32_t zmq::generate_random ()
#endif

#if !ZMQ_HAVE_THREADSAFE_STATIC_LOCAL_INIT \
&& (defined(ZMQ_USE_LIBSODIUM) \
|| (defined(ZMQ_USE_TWEETNACL) && !defined(ZMQ_HAVE_WINDOWS) \
&& !defined(ZMQ_HAVE_GETRANDOM)))
&& (defined(ZMQ_USE_TWEETNACL) && !defined(ZMQ_HAVE_WINDOWS) \
&& !defined(ZMQ_HAVE_GETRANDOM))
static unsigned int random_refcount = 0;
static zmq::mutex_t random_sync;
#endif

static void manage_random (bool init)
{
#if defined(ZMQ_USE_LIBSODIUM) \
|| (defined(ZMQ_USE_TWEETNACL) && !defined(ZMQ_HAVE_WINDOWS) \
&& !defined(ZMQ_HAVE_GETRANDOM))
#if defined(ZMQ_USE_TWEETNACL) && !defined(ZMQ_HAVE_WINDOWS) \
&& !defined(ZMQ_HAVE_GETRANDOM)

#if ZMQ_HAVE_THREADSAFE_STATIC_LOCAL_INIT
static int random_refcount = 0;
Expand All @@ -140,6 +145,14 @@ static void manage_random (bool init)
randombytes_close ();
}
}

#elif defined(ZMQ_USE_LIBSODIUM)
if (init) {
int rc = sodium_init ();
zmq_assert (rc != -1);
} else {
randombytes_close ();
}
#endif
}

Expand Down

0 comments on commit 5fa0f0d

Please sign in to comment.