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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
