Fix TSSLSocket building with LibreSSL

[brad0]

LibreSSL does not have the function OPENSSL_thread_stop().

• Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit? (not required, but preferred)
• 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.

[uros-b]

+1, LGTM

[Jens-G]

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 98be76f. 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), 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:

#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.

thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp

Lines 160 to 171 in b1b9a60

	#if OPENSSL_VERSION_NUMBER >= 0x10100000
	// Do nothing unless an openssl derivative is detected
	# if !defined(OPENSSL_IS_BORINGSSL) && !defined(OPENSSL_IS_AWSLC) && !defined(LIBRESSL_VERSION_NUMBER)
	// https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_thread_stop.html
	OPENSSL_thread_stop();
	# endif
	#else
	// ERR_remove_state() was deprecated in OpenSSL 1.0.0 and ERR_remove_thread_state()
	// was deprecated in OpenSSL 1.1.0; these functions and should not be used.
	// https://www.openssl.org/docs/manmaster/man3/ERR_remove_state.html
	ERR_remove_state(0);
	#endif

thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp

Lines 408 to 419 in b1b9a60

	#if OPENSSL_VERSION_NUMBER >= 0x10100000
	// Do nothing unless an openssl derivative is detected
	# if !defined(OPENSSL_IS_BORINGSSL) && !defined(OPENSSL_IS_AWSLC) && !defined(LIBRESSL_VERSION_NUMBER)
	// https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_thread_stop.html
	OPENSSL_thread_stop();
	# endif
	#else
	// ERR_remove_state() was deprecated in OpenSSL 1.0.0 and ERR_remove_thread_state()
	// was deprecated in OpenSSL 1.1.0; these functions and should not be used.
	// https://www.openssl.org/docs/manmaster/man3/ERR_remove_state.html
	ERR_remove_state(0);
	#endif

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

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[brad0]

The original patch although it did build was based on a mistake. After conferring with a LibreSSL developer the updated patch was the intent.

[Jens-G]

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 THRIFT-6174: Fix TSSLSocket build with OpenSSL 4.0 #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.)

thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp

Lines 159 to 172 in 45364c0

	CRYPTO_cleanup_all_ex_data();
	#if OPENSSL_VERSION_NUMBER >= 0x10100000 && !defined(LIBRESSL_VERSION_NUMBER)
	// Do nothing unless an openssl derivative is detected
	# if !defined(OPENSSL_IS_BORINGSSL) && !defined(OPENSSL_IS_AWSLC)
	// https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_thread_stop.html
	OPENSSL_thread_stop();
	# endif
	#else
	// ERR_remove_state() was deprecated in OpenSSL 1.0.0 and ERR_remove_thread_state()
	// was deprecated in OpenSSL 1.1.0; these functions and should not be used.
	// https://www.openssl.org/docs/manmaster/man3/ERR_remove_state.html
	ERR_remove_state(0);
	#endif
	ERR_free_strings();

thrift/lib/cpp/src/thrift/transport/TSSLSocket.cpp

Lines 407 to 420 in 45364c0

	handshakeCompleted_ = false;
	#if OPENSSL_VERSION_NUMBER >= 0x10100000 && !defined(LIBRESSL_VERSION_NUMBER)
	// Do nothing unless an openssl derivative is detected
	# if !defined(OPENSSL_IS_BORINGSSL) && !defined(OPENSSL_IS_AWSLC)
	// https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_thread_stop.html
	OPENSSL_thread_stop();
	# endif
	#else
	// ERR_remove_state() was deprecated in OpenSSL 1.0.0 and ERR_remove_thread_state()
	// was deprecated in OpenSSL 1.1.0; these functions and should not be used.
	// https://www.openssl.org/docs/manmaster/man3/ERR_remove_state.html
	ERR_remove_state(0);
	#endif
	}

2. Commit message is missing the Client: trailer (AGENTS.md says "Commit message includes affected Client: languages")

The commit is a bare subject line with no body; expected Client: cpp.

thrift/AGENTS.md

Lines 33 to 38 in 45364c0

	- PR title format: `THRIFT-9999: Short description of the change`
	- Commit message format (required for code changes):
	```
	THRIFT-9999: Short description of the change
	Client: cpp,py,java (comma-separated list of affected languages)
	```

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[Jens-G]

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.