Skip to content

ocsp: replace stapling_mutex with std::shared_mutex and fix leaks#13226

Open
c-taylor wants to merge 1 commit into
apache:masterfrom
c-taylor:ocsp-stapling-shared-mutex
Open

ocsp: replace stapling_mutex with std::shared_mutex and fix leaks#13226
c-taylor wants to merge 1 commit into
apache:masterfrom
c-taylor:ocsp-stapling-shared-mutex

Conversation

@c-taylor

@c-taylor c-taylor commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Replace the per-certinfo ink_mutex with a std::shared_mutex so readers on the TLS handshake hot path (ssl_callback_ocsp_stapling) and the refresh scan (ocsp_update) can run concurrently; only the cache update takes an exclusive lock. The handshake reader now copies the DER staple to a stack buffer under the shared lock and allocates outside the critical section.

Because the mutex is no longer a C type, certinfo is allocated with new/delete instead of OPENSSL_malloc.

Also fix three pre-existing bugs:

  • certinfo_map_free never freed cinf->cid (leak on every CTX teardown).
  • On BoringSSL the X509 key ref taken via X509_up_ref was never released; free it in certinfo_map_free.
  • ssl_stapling_init_cert's error path could delete a certinfo_map still owned by the SSL_CTX. Only delete a map this call created.

This is a rework of #13097 to separate into 2x independent PRs

std::shared_mutex vs. bravo

The lock pressure for new TLS connections is less than volume or other event/txn mutex. Favoring the reduced memory overhead in the implementation over perfect hypothetical performance. The majority of the benefit is realised from using ANY shared type.

I would suggest that should bravo be favored it become an isolated PR.

@bryancall bryancall requested a review from Copilot June 8, 2026 22:41
@bryancall bryancall added this to the 11.0.0 milestone Jun 8, 2026
@bryancall bryancall requested a review from bneradt June 8, 2026 22:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the OCSP stapling cache synchronization in ATS’ TLS stack by replacing a per-certinfo ink_mutex with std::shared_mutex, allowing concurrent readers on the TLS handshake hot path while keeping cache updates exclusively locked. It also addresses several lifetime / teardown leaks and an SSL_CTX ownership bug in the stapling initialization error path.

Changes:

  • Replace ink_mutex with std::shared_mutex for stapling response data, using shared locks for readers and exclusive locks for updates.
  • Modify the stapling callback to copy the cached DER response under a shared lock and perform heap allocation outside the critical section.
  • Fix resource cleanup bugs in SSL_CTX teardown (free cid, and on BoringSSL release the X509_up_ref() taken for the map key), and correct map deletion behavior on init errors.

@c-taylor

c-taylor commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Expecting minor conflicts after merging #13229
Otherwise this is ready for review. I always had to resolve conflicts when separating the donor PR.

@c-taylor c-taylor marked this pull request as ready for review June 8, 2026 23:08
bneradt
bneradt previously approved these changes Jun 9, 2026
Replace the per-certinfo ink_mutex with a std::shared_mutex so readers
on the TLS handshake hot path (ssl_callback_ocsp_stapling) and the
refresh scan (ocsp_update) can run concurrently; only the cache update
takes an exclusive lock. The handshake reader now copies the DER staple
to a stack buffer under the shared lock and allocates outside the
critical section.
@c-taylor c-taylor force-pushed the ocsp-stapling-shared-mutex branch from c5880c9 to e7c30af Compare June 10, 2026 05:45
@c-taylor c-taylor requested a review from bneradt June 10, 2026 05:48
@c-taylor

Copy link
Copy Markdown
Contributor Author

Rebase + resolve conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

5 participants