openssl: fix the data race when sharing an SSL session between threads#14751
Closed
AkiSakurai wants to merge 3 commits intocurl:masterfrom
Closed
openssl: fix the data race when sharing an SSL session between threads#14751AkiSakurai wants to merge 3 commits intocurl:masterfrom
AkiSakurai wants to merge 3 commits intocurl:masterfrom
Conversation
The SSL_Session object is mutated during connection inside openssl, and it might not be thread-safe. Besides, according to documentation of openssl: ``` SSL_SESSION objects keep internal link information about the session cache list, when being inserted into one SSL_CTX object's session cache. One SSL_SESSION object, regardless of its reference count, must therefore only be used with one SSL_CTX object (and the SSL objects created from this SSL_CTX object). ``` If I understand correctly, it is not safe to share it even in a single thread. Instead, serialize the SSL_SESSION before adding it to the cache, and deserialize it after retrieving it from the cache, so that no concurrent write to the same object is infeasible. Also - add a ci test for thread sanitizer - add a test for sharing ssl sessions concurrently - avoid redefining memory functions when not building libcurl, but including the soruce in libtest - increase the concurrent connections limit in sws Notice that there are fix for a global data race for openssl which is not yet release. The fix is cherry pick for the ci test with thread sanitizer. openssl/openssl@d8def79
vszakats
reviewed
Sep 1, 2024
bagder
reviewed
Sep 1, 2024
vszakats
reviewed
Sep 1, 2024
Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>
bagder
reviewed
Sep 2, 2024
Contributor
|
I believe this PR makes a necessary change. Well done. |
Member
|
Thanks! |
vszakats
added a commit
that referenced
this pull request
Oct 23, 2024
The patch is now part of the 3.4.0 stable release. (Turns out it was part of 3.3.2 already.) Also: - rename this local build to match the scheme used with wolfssl. - drop '3' from local openssl build name. - sync job name with others. - quote step names where missing. Follow-up to a2bcec0 #14751 Closes #15379
pps83
pushed a commit
to pps83/curl
that referenced
this pull request
Apr 26, 2025
The patch is now part of the 3.4.0 stable release. (Turns out it was part of 3.3.2 already.) Also: - rename this local build to match the scheme used with wolfssl. - drop '3' from local openssl build name. - sync job name with others. - quote step names where missing. Follow-up to a2bcec0 curl#14751 Closes curl#15379
This was referenced Aug 12, 2025
vszakats
added a commit
that referenced
this pull request
Aug 13, 2025
Replace autotools with cmake to avoid libtool wrappers that are changing `LD_LIBRARY_PATH` in a way incompatible with the thread sanitizer. To fix the output when the sanitizier is finding something: ``` ==51718==WARNING: Can't write to symbolizer at fd 7 /usr/bin/llvm-symbolizer-18: /home/runner/work/curl/curl/bld/lib/.libs/libcurl.so.4: no version information available (required by /usr/bin/llvm-symbolizer-18) /usr/bin/llvm-symbolizer-18: symbol lookup error: /home/runner/openssl/lib/libcrypto.so.3: undefined symbol: __tsan_func_entry ``` Ref: https://github.com/curl/curl/actions/runs/16911402500/job/47913783729#step:39:4466 After: ``` 13:50:04.117885 == Info:ThreadSanitizer: thread T1 finished with ignores enabled, created at: closing connection #0 #0 pthread_create <null> (libtests+0x6bc0f) (BuildId: 4fe889446291259934205ac03931c397aa0210d3) #1 Curl_thread_create /home/runner/work/curl/curl/lib/curl_threads.c:73:6 (libcurl.so.4+0x55a76) (BuildId: cb0f14ba2ad68c9cab0c980d9a5d7a53cc0782da) #2 async_thrdd_init /home/runner/work/curl/curl/lib/asyn-thrdd.c:500:26 (libcurl.so.4+0x1c153) (BuildId: cb0f14ba2ad68c9cab0c980d9a5d7a53cc0782da) [...] ``` Ref: https://github.com/curl/curl/actions/runs/16939193922/job/48003405272?pr=18274#step:39:4018 Also: - disable memory tracker which turned out to be incompatible with the thread sanitizer and detaching threads. Ref: #18263 and #curl IRC. - the job is ~30 seconds faster after this patch. Reported-by: Stefan Eissing Bug: #18263 (comment) Follow-up to a2bcec0 #14751 Closes #18274
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The SSL_Session object is mutated during connection inside openssl, and it might not be thread-safe. Besides, according to documentation of openssl:
If I understand correctly, it is not safe to share it even in a single thread.
Instead, serialize the SSL_SESSION before adding it to the cache, and deserialize it after retrieving it from the cache, so that concurrent write to the same object is infeasible.
Also
Notice that there are fix for a global data race for openssl which is not yet release. The fix is cherry pick for the ci test with thread sanitizer.
openssl/openssl@d8def79