Bug 185138 – [Curl] Add OpenSSL/LibreSSL multi-threading support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Basuke Suzuki]

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/

[Basuke Suzuki]

Created attachment 339130 [details]
PATCH

[Per Arne Vollan]

Would it be sufficient to require that OpenSSL 1.1.0 or later is used?

[Basuke Suzuki]

We believe so. What is your worry for instance?

[Per Arne Vollan]

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?

[Basuke Suzuki]

Oh, that’s not true. This is required for both OpenSSL prior to 1.1.0 and whole version of LibreSSL.

[Per Arne Vollan]

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?

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 339278 [details]
fix

[Basuke Suzuki]

Created attachment 339279 [details]
fix

[Per Arne Vollan]

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.

[Per Arne Vollan]

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.

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 339340 [details]
Fix potential security issue

[EWS Watchlist]

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.

[Basuke Suzuki]

Created attachment 339353 [details]
Fix style

[Per Arne Vollan]

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.

[Basuke Suzuki]

Created attachment 339391 [details]
Fox

Thanks for r+ Per Arne.

[Basuke Suzuki]

Created attachment 339392 [details]
typo

[WebKit Commit Bot]

Comment on attachment 339392 [details]
typo

Clearing flags on attachment: 339392

Committed r231297: <https://trac.webkit.org/changeset/231297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39934014>