Skip to content

Commit

Permalink
KUDU-3210 Add thread ID callback to OpenSSL init
Browse files Browse the repository at this point in the history
It seems the race condition bug we worked around in
f9f3189 was caused by using the default
thread ID callback.

It seems it's not a bug in SafeLogic after all, but this is likely
reproducible in upstream OpenSSL as well. We didn't find this before as
we always tested in older OpenSSL versions, while the commit[1]
responsible for this behavior was included only in OpenSSL 1.0.2i[2].

The threads(3) man page claims that "If the application does not
register such a callback using CRYPTO_THREADID_set_callback(), then a
default implementation is used - on Windows and BeOS this uses the
system's default thread identifying APIs, and on all other platforms it
uses the address of errno. The latter is satisfactory for thread-safety
if and only if the platform has a thread-local error number facility."

This seems to be no longer true in 1.0.2i and later.

Redefining the thread ID callback seems to fix the problem without any
additional locking and f9f3189 can be
reverted safely. I tested these changes on the host I discovered the
race condition.

[1] openssl/openssl@a43cfd7
[2] https://mta.openssl.org/pipermail/openssl-commits/2016-September/010743.html

Change-Id: Icec6da3a9380206fe6ba4a31ea8fb4dcbc34dd00
Reviewed-on: http://gerrit.cloudera.org:8080/16730
Reviewed-by: Grant Henke <[email protected]>
Reviewed-by: Alexey Serbin <[email protected]>
Tested-by: Kudu Jenkins
  • Loading branch information
attilabukor authored and granthenke committed Nov 17, 2020
1 parent fab3a38 commit d1285eb
Showing 1 changed file with 9 additions and 0 deletions.
9 changes: 9 additions & 0 deletions src/kudu/security/openssl_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@
#include "kudu/util/scoped_cleanup.h"
#include "kudu/util/status.h"
#include "kudu/util/subprocess.h"
#if OPENSSL_VERSION_NUMBER < 0x10100000L
#include "kudu/util/thread.h"
#endif

DEFINE_bool(openssl_defensive_locking,
false,
Expand Down Expand Up @@ -100,6 +103,10 @@ void LockingCB(int mode, int type, const char* /*file*/, int /*line*/) {
m->unlock();
}
}

void ThreadIdCB(CRYPTO_THREADID* tid) {
CRYPTO_THREADID_set_numeric(tid, Thread::UniqueThreadId());
}
#endif

void CheckFIPSMode() {
Expand Down Expand Up @@ -212,6 +219,8 @@ void DoInitializeOpenSSL() {

// Callbacks used by OpenSSL required in a multi-threaded setting.
CRYPTO_set_locking_callback(LockingCB);

CRYPTO_THREADID_set_callback(ThreadIdCB);
}
#endif
CheckFIPSMode();
Expand Down

0 comments on commit d1285eb

Please sign in to comment.