Skip to content

ossl: improve error queue hygiene #267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pgellert merged 5 commits into from
Apr 24, 2026
Merged

ossl: improve error queue hygiene #267

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c8887e2
ossl: use make_ossl_error for BIO creation failure
pgellert Apr 15, 2026
808f6e2
ossl: drain error queue on unexpected error codes
pgellert Apr 15, 2026
9b013e0
ossl: clear error queue after successful SSL_CTX_new
pgellert Apr 15, 2026
8277c2a
ossl: clear error queue after successful SSL operations
pgellert Apr 15, 2026
e5f91e1
ossl: assert clean error queue before SSL operations
pgellert Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 50 additions & 5 deletions src/net/ossl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,10 @@ std::system_error make_ossl_error(const std::string & msg) {
return make_ossl_error(msg, get_all_ossl_errors());
}

std::runtime_error make_unknown_ossl_error(const std::string & msg) {
return std::runtime_error(fmt::format("{}: {}", msg, get_all_ossl_errors()));
}

bool contains_ossl_error(const std::vector<ossl_errc> & error_codes, int lib, int reason) {
return std::any_of(error_codes.cbegin(), error_codes.cend(), [lib, reason](const ossl_errc & code) {
return ERR_GET_LIB(static_cast<unsigned long>(code)) == lib &&
Expand Down Expand Up @@ -1023,7 +1027,7 @@ class session : public enable_shared_from_this<session>, public session_impl {
bio_ptr in_bio(BIO_new(get_method()));
bio_ptr out_bio(BIO_new(get_method()));
if (!in_bio || !out_bio) {
throw std::runtime_error("Failed to create BIOs");
throw make_ossl_error("Failed to create BIOs");
}
if (1 != BIO_ctrl(in_bio.get(), BIO_C_SET_POINTER, 0, this)) {
throw make_ossl_error("Failed to set bio ptr to in bio");
Expand Down Expand Up @@ -1151,7 +1155,7 @@ class session : public enable_shared_from_this<session>, public session_impl {
default:
{
// Some other unhandled situation
auto err = std::runtime_error(
auto err = make_unknown_ossl_error(
"Unknown error encountered during SSL write");
return handle_output_error(std::move(err)).then([] {
return stop_iteration::yes;
Expand Down Expand Up @@ -1191,6 +1195,7 @@ class session : public enable_shared_from_this<session>, public session_impl {
// This do_until runs until either a renegotiation occurs or the packet is empty
while (!eof() && size > 0) {
size_t bytes_written = 0;
verify_clean_error_queue("SSL_write_ex");
auto write_rc = SSL_write_ex(_ssl.get(), ptr, size, &bytes_written);
Comment on lines 1195 to 1199
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new pre-call error-queue invariant (verify_clean_error_queue before SSL_write_ex/SSL_read_ex/SSL_do_handshake/SSL_shutdown) is central to the fix, but there’s no regression test exercising the stale-error-queue scenario described in the PR. Since TLS already has unit coverage (tests/unit/tls_test.cc), consider adding a test that deliberately leaves an error on the OpenSSL per-thread queue and verifies a subsequent, unrelated TLS operation does not misclassify via SSL_get_error (and/or that the queue is drained/handled as intended).

Copilot uses AI. Check for mistakes.
tls_log.trace("{} do_put: SSL_write_ex: {}", *this, write_rc);
if (write_rc != 1) {
Expand All @@ -1201,6 +1206,7 @@ class session : public enable_shared_from_this<session>, public session_impl {
co_return;
}
} else {
clear_stale_ssl_errors("SSL_write_ex");
SEASTAR_ASSERT(bytes_written <= size);
tls_log.trace("{} do_put: bytes_written: {}", *this, bytes_written);
ptr += bytes_written;
Expand Down Expand Up @@ -1282,6 +1288,7 @@ class session : public enable_shared_from_this<session>, public session_impl {
[this] { return connected() || eof(); },
[this] {
try {
verify_clean_error_queue("SSL_do_handshake");
auto n = SSL_do_handshake(_ssl.get());
tls_log.trace("{} do_handshake: SSL_do_handshake: {}", *this, n);
if (n <= 0) {
Expand Down Expand Up @@ -1326,11 +1333,12 @@ class session : public enable_shared_from_this<session>, public session_impl {
return handle_output_error(std::move(err));
}
default:
auto err = std::runtime_error(
auto err = make_unknown_ossl_error(
"Unknown error encountered during handshake");
return handle_output_error(std::move(err));
}
} else {
clear_stale_ssl_errors("SSL_do_handshake");
if (_type == session_type::CLIENT
|| _creds->get_client_auth() != client_auth::NONE) {
verify();
Expand Down Expand Up @@ -1393,6 +1401,7 @@ class session : public enable_shared_from_this<session>, public session_impl {
tls_log.trace("{} do_get: available: {}", *this, avail);
buf_type buf(avail);
size_t bytes_read = 0;
verify_clean_error_queue("SSL_read_ex");
auto read_result = SSL_read_ex(
_ssl.get(), buf.get_write(), avail, &bytes_read);
tls_log.trace("{} do_get: SSL_read_ex: {}", *this, read_result);
Expand Down Expand Up @@ -1451,11 +1460,12 @@ class session : public enable_shared_from_this<session>, public session_impl {
return make_exception_future<buf_type>(_error);
}
default:
_error = std::make_exception_ptr(std::runtime_error(
_error = std::make_exception_ptr(make_unknown_ossl_error(
"Unexpected error condition during SSL read"));
return make_exception_future<buf_type>(_error);
}
} else {
clear_stale_ssl_errors("SSL_read_ex");
buf.trim(bytes_read);
return make_ready_future<buf_type>(std::move(buf));
}
Expand Down Expand Up @@ -1488,11 +1498,14 @@ class session : public enable_shared_from_this<session>, public session_impl {
return make_ready_future();
}

verify_clean_error_queue("SSL_shutdown");
auto res = SSL_shutdown(_ssl.get());
tls_log.trace("{} do_shutdown: SSL_shutdown: {}", *this, res);
if (res == 1) {
clear_stale_ssl_errors("SSL_shutdown");
return wait_for_output();
} else if (res == 0) {
clear_stale_ssl_errors("SSL_shutdown");
return yield().then([this] { return do_shutdown(); });
} else {
auto ssl_err = SSL_get_error(_ssl.get(), res);
Expand Down Expand Up @@ -1536,7 +1549,7 @@ class session : public enable_shared_from_this<session>, public session_impl {
}
default:
{
auto err = std::runtime_error(
auto err = make_unknown_ossl_error(
"Unknown error occurred during SSL shutdown");
return handle_output_error(std::move(err));
}
Expand Down Expand Up @@ -1821,6 +1834,33 @@ SEASTAR_INTERNAL_END_IGNORE_DEPRECATIONS
}

private:
// Some SSL operations return success while leaving stale errors on the
// queue (e.g. from internal BIO write failures that OpenSSL absorbed).
// Drain them so they don't poison the next operation on this shard.
void clear_stale_ssl_errors(const char* operation) {
if (ERR_peek_error() == 0) [[likely]] {
return;
}
auto errors = get_all_ossl_errors();
tls_log.debug("{} {}: ignoring stale errors on queue: {}", *this, operation, errors);
}

// Checks that the OpenSSL per-thread error queue is clean before
// calling an SSL function. A dirty queue can cause SSL_get_error
// to misclassify results (e.g. reporting SSL_ERROR_SYSCALL instead
// of SSL_ERROR_SSL), which can affect unrelated sessions that share
// the same thread.
void verify_clean_error_queue(const char* operation) {
auto err = ERR_peek_error();
if (err == 0) [[likely]] {
return;
}
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");
Comment on lines +1860 to +1861
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify_clean_error_queue() calls SEASTAR_ASSERT unconditionally when the thread error queue is non-empty. SEASTAR_ASSERT is active in all build modes (include/seastar/util/assert.hh:29), so any stale OpenSSL error (including ones OpenSSL may legitimately leave behind on success) will abort the process. Consider draining/clearing the error queue here to preserve the intended hygiene in production, and gate the hard assert behind a debug-only macro if you still want a fail-fast diagnostic.

Suggested change
tls_log.warn("{} stale error on queue before {}: {}", *this, operation, buf);
SEASTAR_ASSERT(0 && "stale errors on OpenSSL error queue");
size_t cleared = 0;
while (ERR_get_error() != 0) {
++cleared;
}
tls_log.warn("{} cleared {} stale error(s) on queue before {}: {}", *this, cleared, operation, buf);
assert(false && "stale errors on OpenSSL error queue");

Copilot uses AI. Check for mistakes.
}

std::vector<subject_alt_name> do_get_alt_name_information(const x509_ptr &peer_cert,
const std::unordered_set<subject_alt_name_type> &types) const {
int ext_idx = X509_get_ext_by_NID(
Expand Down Expand Up @@ -1989,6 +2029,11 @@ SEASTAR_INTERNAL_END_IGNORE_DEPRECATIONS
throw make_ossl_error(
"Failed to initialize SSL context");
}
// SSL_CTX_new can return a valid context while leaving errors on the
// error queue from partially-failed system config parsing (e.g. an
// invalid Ciphersuites value in the system openssl.cnf).
// See https://github.com/openssl/openssl/issues/30760
clear_stale_ssl_errors("SSL_CTX_new");
const auto& ck_pair = _creds->get_certkey_pair();
if (type == session_type::SERVER) {
if (!ck_pair) {
Expand Down
Loading