Serialize curl_multi_cleanup calls for OpenSSL <1.1.0
#5321
Conversation
Motivation:
`URLSession` elusively crashes due to race conditions in OpenSSL <1.1.0 when calling `curl_multi_cleanup` concurrently on different threads.
Modifications:
- Add `CFURLSessionSSLVersion CFURLSessionSSLVersionInfo(void)` for detecting the OpenSSL version in use.
- Adjust `_MultiHandle.deinit` to use a process-wide lock to serialize `curl_multi_cleanup` calls for environments with OpenSSL <1.1.0.
Result:
Prevents the crash while avoiding unnecessary performance overhead for newer OpenSSL versions which are thread-safe.
Testing:
Passing existing tests is sufficient. However, without this patch, the code snippet below crashes about once in 20 runs on Amazon Linux 2
> $ curl --version
> _curl 8.3.0 (aarch64-koji-linux-gnu) libcurl/8.3.0 OpenSSL/1.0.2k-fips zlib/1.2.7 libidn2/2.3.0 libpsl/0.21.5 (+libidn2/2.3.0) libssh2/1.4.3 nghttp2/1.41.0 OpenLDAP/2.4.44_
> _Release-Date: 2023-09-13_
...
```
DispatchQueue.concurrentPerform(iterations: 100) { _ in
let session: URLSession = URLSession(configuration: URLSessionConfiguration.default)
DispatchQueue.concurrentPerform(iterations: 100) { _ in
_ = session
}
}
```
|
@swift-ci please test |
| // Only use serialization for OpenSSL < 1.1.0 which has race conditions during cleanup | ||
| private static let _needsCleanupSerialization: Bool = { | ||
| let version = CFURLSessionSSLVersionInfo() | ||
| return version.major < 1 || (version.major == 1 && version.minor < 1) |
There was a problem hiding this comment.
Since curl can be built with TLS libraries other than OpenSSL, and those libraries would return {0, 0, 0} here, we'd be performing cleanup serialization that wouldn't have happened before/might not be needed. Would we want to add some way to check that SSL version parsing did succeed and is in fact OpenSSL before checking the version number?
There was a problem hiding this comment.
Yes! I missed adding a comment that I was thinking "if the SSL library is not known, then serialise cleanup". But true, it makes sense to not be unnecessarily defensive in this case. So I'll refactor as needed.
|
|
||
| CFURLSessionSSLVersion CFURLSessionSSLVersionInfo(void) { | ||
| curl_version_info_data *info = curl_version_info(CURLVERSION_NOW); | ||
| CFURLSessionSSLVersion version = {0, 0, 0}; |
There was a problem hiding this comment.
Related to my comment above, maybe we could add a boolean to the struct like isOpenSSL and set that to true if parsing succeeds below? An enum for different TLS libraries could be nice, but I don't think it's really needed since we're just special casing an outdated version of OpenSSL.
2 participants
Motivation:
URLSessionelusively crashes due to race conditions in OpenSSL <1.1.0 when callingcurl_multi_cleanupconcurrently on different threads.Modifications:
CFURLSessionSSLVersion CFURLSessionSSLVersionInfo(void)for detecting the OpenSSL version in use._MultiHandle.deinitto use a process-wide lock to serializecurl_multi_cleanupcalls for environments with OpenSSL <1.1.0.Result:
Prevents the crash while avoiding unnecessary performance overhead for newer OpenSSL versions which are thread-safe.
Testing:
Passing existing tests is sufficient. However, without this patch, the code snippet below crashes about once in 20 runs on Amazon Linux 2