[v26.1.x] ossl: error queue assert to only terminate in debug builds#275
Conversation
pgellert
requested review from
Copilot,
dotnwat and
nguyen-andrew
and removed request for
a team and
nguyen-andrew
pgellert
changed the title
There was a problem hiding this comment.
Pull request overview
This PR modifies the TLS/OpenSSL session error-queue verification logic to avoid terminating the process when OpenSSL leaves stale errors on the per-thread error queue, by replacing an always-on SEASTAR_ASSERT with a standard assert.
Changes:
- Replace
SEASTAR_ASSERTwithassertinverify_clean_error_queue()when stale OpenSSL errors are detected.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char buf[256]; | ||
| ERR_error_string_n(err, buf, sizeof(buf)); | ||
| tls_log.warn("{} stale error on queue before {}: {}", *this, operation, buf); | ||
| SEASTAR_ASSERT(0 && "stale errors on OpenSSL error queue"); | ||
| assert(0 && "stale errors on OpenSSL error queue"); | ||
| } |
There was a problem hiding this comment.
In release builds (when NDEBUG is defined), assert(...) compiles out, so this function will only log a warning and then continue without clearing the OpenSSL error queue. That leaves the queue dirty, which contradicts the function’s contract and can still poison subsequent SSL_get_error classifications (as noted in the comment above). Consider draining/clearing the error queue here in all builds, and making the termination behavior conditional on a debug-only define (e.g., SEASTAR_DEBUG) rather than relying on assert disappearing.
pgellert
changed the title
Openssl's api contract is too loose and the impact to wide (e.g. low-priority https traffic could crash the whole process) that it makes sense to only terminate here in debug builds. (cherry picked from commit 6eac50d)
pgellert
force-pushed
the
ossl/relax-err-queue-assert-26-1
branch
from
2f22d7e to
6447189
Compare
|
force-push: noop, fix typo in commit title |
Openssl's api contract is too loose and the impact to wide (e.g. low-priority https traffic could crash the whole process) that it makes sense to only terminate here in debug builds.
(cherry picked from commit 6eac50d)
CORE-15655