utils: removed deprecated check and function for OpenSSL compatiblity #11602

azavalla commented at 8:01 PM on November 3, 2017: none

refs https://github.com/bitcoin/bitcoin/issues/11601

removed deprecated check and function 7b0b1665f4

TheBlueMatt commented at 9:58 PM on November 3, 2017: member

It looks like openssl removed the threading stuff out from under us - we should proabbly do something like check OPENSSL_THREADS (ie if openssl was built with threading support - can we also do this at runtime?) and fail to build/run if it wasn't, only setting the locking callbacks if the openssl version is low enough that they are needed.

fanquake added the label Utils and libraries on Nov 4, 2017

laanwj commented at 10:42 AM on November 6, 2017: member

It looks like openssl removed the threading stuff out from under us

What I don't understand is why this doesn't fail because it still calls CRYPTO_set_locking_callback? If they removed the threading stuff and associated flags why not that?

can we also do this at runtime?

Maybe - though even checking the OpenSSL version at runtime isn't straightforward. E.g. there used to be a function SSLeay to get a version number which they suddenly removed and replaced with OpenSSL_version_num (see https://github.com/openssl/openssl/issues/862). Also would have to consider alternative implementations such as LibreSSL and PolarSSL, which try to be compatible but with whoknowswhat...

What a mess.

TheBlueMatt commented at 1:34 PM on November 6, 2017: member

What I don't understand is why this doesn't fail because it still calls CRYPTO_set_locking_callback?

The functions themselves were kept, except they are now just #defines to nothing, the constant was removed.

I am somewhat concerned that the current code may break if OpenSSL is complied without threading support, though I'd hope that's rather rare in any case.

On November 6, 2017 5:42:29 AM EST, "Wladimir J. van der Laan" notifications@github.com wrote:

It looks like openssl removed the threading stuff out from under us

What I don't understand is why this doesn't fail because it still calls CRYPTO_set_locking_callback?

can we also do this at runtime?

Maybe - though even checking the OpenSSL version at runtime isn't straightforward. E.g. there used to be a function SSLeay to get a version number which they suddenly removed and replaced with OpenSSL_version_num (see https://github.com/openssl/openssl/issues/862). Also would have to consider alternative implementations such as LibreSSL and PolarSSL, which try to be compatible but with whoknowswhat...

What a mess.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11602#issuecomment-342111099

laanwj commented at 7:44 AM on November 7, 2017: member

I am somewhat concerned that the current code may break if OpenSSL is complied without threading support, though I'd hope that's rather rare in any case.

I had hoped that the thread-related functions would be missing from the library in that case, causing either compilation error or a runtime error in the dynamic linker. (one'd hope they don't #define them away in the case of no thread support, only because the API for thread support doesn't need them anymore, but who knows..)

laanwj renamed this:
~~utils: removed deprecated check and function~~
utils: removed deprecated check and function for OpenSSL compatiblity
on Nov 8, 2017

laanwj commented at 7:41 AM on November 8, 2017: member

In any case this can't be merged as-is. It breaks the tests, and breaks compatibility with older OpenSSL. At the least it needs #if to delimit the OpenSSL versions where it is needed versus not needed.

azavalla commented at 12:33 AM on November 9, 2017: none

They removed the thread-related functions and then #defined them again as no-ops. This means that:

Everything will work just fine for a openssl version low enough.
Compilation will fail for the openssl versions where they removed those functions.
Possibly obscure race conditions at run time for openssl versions with the no-op defines?

Is that correct?

in src/util.cpp:110 in 7b0b1665f4

 106 | @@ -107,7 +107,7 @@ std::atomic<uint32_t> logCategories(0);
 107 |  static std::unique_ptr<CCriticalSection[]> ppmutexOpenSSL;
 108 |  void locking_callback(int mode, int i, const char* file, int line) NO_THREAD_SAFETY_ANALYSIS
 109 |  {
 110 | -    if (mode & CRYPTO_LOCK) {
 111 | +    if (mode) {

laanwj commented at 11:38 AM on November 9, 2017:

Does the removal of CRYPTO_LOCK here have any influence on older versions of OpenSSL which need this? (as this was a bit field, it may now inadvertently trigger) Might be better to add the define here, if it's not defined.

in src/util.cpp:145 in 7b0b1665f4

 140 | @@ -141,8 +141,6 @@ class CInit
 141 |      }
 142 |      ~CInit()
 143 |      {
 144 | -        // Securely erase the memory used by the PRNG
 145 | -        RAND_cleanup();

laanwj commented at 11:38 AM on November 9, 2017:

We'd still want to call this function for versions of OpenSSL that need it.

laanwj commented at 11:02 AM on November 30, 2017: member

Ping @azavalla

laanwj added the label Up for grabs on Dec 7, 2017

laanwj commented at 12:53 PM on December 7, 2017: member

Closing due to inactivity, added "up for grabs" label, in case anyone else wants to have a go at this. (or let me know when you want to pick this back up)

laanwj closed this on Dec 7, 2017

MarcoFalke removed the label Up for grabs on Mar 26, 2018

MarcoFalke added the label Up for grabs on Mar 26, 2018

adamjonas commented at 8:37 PM on October 18, 2019: member

Can remove up for grabs label now that #11601 is closed.

MarcoFalke removed the label Up for grabs on Oct 18, 2019

MarcoFalke locked this on Dec 16, 2021