ossl: error queue assert to only terminate in debug builds#274
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes how the OpenSSL error-queue “stale error” invariant is enforced in the Seastar TLS (OpenSSL-based) implementation, aiming to avoid process termination in production when OpenSSL leaves unexpected errors on the per-thread error queue.
Changes:
- Replace an always-on
SEASTAR_ASSERTwith standardassert()when detecting stale OpenSSL errors on the error queue.
💡 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.
assert() is compiled out when NDEBUG is set, so in release builds this function will just log and then continue while leaving the OpenSSL per-thread error queue dirty. That defeats the purpose of verify_clean_error_queue() and can still poison subsequent SSL_get_error() calls. Consider draining/clearing the queue (e.g., via the existing clear_stale_ssl_errors(operation) / get_all_ossl_errors() path) when a stale error is detected, while keeping an abort/assert behavior only for debug builds if desired.
There was a problem hiding this comment.
We could do that, but I explicitly wanted to avoid clean errors of "unknown origin", which is why I think the current behaviour is the ideal behaviour.
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.
pgellert
force-pushed
the
ossl/relax-err-queue-assert
branch
from
6eac50d to
0c39c1a
Compare
|
force-push: fix commit message typo |
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.
CORE-15655