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.

Sign up for GitHub

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

Problem: global random init/deinit breaks existing applications #3001

Merged
merged 1 commit into from
Mar 19, 2018
Merged

Problem: global random init/deinit breaks existing applications #3001

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/random.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,14 @@ 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 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.
// 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 +113,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 +146,14 @@ static void manage_random (bool init)
randombytes_close ();
}
}

#elif defined(ZMQ_USE_LIBSODIUM)
if (init) {
int rc = sodium_init ();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, just wanted to be sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

zmq_assert (rc != -1);
} else {
randombytes_close ();
}
#endif
}

Expand Down