You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "brad0 (via GitHub)" <gi...@apache.org> on 2026/08/23 01:24:47 UTC
[PR] Fix TSSLSocket building with LibreSSL [thrift]
brad0 opened a new pull request, #3736:
URL: https://github.com/apache/thrift/pull/3736
LibreSSL does not have the function OPENSSL_thread_stop().
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [X] Did you squash your changes to a single commit? (not required, but preferred)
- [X] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TSSLSocket building with LibreSSL [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3736:
URL: https://github.com/apache/thrift/pull/3736#issuecomment-5649528219
Superseded by #3850, credits retained.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TSSLSocket building with LibreSSL [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3736:
URL: https://github.com/apache/thrift/pull/3736#issuecomment-5566598973
Any news here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TSSLSocket building with LibreSSL [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G closed pull request #3736: Fix TSSLSocket building with LibreSSL
URL: https://github.com/apache/thrift/pull/3736
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TSSLSocket building with LibreSSL [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3736:
URL: https://github.com/apache/thrift/pull/3736#issuecomment-5468166614
> The original patch although it did build was based on a mistake. After conferring with a LibreSSL developer the updated patch was the intent.
So you could theoretically close this PR or what are you trying to say?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TSSLSocket building with LibreSSL [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3736:
URL: https://github.com/apache/thrift/pull/3736#issuecomment-5445394692
### Code review
Found 1 issue:
1. Skipping `OPENSSL_thread_stop()` leaves LibreSSL with no per-thread error-state cleanup at all. LibreSSL hardcodes `OPENSSL_VERSION_NUMBER` to `0x20000000L`, so it always takes the `>= 0x10100000` branch and can never reach the `ERR_remove_state(0)` fallback in the `#else`. Excluding it from the inner guard therefore removes the only cleanup call on both paths — the same shape of gap THRIFT-5482 closed in 98be76fc. Unlike BoringSSL and AWS-LC, LibreSSL does ship a working equivalent: `ERR_remove_state()` -> `ERR_remove_thread_state()` -> `err_thread_del_item()` ([lib/libcrypto/err/err.c](https://github.com/openbsd/src/blob/master/lib/libcrypto/err/err.c)), and there is no pthread_key destructor, so the per-thread `ERR_STATE` otherwise stays in the global hash keyed by tid. Substituting rather than skipping would preserve the cleanup:
```c
#if defined(LIBRESSL_VERSION_NUMBER)
ERR_remove_state(0);
#elif !defined(OPENSSL_IS_BORINGSSL) && !defined(OPENSSL_IS_AWSLC)
OPENSSL_thread_stop();
#endif
```
This is clear-cut at the `cleanupOpenSSL()` site. At the `TSSLSocket::close()` site it is more of a judgement call, since `ERR_remove_state(0)` there would also discard error state the calling thread has not read yet — skipping is defensible if that is the intent.
https://github.com/apache/thrift/blob/b1b9a606aa88f5d7045cda4f73ee7eddc412bf54/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L160-L171
https://github.com/apache/thrift/blob/b1b9a606aa88f5d7045cda4f73ee7eddc412bf54/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L408-L419
Everything else checked out: both `OPENSSL_thread_stop()` call sites are covered, the guard is on the inner `#if` (avoiding the `#else` fallthrough caught in #3055), `<openssl/opensslv.h>` is included at line 47 so `LIBRESSL_VERSION_NUMBER` is visible, and `CONF_modules_unload` is correctly left un-guarded since LibreSSL exports it.
🤖 Generated with [Claude Code](https://claude.ai/code)
<sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TSSLSocket building with LibreSSL [thrift]
Posted by "brad0 (via GitHub)" <gi...@apache.org>.
brad0 commented on PR #3736:
URL: https://github.com/apache/thrift/pull/3736#issuecomment-5446915527
The original patch although it did build was based on a mistake. After conferring with a LibreSSL developer the updated patch was the intent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TSSLSocket building with LibreSSL [thrift]
Posted by "brad0 (via GitHub)" <gi...@apache.org>.
brad0 commented on PR #3736:
URL: https://github.com/apache/thrift/pull/3736#issuecomment-5566776299
> Any news here?
My patch was integrated into the other PR and as I said there that PR builds fine for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TSSLSocket building with LibreSSL [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3736:
URL: https://github.com/apache/thrift/pull/3736#issuecomment-5468794795
I want to add that there is an upcoming conflict between #3736 and #3761.
Therefore I would like to suggest that you both discuss this and come up with **one** patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TSSLSocket building with LibreSSL [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3736:
URL: https://github.com/apache/thrift/pull/3736#issuecomment-5468776783
### Code review
Re-reviewed after the force-push. The revised patch resolves the earlier finding: moving `!defined(LIBRESSL_VERSION_NUMBER)` from the inner guard to the outer one means LibreSSL now falls through to the `#else` and calls `ERR_remove_state(0)`, so it gets real per-thread cleanup instead of none. Verified against current LibreSSL sources: it still exports `ERR_remove_state()` (kept deliberately — `err.h` notes "Still used in 2023"), has never shipped `OPENSSL_thread_stop()`, and `LIBRESSL_VERSION_NUMBER` comes in via the `<openssl/opensslv.h>` already included at line 47. The outer-guard placement also matches the AWS-LC precedent in `61080c673f`, while correctly leaving BoringSSL/AWS-LC on the inner guard, since those two have no `ERR_remove_state()` at all.
Found 2 issues:
1. Merge sequencing: this fix now depends on the `ERR_remove_state(0)` call that #3752 deletes
#3752 removes `ERR_remove_state(0)` from both of these `#else` branches on the grounds that it is a no-op since OpenSSL 1.1 — true for mainline, but after this PR LibreSSL is the branch's main consumer. If #3752 lands after #3736, LibreSSL silently goes back to no cleanup at all, which is exactly the defect the force-push just fixed. Worth sequencing the two, or keeping a LibreSSL carve-out in whichever lands second. (#3761 removes `ERR_remove_state` only from the c_glib file, so it does not collide here.)
https://github.com/apache/thrift/blob/45364c031a18e4f63035a7972bd6351f3eb3ee55/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L159-L172
https://github.com/apache/thrift/blob/45364c031a18e4f63035a7972bd6351f3eb3ee55/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L407-L420
2. Commit message is missing the `Client:` trailer ([AGENTS.md](https://github.com/apache/thrift/blob/45364c031a18e4f63035a7972bd6351f3eb3ee55/AGENTS.md#L33-L38) says "Commit message includes affected `Client:` languages")
The commit is a bare subject line with no body; expected `Client: cpp`.
https://github.com/apache/thrift/blob/45364c031a18e4f63035a7972bd6351f3eb3ee55/AGENTS.md#L33-L38
🤖 Generated with [Claude Code](https://claude.ai/code)
<sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TSSLSocket building with LibreSSL [thrift]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #3736:
URL: https://github.com/apache/thrift/pull/3736#issuecomment-5392604662
+1, LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org