The older OpenSSL manual says the locking_function and threadid_funcion should set when use it in multi-threading environment. This applies to LibreSSL also. https://www.openssl.org/docs/man1.0.2/crypto/threads.html For unix and other similar os, the default threadId_function implementation is good enough. We'll set custom callback only for Windows OS. Note it's not required for OpenSSL 1.1.0 and after. https://www.openssl.org/blog/blog/2017/02/21/threads/
Created attachment 339130 [details] PATCH
Would it be sufficient to require that OpenSSL 1.1.0 or later is used?
We believe so. What is your worry for instance?
I was just wondering if this change is strictly needed, since this issue has been addressed in OpenSSL 1.1.0 or later. But this is perhaps not the case for LibreSSL?
Oh, that’s not true. This is required for both OpenSSL prior to 1.1.0 and whole version of LibreSSL.
Comment on attachment 339130 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=339130&action=review > Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:147 > + if (!lock || (type > lockNum)) > + return; I think this should be '(type >= lockNum)'. Also, should we return early if type < 0?
Comment on attachment 339130 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=339130&action=review Thank you for the review. >> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:147 >> + return; > > I think this should be '(type >= lockNum)'. Also, should we return early if type < 0? type argument are hard-coded in OpenSSL/LibreSSL code and used internally only. I think it should be not run-time check, but assert. Also lock shouldn't be checked like this. If it failed, it may fail in multi thread env. I will move it to more robust place to define locks.
Created attachment 339278 [details] fix
Created attachment 339279 [details] fix
Comment on attachment 339279 [details] fix View in context: https://bugs.webkit.org/attachment.cgi?id=339279&action=review R=me. > Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:144 > + ASSERT(type >= 0 && type < CRYPTO_NUM_LOCKS); I think we should use RELEASE_ASSERT here.
Comment on attachment 339279 [details] fix View in context: https://bugs.webkit.org/attachment.cgi?id=339279&action=review > Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:143 > + static Lock lock[CRYPTO_NUM_LOCKS]; Also, I think you need to make sure the initialization of lock is thread safe, by using call_once, for example.
Comment on attachment 339279 [details] fix View in context: https://bugs.webkit.org/attachment.cgi?id=339279&action=review Thanks for the review. >> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:143 >> + static Lock lock[CRYPTO_NUM_LOCKS]; > > Also, I think you need to make sure the initialization of lock is thread safe, by using call_once, for example. I think Lock is constexpr constructor class and it is initialized in compile time, which means no run-time initialization. I am not confident about this behavior so I am asking about this my co-workers. >> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:144 >> + ASSERT(type >= 0 && type < CRYPTO_NUM_LOCKS); > > I think we should use RELEASE_ASSERT here. Sure I do.
Created attachment 339340 [details] Fix potential security issue
Attachment 339340 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/CurlSSLHandle.h:69: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/platform/network/curl/CurlSSLHandle.h:74: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 339353 [details] Fix style
Comment on attachment 339353 [details] Fix style View in context: https://bugs.webkit.org/attachment.cgi?id=339353&action=review R=me. > Source/WebCore/ChangeLog:6 > + The older OpenSSL manual says the locking_function and threadid_funcion should Small typo, change phrase to 'threadid_function should be set'. > Source/WebCore/platform/network/curl/CurlSSLHandle.h:74 > + static ThreadSupport shared; I think we can use NeverDestroyed here.
Created attachment 339391 [details] Fox Thanks for r+ Per Arne.
Created attachment 339392 [details] typo
Comment on attachment 339392 [details] typo Clearing flags on attachment: 339392 Committed r231297: <https://trac.webkit.org/changeset/231297>
All reviewed patches have been landed. Closing bug.
<rdar://problem/39934014>